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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
