[Pseudo Probe] Placing .pseudoprobe section in the same comdat group with .text

[hoy]

Summary

Previously the .pseudoprobe data section of a function was supposed to be placed in its own comdat group so that it can de deduplicated against same-named data (such as weak symbols). Unfortunately a bug in the group name setting blocked such, therefore probes for different copies of the same function were not deduplicated and duplicated probes were reported for the final binary code.

Fixing the bug should unblock the deduplication. However, it may cause another issue where the final function code and the probe data are mismatched, because the code and the probes are deduplicated independently. I'm fixing this be placing the probes in the same comdat group as the code so that they can be deduplicated synchronously.

As an example below, with the current change we have section layout :

COMDAT group section [    3] `.group' [foo] contains 3 sections:
  [Index]    Name
  [    4]   .text.foo
  [    5]   .rela.text.foo
  [   17]   .pseudo_probe.foo
COMDAT group section [    6] `.group' [foo2] contains 3 sections:
  [Index]    Name
  [    7]   .text.foo2
  [    8]   .rela.text.foo2
  [   18]   .pseudo_probe.foo2

COMDAT group section [   10] `.group' [.pseudo_probe_desc_foo] contains 1 sections:
  [Index]    Name
  [   11]   .pseudo_probe_desc
COMDAT group section [   12] `.group' [.pseudo_probe_desc_foo2] contains 1 sections:
  [Index]    Name
  [   13]   .pseudo_probe_desc

Note that the probe desc data are still placed in separate groups because they are not bound to the final code unlike the probe data. Once a function is inlined, its probe data are included in the caller's probe data, but its probe desc is still there in separate. The probe descs do not need to be deduplicated with the code together.

Test Plan:

[hoy]

Updating D146853: [Pseudo Probe] Placing .pseudoprobe section in the same comdat group with .txt

[hoy]

Updating D146853: [Pseudo Probe] Placing .pseudoprobe section in the same comdat group with .text

[hoy]

Updating D146853: [Pseudo Probe] Placing .pseudoprobe section in the same comdat group with .text

[hoy]

Removing an assert.

[MaskRay]

Thank you for mentioning lld/ELF, otherwise I may not notice this patch:) The part is not so related to the main functionality and I usually have a strong opinion on lld/test/ELF testing...

Actually, for LLVM metadata sections, I think the convention is not to have a suffix like .text.foo. You may want to use .pseudo_probe without the suffix (MCSectionELF::UniqueID).

[hoy]

In D146853#4428673, @MaskRay wrote:

Thank you for mentioning lld/ELF, otherwise I may not notice this patch:) The part is not so related to the main functionality and I usually have a strong opinion on lld/test/ELF testing...

A test on lld side sounds good. I'll add it.

Actually, for LLVM metadata sections, I think the convention is not to have a suffix like .text.foo. You may want to use .pseudo_probe without the suffix (MCSectionELF::UniqueID).

D152546 is one implementation that treats .pseudo_probe as an independent section of .text, but after iterations we think they should be combined when it comes to deduplication and gc-sections, so I'm falling back to this patch.

[hoy]

Adding a lld test.

[hoy]

Adding a lld test.

[MaskRay]

In D146853#4428709, @hoy wrote:

In D146853#4428673, @MaskRay wrote:

Thank you for mentioning lld/ELF, otherwise I may not notice this patch:) The part is not so related to the main functionality and I usually have a strong opinion on lld/test/ELF testing...

A test on lld side sounds good. I'll add it.

Actually, for LLVM metadata sections, I think the convention is not to have a suffix like .text.foo. You may want to use .pseudo_probe without the suffix (MCSectionELF::UniqueID).

D152546 is one implementation that treats .pseudo_probe as an independent section of .text, but after iterations we think they should be combined when it comes to deduplication and gc-sections, so I'm falling back to this patch.

My idea is that we should not have .pseudo_probe.xxx (waste .strtab size). Just have a number of .pseudo_probe (either differentiated by a comdat key or a UniqueID). This scheme matches many other metadata sections from all sorts of instrumentations.

[hoy]

In D146853#4429265, @MaskRay wrote:

In D146853#4428709, @hoy wrote:

In D146853#4428673, @MaskRay wrote:

Thank you for mentioning lld/ELF, otherwise I may not notice this patch:) The part is not so related to the main functionality and I usually have a strong opinion on lld/test/ELF testing...

A test on lld side sounds good. I'll add it.

Actually, for LLVM metadata sections, I think the convention is not to have a suffix like .text.foo. You may want to use .pseudo_probe without the suffix (MCSectionELF::UniqueID).

D152546 is one implementation that treats .pseudo_probe as an independent section of .text, but after iterations we think they should be combined when it comes to deduplication and gc-sections, so I'm falling back to this patch.

My idea is that we should not have .pseudo_probe.xxx (waste .strtab size). Just have a number of .pseudo_probe (either differentiated by a comdat key or a UniqueID). This scheme matches many other metadata sections from all sorts of instrumentations.

Is it required that sections of a comdat group should have a suffix same to the group name? E.g, .rela.text.foo, is it fine to remove the function name from it?

[MaskRay]

In D146853#4429306, @hoy wrote:

In D146853#4429265, @MaskRay wrote:

In D146853#4428709, @hoy wrote:

In D146853#4428673, @MaskRay wrote:

Thank you for mentioning lld/ELF, otherwise I may not notice this patch:) The part is not so related to the main functionality and I usually have a strong opinion on lld/test/ELF testing...

A test on lld side sounds good. I'll add it.

Actually, for LLVM metadata sections, I think the convention is not to have a suffix like .text.foo. You may want to use .pseudo_probe without the suffix (MCSectionELF::UniqueID).

D152546 is one implementation that treats .pseudo_probe as an independent section of .text, but after iterations we think they should be combined when it comes to deduplication and gc-sections, so I'm falling back to this patch.

My idea is that we should not have .pseudo_probe.xxx (waste .strtab size). Just have a number of .pseudo_probe (either differentiated by a comdat key or a UniqueID). This scheme matches many other metadata sections from all sorts of instrumentations.

Is it required that sections of a comdat group should have a suffix same to the group name? E.g, .rela.text.foo, is it fine to remove the function name from it?

There is no such requirement. LLVM instrumentation generally omits the suffix for metadata sections to save .strtab size (related to relocatable object file sizes).

[hoy]

In D146853#4429307, @MaskRay wrote:

In D146853#4429306, @hoy wrote:

In D146853#4429265, @MaskRay wrote:

In D146853#4428709, @hoy wrote:

In D146853#4428673, @MaskRay wrote:

Thank you for mentioning lld/ELF, otherwise I may not notice this patch:) The part is not so related to the main functionality and I usually have a strong opinion on lld/test/ELF testing...

A test on lld side sounds good. I'll add it.

Actually, for LLVM metadata sections, I think the convention is not to have a suffix like .text.foo. You may want to use .pseudo_probe without the suffix (MCSectionELF::UniqueID).

D152546 is one implementation that treats .pseudo_probe as an independent section of .text, but after iterations we think they should be combined when it comes to deduplication and gc-sections, so I'm falling back to this patch.

My idea is that we should not have .pseudo_probe.xxx (waste .strtab size). Just have a number of .pseudo_probe (either differentiated by a comdat key or a UniqueID). This scheme matches many other metadata sections from all sorts of instrumentations.

Is it required that sections of a comdat group should have a suffix same to the group name? E.g, .rela.text.foo, is it fine to remove the function name from it?

There is no such requirement. LLVM instrumentation generally omits the suffix for metadata sections to save .strtab size (related to relocatable object file sizes).

I see. BTW, why is `UniqueID required? I'm seeing even with MCSection::NonUniqueID the sections are still separate, something like

COMDAT group section [    3] `.group' [foo] contains 2 sections:
   [Index]    Name
   [    4]   .text.foo
   [   11]   .pseudo_probe

COMDAT group section [    5] `.group' [foo2] contains 3 sections:
   [Index]    Name
   [    6]   .text.foo2
   [    7]   .rela.text.foo2
   [   12]   .pseudo_probe

lld/ELF/LinkerScript.cpp

if (config->zKeepTextSectionPrefix)
         if (isSectionPrefix(v.substr(5), s->name.substr(5)))
           return v;
     return ".text";
   }
 
   for (StringRef v :
        {".data.rel.ro", ".data", ".rodata", ".bss.rel.ro", ".bss", ".ldata",
         ".lrodata", ".lbss", ".gcc_except_table", ".init_array", ".fini_array",
-        ".tbss", ".tdata", ".ARM.exidx", ".ARM.extab", ".ctors", ".dtors"})
+        ".tbss", ".tdata", ".ARM.exidx", ".ARM.extab", ".ctors", ".dtors",
+        ".pseudo_probe"})
     if (isSectionPrefix(v, s->name))
       return v;
 
   return s->name;
 }
 
 uint64_t ExprValue::getValue() const {
   if (sec)

lld/test/ELF/pseudo-probe-section.s

+# REQUIRES: x86_64-linux
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: llvm-readelf -g %t.o | FileCheck %s --check-prefix=CHECK-OBJ
+# RUN: ld.lld -shared --soname=t2 %t.o -o %t2.so
+# RUN: llvm-objdump --section-headers  %t2.so | FileCheck %s --check-prefix=CHECK-ELF
+
+
+  .text
+  .file "<stdin>"
+  .section  .text.foo,"axG",@progbits,foo,comdat
+  .globl  foo                             # -- Begin function foo
+  .p2align  4, 0x90
+  .type foo,@function
+foo:                                    # @foo
+  .cfi_startproc
+# %bb.0:                                # %bb0
+  .pseudoprobe  6699318081062747564 1 0 0 foo
+  testl %edi, %edi
+  je  .LBB0_1
+# %bb.2:                                # %bb2
+  .pseudoprobe  6699318081062747564 3 0 0 foo
+  .pseudoprobe  6699318081062747564 4 0 0 foo
+  retq
+.LBB0_1:                                # %bb1
+  .pseudoprobe  6699318081062747564 2 0 0 foo
+  .pseudoprobe  6699318081062747564 4 0 0 foo
+  retq
+.Lfunc_end0:
+  .size foo, .Lfunc_end0-foo
+  .cfi_endproc
+                                        # -- End function
+  .section  .text.foo2,"axG",@progbits,foo2,comdat
+  .p2align  4, 0x90                         # -- Begin function foo2
+  .type foo2,@function
+foo2:                                   # @foo2
+  .cfi_startproc
+# %bb.0:                                # %entry
+  pushq %rax
+  .cfi_def_cfa_offset 16
+  .pseudoprobe  2494702099028631698 1 0 0 foo2
+  movl  $1, %edi
+  .pseudoprobe  2494702099028631698 2 2 0 foo2
+  callq foo@PLT
+  popq  %rax
+  .cfi_def_cfa_offset 8
+  retq
+.Lfunc_end1:
+  .size foo2, .Lfunc_end1-foo2
+  .cfi_endproc
+                                        # -- End function
+  .type a,@object                       # @a
+  .bss
+  .globl  a
+  .p2align  2, 0x0
+a:
+  .long 0                               # 0x0
+  .size a, 4
+
+
+# CHECK-OBJ: COMDAT group section [    3] `.group' [foo] contains 2 sections:
+# CHECK-OBJ-NEXT:   [Index]    Name
+# CHECK-OBJ-NEXT:   [    4]   .text.foo
+# CHECK-OBJ-NEXT:   [   11]   .pseudo_probe.foo
+# CHECK-OBJ: COMDAT group section [    5] `.group' [foo2] contains 3 sections:
+# CHECK-OBJ-NEXT:   [Index]    Name
+# CHECK-OBJ-NEXT:   [    6]   .text.foo2
+# CHECK-OBJ-NEXT:   [    7]   .rela.text.foo2
+# CHECK-OBJ-NEXT:   [   12]   .pseudo_probe.foo2
+
+# CHECK-ELF: .text
+# CHECK-ELF: .pseudo_probe
+# CHECK-ELF-NOT: .pseudo_probe.foo
+# CHECK-ELF-NOT: .pseudo_probe.foo2

llvm/lib/MC/MCObjectFileInfo.cpp

 MCSection *
 MCObjectFileInfo::getPseudoProbeSection(const MCSection &TextSec) const {
   if (Ctx->getObjectFileType() == MCContext::IsELF) {
     const auto &ElfSec = static_cast<const MCSectionELF &>(TextSec);
     // Create a separate section for probes that comes with a comdat function.
     if (const MCSymbol *Group = ElfSec.getGroup()) {
       auto *S = static_cast<MCSectionELF *>(PseudoProbeSection);
       auto Flags = S->getFlags() | ELF::SHF_GROUP;
-      return Ctx->getELFSection(S->getName(), S->getType(), Flags,
-                                S->getEntrySize(), Group->getName(),
+      return Ctx->getELFSection(S->getName() + "." + Group->getName(),
+                                S->getType(), Flags, S->getEntrySize(),
+                                Group->getName(),
                                 /*IsComdat=*/true);
     }
   }
   return PseudoProbeSection;
 }
 
 MCSection *
 MCObjectFileInfo::getPseudoProbeDescSection(StringRef FuncName) const {

llvm/lib/MC/MCPseudoProbe.cpp

MCPseudoProbeDecoder::getCallProbeForAddr(uint64_t Address) const {
   auto It = Address2ProbesMap.find(Address);
   if (It == Address2ProbesMap.end())
     return nullptr;
   const auto &Probes = It->second;
 
   const MCDecodedPseudoProbe *CallProbe = nullptr;
   for (const auto &Probe : Probes) {
     if (Probe.isCall()) {
-      assert(!CallProbe &&
-             "There should be only one call probe corresponding to address "
-             "which is a callsite.");
       CallProbe = &Probe;
+      // Return the first call probe seen so far. Subsequent call probes, if
+      // any, are ignored as they should be a duplication of the first one due
+      // to the the way .pseudo_probe sections is organized. This should only
+      // happen to static functions with overlapping names.
+      break;
     }
   }
   return CallProbe;
 }
 
 const MCPseudoProbeFuncDesc *
 MCPseudoProbeDecoder::getFuncDescForGUID(uint64_t GUID) const {
   auto It = GUID2FuncDescMap.find(GUID);

llvm/test/Transforms/SampleProfile/pseudo-probe-emit.ll

 ; REQUIRES: x86_64-linux
 ; RUN: opt < %s -passes=pseudo-probe -function-sections -S -o %t
 ; RUN: FileCheck %s < %t --check-prefix=CHECK-IL
 ; RUN: llc %t -stop-after=pseudo-probe-inserter -o - | FileCheck %s --check-prefix=CHECK-MIR
 ; RUN: llc %t -function-sections -filetype=asm -o %t1
 ; RUN: FileCheck %s < %t1 --check-prefix=CHECK-ASM
 ; RUN: llc %t -function-sections -filetype=obj -o %t2
 ; RUN: llvm-objdump --section-headers  %t2 | FileCheck %s --check-prefix=CHECK-OBJ
+; RUN: llvm-readelf -g  %t2 | FileCheck %s --check-prefix=CHECK-ELF
 ; RUN: llvm-mc %t1 -filetype=obj -o %t3
 ; RUN: llvm-objdump --section-headers  %t3 | FileCheck %s --check-prefix=CHECK-OBJ
 
 ;; Check the generation of pseudoprobe intrinsic call.
 
 @a = dso_local global i32 0, align 4
 
-define void @foo(i32 %x) !dbg !3 {
+$foo = comdat any
+$foo2 = comdat any
+
+define void @foo(i32 %x) comdat !dbg !3 {
 bb0:
   %cmp = icmp eq i32 %x, 0
 ; CHECK-IL: call void @llvm.pseudoprobe(i64 [[#GUID:]], i64 1, i32 0, i64 -1), !dbg ![[#FAKELINE:]]
 ; CHECK-MIR: PSEUDO_PROBE [[#GUID:]], 1, 0, 0
 ; CHECK-ASM: .pseudoprobe [[#GUID:]] 1 0 0 foo
   br i1 %cmp, label %bb1, label %bb2
 
 bb1:

 
 bb3:
 ; CHECK-IL: call void @llvm.pseudoprobe(i64 [[#GUID]], i64 4, i32 0, i64 -1), !dbg ![[#REALLINE:]]
   ret void, !dbg !12
 }
 
 declare void @bar(i32 %x)
 
-define internal void @foo2(ptr %f) !dbg !4 {
+define internal void @foo2(ptr %f) comdat !dbg !4 {
 entry:
 ; CHECK-IL: call void @llvm.pseudoprobe(i64 [[#GUID2:]], i64 1, i32 0, i64 -1)
 ; CHECK-MIR: PSEUDO_PROBE [[#GUID2:]], 1, 0, 0
 ; CHECK-ASM: .pseudoprobe [[#GUID2:]] 1 0 0 foo2
 ; Check pseudo_probe metadata attached to the indirect call instruction.
 ; CHECK-IL: call void %f(i32 1), !dbg ![[#PROBE0:]]
 ; CHECK-MIR: PSEUDO_PROBE [[#GUID2]], 2, 1, 0
 ; CHECK-ASM: .pseudoprobe [[#GUID2]] 2 1 0 foo2

 ; CHECK-ASM-NEXT: .ascii  "foo"
 ; CHECK-ASM-NEXT: .section  .pseudo_probe_desc,"G",@progbits,.pseudo_probe_desc_foo2,comdat
 ; CHECK-ASM-NEXT: .quad [[#GUID2]]
 ; CHECK-ASM-NEXT: .quad [[#HASH2:]]
 ; CHECK-ASM-NEXT: .byte 4
 ; CHECK-ASM-NEXT: .ascii  "foo2"
 
 ; CHECK-OBJ-COUNT-2: .pseudo_probe_desc
-; CHECK-OBJ: .pseudo_probe
+; CHECK-OBH-COUNT-2: .pseudo_probe
 ; CHECK-OBJ-NOT: .rela.pseudo_probe
 
+; CHECK-ELF: COMDAT group section [    3] `.group' [foo] contains 3 sections:
+; CHECK-ELF-NEXT:   [Index]    Name
+; CHECK-ELF-NEXT:   [    4]   .text.foo
+; CHECK-ELF-NEXT:   [    5]   .rela.text.foo
+; CHECK-ELF-NEXT:   [   17]   .pseudo_probe.foo
+; CHECK-ELF: COMDAT group section [    6] `.group' [foo2] contains 3 sections:
+; CHECK-ELF-NEXT:   [Index]    Name
+; CHECK-ELF-NEXT:   [    7]   .text.foo2
+; CHECK-ELF-NEXT:   [    8]   .rela.text.foo2
+; CHECK-ELF-NEXT:   [   18]   .pseudo_probe.foo2
+; CHECK-ELF: COMDAT group section [   10] `.group' [.pseudo_probe_desc_foo] contains 1 sections:
+; CHECK-ELF-NEXT:   [Index]    Name
+; CHECK-ELF-NEXT:   [   11]   .pseudo_probe_desc
+
+; CHECK-ELF: COMDAT group section [   12] `.group' [.pseudo_probe_desc_foo2] contains 1 sections:
+; CHECK-ELF-NEXT:   [Index]    Name
+; CHECK-ELF-NEXT:   [   13]   .pseudo_probe_desc
+
+; Check the generation of .pseudo_probe_desc section with CFG encoding.
+; CHECK-CFG: .section .pseudo_probe_desc,"",@progbits
+; CHECK-CFG-NEXT: .quad [[#GUID:]]
+; CHECK-CFG-NEXT: .quad [[#HASH:]]
+; CHECK-CFG-NEXT: .byte  3
+; CHECK-CFG-NEXT: .ascii  "foo"
+; CHECK-CFG-NEXT: .byte 4
+; CHECK-CFG-NEXT: .byte 2
+; CHECK-CFG-NEXT: .byte 2
+; CHECK-CFG-NEXT: .byte 3
+; CHECK-CFG-NEXT: .byte 1
+; CHECK-CFG-NEXT: .byte 4
+; CHECK-CFG-NEXT: .byte 1
+; CHECK-CFG-NEXT: .byte 4
+; CHECK-CFG-NEXT: .byte 0
+; CHECK-CFG-NEXT: .quad [[#GUID2:]]
+; CHECK-CFG-NEXT: .quad [[#HASH2:]]
+; CHECK-CFG-NEXT: .byte 4
+; CHECK-CFG-NEXT: .ascii  "foo2"
+; CHECK-CFG-NEXT: .byte 1
+; CHECK-CFG-NEXT: .byte 0
+
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!9, !10}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
 !1 = !DIFile(filename: "test.c", directory: "")
 !2 = !{}
 !3 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !5, unit: !0, retainedNodes: !2)
 !4 = distinct !DISubprogram(name: "foo2", scope: !1, file: !1, line: 2, type: !5, unit: !0, retainedNodes: !2)