[PATCH] D79590: [lld] Add a new output section ".text.unknown" for funtions with unknown hotness info especially in sampleFDO

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Writer.cpp:136
     for (StringRef v :
-         {".text.hot.", ".text.unlikely.", ".text.startup.", ".text.exit."})
+         {".text.hot.", ".text.unknown.", ".text.unlikely.", ".text.startup.", ".text.exit."})
       if (isSectionPrefix(v, s->name))
----------------
tmsriram wrote:
> wmi wrote:
> > MaskRay wrote:
> > > wmi wrote:
> > > > MaskRay wrote:
> > > > > tmsriram wrote:
> > > > > > wmi wrote:
> > > > > > > tmsriram wrote:
> > > > > > > > The new prefix looks good to me.
> > > > > > > > 
> > > > > > > > Just curious about input sections that are named just ".text.hot" or ".text.unknown" without the traling period.  This could happen with -fno-unique-section-names I think?  In that case, this handling seems insufficient.  This is related to -keep-text-section-prefix in general. I can take a look at this.
> > > > > > > isSectionPrefix seems considering the section name without trailing period (prefix.drop_back())?
> > > > > > > 
> > > > > > > ```
> > > > > > > static bool isSectionPrefix(StringRef prefix, StringRef name) {
> > > > > > >   return name.startswith(prefix) || name == prefix.drop_back();
> > > > > > > }
> > > > > > > ```
> > > > > > Ah, thanks!
> > > > > I know what you are discussing... (And I mentioned this problem internally previously)
> > > > > 
> > > > > To prevent a linker change (the GNU ld and gold approach has an small annoying behavior), I created https://reviews.llvm.org/D79600
> > > > I am not familiar with linker so I don't know the behavior of GNU ld and gold, but lld seems recognize ".text.hot" and return ".text.hot." for it, so I don't fully understand the intention of D79600, could you explain it a little bit? Thanks.
> > > I updated the description of D79600 a bit. Hope it is clear now... I can clarify if you still think that unclear.
> > > 
> > > In lld, I hope we only recognize `.text.exit.*` but not `.text.exit`, so that the function `exit` in a function section will not be ordered strangely.
> > > 
> > > To achieve that goal, we can rename `.text.exit` (without function sections, or with -fno-unique-section-names) to `.text.exit.`
> > Thanks, now I see what you and Sri mean. Although ".text.hot" is mapped to ".text.hot." when outputing the section, it is not recognized during section ordering or other phases in linker. The function isSectionPrefix is only called by getOutputSectionName so only section outputing phase recognize ".text.hot" 
> > 
> > Then D79600 makes sense to me. Thanks for clarifying!
> Isn't handling it in the linker  the right solution?  
> 
> Prefixes like ".text.unikely", ".text.hot", etc. are produced by all compilers.  That also allows interoperability of various compilers and linkers.  LLD can be fixed to handle the prefixes without the trailing period right?  I dont have a strong opinion, just my 2 cents.
By all compilers, do you mean GCC? I think we can patch GCC as well (if we still care about the use case) to avoid the aforementioned ambiguity.
I prefer the current way LLD handles things to avoid ambiguity.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D79590