[PATCH] D104080: [LLD][LLVM] CG Graph profile using relocations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:674
+      if (sec.sh_info == cgProfileSectionIndex)
+        cgProfileRela = CHECK(getObj().relas(sec), this);
       this->sections[i] = createInputSection(sec);
----------------
ayermolo wrote:
> MaskRay wrote:
> > ayermolo wrote:
> > > ayermolo wrote:
> > > > jrtc27 wrote:
> > > > > ayermolo wrote:
> > > > > > jrtc27 wrote:
> > > > > > > What if it's SHT_REL? This won't fail nicely, it'll give an error about sh_entsize not matching. You should either handle SHT_REL properly (i.e. support it) or not even attempt to get an Elf_Rela for it.
> > > > > > Something like this?
> > > > > Why not error?
> > > > With error it will prevent binary from being linked. I thought warning would be more appropriate so that result will still be a valid binary minus this optimization. I can change to error if you and @MaskRay think that is more appropriate.
> > > > With error it will prevent binary from being linked. I thought warning would be more appropriate so that result will still be a valid binary minus this optimization. I can change to error if you and @MaskRay think that is more appropriate.
> > > 
> > > readCallGraphsFromObjectFiles will also need to be modified, otherwise it will fatal there due to obj->cgProfileRela.size() != obj->cgProfile.size() * 2
> > Why doesn't SHT_REL work?
> > 
> > A warning is appropriate, as it is an optional optimization anyway.
> I didn't think SHT_REL is a common case/used. For X86 only I386 and IAMCU use REL, looking at some of the other architectures it's either ON by default or in case of MIPS enabled for 64 bit.
> 
> 
> ```
> bool HasRelocationAddend = TT.isArch64Bit();
>   return std::make_unique<MipsELFObjectWriter>(OSABI, HasRelocationAddend,
>                                                 IsN64);
> ```
> 
> I didn't want to over-complicate the implementation un-necessary, but might have been wrong about it. Should I add support?
32-bit arm uses REL so it is not that uncommon. x86-32 does matter less these days and I don't know who want to use -fprofile-use for it. You can leave REL as-is. I can fix it afterwards.

The description needs to be fixed, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104080