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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
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
----------------
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?


================
Comment at: llvm/include/llvm/MC/MCPseudoProbe.h:281
 /// unit.
-class MCPseudoProbeSection {
+class MCPseudoProbeSections {
 public:
----------------
do you want to rename this to MCPseudoProbeFunctions? 


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:52
+  bool IsSentinel = isSentinelProbe(getAttributes());
+  assert(LastProbe ||
+         IsSentinel && "Last probe should not be null for non-sentinel probes");
----------------
Did you mean `assert((LastProbe || IsSentinel) && "...")`?


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:80
   } else {
-    // Emit label as a symbolic code address.
-    MCOS->emitSymbolValue(
-        Label, MCOS->getContext().getAsmInfo()->getCodePointerSize());
+    // Emit the GUID of the code range that the sentinel probe represents.
+    MCOS->emitInt64(Guid);
----------------
To be accurate, this is the GUID of the (split) binary function name / elf symbol name? 


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:140
   });
+  assert(!isRoot() && "Root should not come here");
+
----------------
nit: this message doesn't provide additional info other than the assertion itself. the message could be something more meaningful - why we don't expect root node here


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:155
+           "Starting probe of a top-level function should be a sentinel probe");
+    if (LastProbe->getGuid() != Guid)
+      NeedSentinel = true;
----------------
Is this checking the top level binary function name is different from the top level source function name?  And this is to avoid emitting sentinel for main function body?


================
Comment at: llvm/lib/MC/MCPseudoProbe.cpp:214
+      using InlineeType = std::pair<InlineSite, MCPseudoProbeInlineTree *>;
+      auto Sorter = [](const InlineeType &A, const InlineeType &B) {
+        return A.first < B.first;
----------------
nit: this is a comparator instead of a sorter. same for the one above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135912