[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 9 inline comments as done.
lemo added a comment.

In D55142#1316153 <https://reviews.llvm.org/D55142#1316153>, @labath wrote:

> I don't see any tests :(.
>  Also, the three bullet points in the description sound like rather independent pieces of functionality. Would it be possible to split them up into separate patches? That would make things easier to review, particularly for those who don't look at this code very often :).


I agree with both comments. The intention is to add some tests but I wanted to get the review out early to surface concerns, if any. I also needed more time to investigate a few complex failures uncovered by this change (ex. https://bugs.llvm.org/show_bug.cgi?id=39882 and https://bugs.llvm.org/show_bug.cgi?id=39897)

Yes, this change can also be split in three parts: the reason it's bundled up in this review is that all three parts are required to enable the basic functionality (and overall it's a relatively small change). Maybe it was better if I sent out the parts separately, but right now I'd like to preserve the continuity in the review comments. 
I'm about to send out a new revision and once this review satisfies all the comments I'll split it out and send individual reviews.



================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:132-135
+    if (so.segment == 0) {
+      lldbassert(so.offset == 0);
+      continue;
+    }
----------------
zturner wrote:
> What kind of symbols have a segment and offset of 0?
    68 | S_LDATA32 [size = 44] `trace_event_unique_atomic140`
         type = 0x0013 (__int64), addr = 0000:0000
   112 | S_LDATA32 [size = 44] `trace_event_unique_atomic171`
         type = 0x0013 (__int64), addr = 0000:0000
   156 | S_LDATA32 [size = 44] `trace_event_unique_atomic196`
         type = 0x0013 (__int64), addr = 0000:0000
...

Tracked by : https://bugs.llvm.org/show_bug.cgi?id=39882



================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:150-157
     // The odds of an error in some function such as GetSegmentAndOffset or
     // MakeVirtualAddress are much higher than the odds of encountering bad
     // debug info, so assert that this item was inserted in the map as opposed
     // to having already been there.
-    lldbassert(insert_result.second);
+    //
+    // TODO: revisit this check since it fires for apparently valid PDBs
+    //
----------------
zturner wrote:
> If we're going to comment it out, then just delete the code IMO.
> 
> Do you have some llvm-pdbutil output that demonstrates this on a valid PDB?  I guess we'd be looking for 2 symbols with the same Virtual Address.  Seems odd to have that, but maybe an example of where it's happening would make it clear whether this is actually a valid case.
I spent more time to investigate this and opened a bug to track it: https://bugs.llvm.org/show_bug.cgi?id=39897

With very large PDBs it's not easy to have a definitive verdict so a 2nd opinion is welcome, but as I noted in the bug is seems that ICF is one case where we can end up with multiple symbols at the same address. This means that the data structure needs to be revisited.

For now, I think it's better to suppress the assert so we can exercise the rest of the native PDB reader (I updated the comment to point to the new bug I just opened)


================
Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:106
 static std::unique_ptr<PDBFile>
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator &allocator) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+                    llvm::BumpPtrAllocator &allocator) {
----------------
zturner wrote:
> Perhaps `obj_file` should be a reference just to clarify that it can't be null.
That would make sense. Unfortunately, obj_file can't be const (ObjectFile::GetUUID() is non const and it's not easy to surgically fix it)

So we'd have to pass by non-const ref, which would read "out-parameter", which IMO is more confusing than the non-null part is worth.


================
Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:114-115
 
-  auto *obj = llvm::dyn_cast<llvm::object::COFFObjectFile>(binary.getBinary());
-  if (!obj)
-    return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+    OwningBinary<Binary> binary = std::move(*expected_binary);
----------------
zturner wrote:
> Instead of trying to load it as a PE/COFF fail and then trying something else if it fails (hoping that it's a minidump), perhaps we could use `llvm::identify_magic()` to figure out what it is actually is first.  That function currently cannot detect a Windows minidump, but we coudl teach it to support that.  I think that would make this code more logical.  
> 
> ```
> if (type == exe) {
> } else if (type == minidump) {
> } else {
>   // actual error
> }
> ```
We need to load the binary anyway (this part was kept from the original code). If we can't find the binary we just look for the PDB in the current directly, none of which is specific to minidumps.

I'm sure there's a more elegant solution but again, this is a (hopefully short lived) stop-gap that should go away.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55142