AMDGPUTargetStreamer generates .kd symbols, breaking LTO requirement, may be discarded by --gc-sections

[benvanik]

The caching added in 3733ed6 by @MaskRay seems to have broken LTO and --gc-sections for certain use cases. Specifically the change made in lld/ELF/MarkLive.cpp to use the cached isExported value instead of calling includeInDynsym seems to be causing additional sections to be dropped that should not be (or at least were not before the change): 3733ed6#diff-3c88c62d912008cc04f796b330a035ecda925645264eaef43185ad43991cb8e9L224)

The AMDGPU target inserts special kernel descriptor object symbols that must be preserved into the final ELF for the runtime to load. These match any exported kernel in name with a .kd suffix and are emitted by AMDGPUTargetELFStreamer::EmitAmdhsaKernelDescriptor. Prior to the referenced commit these symbols existed and after they don't.

By reverting the mentioned line in MarkLive.cpp the original behavior is restored. I'm not familiar with the codebase but I suspect isExported is not initialized or not safe to cache at that location.

The following repro shows the issue (lld_lto_bug.c):

[[clang::amdgpu_kernel, gnu::visibility("protected")]] void some_kernel(int n) {
  //
}

compiled using

$ clang \
  -x c -std=c23 \
  -target amdgcn-amd-amdhsa -march=gfx1100 \
  -nogpulib \
  -fgpu-rdc \
  -fno-ident \
  -fvisibility=hidden \
  -O3 \
  lld_lto_bug.c \
  -c -emit-llvm -o lld_lto_bug.bc

or since bc files cannot be attached:

; ModuleID = 'lld_lto_bug.bc'
source_filename = "lld_lto_bug.c"
target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"
target triple = "amdgcn-amd-amdhsa"

@__oclc_ABI_version = weak_odr hidden local_unnamed_addr addrspace(4) constant i32 500

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
define protected amdgpu_kernel void @some_kernel(i32 noundef %n) local_unnamed_addr #0 {
entry:
  ret void
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) "amdgpu-no-agpr" "amdgpu-no-completion-action" "amdgpu-no-default-queue" "amdgpu-no-dispatch-id" "amdgpu-no-dispatch-ptr" "amdgpu-no-heap-ptr" "amdgpu-no-hostcall-ptr" "amdgpu-no-implicitarg-ptr" "amdgpu-no-lds-kernel-id" "amdgpu-no-multigrid-sync-arg" "amdgpu-no-queue-ptr" "amdgpu-no-workgroup-id-x" "amdgpu-no-workgroup-id-y" "amdgpu-no-workgroup-id-z" "amdgpu-no-workitem-id-x" "amdgpu-no-workitem-id-y" "amdgpu-no-workitem-id-z" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx1100" "target-features"="+16-bit-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot10-insts,+dot12-insts,+dot5-insts,+dot7-insts,+dot8-insts,+dot9-insts,+dpp,+gfx10-3-insts,+gfx10-insts,+gfx11-insts,+gfx8-insts,+gfx9-insts,+wavefrontsize32" "uniform-work-group-size"="false" }

!llvm.module.flags = !{!0, !1, !2}

!0 = !{i32 1, !"amdhsa_code_object_version", i32 500}
!1 = !{i32 1, !"wchar_size", i32 4}
!2 = !{i32 8, !"PIC Level", i32 2}

Linking with LTO and gc-sections:

lld \
  -flavor gnu \
  -m elf64_amdgpu \
  -shared \
  -plugin-opt=mcpu=gfx1100 \
  -plugin-opt=O3 \
  --lto-CGO3 \
  --gc-sections \
  --print-gc-sections \
  --strip-debug \
  --discard-all \
  --discard-locals \
  -o lld_lto_bug.so \
  lld_lto_bug.bc

Before the commit this will print the expected output (no removal of the rodata):

removing unused section lld_lto_bug_patched.so.lto.o:(.text)

After the commit with the regression removing the rodata:

removing unused section lld_lto_bug.so.lto.o:(.text)
removing unused section lld_lto_bug.so.lto.o:(.rodata)

This can be verified with llvm-readelf as before:

Symbol table '.dynsym' contains 3 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000001500     4 FUNC    GLOBAL PROTECTED   7 some_kernel
     2: 0000000000000480    64 OBJECT  GLOBAL PROTECTED   6 some_kernel.kd

The some_kernel.kd OBJECT is what is required at runtime to use the ELF.

And after:

Symbol table '.dynsym' contains 2 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000001500     4 FUNC    GLOBAL PROTECTED   6 some_kernel

[llvmbot]

@llvm/issue-subscribers-lld-elf

Author: Ben Vanik (benvanik)

The caching added in https://github.com/llvm/llvm-project/commit/3733ed6f1c6b0eef1e13e175ac81ad309fc0b080 by @MaskRay seems to have broken LTO and `--gc-sections` for certain use cases. Specifically the change made in `lld/ELF/MarkLive.cpp` to use the cached `isExported` value instead of calling `includeInDynsym` seems to be causing additional sections to be dropped that should not be (or at least were not before the change): https://github.com/llvm/llvm-project/commit/3733ed6f1c6b0eef1e13e175ac81ad309fc0b080#diff-3c88c62d912008cc04f796b330a035ecda925645264eaef43185ad43991cb8e9L224)

The AMDGPU target inserts special kernel descriptor object symbols that must be preserved into the final ELF for the runtime to load. These match any exported kernel in name with a .kd suffix and are emitted by AMDGPUTargetELFStreamer::EmitAmdhsaKernelDescriptor. Prior to the referenced commit these symbols existed and after they don't.

By reverting the mentioned line in MarkLive.cpp the original behavior is restored. I'm not familiar with the codebase but I suspect isExported is not initialized or not safe to cache at that location.

The following repro shows the issue (lld_lto_bug.c):

[[clang::amdgpu_kernel, gnu::visibility("protected")]] void some_kernel(int n) {
  //
}

compiled using

$ clang \
  -x c -std=c23 \
  -target amdgcn-amd-amdhsa -march=gfx1100 \
  -nogpulib \
  -fgpu-rdc \
  -fno-ident \
  -fvisibility=hidden \
  -O3 \
  lld_lto_bug.c \
  -c -emit-llvm -o lld_lto_bug.bc

or since bc files cannot be attached:

; ModuleID = 'lld_lto_bug.bc'
source_filename = "lld_lto_bug.c"
target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"
target triple = "amdgcn-amd-amdhsa"

@<!-- -->__oclc_ABI_version = weak_odr hidden local_unnamed_addr addrspace(4) constant i32 500

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
define protected amdgpu_kernel void @<!-- -->some_kernel(i32 noundef %n) local_unnamed_addr #<!-- -->0 {
entry:
  ret void
}

attributes #<!-- -->0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) "amdgpu-no-agpr" "amdgpu-no-completion-action" "amdgpu-no-default-queue" "amdgpu-no-dispatch-id" "amdgpu-no-dispatch-ptr" "amdgpu-no-heap-ptr" "amdgpu-no-hostcall-ptr" "amdgpu-no-implicitarg-ptr" "amdgpu-no-lds-kernel-id" "amdgpu-no-multigrid-sync-arg" "amdgpu-no-queue-ptr" "amdgpu-no-workgroup-id-x" "amdgpu-no-workgroup-id-y" "amdgpu-no-workgroup-id-z" "amdgpu-no-workitem-id-x" "amdgpu-no-workitem-id-y" "amdgpu-no-workitem-id-z" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx1100" "target-features"="+16-bit-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot10-insts,+dot12-insts,+dot5-insts,+dot7-insts,+dot8-insts,+dot9-insts,+dpp,+gfx10-3-insts,+gfx10-insts,+gfx11-insts,+gfx8-insts,+gfx9-insts,+wavefrontsize32" "uniform-work-group-size"="false" }

!llvm.module.flags = !{!0, !1, !2}

!0 = !{i32 1, !"amdhsa_code_object_version", i32 500}
!1 = !{i32 1, !"wchar_size", i32 4}
!2 = !{i32 8, !"PIC Level", i32 2}

Linking with LTO and gc-sections:

lld \
  -flavor gnu \
  -m elf64_amdgpu \
  -shared \
  -plugin-opt=mcpu=gfx1100 \
  -plugin-opt=O3 \
  --lto-CGO3 \
  --gc-sections \
  --print-gc-sections \
  --strip-debug \
  --discard-all \
  --discard-locals \
  -o lld_lto_bug.so \
  lld_lto_bug.bc

Before the commit this will print the expected output (no removal of the rodata):

removing unused section lld_lto_bug_patched.so.lto.o:(.text)

After the commit with the regression removing the rodata:

removing unused section lld_lto_bug.so.lto.o:(.text)
removing unused section lld_lto_bug.so.lto.o:(.rodata)

This can be verified with llvm-readelf as before:

Symbol table '.dynsym' contains 3 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000001500     4 FUNC    GLOBAL PROTECTED   7 some_kernel
     2: 0000000000000480    64 OBJECT  GLOBAL PROTECTED   6 some_kernel.kd

The some_kernel.kd OBJECT is what is required at runtime to use the ELF.

And after:

Symbol table '.dynsym' contains 2 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000001500     4 FUNC    GLOBAL PROTECTED   6 some_kernel

[MaskRay]

Thanks for the detailed reproduce. I think the issue is on AMDGPU's side about the .kd symbol.

If we add --save-temps to get lld_lto_bug.so.lto.o and perform a link using the ELF relocatable object file, some_kernel.kd (.rodata) will be retained as expected.

% myld.lld -shared --gc-sections --print-gc-sections lld_lto_bug.so.lto.o
removing unused section lld_lto_bug.so.lto.o:(.text)

There is a difference with the bitcode compilation case.

LLVM LTO requires that each bitcode file lists all symbol resolution.
(Some backend may generate runtime library calls while these libcall symbols appear in the IR symtab but are not listed as referenced.)

(You can find a comment "changing the symbol table could break the semantic assumptions of LTO".)

However, the AMDGPU backend breaks this contract by not recording some_kernel.kd in the IR symbol table, so the LTO library doesn't know some_kernel.kd.

While a symbol some_kernel.kd is created as LTO compilation output, this is no guarantee on whether the symbol should be retained/discarded.

% myllvm-nm a.bc
---------------- W __oclc_ABI_version
---------------- T some_kernel

In older MarkLive.cpp, includeInDynsym returns true due to -shared (retain), and now isExported is false (discard).

To fix this, you can either ensure that the IR symbol table contains some_kernel.kd or synthesize a R_AMDGPU_NONE marker relocation from some_kernel to some_kernel.kd.

If you know the symbol name, you can additionally specify -u some_kernel.kd to retain the symbol.
--export-dynamic-symbol '*.kd' cannot retain the symbol because some_kernel.kd is unknown to lld LTO.

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

Author: Ben Vanik (benvanik)

The caching added in https://github.com/llvm/llvm-project/commit/3733ed6f1c6b0eef1e13e175ac81ad309fc0b080 by @MaskRay seems to have broken LTO and `--gc-sections` for certain use cases. Specifically the change made in `lld/ELF/MarkLive.cpp` to use the cached `isExported` value instead of calling `includeInDynsym` seems to be causing additional sections to be dropped that should not be (or at least were not before the change): https://github.com/llvm/llvm-project/commit/3733ed6f1c6b0eef1e13e175ac81ad309fc0b080#diff-3c88c62d912008cc04f796b330a035ecda925645264eaef43185ad43991cb8e9L224)

The AMDGPU target inserts special kernel descriptor object symbols that must be preserved into the final ELF for the runtime to load. These match any exported kernel in name with a .kd suffix and are emitted by AMDGPUTargetELFStreamer::EmitAmdhsaKernelDescriptor. Prior to the referenced commit these symbols existed and after they don't.

By reverting the mentioned line in MarkLive.cpp the original behavior is restored. I'm not familiar with the codebase but I suspect isExported is not initialized or not safe to cache at that location.

The following repro shows the issue (lld_lto_bug.c):

[[clang::amdgpu_kernel, gnu::visibility("protected")]] void some_kernel(int n) {
  //
}

compiled using

$ clang \
  -x c -std=c23 \
  -target amdgcn-amd-amdhsa -march=gfx1100 \
  -nogpulib \
  -fgpu-rdc \
  -fno-ident \
  -fvisibility=hidden \
  -O3 \
  lld_lto_bug.c \
  -c -emit-llvm -o lld_lto_bug.bc

or since bc files cannot be attached:

; ModuleID = 'lld_lto_bug.bc'
source_filename = "lld_lto_bug.c"
target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"
target triple = "amdgcn-amd-amdhsa"

@<!-- -->__oclc_ABI_version = weak_odr hidden local_unnamed_addr addrspace(4) constant i32 500

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
define protected amdgpu_kernel void @<!-- -->some_kernel(i32 noundef %n) local_unnamed_addr #<!-- -->0 {
entry:
  ret void
}

attributes #<!-- -->0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) "amdgpu-no-agpr" "amdgpu-no-completion-action" "amdgpu-no-default-queue" "amdgpu-no-dispatch-id" "amdgpu-no-dispatch-ptr" "amdgpu-no-heap-ptr" "amdgpu-no-hostcall-ptr" "amdgpu-no-implicitarg-ptr" "amdgpu-no-lds-kernel-id" "amdgpu-no-multigrid-sync-arg" "amdgpu-no-queue-ptr" "amdgpu-no-workgroup-id-x" "amdgpu-no-workgroup-id-y" "amdgpu-no-workgroup-id-z" "amdgpu-no-workitem-id-x" "amdgpu-no-workitem-id-y" "amdgpu-no-workitem-id-z" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx1100" "target-features"="+16-bit-insts,+atomic-fadd-rtn-insts,+ci-insts,+dl-insts,+dot10-insts,+dot12-insts,+dot5-insts,+dot7-insts,+dot8-insts,+dot9-insts,+dpp,+gfx10-3-insts,+gfx10-insts,+gfx11-insts,+gfx8-insts,+gfx9-insts,+wavefrontsize32" "uniform-work-group-size"="false" }

!llvm.module.flags = !{!0, !1, !2}

!0 = !{i32 1, !"amdhsa_code_object_version", i32 500}
!1 = !{i32 1, !"wchar_size", i32 4}
!2 = !{i32 8, !"PIC Level", i32 2}

Linking with LTO and gc-sections:

lld \
  -flavor gnu \
  -m elf64_amdgpu \
  -shared \
  -plugin-opt=mcpu=gfx1100 \
  -plugin-opt=O3 \
  --lto-CGO3 \
  --gc-sections \
  --print-gc-sections \
  --strip-debug \
  --discard-all \
  --discard-locals \
  -o lld_lto_bug.so \
  lld_lto_bug.bc

Before the commit this will print the expected output (no removal of the rodata):

removing unused section lld_lto_bug_patched.so.lto.o:(.text)

After the commit with the regression removing the rodata:

removing unused section lld_lto_bug.so.lto.o:(.text)
removing unused section lld_lto_bug.so.lto.o:(.rodata)

This can be verified with llvm-readelf as before:

Symbol table '.dynsym' contains 3 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000001500     4 FUNC    GLOBAL PROTECTED   7 some_kernel
     2: 0000000000000480    64 OBJECT  GLOBAL PROTECTED   6 some_kernel.kd

The some_kernel.kd OBJECT is what is required at runtime to use the ELF.

And after:

Symbol table '.dynsym' contains 2 entries:
   Num:    Value          Size Type    Bind   Vis       Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT   UND 
     1: 0000000000001500     4 FUNC    GLOBAL PROTECTED   6 some_kernel

[arsenm]

synthesize a R_AMDGPU_NONE marker relocation from some_kernel to some_kernel.kd.

How would you do this?

[benvanik]

Thanks for looking into it!

As a user is there anything I can do for the R_AMDGPU_NONE relocation or whatnot when coming through clang? Shoving some attributes somewhere perhaps? I can certainly add a bunch of -u's to my compile commands but that's not great and will likely bite other users too :)

I understand the bug may be in the AMDGPU target but this change did break the existing (incorrect, but relied upon) behavior. Is there anyone you're aware of who knows that part of the system? (sounds like @arsenm may be the best contact!) If it's something missing in AMDGPUTargetELFStreamer::EmitAmdhsaKernelDescriptor that builds the MCSymbolELF is there any hints as to what it may be? I can't seem to find anywhere that tests this case (emitting the ELFs with LLD using LTO) but I'm not an LLVM dev and this is a tricky intersection of subprojects.

[rnk]

However, the AMDGPU backend breaks this contract by not recording some_kernel.kd in the IR symbol table, so the LTO library doesn't know some_kernel.kd.

I'm not sure this contract is followed in practice. I can think of a wide variety of transforms that should run after LTO and can potentially introduce new symbols. Most instrumentation passes (sanitizers, PGO) run after LTO and generate new symbols that weren't in the IR symbol table. I believe in the last year coroutine splitting was moved after LTO, which splits coroutines into three symbols.

Perhaps the contract should be that post-LTO IR passes cannot introduce new exported, dynamic, non-hidden symbols. The AMD backend pass can still use other mechanisms to ensure that kernel descriptors are not GC'd like @llvm.used or referencing descriptors from a section used in combination with __start_/__end_ symbols or whatever the platform native registration discovery mechanism happens to be.

Instrumentation passes generally do not need to emit new dynamic symbols. They are pretty much always hidden internal symbols. I'm not sure about coroutines. It seems like you could get into trouble if a coroutine gets inlined into two DSOs and then each DSO references different resume and destroy functions.

[jhuber6]

Does anyone know why we use the .kd thing in the first place? I always thought it was weird that we didn't just use the actual symbol, but I'm assuming that's far in the past.

[benvanik]

(only recently digging into all this, so I don't know the history) the .kd symbol is a descriptor in .rodata that lets the runtime resolve metadata about the function - the non-.kd (original) symbol is the function itself in .text - it's unfortunate that it was named .kd as I'm finding most flags (__start/__end IIRC) seem to only accept valid C names.

[rnk]

Sure, but how does the runtime discover the .kd symbols? Typically custom sections are used to implement these sorts of reflection and registration schemes, but there are other ways.

[jhuber6]

(only recently digging into all this, so I don't know the history) the .kd symbol is a descriptor in .rodata that lets the runtime resolve metadata about the function - the non-.kd (original) symbol is the function itself in .text - it's unfortunate that it was named .kd as I'm finding most flags (__start/__end IIRC) seem to only accept valid C names.

Yeah, it's just confusing to me because we could've easily made a custom symbol type on the kernel functions (right now they're indistinguishable from normal functions). After that it would just be a key-value lookup in the .notes section where we store the metadata. I have no clue why we need an extra symbol, but at this point it's probably baked into the runtime too deep.