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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D55142#1320447 <https://reviews.llvm.org/D55142#1320447>, @lemo wrote:

> 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.


Sounds good. I think it would be nice to see the isolated patches standing next to their tests.



================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:134-136
     lldb::SectionSP section_sp(new Section(
         shared_from_this(),     // Module to which this section belongs.
+        GetObjectFile(),        // ObjectFile
----------------
I don't know whether you'll run into any problems because of this, but the way this works for "normal" object files is that the each object file has a list of "own" sections, and then the Module has a "unified" list, which contains the sections of the main object file possibly combined with sections from the symbol object files. Here you are adding a section to the unified list without adding it to the object file, which is a bit nonstandard. I think it would be better to just add it to both places.


================
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) {
----------------
lemo wrote:
> 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.
I don't think the `ObjectFile&` would be an out-parameter any more than the existing `BumpPtrAllocator&` is an out-parameter. So making this a reference would also make things locally consistent.


================
Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:150
+    auto uuid_bytes = uuid.GetBytes();
+    if (uuid_bytes.size() != sizeof(llvm::codeview::GUID) + 4) // CvRecordPdb70
+      return nullptr;
----------------
make this `sizeof(guid)` for consistency.


================
Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:171-174
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
     return nullptr;
+
----------------
Mainly out of curiosity, what's the complication here? The llvm interface does not provide the means to retrieve the age?


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

https://reviews.llvm.org/D55142