[Lldb-commits] [PATCH] D25196: Adding a new Minidump post-mortem debugging plugin

[Pavel Labath via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

I have a bunch of small comments. I'll have another look through this once they are done.

The high-level change we need is to avoid choosing the plugin to use at compile-time (your plugin should be host independent, so-lets use it everywhere). Since you still don't have full feature parity with the windows plugin, add a runtime check, which makes sure that your plugin does not kick in for windows minidump files.  Once you have feature parity, we can remove that check and ask Zachary and Adrian to validate everything still works as they expect.



> TestMiniDumpNew.py:19
> +
> +    @no_debug_info_test
> +    def test_process_info_in_mini_dump(self):

You can replace these with `NO_DEBUG_INFO_TESTCASE = True` at class level.

> TestMiniDumpNew.py:66
> +
> +    @not_remote_testsuite_ready
> +    def test_deeper_stack_in_mini_dump(self):

I don't think these are necessary.

> MinidumpParser.cpp:228
> +
> +bool MinidumpParser::FindMemoryRange(lldb::addr_t addr, Range *range_out) {
> +  llvm::ArrayRef<uint8_t> data = GetStream(MinidumpStreamType::MemoryList);

Return `llvm::Optional<range_out>` ?

> MinidumpParser.h:36
> +  lldb::addr_t start; // virtual address of the beginning of the range
> +  size_t size;        // size of the range in bytes
> +  const uint8_t *ptr; // absolute pointer to the first byte of the range

Make these two an ArrayRef ?

> ProcessMinidump.cpp:47
> +  lldb::ProcessSP process_sp;
> +  if (crash_file) {
> +    // Read enough data for the Minidump header

Please rewrite this to use the early-return style:

  if (!crash_file)
    return nullptr;

etc.

> ProcessMinidump.cpp:55
> +
> +    if (data_sp && data_sp->GetByteSize() == header_size && header != nullptr) {
> +      lldb::DataBufferSP all_data_sp(crash_file->MemoryMapFileContents());

the check for non-nullness of `data_sp` is superfluous (or too late).

> ProcessMinidump.cpp:67
> +
> +// TODO leave it to be only "return true" ?
> +bool ProcessMinidump::CanDebug(lldb::TargetSP target_sp,

It already is return true.

Is that correct?

> ProcessMinidump.cpp:121
> +
> +// TODO is this ok? copied from gdb-remote
> +DynamicLoader *ProcessMinidump::GetDynamicLoader() {

I think it is. You can remove the TODO.

> ThreadMinidump.cpp:37
> +    : Thread(process, td.thread_id), m_thread_reg_ctx_sp(), m_gpregset_data() {
> +  ProcessMinidump *process_ =
> +      static_cast<ProcessMinidump *>(GetProcess().get());

If you make the constructor take a `ProcessMinidump &` you can avoid this cast. (although, if we change the code below, you may not even need it).

> ThreadMinidump.cpp:39
> +      static_cast<ProcessMinidump *>(GetProcess().get());
> +  llvm::ArrayRef<uint8_t> arr_ref(process_->m_minidump_parser.GetData().data() +
> +                                      td.thread_context.rva,

This should be implemented inside the minidump parser. There's no reason for doing memory slicing at this level.

> ThreadMinidump.cpp:48
> +ThreadMinidump::~ThreadMinidump() {
> +  // TODO should we use this? WinMinidump doesn't use it but elf-core does
> +  DestroyThread();

If you don't know whether this is necessary, prefer simplicity and don't do it

> ThreadMinidump.cpp:66
> +RegisterContextSP ThreadMinidump::GetRegisterContext() {
> +  if (m_reg_context_sp.get() == NULL) {
> +    m_reg_context_sp = CreateRegisterContextForFrame(NULL);

`if (!m_reg_context_sp)`

also use `nullptr` instead of `NULL`.

> ThreadMinidump.h:1
> +//===-- ThreadMinidump.-h ---------------------------------------*- C++ -*-===//
> +//

typo (`-h`)

> MinidumpParserTest.cpp:142
>  
> +//TODO add tests for MemoryList parsing
> +

Please do this now.

https://reviews.llvm.org/D25196