[PATCH] D135912: [PseudoProbe] Replace relocation with offset for entry probe.

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> For those don't, a sentinel probe is emitted for each of the binary functions with a different name from the source.

Nit but it indeed caused confusion for me: if the definition of binary function is each MCSymbol, the above isn't accurate because we never add sentinel for the original function.

Also the term binary function is a bit confusing too. Intuitively a binary function has 1:1 mapping to source function, which is how BOLT defines binary function I believe. I also didn't find precedence in LLVM/MC where these funclets are called binary function. Can we make it straightforward and just call it split functions in code and comments?

Otherwise this looks good. Thanks for working on relocation removal.



================
Comment at: llvm/include/llvm/MC/MCPseudoProbe.h:17-18
 // FUNCTION BODY (one for each outlined function present in the text section)
 //    GUID (uint64)
 //        GUID of the function
 //    NPROBES (ULEB128)
----------------
hoy wrote:
> wenlei wrote:
> > Have you considered removing this GUID field, but always require a sentinel probe for any (split) binary function including the main body? 
> The GUID field here is the GUID of the source/dwarf name, which may be different from the actually symbol linkage name, even for the main body of the function.  The source GUID will be used to decode and generate a profile against the source function name.
> 
Ok, makes sense. Maybe clarify this in this comment section as well? 


================
Comment at: llvm/include/llvm/MC/MCPseudoProbe.h:32
 //          ADDRESS_TYPE (uint1)
-//            0 - code address, 1 - address delta
+//            0 - code address for regular probes
+//              - GUID of linkage name for sentinel probes
----------------
hoy wrote:
> wenlei wrote:
> > I thought we now have GUID for symbol name for sentinel, and address offset for non-sentinel. What does regular probe that doesn't use offset refer to?
> This is mainly for downwards compatibility, i.e, to have the new llvm-profgen handle old binaries. Also Bolt re-encoding uses the old encoding scheme for simplicity. 
How about adding a TODO comment to remove this later? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135912