Make section attribute and -ffunction-sections play nicely

[probinson]

Summary

People use -ffunction-sections to put each function into its own
object-file section; this makes linker garbage-collection simpler.
However, if there's an explicit attribute((section("name"))
on the function, all functions with that attribute end up in a
single section, defeating the linker GC.

Use section groups to make these things work together.

[probinson]

This works in my simple test cases, but I'm not 100% sure it's (a) the best place to do this, or (b) the only place that needs to do this.
Really we should guarantee that comdat any wins over comdat nodeduplicate in all cases.

[MaskRay]

[...] all functions with that attribute end up in a single section, defeating the linker GC.

This can be made preciser.

Say we have

__attribute__((section("foo"))) void f() {}
__attribute__((section("foo"))) void g() {}

f and g don't use COMDAT, therefore they are in the same section foo in Clang and GCC (coarse-grained). This foo section is retained or discarded as a unit. Considering the whole link, a foo section in a translation unit can be discarded.

I suspect that some people may find the new behavior surprising, so an option is probably needed. Ideally inform GCC about this new toggle as well.

I think ThinLTO's comdat nodeduplicate handling is great. In D108879 I noted an interesting issue related to regular LTO. I haven't tested more scenarios about regular LTO. If you want to use it at scale, consider more testing.

[probinson]

I'm not sure I understand the linker's mechanics here. Let me say some things and you can describe my misunderstanding.

• If the linker was going to discard all of section foo (in the current scheme), that means it had no reason to retain either f() or g(). In the new scheme, the net result would be the same, both f() and g() are dead and would be discarded. So, no behavior change.
• If the linker wanted to retain f(), but had no reason to retain g(), then in the current scheme it would retain all of section foo. In the new scheme it would retain f() but discard g(). This is the desired behavior change.

Is that second point the "surprising" behavior? Note that this change applies only to functions, not variables.

Yes, more testing is definitely a good thing.

If we do have to have an option, I suppose it could be something like:
-ffunction-sections[=(default,all)] where -ffunction-sections or -ffunction-sections=default will apply only to .text (or other default text section), while -ffunction-sections=all does what my patch says.

[MaskRay]

If one TU has more than one section of the same name, there is no guarantee they are adjacent after linking. Linker scripts and --shuffle-sections can adjust them.
I don't mind a behavior change if GCC is willing to do the same. But without, it's safer to use an option.

Created a feature request for GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108761

[probinson]

Discussion on the GCC bug has persuaded me this is not a good idea. I'll solve my user's problem a different way.
@MaskRay you can close the GCC bug, it looks like I can't do it myself.

[probinson]

See D145173 for a different tactic to solve this.

clang/lib/CodeGen/CodeGenModule.cpp

if (D) {
     }
 
     if (auto *F = dyn_cast<llvm::Function>(GO)) {
       if (D->hasAttr<RetainAttr>())
         addUsedGlobal(F);
       if (auto *SA = D->getAttr<PragmaClangTextSectionAttr>())
         if (!D->getAttr<SectionAttr>())
           F->addFnAttr("implicit-section-name", SA->getName());
+      // If we have -ffunction-sections, and also an explicit section name,
+      // put the function into a section group so that various linker GC
+      // operations will still work with this function.
+      if (CodeGenOpts.FunctionSections && getTriple().supportsCOMDAT() &&
+          D->hasAttr<SectionAttr>()) {
+        // Don't replace a real comdat.
+        if (!F->getComdat()) {
+          llvm::Comdat *C = TheModule.getOrInsertComdat(F->getName());
+          C->setSelectionKind(llvm::Comdat::NoDeduplicate);
+          F->setComdat(C);
+        }
+      }
 
       llvm::AttrBuilder Attrs(F->getContext());
       if (GetCPUAndFeaturesAttributes(GD, Attrs)) {
         // We know that GetCPUAndFeaturesAttributes will always have the
         // newest set, since it has the newest possible FunctionDecl, so the
         // new ones should replace the old.
         llvm::AttributeMask RemoveAttrs;
         RemoveAttrs.addAttribute("target-cpu");

clang/test/CodeGen/section-attr-comdat.cpp

+// RUN: %clang_cc1 -triple=x86_64-linux -emit-llvm  %s -o - | \
+// RUN:     FileCheck %s --check-prefix=NOCOMDAT
+// RUN: %clang_cc1 -triple=x86_64-linux -emit-llvm -ffunction-sections %s -o - | \
+// RUN:     FileCheck %s --check-prefix=COMDAT
+
+// template function = comdat always.
+template<typename T>
+__attribute__((section("foo"))) T ftemplate(T a) { return a + 1; }
+__attribute__((section("foo"))) int fglobal(int a) { return ftemplate(a) + 2; }
+
+// NOCOMDAT-DAG: ${{.*}}ftemplate{{.*}} = comdat any
+// NOCOMDAT-DAG: define {{.*}}ftemplate{{.*}} section "foo" comdat {
+// NOCOMDAT-DAG: define {{.*}}fglobal{{.*}} section "foo" {
+
+// COMDAT-DAG: ${{.*}}ftemplate{{.*}} = comdat any
+// COMDAT-DAG: ${{.*}}fglobal{{.*}} = comdat nodeduplicate
+// COMDAT-DAG: define {{.*}}ftemplate{{.*}} section "foo" comdat {
+// COMDAT-DAG: define {{.*}}fglobal{{.*}} section "foo" comdat {