From 52015e0c9a4e3bc8dc558929a85461f079dda303 Mon Sep 17 00:00:00 2001 From: pommicket Date: Wed, 13 Sep 2023 23:11:15 -0400 Subject: batch write requests, fix tiny memory leak in ide-autocomplete --- README.md | 3 +- base.h | 20 +++--- buffer.c | 2 +- control | 2 +- ds.h | 19 ++++-- ide-autocomplete.c | 2 + lsp-parse.c | 2 +- lsp-write.c | 130 +++++++++++++++++---------------------- lsp.c | 102 ++++++++++++++++++++---------- lsp.h | 7 ++- ted.h | 2 +- valgrind_suppressions.txt | 16 ++--- windows_installer/ted/ted.vdproj | 6 +- 13 files changed, 174 insertions(+), 139 deletions(-) diff --git a/README.md b/README.md index aacb3ef..5a0b12d 100644 --- a/README.md +++ b/README.md @@ -281,7 +281,7 @@ Then run `make.bat release`. To build the .msi file, you will need Visual Studio 2022, as well as the [Visual Studio Installer Projects extension](https://marketplace.visualstudio.com/items?itemName=VisualStudioClient.MicrosoftVisualStudio2022InstallerProjects). -Then, open windows\_installer\\ted\\ted.sln, and build. +Then, open windows\_installer\\ted.sln, and build. ## Version history @@ -325,6 +325,7 @@ Then, open windows\_installer\\ted\\ted.sln, and build. 2.5 Rename symbol, document links, bug fixes 2023 Aug 15 2.5.1 Bug fixes 2023 Aug 26 2.6 LSP diagnostics, LSP over TCP, GDScript support, & more 2023 Sep 10 +2.6.1 LSP-related bugfixes 2023 Sep 14 ## License diff --git a/base.h b/base.h index b438b19..da01fdc 100644 --- a/base.h +++ b/base.h @@ -66,6 +66,8 @@ typedef uint32_t char32_t; #define static_assert_if_possible(cond) #endif +#ifndef SHORT_FIXED_SIZE_TYPES +#define SHORT_FIXED_SIZE_TYPES /// 8-bit unsigned integer typedef uint8_t u8; /// 16-bit unsigned integer @@ -75,6 +77,16 @@ typedef uint32_t u32; /// 64-bit unsigned integer typedef uint64_t u64; +/// 8-bit signed integer +typedef int8_t i8; +/// 16-bit signed integer +typedef int16_t i16; +/// 32-bit signed integer +typedef int32_t i32; +/// 64-bit signed integer +typedef int64_t i64; +#endif // SHORT_FIXED_SIZE_TYPES + /// maximum value of \ref u8 #define U8_MAX 0xff /// maximum value of \ref u16 @@ -84,14 +96,6 @@ typedef uint64_t u64; /// maximum value of \ref u64 #define U64_MAX 0xffffffffffffffff -/// 8-bit signed integer -typedef int8_t i8; -/// 16-bit signed integer -typedef int16_t i16; -/// 32-bit signed integer -typedef int32_t i32; -/// 64-bit signed integer -typedef int64_t i64; /// minimum value of \ref i8 #define I8_MIN ((i8)0x80) diff --git a/buffer.c b/buffer.c index d840547..85e1dce 100644 --- a/buffer.c +++ b/buffer.c @@ -3471,7 +3471,7 @@ void buffer_render(TextBuffer *buffer, Rect r) { line_number_color = alt_line_number_color; } Rect rect = rect4(x1, y, diagnostic_x2, y + char_height); - buffer_clip_rect(buffer, &rect); + rect_clip_to_rect(&rect, rect4(x1, y1, x2, y2)); gl_geometry_rect( rect, color & 0xffffff7f diff --git a/control b/control index 5f0cf8b..f94bfbd 100644 --- a/control +++ b/control @@ -1,5 +1,5 @@ Package: ted -Version: 2.6 +Version: 2.6.1 Section: text Priority: optional Architecture: amd64 diff --git a/ds.h b/ds.h index a5033f7..8cd30e6 100644 --- a/ds.h +++ b/ds.h @@ -28,9 +28,20 @@ IMPORTANT NOTE: If you are using this with structures containing `long double`s, #include #include #include - -typedef uint32_t u32; -typedef uint8_t u8; +#include +#include + +#ifndef SHORT_FIXED_SIZE_TYPES + typedef uint8_t u8; + typedef uint16_t u16; + typedef uint32_t u32; + typedef uint64_t u64; + typedef int8_t i8; + typedef int16_t i16; + typedef int32_t i32; + typedef int64_t i64; + #define SHORT_FIXED_SIZE_TYPES +#endif typedef union { long num; @@ -407,7 +418,7 @@ static void str_builder_append_null(StrBuilder *builder, size_t n) { arr_set_len(builder->str, arr_len(builder->str) + n); } -static u32 str_builder_len(StrBuilder *builder) { +static u32 str_builder_len(const StrBuilder *builder) { assert(builder->str); return arr_len(builder->str) - 1; } diff --git a/ide-autocomplete.c b/ide-autocomplete.c index 0ad78ed..6089818 100644 --- a/ide-autocomplete.c +++ b/ide-autocomplete.c @@ -399,6 +399,7 @@ void autocomplete_process_lsp_response(Ted *ted, const LSPResponse *response) { // only show phantom if there is exactly 1 possible completion. if (ncandidates == 1) { + free(ac->phantom); ac->phantom = str_dup(candidate); } else { autocomplete_clear_phantom(ac); @@ -429,6 +430,7 @@ void autocomplete_process_lsp_response(Ted *ted, const LSPResponse *response) { } if (ac->last_request_phantom) { assert(ncompletions == 1); + free(ac->phantom); ac->phantom = str_dup(ac->completions[0].text); autocomplete_clear_completions(ac); return; diff --git a/lsp-parse.c b/lsp-parse.c index ff243fb..16e9804 100644 --- a/lsp-parse.c +++ b/lsp-parse.c @@ -1141,7 +1141,7 @@ void process_message(LSP *lsp, JSON *json) { .type = LSP_REQUEST_INITIALIZED, .data = {{0}}, }; - write_request(lsp, &initialized); + lsp_send_request_direct(lsp, &initialized); // we can now send requests which have nothing to do with initialization lsp->initialized = true; if (lsp->configuration_to_send) { diff --git a/lsp-write.c b/lsp-write.c index e0beea2..7d9665c 100644 --- a/lsp-write.c +++ b/lsp-write.c @@ -29,35 +29,37 @@ static const char *lsp_language_id(u64 lang) { typedef struct { LSP *lsp; - StrBuilder builder; + StrBuilder *builder; bool is_first; + size_t length_idx; + size_t content_start_idx; } JSONWriter; -static JSONWriter json_writer_new(LSP *lsp) { +static JSONWriter json_writer_new(LSP *lsp, StrBuilder *builder) { return (JSONWriter){ .lsp = lsp, - .builder = str_builder_new(), + .builder = builder, .is_first = true }; } static void write_obj_start(JSONWriter *o) { - str_builder_append(&o->builder, "{"); + str_builder_append(o->builder, "{"); o->is_first = true; } static void write_obj_end(JSONWriter *o) { - str_builder_append(&o->builder, "}"); + str_builder_append(o->builder, "}"); o->is_first = false; } static void write_arr_start(JSONWriter *o) { - str_builder_append(&o->builder, "["); + str_builder_append(o->builder, "["); o->is_first = true; } static void write_arr_end(JSONWriter *o) { - str_builder_append(&o->builder, "]"); + str_builder_append(o->builder, "]"); o->is_first = false; } @@ -65,12 +67,12 @@ static void write_arr_elem(JSONWriter *o) { if (o->is_first) { o->is_first = false; } else { - str_builder_append(&o->builder, ","); + str_builder_append(o->builder, ","); } } static void write_escaped(JSONWriter *o, const char *string) { - StrBuilder *b = &o->builder; + StrBuilder *b = o->builder; size_t output_index = str_builder_len(b); size_t capacity = 2 * strlen(string) + 1; // append a bunch of null bytes which will hold the escaped string @@ -79,18 +81,18 @@ static void write_escaped(JSONWriter *o, const char *string) { // do the escaping size_t length = json_escape_to(out, capacity, string); // shrink down to just the escaped text - str_builder_shrink(&o->builder, output_index + length); + str_builder_shrink(o->builder, output_index + length); } static void write_string(JSONWriter *o, const char *string) { - str_builder_append(&o->builder, "\""); + str_builder_append(o->builder, "\""); write_escaped(o, string); - str_builder_append(&o->builder, "\""); + str_builder_append(o->builder, "\""); } static void write_key(JSONWriter *o, const char *key) { // NOTE: no keys in the LSP spec need escaping. - str_builder_appendf(&o->builder, "%s\"%s\":", o->is_first ? "" : ",", key); + str_builder_appendf(o->builder, "%s\"%s\":", o->is_first ? "" : ",", key); o->is_first = false; } @@ -115,7 +117,7 @@ static void write_arr_elem_arr_start(JSONWriter *o) { } static void write_number(JSONWriter *o, double number) { - str_builder_appendf(&o->builder, "%g", number); + str_builder_appendf(o->builder, "%g", number); } static void write_key_number(JSONWriter *o, const char *key, double number) { @@ -129,7 +131,7 @@ static void write_arr_elem_number(JSONWriter *o, double number) { } static void write_null(JSONWriter *o) { - str_builder_append(&o->builder, "null"); + str_builder_append(o->builder, "null"); } static void write_key_null(JSONWriter *o, const char *key) { @@ -138,7 +140,7 @@ static void write_key_null(JSONWriter *o, const char *key) { } static void write_bool(JSONWriter *o, bool b) { - str_builder_append(&o->builder, b ? "true" : "false"); + str_builder_append(o->builder, b ? "true" : "false"); } static void write_key_bool(JSONWriter *o, const char *key, bool b) { @@ -163,10 +165,10 @@ static void write_arr_elem_string(JSONWriter *o, const char *s) { static void write_file_uri(JSONWriter *o, LSPDocumentID document) { const char *path = lsp_document_path(o->lsp, document); - str_builder_append(&o->builder, "\"file://"); + str_builder_append(o->builder, "\"file://"); #if _WIN32 // why the fuck is there another slash it makes no goddamn sense - str_builder_append(&o->builder, "/"); + str_builder_append(o->builder, "/"); #endif for (const char *p = path; *p; ++p) { char c = *p; @@ -187,12 +189,12 @@ static void write_file_uri(JSONWriter *o, LSPDocumentID document) { #endif ); if (escaped) { - str_builder_appendf(&o->builder, "%%%02x", (uint8_t)c); + str_builder_appendf(o->builder, "%%%02x", (uint8_t)c); } else { - str_builder_appendf(&o->builder, "%c", c); + str_builder_appendf(o->builder, "%c", c); } } - str_builder_append(&o->builder, "\""); + str_builder_append(o->builder, "\""); } static void write_key_file_uri(JSONWriter *o, const char *key, LSPDocumentID document) { @@ -311,51 +313,33 @@ static const char *lsp_request_method(LSPRequest *request) { return "$/ignore"; } -static const size_t max_header_size = 64; -static JSONWriter message_writer_new(LSP *lsp) { - JSONWriter writer = json_writer_new(lsp); - // this is where our header will go - str_builder_append_null(&writer.builder, max_header_size); +static JSONWriter message_writer_new(LSP *lsp, StrBuilder *builder) { + JSONWriter writer = json_writer_new(lsp, builder); + str_builder_append(builder, "Content-Length: "); + writer.length_idx = str_builder_len(builder); + str_builder_append(builder, "XXXXXXXXXX\r\n\r\n"); + writer.content_start_idx = str_builder_len(builder); return writer; } -static void message_writer_write_and_free(LSP *lsp, JSONWriter *o) { - StrBuilder builder = o->builder; - - // this is kind of hacky but it lets us send the whole request with one write call. - // probably not *actually* needed. i thought it would help fix an error but it didn't. - size_t content_length = str_builder_len(&builder) - max_header_size; - char header_str[64]; - sprintf(header_str, "Content-Length: %zu\r\n\r\n", content_length); - size_t header_size = strlen(header_str); - char *content = &builder.str[max_header_size - header_size]; - memcpy(content, header_str, header_size); - - if (lsp->log) { - fprintf(lsp->log, "LSP MESSAGE FROM CLIENT TO SERVER\n%s\n\n", content + header_size); - } - - long long bytes_written = lsp->socket - ? socket_write(lsp->socket, content, strlen(content)) - : process_write(lsp->process, content, strlen(content)); - - if (bytes_written == -1) { - // we'll handle this properly next time we do a read. - if (lsp->log) fprintf(lsp->log, "LSP server closed connection unexpectedly\n"); - } else if (bytes_written < 0) { - if (lsp->log) fprintf(lsp->log, "Error writing to LSP server (errno = %d)\n", errno); - } else { - assert((size_t)bytes_written == strlen(content)); - } - - str_builder_free(&builder); - - #if LSP_SHOW_C2S - const int limit = 1000; - debug_println("%s%.*s%s%s",term_bold(stdout),limit,content, - strlen(content) > (size_t)limit ? "..." : "", - term_clear(stdout)); - #endif +static void message_writer_finish(JSONWriter *o) { + StrBuilder *builder = o->builder; + char content_len_str[32] = {0}; + size_t content_len = str_builder_len(builder) - o->content_start_idx; + if (content_len > 9999999999) + return; // fuckin what + strbuf_printf(content_len_str, "%zu", content_len); + assert(strlen(content_len_str) <= 10); + char *content_len_out = &builder->str[o->length_idx]; + memcpy(content_len_out, content_len_str, strlen(content_len_str)); + // slide everything over + // ideally, we would just use %10zu, + // but rust-analyzer rejects extra whitespace + // (even though its legal in HTTP) + memmove(content_len_out + strlen(content_len_str), + content_len_out + 10, + 4 /* \r\n\r\n */ + content_len); + str_builder_shrink(builder, str_builder_len(builder) - (10 - strlen(content_len_str))); } static void write_symbol_tag_support(JSONWriter *o) { @@ -395,8 +379,8 @@ static void write_symbol_kind_support(JSONWriter *o) { // NOTE: don't call lsp_request_free after calling this function. // I will do it for you. -void write_request(LSP *lsp, LSPRequest *request) { - JSONWriter writer = message_writer_new(lsp); +void write_request(LSP *lsp, LSPRequest *request, StrBuilder *builder) { + JSONWriter writer = message_writer_new(lsp, builder); JSONWriter *o = &writer; write_obj_start(o); @@ -658,7 +642,7 @@ void write_request(LSP *lsp, LSPRequest *request) { const LSPRequestConfiguration *config = &request->data.configuration; write_key_obj_start(o, "params"); write_key(o, "settings"); - str_builder_append(&o->builder, lsp_request_string(request, config->settings)); + str_builder_append(o->builder, lsp_request_string(request, config->settings)); write_obj_end(o); } break; case LSP_REQUEST_FORMATTING: @@ -681,7 +665,7 @@ void write_request(LSP *lsp, LSPRequest *request) { write_obj_end(o); - message_writer_write_and_free(lsp, o); + message_writer_finish(o); if (request->id) { SDL_LockMutex(lsp->messages_mutex); @@ -694,9 +678,9 @@ void write_request(LSP *lsp, LSPRequest *request) { // NOTE: don't call lsp_response_free after calling this function. // I will do it for you. -static void write_response(LSP *lsp, LSPResponse *response) { +static void write_response(LSP *lsp, LSPResponse *response, StrBuilder *builder) { - JSONWriter writer = message_writer_new(lsp); + JSONWriter writer = message_writer_new(lsp, builder); JSONWriter *o = &writer; LSPRequest *request = &response->request; @@ -724,17 +708,17 @@ static void write_response(LSP *lsp, LSPResponse *response) { } write_obj_end(o); - message_writer_write_and_free(lsp, o); + message_writer_finish(o); lsp_response_free(response); } -void write_message(LSP *lsp, LSPMessage *message) { +void write_message(LSP *lsp, LSPMessage *message, StrBuilder *builder) { switch (message->type) { case LSP_REQUEST: - write_request(lsp, &message->request); + write_request(lsp, &message->request, builder); break; case LSP_RESPONSE: - write_response(lsp, &message->response); + write_response(lsp, &message->response, builder); break; } } diff --git a/lsp.c b/lsp.c index 274176c..bb1b5e9 100644 --- a/lsp.c +++ b/lsp.c @@ -483,6 +483,42 @@ static bool lsp_receive(LSP *lsp, size_t max_size) { return true; } +/// send string to LSP +static void lsp_send_string(LSP *lsp, const char *str, size_t len) { + if (len == 0) { + return; + } + const long long bytes_written = lsp->socket + ? socket_write(lsp->socket, str, len) + : process_write(lsp->process, str, len); + if (bytes_written == -1) { + // we'll handle this properly next time we do a read. + if (lsp->log) fprintf(lsp->log, "LSP server closed connection unexpectedly\n"); + } else if (bytes_written < 0) { + if (lsp->log) fprintf(lsp->log, "Error writing to LSP server (errno = %d)\n", errno); + } else { + assert((size_t)bytes_written == len); + } + + if (lsp->log) { + fprintf(lsp->log, "LSP CLIENT TO SERVER\n%.*s\n\n", (int)len, str); + } + #if LSP_SHOW_C2S + const int limit = (int)min_u64(1000, len); + debug_println("%s%.*s%s%s",term_bold(stdout),limit,str, + len > (size_t)limit ? "..." : "", + term_clear(stdout)); + #endif + +} + +void lsp_send_request_direct(LSP *lsp, LSPRequest *request) { + StrBuilder b = str_builder_new(); + write_request(lsp, request, &b); + lsp_send_string(lsp, b.str, str_builder_len(&b)); + str_builder_free(&b); +} + /// send requests. /// /// returns `false` if we should quit. @@ -494,22 +530,28 @@ static bool lsp_send(LSP *lsp) { LSPMessage *messages = NULL; SDL_LockMutex(lsp->messages_mutex); - size_t n_messages = arr_len(lsp->messages_client2server); - messages = calloc(n_messages, sizeof *messages); - memcpy(messages, lsp->messages_client2server, n_messages * sizeof *messages); - - #if __GNUC__ && !__clang__ - #pragma GCC diagnostic push - // i don't know why GCC is giving me this. some compiler bug. - #pragma GCC diagnostic ignored "-Wfree-nonheap-object" - #endif - arr_clear(lsp->messages_client2server); - #if __GNUC__ && !__clang__ - #pragma GCC diagnostic pop - #endif - + size_t n_messages = arr_len(lsp->messages_client2server); + if (n_messages) { + messages = calloc(n_messages, sizeof *messages); + memcpy(messages, lsp->messages_client2server, n_messages * sizeof *messages); + } + #if __GNUC__ && !__clang__ + #pragma GCC diagnostic push + // i don't know why GCC is giving me this. some compiler bug. + #pragma GCC diagnostic ignored "-Wfree-nonheap-object" + #endif + arr_clear(lsp->messages_client2server); + #if __GNUC__ && !__clang__ + #pragma GCC diagnostic pop + #endif SDL_UnlockMutex(lsp->messages_mutex); + if (!messages) { + // nothing to do + return true; + } + + StrBuilder builder = str_builder_new(); bool alive = true; for (size_t i = 0; i < n_messages; ++i) { LSPMessage *m = &messages[i]; @@ -538,7 +580,7 @@ static bool lsp_send(LSP *lsp) { } if (send) { - write_message(lsp, m); + write_message(lsp, m, &builder); } else { lsp_message_free(m); } @@ -547,7 +589,8 @@ static bool lsp_send(LSP *lsp) { alive = false; } } - + lsp_send_string(lsp, builder.str, str_builder_len(&builder)); + str_builder_free(&builder); free(messages); return alive; } @@ -571,25 +614,15 @@ static int lsp_communication_thread(void *data) { .type = LSP_REQUEST_INITIALIZE }; initialize.id = get_request_id(); - write_request(lsp, &initialize); + lsp_send_request_direct(lsp, &initialize); - const double send_delay = lsp->send_delay; - double last_send = -DBL_MAX; + const u32 send_delay_ms = max_u32(16, (u32)(lsp->send_delay * 1000)); while (1) { - bool send = true; - if (send_delay > 0) { - double t = time_get_seconds(); - if (t - last_send > send_delay) { - last_send = t; - } else { - send = false; - } - } - if (send && !lsp_send(lsp)) + if (!lsp_send(lsp)) break; if (!lsp_receive(lsp, (size_t)10<<20)) break; - if (SDL_SemWaitTimeout(lsp->quit_sem, 5) == 0) + if (SDL_SemWaitTimeout(lsp->quit_sem, send_delay_ms) == 0) break; } @@ -611,8 +644,13 @@ static int lsp_communication_thread(void *data) { // just spam these things // we're supposed to be nice and wait for the shutdown // response, but who gives a fuck - write_request(lsp, &shutdown); - write_request(lsp, &exit); + { + StrBuilder b = str_builder_new(); + write_request(lsp, &shutdown, &b); + write_request(lsp, &exit, &b); + lsp_send_string(lsp, b.str, str_builder_len(&b)); + str_builder_free(&b); + } } return 0; } diff --git a/lsp.h b/lsp.h index b1e8496..88a1478 100644 --- a/lsp.h +++ b/lsp.h @@ -868,8 +868,11 @@ typedef struct { void process_message(LSP *lsp, JSON *json); -void write_request(LSP *lsp, LSPRequest *request); -void write_message(LSP *lsp, LSPMessage *message); +/// write request to string builder +void write_request(LSP *lsp, LSPRequest *request, StrBuilder *builder); +void write_message(LSP *lsp, LSPMessage *message, StrBuilder *builder); +/// send request without any kind of batching. don't use this often. +void lsp_send_request_direct(LSP *lsp, LSPRequest *request); void lsp_request_free(LSPRequest *r); void lsp_response_free(LSPResponse *r); diff --git a/ted.h b/ted.h index 1ef3562..27e6cc2 100644 --- a/ted.h +++ b/ted.h @@ -22,7 +22,7 @@ extern "C" { #include "command.h" /// Version number -#define TED_VERSION "2.6" +#define TED_VERSION "2.6.1" /// Maximum path size ted handles. #define TED_PATH_MAX 1024 /// Config filename diff --git a/valgrind_suppressions.txt b/valgrind_suppressions.txt index 069fe72..7446adf 100644 --- a/valgrind_suppressions.txt +++ b/valgrind_suppressions.txt @@ -73,7 +73,7 @@ { nvidia_driver Memcheck:Leak - match-leak-kinds: reachable + match-leak-kinds: all ... obj:/usr/lib/x86_64-linux-gnu/libnvidia-glcore.so.* ... @@ -87,16 +87,8 @@ { Memcheck:Leak - match-leak-kinds: reachable - fun:realloc - obj:/usr/lib/x86_64-linux-gnu/libdbus* + match-leak-kinds: all ... -} -{ - - Memcheck:Leak - match-leak-kinds: reachable - fun:malloc obj:/usr/lib/x86_64-linux-gnu/libdbus* ... } @@ -127,7 +119,7 @@ { Memcheck:Leak - match-leak-kinds: reachable + match-leak-kinds: all ... fun:_dl_catch_exception ... @@ -135,7 +127,7 @@ { Memcheck:Leak - match-leak-kinds: reachable + match-leak-kinds: all ... fun:_dl_init } diff --git a/windows_installer/ted/ted.vdproj b/windows_installer/ted/ted.vdproj index 785cb4d..f927065 100644 --- a/windows_installer/ted/ted.vdproj +++ b/windows_installer/ted/ted.vdproj @@ -620,15 +620,15 @@ { "Name" = "8:Microsoft Visual Studio" "ProductName" = "8:ted" - "ProductCode" = "8:{6F007966-582C-49F8-8EC7-81A9AC6ADD5E}" - "PackageCode" = "8:{C0778A28-595F-4295-926B-8766F6D9C6BC}" + "ProductCode" = "8:{57EBA176-3202-422A-93DC-234C762F5565}" + "PackageCode" = "8:{B63C253B-5AAA-40CC-B743-1B7CBD0C23CC}" "UpgradeCode" = "8:{844F6C2B-DF3B-4A81-9BD5-603401BBA651}" "AspNetVersion" = "8:2.0.50727.0" "RestartWWWService" = "11:FALSE" "RemovePreviousVersions" = "11:TRUE" "DetectNewerInstalledVersion" = "11:FALSE" "InstallAllUsers" = "11:FALSE" - "ProductVersion" = "8:23.09.1000" + "ProductVersion" = "8:23.09.1401" "Manufacturer" = "8:ted" "ARPHELPTELEPHONE" = "8:" "ARPHELPLINK" = "8:" -- cgit v1.2.3