[Lldb-commits] [PATCH] D68096: Add Linux signal support to ProcessMinidump

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The functionality is fine, though I'd hope it can be cleaned up a bit.

As for testing, yes, yaml2obj has some problems roundtripping complex minidumps (and elf files). Fortunately, for the functionality you're testing, I don't think you really need the executable file, so you can just ignore the elf bit and test with a plain `lldb -c foo.dmp` (obviously, you won't get the backtrace that way, but you don't really need that here. Round-tripping minidumps is also WIP -- I've only implemented the bits I have needed so far (I am adding MemoryInfoList stream as we speak). For some streams that just means, they're not getting pretty-printed, while for others (those that contain pointers to other parts of the file) it means the reconstituted file ends up containing garbage. The exception stream is one of those cases, but here it looks like you actually want to test the case where the exception stream is not present (?), so that might be fine. Or it might be fine if you manually remove the ExceptionStream from the yaml? If you can send me the minidump you have generated I can try to play around with it to see if I can make a reasonable yaml out of it.

As for the "lldb/lit/Minidump or packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new" part, the main difference there is how the test is written. Which one is better depends on what you want to test and to some extent, also on personal preference. The python tests are better when you want to programatically drive lldb and have complex interactions with it. the lit tests are good for simply executing a bunch of known commands, and verifying static output. For this case, I think a lit test is definitely the easier route.



================
Comment at: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp:219-237
+
+  if (arch.GetTriple().isOSLinux()) {
+
+    SetUnixSignals(UnixSignals::Create(GetArchitecture()));
+
+    if (!m_thread_list.empty() &&
+        (!m_active_exception ||
----------------
Would it be possible to move this code (except maybe the SetUnixSignals bit) into the `RefreshStateAfterStop` function? Would be less confusing and it would avoid the need for extra member variables...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68096/new/

https://reviews.llvm.org/D68096