[PATCH] D63521: Teach the symbolizer lib symbolize objects directly.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:82
+
+  // TODO: codeview is not supported yet.
+  Expected<SymbolizableModule *> InfoOrErr = createModuleInfo({&Obj, &Obj},
----------------
ychen wrote:
> jhenderson wrote:
> > This TODO seems somewhat arbitrary in this location. Is CodeView supported for other parts of LLVM symbolizer? What would happen if somebody tried to use this bit of code with CodeView, compared with old behaviour?
> I'm not entirely sure. I know PDB and CodeView are not exactly the same thing. Looks like the name is used interchangeably in this context? `COFFObjectFile::getDebugPDBInfo ` said it is `COFF::IMAGE_DEBUG_TYPE_CODEVIEW`. If somebody uses this code with CodeView, `loadDataForEXE` would fail because of `Objects.first->getFileName()`, `symbolizeCode` would return error. Old behavior of `symbolizeCode(const ObjectFile &Obj, ..)` would always create a DWARFContext.
> 
> ```
>   // If this is a COFF object containing PDB info, use a PDBContext to
>   // symbolize. Otherwise, use DWARF.
>   if (auto CoffObject = dyn_cast<COFFObjectFile>(Objects.first)) {
>     const codeview::DebugInfo *DebugInfo;
>     StringRef PDBFileName;
>     auto EC = CoffObject->getDebugPDBInfo(DebugInfo, PDBFileName);
>     if (!EC && DebugInfo != nullptr && !PDBFileName.empty()) {
>       using namespace pdb;
>       std::unique_ptr<IPDBSession> Session;
>       if (auto Err = loadDataForEXE(PDB_ReaderType::DIA,
>                                     Objects.first->getFileName(), Session)) {
>         Modules.insert(
>             std::make_pair(ModuleName, std::unique_ptr<SymbolizableModule>()));
>         // Return along the PDB filename to provide more context
>         return createFileError(PDBFileName, std::move(Err));
>       }
>       Context.reset(new PDBContext(*CoffObject, std::move(Session)));
>     }
>   }
> ```
I'm not comfortable with a loss in functionality versus the previous version, unless the old functionality was broken and wouldn't work anyway. Not knowing enough about how CodeView/PDB work, I'm not sure I can comment much further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63521