zturner added inline comments.
================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:60
+ void Dump(Stream *s) override { *s << "PlaceholderObjectFile\n"; }
+ uint32_t GetAddressByteSize() const override { return 0; }
+ uint32_t GetDependentModules(FileSpecList &file_list) override { return 0; }
----------------
Should we return something valid for this function?
================
Comment at: source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h:92
+
+ bool HasIndexForModule(uint16_t modi) const;
};
----------------
This method name is a little bit confusing. Is it referring to a numeric index
like 1, 2, 3, 4, 5. Or a `CompilandIndexItem`? I would call this something
like `HasCompilandInfo(uint16_t modi)`
================
Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:132-135
+ if (so.segment == 0) {
+ lldbassert(so.offset == 0);
+ continue;
+ }
----------------
What kind of symbols have a segment and offset of 0?
================
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
+ //
----------------
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.
================
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) {
----------------
Perhaps `obj_file` should be a reference just to clarify that it can't be null.
================
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);
----------------
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
}
```
Repository:
rLLDB LLDB
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
