[PATCH] D143745: Make section attribute and -ffunction-sections play nicely

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

See D145173  for a different tactic to solve 
this.


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

https://reviews.llvm.org/D143745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143745: Make section attribute and -ffunction-sections play nicely

[Paul Robinson via Phabricator via cfe-commits]

probinson abandoned this revision.
probinson added a comment.

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.


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

https://reviews.llvm.org/D143745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143745: Make section attribute and -ffunction-sections play nicely

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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


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

https://reviews.llvm.org/D143745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143745: Make section attribute and -ffunction-sections play nicely

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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.


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

https://reviews.llvm.org/D143745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143745: Make section attribute and -ffunction-sections play nicely

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> [...] 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(i) {}
  __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.


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

https://reviews.llvm.org/D143745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143745: Make section attribute and -ffunction-sections play nicely

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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.


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

https://reviews.llvm.org/D143745

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143745: Make section attribute and -ffunction-sections play nicely

[Paul Robinson via Phabricator via cfe-commits]

probinson created this revision.
probinson added reviewers: rjmccall, MaskRay.
Herald added a project: All.
probinson requested review of this revision.

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.


https://reviews.llvm.org/D143745

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/section-attr-comdat.cpp


Index: clang/test/CodeGen/section-attr-comdat.cpp
===
--- /dev/null
+++ clang/test/CodeGen/section-attr-comdat.cpp
@@ -0,0 +1,18 @@
+// 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
+__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 {
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2293,6 +2293,18 @@
   if (auto *SA = D->getAttr())
 if (!D->getAttr())
   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()) {
+// 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)) {


Index: clang/test/CodeGen/section-attr-comdat.cpp
===
--- /dev/null
+++ clang/test/CodeGen/section-attr-comdat.cpp
@@ -0,0 +1,18 @@
+// 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
+__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 {
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2293,6 +2293,18 @@
   if (auto *SA = D->getAttr())
 if (!D->getAttr())
   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()) {
+// 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)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits