Merge pull request #38675 from bfredl/errdefer

feat(ui_client): "press ENTER" free nvim crashes

CMakeLists.txt

@@ -153,9 +153,9 @@ set(NVIM_VERSION_PATCH 0)
 set(NVIM_VERSION_PRERELEASE "-dev") # for package maintainers
 
 # API level
-set(NVIM_API_LEVEL 14)        # Bump this after any API/stdlib change.
+set(NVIM_API_LEVEL 15)        # Bump this after any API/stdlib change.
 set(NVIM_API_LEVEL_COMPAT 0)  # Adjust this after a _breaking_ API change.
-set(NVIM_API_PRERELEASE false)
+set(NVIM_API_PRERELEASE true)
 
 # We _want_ assertions in RelWithDebInfo build-type.
 if(CMAKE_C_FLAGS_RELWITHDEBINFO MATCHES DNDEBUG)

build.zig

@@ -12,9 +12,9 @@ const version = struct {
     const patch = 0;
     const prerelease = "-dev";
 
-    const api_level = 14;
+    const api_level = 15;
     const api_level_compat = 0;
-    const api_prerelease = false;
+    const api_prerelease = true;
 };
 
 pub const SystemIntegrationOptions = packed struct {

runtime/doc/api.txt

@@ -1871,6 +1871,15 @@ nvim__redraw({opts})                                          *nvim__redraw()*
     See also: ~
       • |:redraw|
 
+                                                *nvim__set_restart_on_crash()*
+nvim__set_restart_on_crash({progpath}, {argv})
+    WARNING: This feature is experimental/unstable.
+
+
+    Parameters: ~
+      • {progpath}  (`string`)
+      • {argv}      (`any[]`)
+
 nvim__stats()                                                  *nvim__stats()*
     WARNING: This feature is experimental/unstable.
 

runtime/doc/dev_tools.txt

@@ -530,6 +530,15 @@ Logs will be written to `${HOME}/logs/*san.PID` then.
 
 For more information: https://github.com/google/sanitizers/wiki/SanitizerCommonFlags
 
-
+If you run nvim in the builtin TUI, it can be asked to load the crash log
+after a crash as a quickfix list in a restarted nvim instance.
+Put this in init.lua (adjust quickfix commands to taste):
+>lua
+    if vim.env.ASAN_OPTIONS ~= nil then
+      local log_name = vim.env.HOME.."/logs/asan."..vim.uv.getpid()
+      local args = {"--embed", "-n", "+set efm=%+A%*[^/]%f:%l:%c", "+silent cfile "..log_name, "+silent cfirst", "+silent copen"}
+      vim.api.nvim__set_restart_on_crash("nvim", args)
+    end
+<
 
  vim:tw=78:ts=8:sw=4:et:ft=help:norl:

runtime/lua/vim/_meta/api.gen.lua

@@ -165,6 +165,12 @@ function vim.api.nvim__runtime_inspect() end
 --- @param path string
 function vim.api.nvim__screenshot(path) end
 
+--- WARNING: This feature is experimental/unstable.
+---
+--- @param progpath string
+--- @param argv any[]
+function vim.api.nvim__set_restart_on_crash(progpath, argv) end
+
 --- WARNING: This feature is experimental/unstable.
 ---
 --- Gets internal stats.

src/gen/gen_api_ui_events.lua

@@ -106,7 +106,8 @@ for i = 1, #events do
   local ev = events[i]
   assert(ev.return_type == 'void')
 
-  if ev.since == nil and not ev.noexport then
+  -- Allow unstabilized events starting with "_". Compare "nvim__" for methods.
+  if ev.since == nil and not ev.noexport and not vim.startswith(ev.name, '_') then
     print('Ui event ' .. ev.name .. ' lacks since field.\n')
     os.exit(1)
   end
@@ -211,7 +212,7 @@ for _, ev in ipairs(events) do
       p[1] = 'Dictionary'
     end
   end
-  if not ev.noexport then
+  if not ev.noexport and not vim.startswith(ev.name, '_') then
     exported_events[#exported_events + 1] = ev_exported
   end
 end

src/nvim/api/ui_events.in.h

@@ -178,13 +178,23 @@ void msg_ruler(Array content)
 void msg_history_show(Array entries, Boolean prev_cmd)
   FUNC_API_SINCE(6) FUNC_API_REMOTE_ONLY;
 
-// This UI event is currently undocumented.
+// These UI events are currently documented only internally:
 // - When the server needs to intentionally exit with an exit code, and there is no
 //   message in server stderr for the user, this event is sent with positive `status`
 //   argument, to indicate that the UI should exit normally with `status`.
 // - When the server has crashed or there is a message in server stderr for the user,
 //   this event is not sent, and the UI should make server stderr visible.
+//   If "_set_restart_on_crash_exit" has been sent, the UI may start
+//   a new nvim server with this command line and attach to it.
 // - When :detach is used on the server, this event is sent with a zero `status`
 //   argument, to indicate that the UI shouldn't wait for server exit.
 void error_exit(Integer status)
   FUNC_API_SINCE(12) FUNC_API_CLIENT_IMPL;
+
+// Sets a hint for how a client can handle neovim unexpectedly exiting
+// with an error code or signal (without using "error_exit" to indicate
+// an intentional `:cquit`). This should then be handled by starting a new
+// server. Use `progpath` as the full path to the Nvim executable
+// |v:progpath| and `argv` as its arguments |v:argv|, and reattach to the new
+// server.
+void _set_restart_on_crash_exit(String progpath, Array argv) FUNC_API_CLIENT_IMPL;

src/nvim/api/vim.c

@@ -2574,3 +2574,8 @@ void nvim__redraw(Dict(redraw) *opts, Error *err)
   RedrawingDisabled = save_rd;
   p_lz = save_lz;
 }
+
+void nvim__set_restart_on_crash(String progpath, Array argv)
+{
+  ui_call__set_restart_on_crash_exit(progpath, argv);
+}

src/nvim/channel.c

@@ -804,9 +804,9 @@ static void channel_proc_exit_cb(Proc *proc, int status, void *data)
   // - EOF not received in receive_msgpack, then doesn't call chan_close_on_err().
   // - proc_close_handles not tickled by ui_client.c's LOOP_PROCESS_EVENTS?
   if (!exiting && ui_client_channel_id == chan->id) {
-    // Need to call ui_client_attach_to_restarted_server() here as well, as sometimes
-    // rpc_close_event() hasn't been called yet (also see comments above).
-    ui_client_attach_to_restarted_server();
+    // rpc_close_event() could handle this in principle also for processes, but
+    // sometimes it gets called later than this, and we do care about the exit status
+    ui_client_attach_to_restarted_server(proc->status != 0);
     if (ui_client_channel_id == chan->id) {
       // If the current embedded server has exited and no new server is started,
       // the client should exit with the same status.

src/nvim/event/socket.c

@@ -27,7 +27,7 @@
 ///
 /// @param address Address string
 /// @return pointer to the end of the host part of the address, or NULL if it is not a TCP address
-char *socket_address_tcp_host_end(char *address)
+char *socket_address_tcp_host_end(const char *address)
 {
   if (address == NULL) {
     return NULL;
@@ -39,7 +39,7 @@ char *socket_address_tcp_host_end(char *address)
     return NULL;
   }
 
-  char *colon = strrchr(address, ':');
+  char *colon = strrchr((char *)address, ':');
   return colon != NULL && colon != address ? colon : NULL;
 }
 

src/nvim/func_attr.h

@@ -228,6 +228,11 @@
 # define FUNC_API_SINCE(X)
 /// API function deprecated since the given API level.
 # define FUNC_API_DEPRECATED_SINCE(X)
+
+# define FUNC_API_REMOTE_IMPL
+# define FUNC_API_CLIENT_IMPL
+# define FUNC_API_CLIENT_IGNORE
+# define FUNC_API_COMPOSITOR_IMPL
 #endif
 
 #ifdef DEFINE_FUNC_ATTRIBUTES

src/nvim/msgpack_rpc/channel.c

@@ -501,7 +501,9 @@ static void rpc_close_event(void **argv)
 
   bool is_ui_client = ui_client_channel_id && channel->id == ui_client_channel_id;
   if (is_ui_client) {
-    ui_client_attach_to_restarted_server();
+    if (channel->streamtype != kChannelStreamProc) {
+      ui_client_attach_to_restarted_server(false);
+    }
     if (ui_client_channel_id != channel->id) {
       // Attached to new server. Don't exit.
       return;

src/nvim/ui_client.c

@@ -318,6 +318,10 @@ static void channel_connect_event(void **argv)
 static Array restart_args = ARRAY_DICT_INIT;
 static bool restart_pending = false;
 
+// A server might indicate in advance how a new server should be restarted
+// in cases it crashes due to an unexpected error.
+static Array restart_args_after_crash_exit = ARRAY_DICT_INIT;
+
 /// Handles the "restart" ui-event.
 void ui_client_event_restart(Array args)
 {
@@ -330,38 +334,73 @@ void ui_client_event_restart(Array args)
   restart_pending = true;
 }
 
-/// Called during "restart" when the old server just exited.
-void ui_client_attach_to_restarted_server(void)
+void ui_client_event__set_restart_on_crash_exit(Array args)
 {
+  // Save the arguments for ui_client_may_restart_server() later.
+  api_free_array(restart_args_after_crash_exit);
+  restart_args_after_crash_exit = copy_array(args, NULL);
+}
+
+/// Called during "restart" when the old server just exited.
+void ui_client_attach_to_restarted_server(bool error_restart)
+{
+  Array args = restart_args;
+  bool restart = false;
   if (!restart_pending) {
-    return;
+    if (error_restart && ui_client_error_exit == -1 && restart_args_after_crash_exit.size > 0) {
+      restart = true;
+      args = restart_args_after_crash_exit;
+    } else {
+      return;
+    }
   }
 
   restart_pending = false;
 
-  if (restart_args.size < 1 || restart_args.items[0].type != kObjectTypeString) {
+  if (args.size < 1 || args.items[0].type != kObjectTypeString) {
     ELOG("Error handling ui event 'restart'");
     goto cleanup;
   }
 
-  char *listen_addr = restart_args.items[0].data.string.data;
-  bool is_tcp = socket_address_tcp_host_end(listen_addr) != NULL;
-  const char *err = "";
-  uint64_t chan_id = channel_connect(is_tcp, listen_addr, true, CALLBACK_READER_INIT, 50, &err);
-
-  if (!strequal(err, "")) {
-    ELOG("cannot connect to server %s: %s", listen_addr, err);
-    goto cleanup;
+  uint64_t chan_id;
+  const char *first_arg = args.items[0].data.string.data;
+  if (restart) {
+    if (args.size < 2 || args.items[1].type != kObjectTypeArray) {
+      ELOG("Error handling ui event 'restart'");
+      goto cleanup;
+    }
+    Array cmdargs = args.items[1].data.array;
+    char **argv = xcalloc(cmdargs.size + 1, sizeof(char *));
+    for (size_t i = 0; i < cmdargs.size; i++) {
+      if (cmdargs.items[i].type == kObjectTypeString) {
+        argv[i] = cmdargs.items[i].data.string.data;
+      }
+      if (argv[i] == NULL) {
+        argv[i] = "";
+      }
+    }
+    chan_id = ui_client_start_server(first_arg, cmdargs.size, argv);
+    ui_client_error_exit = -1;
+  } else {
+    bool is_tcp = socket_address_tcp_host_end(first_arg) != NULL;
+    const char *err = NULL;
+    chan_id = channel_connect(is_tcp, first_arg, true, CALLBACK_READER_INIT, 50, &err);
+    if (err != NULL) {
+      ELOG("cannot connect to server %s: %s", first_arg, err);
+      goto cleanup;
+    }
   }
 
   // Client-side server re-attach.
   ui_client_channel_id = chan_id;
   ui_client_attach(tui_width, tui_height, tui_term, tui_rgb);
 
-  ILOG("restarted server address=%s id=%" PRId64, listen_addr, chan_id);
+  ILOG("restarted server address=%s id=%" PRId64, first_arg, chan_id);
 cleanup:
   api_free_array(restart_args);
   restart_args = (Array)ARRAY_DICT_INIT;
+  api_free_array(restart_args_after_crash_exit);
+  restart_args_after_crash_exit = (Array)ARRAY_DICT_INIT;
 }
 
 /// Handles the "error_exit" ui-event.