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

[Pavel Labath via lldb-commits]

labath added a comment.

Just a couple more details and I think we're ready.



> MinidumpParser.cpp:105
> +MinidumpParser::GetThreadContext(const MinidumpThread &td) {
> +  return td.GetContext(GetData().data());
> +}

I think you have made it over-encapsulated now. :)
Either a Parser function which takes a thread or a Thread function which takes a parser as argument should be enough. Is there a reason you need both?

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

Not necessary.

> MinidumpParser.cpp:255
> +          llvm::ArrayRef<uint8_t>(GetData().data() + loc_desc.rva, range_size);
> +      return range_out;
> +    }

`return Range(...)` is much cleaner and shorter.

Add an appropriate constructor if it is needed to make this work.

> MinidumpTypes.cpp:94
> +MinidumpThread::GetContext(const uint8_t *base_ptr) const {
> +  return {base_ptr + thread_context.rva, thread_context.data_size};
> +}

This feels very unsafe. Is there anything guaranteeing that `thread_context.rva` does not point beyond the end of the file?

> ProcessMinidump.cpp:69
> +  // work-in-progress
> +  if (minidump_parser &&
> +      minidump_parser->GetArchitecture().GetTriple().getOS() !=

This could also be an early-return.

https://reviews.llvm.org/D25196