[lldb] Copy log files into diagnostic directory

[JDevlieghere]

Summary

This patch copies over log files to the diagnostic directory. The caveat here is that this only works for logs that are redirected to a file. The implementation piggybacks of the mapping kept by the debugger. The advantage is that it's free until you generate the diagnostics, at which point you only pay the price of copying over the file.

[labath]

seems fine to me.

lldb/test/Shell/Diagnostics/TestCopyLogs.test

4

That's cute, but I suspect windows will have a problem with that. -s %s (and putting the commands in this file) would be safer.

[DavidSpickett]

So either way, if someone was running with no logs enabled we would have to ask them to enable them then re-run and get a new set of diagnostics (which is fine as is , not a criticism).

Option 1 they would need to add -o "log enable gdb-remote packets -f <some arbitrary path>".
Option 2 they would need to add -o "log enable gdb-remote packets".

So a few made up file names vs a performance decrease I'd take the former (without any measurement of the latter).

Given that diagnostics are going to be written on crash is it any more "safe" to just copy a file than rely on a ring buffer in memory? I don't think it is, unless the crash itself was in logging itself and all bets are off then in any case.

Another disadvantage to the ring buffer is a size limit, so you could lose early log data from a long session before you get to the crash. However you could work around that by sending each log to a file, once you had realised that the buffer didn't go far back enough (and we're asking people to re-run the reproducer in any case).

Saving logs that are already written to a file seems logical and isn't much harder to guide people how to do, vs. the ring buffer.

[DavidSpickett]

Also is the buffer option the same as "circular" here?

-h <log-handler> ( --log-handler <log-handler> )
     Specify a log handler which determines where log messages are written.
     Values: default | stream | circular | os

So we could have diagnostics also save anything kept in "circular", on the off chance that writing to a file isn't possible.

[JDevlieghere]

In D135631#3849295, @DavidSpickett wrote:

So either way, if someone was running with no logs enabled we would have to ask them to enable them then re-run and get a new set of diagnostics (which is fine as is , not a criticism).

Yeah, the idea is that if we ask someone to enable logs (which is still very likely even with the diagnostics) we'd automatically include them in the diagnostics so they don't need to go look in multiple places.

In D135631#3849307, @DavidSpickett wrote:

Also is the buffer option the same as "circular" here?

-h <log-handler> ( --log-handler <log-handler> )
     Specify a log handler which determines where log messages are written.
     Values: default | stream | circular | os

Yup

So we could have diagnostics also save anything kept in "circular", on the off chance that writing to a file isn't possible.

Technically yes, but then we have two ad-hoc solutions that support half the log handlers. I think if we care about that we might as well go with the T-based log handler behind the scenes.

lldb/test/Shell/Diagnostics/TestCopyLogs.test

4

I thought about that, but I still need %t to be expanded. I guess I could put log enable lldb commands -f in a file and append %t/commands.log to it.

[DavidSpickett]

Technically yes, but then we have two ad-hoc solutions that support half the log handlers. I think if we care about that we might as well go with the T-based log handler behind the scenes.

Sure. More wondering whether the choice between circular buffer and file is a whole lot of code, but it isn't given that we already have buffers.

Saving log files only sounds good to me.

[JDevlieghere]

Rebase

[JDevlieghere]

ping

[labath]

lldb/source/Core/Debugger.cpp

846

A default reference capture is dangerous when the lambda is supposed to run after the enclosing function returns. I guess you really wanted to capture this (?)
Also, I would expect that means that you need to disable the callback in the Debugger destructor.

lldb/test/Shell/Diagnostics/TestCopyLogs.test

4

hm.. the %t substitution makes it tricky, but I don't think this is better than what we had before. It still has nested quotes, which is the thing that causes problems on windows. I think you could work around this by creating an alias in a command file (say command alias logcommands log enable lldb commands) and then invoking it from the command line (-o "logcommands -f %t/commands.log").

[JDevlieghere]

Remove callback in dtor

[mib]

LGTM!

lldb/source/Core/Debugger.cpp

852

re-use log_path for more clarity

lldb/include/lldb/Core/Debugger.h

 #include "lldb/Core/UserSettingsController.h"
 #include "lldb/Host/HostThread.h"
 #include "lldb/Host/Terminal.h"
 #include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Platform.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Utility/Broadcaster.h"
 #include "lldb/Utility/ConstString.h"
+#include "lldb/Utility/Diagnostics.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-types.h"
protected:
   LoadedPluginsList m_loaded_plugins;
   HostThread m_event_handler_thread;
   HostThread m_io_handler_thread;
   Broadcaster m_sync_broadcaster; ///< Private debugger synchronization.
   Broadcaster m_broadcaster;      ///< Public Debugger event broadcaster.
   lldb::ListenerSP m_forward_listener_sp;
   llvm::once_flag m_clear_once;
   lldb::TargetSP m_dummy_target_sp;
+  Diagnostics::CallbackID m_diagnostics_callback_id;
 
   lldb_private::DebuggerDestroyCallback m_destroy_callback = nullptr;
   void *m_destroy_callback_baton = nullptr;
 
   // Events for m_sync_broadcaster
   enum {
     eBroadcastBitEventThreadIsListening = (1 << 0),
   };

lldb/source/Core/Debugger.cpp

 #include "lldb/Target/Language.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StructuredDataPlugin.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadList.h"
 #include "lldb/Utility/AnsiTerminal.h"
-#include "lldb/Utility/Diagnostics.h"
 #include "lldb/Utility/Event.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Listener.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/lldb-enumerations.h"
Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
   // Turn off use-color if this is a dumb terminal.
   const char *term = getenv("TERM");
   if (term && !strcmp(term, "dumb"))
     SetUseColor(false);
   // Turn off use-color if we don't write to a terminal with color support.
   if (!GetOutputFile().GetIsTerminalWithColors())
     SetUseColor(false);
 
+  if (Diagnostics::Enabled()) {
+    m_diagnostics_callback_id = Diagnostics::Instance().AddCallback(
+        [this](const FileSpec &dir) -> llvm::Error {
+          for (auto &entry : m_stream_handlers) {
+            llvm::StringRef log_path = entry.first();
+            llvm::StringRef file_name = llvm::sys::path::filename(log_path);
+            FileSpec destination = dir.CopyByAppendingPathComponent(file_name);
+            std::error_code ec =
+                llvm::sys::fs::copy_file(log_path, destination.GetPath());
+            if (ec)
+              return llvm::errorCodeToError(ec);
+          }
+          return llvm::Error::success();
+        });
+  }
+
 #if defined(_WIN32) && defined(ENABLE_VIRTUAL_TERMINAL_PROCESSING)
   // Enabling use of ANSI color codes because LLDB is using them to highlight
   // text.
   llvm::sys::Process::UseANSIEscapeCodes(true);
 #endif
 }
 
 Debugger::~Debugger() { Clear(); }
llvm::call_once(m_clear_once, [this]() {
     m_broadcaster_manager_sp->Clear();
 
     // Close the input file _before_ we close the input read communications
     // class as it does NOT own the input file, our m_input_file does.
     m_terminal_state.Clear();
     GetInputFile().Close();
 
     m_command_interpreter_up->Clear();
+
+    if (Diagnostics::Enabled())
+      Diagnostics::Instance().RemoveCallback(m_diagnostics_callback_id);
   });
 }
 
 bool Debugger::GetCloseInputOnEOF() const {
   //    return m_input_comm.GetCloseOnEOF();
   return false;
 }
 

lldb/test/Shell/Diagnostics/Inputs/TestCopyLogs.in

+command alias logcommands log enable lldb commands

lldb/test/Shell/Diagnostics/TestCopyLogs.test

+# RUN: rm -rf %t
+# RUN: mkdir -p %t
+
+# RUN: %lldb -s %S/Inputs/TestCopyLogs.in -o 'logcommands -f %t/commands.log' -o 'diagnostics dump -d %t/diags'
+
+# RUN: cat %t/diags/commands.log | FileCheck %s
+# CHECK: Processing command: diagnostics dump