[PATCH] D89712: [CSSPGO][llvm-profgen] Disassemble text sections

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:59
+// in memory.
+static uint64_t getELFImageLMAForSec(const object::ELFSectionRef &Sec) {
+  if (const auto *ELFObj = dyn_cast<ELF32LEObjectFile>(Sec.getObject()))
----------------
wlei wrote:
> wenlei wrote:
> > Can we sink the 4 `getELFImageLMAForSec` manually to avoid duplication and make it more readable?
> Seems we can't merge those IF together by "||",  I tried and got error.
> you can see in llvm/lib/DebugInfo/Symbolize/Symbolize.cpp, they also use the same style.
> 
> ```
> Optional<ArrayRef<uint8_t>> getBuildID(const ELFObjectFileBase *Obj) {
>   Optional<ArrayRef<uint8_t>> BuildID;
>   if (auto *O = dyn_cast<ELFObjectFile<ELF32LE>>(Obj))
>     BuildID = getBuildID(O->getELFFile());
>   else if (auto *O = dyn_cast<ELFObjectFile<ELF32BE>>(Obj))
>     BuildID = getBuildID(O->getELFFile());
>   else if (auto *O = dyn_cast<ELFObjectFile<ELF64LE>>(Obj))
>     BuildID = getBuildID(O->getELFFile());
>   else if (auto *O = dyn_cast<ELFObjectFile<ELF64BE>>(Obj))
>     BuildID = getBuildID(O->getELFFile());
>   else
>     llvm_unreachable("unsupported file format");
>   return BuildID;
> }
> ```
> (https://github.com/llvm/llvm-project/blob/master/llvm/lib/DebugInfo/Symbolize/Symbolize.cpp)
What I was thinking about was to hoist `getELFImageLMAForSec` only. But not a big deal, current one is fine too. 

```
auto *ELFObj = dyn_cast<ELF32LEObjectFile>(Sec.getObject());
if (!ELFObj)
  ELFObj = dyn_cast<ELF32BEObjectFile>(Sec.getObject());
if (!ELFObj)
  ELFObj = dyn_cast<ELF64BEObjectFile>(Sec.getObject());
if (!ELFObj)
  ELFObj = dyn_cast<ELF64BEObjectFile>(Sec.getObject());

return getELFImageLMAForSec(ELFObj->getELFFile(), Sec, ELFObj->getFileName());
```





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89712