[lld][ELF] __start_/__stop_ refs don't retain C-ident named group sections

[phosek]

Summary

The special root semantics for identifier-named sections is meant
specifically for the metadata sections. In the context of group
semantics, where group members are always retained or discarded as a
unit, it's natural not to have this semantics apply to a section in a
group, otherwise we would never discard the group defeating the purpose
of using the group in the first place.

This change modifies the GC behavior so that start_/stop_ references
don't retain C identifier named sections in section groups which allows
for these groups to be collected. This matches the behavior of BFD ld.

The only kind of existing case that might break is interdependent
metadata sections that are all in a group together, but that group
doesn't contain any other sections referenced by anything except
implicit inclusion in a __start_ and/or __stop_-referenced
identifier-named section, but such cases should be unlikely.

[MaskRay]

I actually want to get rid of the "C identifier name sections have GC root semantics" https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order#c-identifier-name-sections
See also https://sourceware.org/bugzilla/show_bug.cgi?id=27259#c9 . I started a generic-abi thread as Alan Modra requested.
I would sent a patch like this as well but I am waiting for some feedback.

If we have had SHF_GNU_RETAIN earlier, we would not have needed the special case. Do you know what sections we may break by deleting the isValidCIdentifier(sec->name) branch?

[mcgrathr]

I think this clearly the correct semantics. The whole purpose of groups really is for GC and de-dup behavior. The "automatic" anchor semantics for identifier-named sections clearly only makes sense for sections outside groups where there is no other way to tell if they are meant to be referenced. When such a section is in a group, it clearly is meant to go with the symbol-referenced entities defined in the group (i.e. the function or data object whose symbol is the group's signature symbol in the COMDAT case). Group semantics require that the whole group be kept or discarded together, so if an identifier-named section within a group were always kept, then that whole group must always be kept--keeping the identifier named sections while discarding others in the same group would clearly be a bug by the spec. Keeping the whole group because of the identifier-named section inside it is clearly not correct for GC or COMDAT cases.

[MaskRay]

Code LG. I hope in the future we can entirely drop the code.

else if (isValidCIdentifier(sec->name) && !sec->nextInSectionGroup) {
  cNamedSections[saver.save("__start_" + sec->name)].push_back(sec);
  cNamedSections[saver.save("__stop_" + sec->name)].push_back(sec);
}

I'll check how to make __attribute__((used)) set SHF_GNU_RETAIN.

---

GC semantics are unfortunate. We have a special rule for SHT_NOTE, which has confused FreeBSD folks a bit:
(See the code touched by D70146)

+  case SHT_NOTE:
+    // SHT_NOTE sections in a group are subject to garbage collection.
+    return !sec->nextInSectionGroup;

lld/test/ELF/gc-sections-group-startstop.s

1 ↗

(On Diff #323894)

The test can be written in gc-sections-metadata-startstop.s

32 ↗

(On Diff #323894)

bbb and ccc are not needed.

You can rename startstop-gccollect.s to gc-sections-* and add a section group case.

[jhenderson]

This seems entirely reasonable to me. Actually, I wouldn't be entirely surprised if some users don't expect the current preservation behaviour, even for those that aren't group members.

[MaskRay]

Don't consider C identifier-named sections as GC roots

The description needs to be fixed. It is "__start_/__stop_ references retain C identifier name sections". When __start_/__stop_ references are guaranteed to be present in live sections, C identifier-named sections _behave like_ GC roots.

[phosek]

Updated test and change description.

[phosek]

@MaskRay let me know if the updated test and change description looks good to you.

[MaskRay]

Looks great!

lld/ELF/MarkLive.cpp

if (sec->flags & SHF_GNU_RETAIN) {
       enqueue(sec, 0);
       continue;
     }
     if (sec->flags & SHF_LINK_ORDER)
       continue;
 
     if (isReserved(sec) || script->shouldKeep(sec)) {
       enqueue(sec, 0);
-    } else if (isValidCIdentifier(sec->name)) {
+    } else if (isValidCIdentifier(sec->name) && !sec->nextInSectionGroup) {
       cNamedSections[saver.save("__start_" + sec->name)].push_back(sec);
       cNamedSections[saver.save("__stop_" + sec->name)].push_back(sec);
     }
   }
 
   mark();
 }
 

lld/test/ELF/gc-sections-startstop.s

+## Check that group members are retained or discarded as a unit, and
+## sections whose names are C identifiers aren't considered roots if
+## they're members of a group.
+
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: ld.lld %t.o --gc-sections -o %t
+# RUN: llvm-readelf -s %t | FileCheck %s
+
+# RUN: echo ".global __start___data; __start___data:" > %t2.s
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t2.s -o %t2.o
+# RUN: ld.lld -shared %t2.o -o %t2.so
+# RUN: ld.lld %t.o --gc-sections -o %t2 %t2.so
+# RUN: llvm-readelf -s %t2 | FileCheck %s
+
+# CHECK:     [[#%x,ADDR:]] {{.*}} __start___data
+# CHECK:     [[#ADDR + 8]] {{.*}} __stop___data
+# CHECK:     _start
+# CHECK:     f
+# CHECK-NOT: g
+
+.weak __start___data
+.weak __stop___data
+
+.section .text,"ax",@progbits
+.global _start
+_start:
+  .quad __start___data - .
+  .quad __stop___data - .
+  call f
+
+.section __data,"axG",@progbits,f
+.quad 0
+
+.section .text.f,"axG",@progbits,f
+.global f
+f:
+  nop
+
+.section __data,"axG",@progbits,g
+.quad 0
+
+.section .text.g,"axG",@progbits,g
+.global g
+g:
+  nop

lld/test/ELF/startstop-gccollect.s

-# REQUIRES: x86
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
-
-## Default run: sections foo and bar exist in output
-# RUN: ld.lld %t -o %tout
-# RUN: llvm-objdump -d %tout | FileCheck --check-prefix=DISASM %s
-
-## Check that foo and bar sections are not garbage collected,
-## we do not want to reclaim sections if they are referred
-## by __start_* and __stop_* symbols.
-# RUN: ld.lld %t --gc-sections -o %tout
-# RUN: llvm-objdump -d %tout | FileCheck --check-prefix=DISASM %s
-
-# RUN: echo ".global __start_foo; __start_foo:" > %t2.s
-# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %t2.s -o %t2.o
-# RUN: ld.lld -shared %t2.o -o %t2.so
-# RUN: ld.lld %t --gc-sections -o %tout %t2.so
-# RUN: llvm-objdump -d %tout | FileCheck --check-prefix=DISASM %s
-
-# DISASM:      <_start>:
-# DISASM-NEXT:   callq   {{.*}} <__start_foo>
-# DISASM-NEXT:   callq   {{.*}} <__stop_bar>
-# DISASM-EMPTY:
-# DISASM-NEXT: Disassembly of section foo:
-# DISASM-EMPTY:
-# DISASM-NEXT: <__start_foo>:
-# DISASM-NEXT:   nop
-# DISASM-EMPTY:
-# DISASM-NEXT: Disassembly of section bar:
-# DISASM-EMPTY:
-# DISASM-NEXT: <bar>:
-# DISASM-NEXT:   nop
-
-.global _start
-.text
-_start:
- callq __start_foo
- callq __stop_bar
-
-.section foo,"ax"
- nop
-
-.section bar,"ax"
- nop