[PATCH] D95232: Symbolizer - Teach symbolizer to work directly on object file.

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:603-626
   std::unique_ptr<DIContext> Context;
   // 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);
----------------
scott.linder wrote:
> The logic for `CoffObject` is not repeated elsewhere, which doesn't seem intended?
> 
> It seems like this method should also be overloaded, i.e. rather than repeat the "try to find it, if not create it" logic in each overload above, just implement `LLVMSymbolizer::getOrCreateModuleInfo(const ObjectFile &)` too, and the bodies elsewhere should become identical. I'd also consider just making the private `*Common` methods templates over the type of the first argument, and have the overloads be one-line wrappers:
> 
> ```
> template <typename T>
> Expected<DILineInfo> symbolizeCommonCode(T ModuleSpecifier, object::SectionedAddress ModuleOffset) {
>     SymbolizableModule *Info;
>     if (auto InfoOrErr = getOrCreateModuleInfo(ModuleSpecifier))
>       Info = InfoOrErr.get();
>     else
>       return InfoOrErr.takeError();
>     // ... the existing body in terms of Info and ModuleOffset
> }
> Expected<DILineInfo> LLVMSymbolizer::symbolizeCode(const std::string &ModuleName, object::SectionedAddress ModuleOffset) { return symbolizeCodeCommon(ModuleName, ModuleOffset); }
> Expected<DILineInfo> LLVMSymbolizer::symbolizeCode(const ObjectFile &ModuleObjectFile, object::SectionedAddress ModuleOffset) { return symbolizeCodeCommon(ModuleObjectFile, ModuleOffset); }
> ```
> 
> I think this minimizes the amount of boilerplate, short of using the preprocessor, while still presenting the nice overloaded interface rather than the templated one.
Small amendment: I'd change the snippet above slightly to begin the method body with:

```
auto InfoOrErr = getOrCreateModuleInfo(ModuleSpecifier);
if (auto Err = InfoOrErr.takeError())
  return Err;
SymbolizableModule *Info = *InfoOrErr;
```

Which is shorter, follows the convention of most of the rest of the file, and follows https://llvm.org/docs/ProgrammersManual.html#recoverable-errors


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

https://reviews.llvm.org/D95232