Skip to content

[ELF] Implement --force-group-allocation #94704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jun 7, 2024

GNU ld's relocatable linking behaviors:

  • Sections with the SHF_GROUP flag are handled like sections matched
    by the --unique=pattern option. They are processed like orphan
    sections and ignored by input section descriptions.
  • Section groups' (usually named .group) content is updated as the
    section indexes are updated. Section groups can be discarded with
    /DISCARD/ : { *(.group) }.

-r --force-group-allocation discards section groups and allows
sections with the SHF_GROUP flag to be matched like normal sections.
If two section group members are placed into the same output section,
their relocation sections (if present) are combined as well.
This behavior can be useful when -r output is used as a pseudo shared
object (e.g., FreeBSD's amd64 kernel modules, CHERIoT compartments).

This patch implements --force-group-allocation:

  • Input SHT_GROUP sections are discarded.
  • Input sections do not get the SHF_GROUP flag, so addInputSec
    will combine relocation sections if their relocated section group
    members are combined.

The default behavior is:

  • Input SHT_GROUP sections are retained.
  • Input SHF_GROUP sections can be matched (unlike GNU ld)
  • Input SHF_GROUP sections keep the SHF_GROUP flag, so addInputSec
    will create different OutputDesc copies.

GNU ld provides the FORCE_GROUP_ALLOCATION command, which is not
implemented.

Created using spr 1.3.5-bogner
@MaskRay MaskRay requested a review from smithp35 June 7, 2024 00:17
@MaskRay MaskRay requested a review from igorkudrin June 7, 2024 00:17
@llvmbot
Copy link
Member

llvmbot commented Jun 7, 2024

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Fangrui Song (MaskRay)

Changes

GNU ld's relocatable linking behaviors:

  • Sections with the SHF_GROUP flag are handled like sections matched
    by the --unique=pattern option. They are processed like orphan
    sections and ignored by input section descriptions.
  • Section groups' (usually named .group) content is updated as the
    section indexes are updated. Section groups can be discarded with
    /DISCARD/ : { *(.group) }.

-r --force-group-allocation discards section groups and allows
sections with the SHF_GROUP flag to be matched like normal sections.
If two section group members are placed into the same output section,
their relocation sections (if present) are combined as well.
This behavior can be useful when -r output is used as a pseudo shared
object (e.g., FreeBSD's amd64 kernel modules, CHERIoT compartments).

This patch implements --force-group-allocation:

  • Input SHT_GROUP sections are discarded.
  • Input sections do not get the SHF_GROUP flag, so addInputSec
    will combine relocation sections if their relocated section group
    members are combined.

--inhibit-group-allocation restores the default behavior:

  • Input SHT_GROUP sections are retained.
  • Input SHF_GROUP sections can be matched (unlike GNU ld)
  • Input SHF_GROUP sections keep the SHF_GROUP flag, so addInputSec
    will create different OutputDesc copies.

GNU ld provides the FORCE_GROUP_ALLOCATION command, which is not
implemented.


Full diff: https://github.com/llvm/llvm-project/pull/94704.diff

7 Files Affected:

  • (modified) lld/ELF/Config.h (+1)
  • (modified) lld/ELF/Driver.cpp (+3)
  • (modified) lld/ELF/InputFiles.cpp (+1-1)
  • (modified) lld/ELF/InputSection.cpp (+5-4)
  • (modified) lld/ELF/Options.td (+5)
  • (modified) lld/docs/ld.lld.1 (+4)
  • (modified) lld/test/ELF/relocatable-comdat.s (+12)
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 883c4a2f84294..0173be396163e 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -286,6 +286,7 @@ struct Config {
   bool relax;
   bool relaxGP;
   bool relocatable;
+  bool resolveGroups;
   bool relrGlibc = false;
   bool relrPackDynRelocs = false;
   llvm::DenseSet<llvm::StringRef> saveTempsArgs;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index ddc574a11314b..e8442cc993821 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1384,6 +1384,9 @@ static void readConfigs(opt::InputArgList &args) {
   config->relaxGP = args.hasFlag(OPT_relax_gp, OPT_no_relax_gp, false);
   config->rpath = getRpath(args);
   config->relocatable = args.hasArg(OPT_relocatable);
+  config->resolveGroups = !args.hasArg(OPT_relocatable) ||
+                          args.hasFlag(OPT_force_group_allocation,
+                                       OPT_inhibit_group_allocation, false);
 
   if (args.hasArg(OPT_save_temps)) {
     // --save-temps implies saving all temps.
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index d760dddcf5ec5..9021bbd91b5f7 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -676,7 +676,7 @@ template <class ELFT> void ObjFile<ELFT>::parse(bool ignoreComdats) {
         symtab.comdatGroups.try_emplace(CachedHashStringRef(signature), this)
             .second;
     if (keepGroup) {
-      if (config->relocatable)
+      if (!config->resolveGroups)
         this->sections[i] = createInputSection(
             i, sec, check(obj.getSectionName(sec, shstrtab)));
       continue;
diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index e6c5996c0b392..0f176f144d46b 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -76,12 +76,13 @@ InputSectionBase::InputSectionBase(InputFile *file, uint64_t flags,
     invokeELFT(parseCompressedHeader,);
 }
 
-// Drop SHF_GROUP bit unless we are producing a re-linkable object file.
-// SHF_GROUP is a marker that a section belongs to some comdat group.
-// That flag doesn't make sense in an executable.
+// SHF_INFO_LINK and SHF_GROUP are normally resolved and not copied to the
+// output section. However, for relocatable linking with the default
+// --inhibit-group-allocation, the SHF_GROUP marker and section groups are
+// retained.
 static uint64_t getFlags(uint64_t flags) {
   flags &= ~(uint64_t)SHF_INFO_LINK;
-  if (!config->relocatable)
+  if (config->resolveGroups)
     flags &= ~(uint64_t)SHF_GROUP;
   return flags;
 }
diff --git a/lld/ELF/Options.td b/lld/ELF/Options.td
index ff61a566f52f7..23385c76182eb 100644
--- a/lld/ELF/Options.td
+++ b/lld/ELF/Options.td
@@ -254,6 +254,11 @@ def fix_cortex_a53_843419: F<"fix-cortex-a53-843419">,
 def fix_cortex_a8: F<"fix-cortex-a8">,
   HelpText<"Apply fixes for ARM Cortex-A8 erratum 657417">;
 
+def force_group_allocation: FF<"force-group-allocation">,
+  HelpText<"Only meaningful for -r. Section groups are discarded. If two section group members are combined into the same output section, combine their relocations as well">;
+def inhibit_group_allocation: FF<"inhibit-group-allocation">,
+  HelpText<"This is the default for -r. Section groups are retained. Section group members' relocations are not combined">;
+
 defm format: Eq<"format", "Change the input format of the inputs following this option">,
   MetaVarName<"[default,elf,binary]">;
 
diff --git a/lld/docs/ld.lld.1 b/lld/docs/ld.lld.1
index da3b926d02a28..82d590a1d084f 100644
--- a/lld/docs/ld.lld.1
+++ b/lld/docs/ld.lld.1
@@ -278,6 +278,10 @@ Set the
 field to the specified value.
 .It Fl -fini Ns = Ns Ar symbol
 Specify a finalizer function.
+.It Fl -force-group-allocation
+Only meaningful for -r. Section groups are discarded. If two section group members are combined into the same output section, combine their relocations as well.
+.It Fl -inhibit-group-allocation
+This is the default for -r. Section groups are retained. Section group members' relocations are not combined.
 .It Fl -format Ns = Ns Ar input-format , Fl b Ar input-format
 Specify the format of the inputs following this option.
 .Ar input-format
diff --git a/lld/test/ELF/relocatable-comdat.s b/lld/test/ELF/relocatable-comdat.s
index 45ca9fb7a2484..8ca8f107f885e 100644
--- a/lld/test/ELF/relocatable-comdat.s
+++ b/lld/test/ELF/relocatable-comdat.s
@@ -47,6 +47,18 @@
 # COMBINE-NEXT: .rela.text
 # COMBINE-NEXT: .rela.text
 
+## If --force-group-allocation is specified, discard .group and combine .rela.* if their relocated sections are combined.
+# RUN: ld.lld -r -T combine.lds a.o a.o --force-group-allocation -o combine-a.ro
+# RUN: llvm-readelf -g -S combine-a.ro | FileCheck %s --check-prefix=COMBINE-A
+## --inhibit-group-allocation restores the default behavior.
+# RUN: ld.lld -r -T combine.lds a.o a.o --force-group-allocation --inhibit-group-allocation -o - | cmp - combine.ro
+
+# COMBINE-A:      Name            Type     Address          Off    Size   ES Flg Lk    Inf   Al
+# COMBINE-A:      .rodata         PROGBITS 0000000000000000 {{.*}} 000002 00   A  0      0    1
+# COMBINE-A-NEXT: .text           PROGBITS 0000000000000000 {{.*}} 000010 00  AX  0      0    4
+# COMBINE-A-NEXT: .rela.text      RELA     0000000000000000 {{.*}} 000030 18   I [[#]] [[#]]  8
+# COMBINE-A-NEXT: .note.GNU-stack
+
 # RUN: echo 'SECTIONS { /DISCARD/ : {*(.rodata.*)} }' > discard-rodata.lds
 # RUN: ld.lld -r -T discard-rodata.lds a.o a.o -o discard-rodata.ro
 # RUN: llvm-readelf -g -S discard-rodata.ro | FileCheck %s --check-prefix=NO-RODATA

@MaskRay MaskRay requested a review from justincady June 7, 2024 00:18
Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM, I'm not sure we need --inhibit-group-allocation although I could have missed a use case.

@@ -1384,6 +1384,9 @@ static void readConfigs(opt::InputArgList &args) {
config->relaxGP = args.hasFlag(OPT_relax_gp, OPT_no_relax_gp, false);
config->rpath = getRpath(args);
config->relocatable = args.hasArg(OPT_relocatable);
config->resolveGroups = !args.hasArg(OPT_relocatable) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I've understood the benefit of --inhibit-group-allocation.

  • For a non relocatable link it is ignored.
  • For a relocatable link its behaviour is the default state, so there is no need to explicitly use it.

The only reason I can think to use it if there is an existing linker command-line that with --force-group-allocation that I want to disable. I don't think that is likely though.

Unless I've missed something, would it be better to keep the command-line interface simple with

config->resolveGroups = !args.hasArg(OPT_relocatable) ||
args.hasFlag(OPT_force_group_allocation, false);

I couldn't find it in the GNU ld documentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that --inhibit-group-allocation could be kept to document the default behavior. I agree that it's not useful and I will remove it.

@justincady
Copy link
Contributor

First, thank you so much for doing this. :)

I applied this patch to 17.0.6 to compare behavior. The group sections portion appears to work as expected, but I'm confused by one aspect of the relocation section combining:

$ cat b.h
#pragma once
struct A {
    A();
    virtual ~A();
    virtual int foo();
};
struct B : public A {
        int foo() override;
};

$ cat b.cc
#include "b.h"
int A::foo() { return 42; }
int B::foo() { return 84; }

$ cat ldscript.amd64
SECTIONS { .text : { *(.text .stub .text.* .gnu.linkonce.t.*) } }

$ cat rela.sh
#!/bin/bash
clang -c b.cc
ld.lld -r --force-group-allocation -T ldscript.amd64 b.o -o rela.fga.ro

$ ./rela.sh
$ llvm-readelf -S rela.fga.ro
There are 14 section headers, starting at offset 0x5e8:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        0000000000000000 000040 000068 00  AX  0   0 16
  [ 2] .rela.text._ZN1BD2Ev RELA         0000000000000000 000188 000048 18   I 11   1  8 <--------------------
  [ 3] .data.rel.ro      PROGBITS        0000000000000000 0000a8 000040 00  WA  0   0  8
  [ 4] .rela.data.rel.ro RELA            0000000000000000 0001d0 0000a8 18   I 11   3  8
  [ 5] .rodata           PROGBITS        0000000000000000 0000e8 000003 00   A  0   0  1
  [ 6] .comment          PROGBITS        0000000000000000 000278 000016 01  MS  0   0  1
  [ 7] .eh_frame         X86_64_UNWIND   0000000000000000 0000f0 000098 00   A  0   0  8
  [ 8] .rela.eh_frame    RELA            0000000000000000 000290 000060 18   I 11   7  8
  [ 9] .llvm_addrsig     LLVM_ADDRSIG    0000000000000000 0002f0 000005 00   E  0   0  1
  [10] .note.GNU-stack   PROGBITS        0000000000000000 0002f5 000000 00      0   0  1
  [11] .symtab           SYMTAB          0000000000000000 0002f8 0001c8 18     13   8  8
  [12] .shstrtab         STRTAB          0000000000000000 0004c0 00009d 00      0   0  1
  [13] .strtab           STRTAB          0000000000000000 00055d 000088 00      0   0  1

The combined RELA section gets the name .rela.text._ZN1BD2Ev; should it match the name of the combined section (.rela.text)?

This seems uncontrollable by linker script either, for example:

$ cat ldscript.amd64
SECTIONS {
  .text : { *(.text .stub .text.* .gnu.linkonce.t.*) }
  .rela.text : { *(.rela.text .rela.text.* .rela.gnu.linkonce.t.*) }
}
$ ./rela.sh
$ llvm-readelf -S rela.fga.ro
There are 14 section headers, starting at offset 0x5e8:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        0000000000000000 000040 000068 00  AX  0   0 16
  [ 2] .rela.text._ZN1BD2Ev RELA         0000000000000000 000188 000048 18   I 11   1  8
  [ 3] .data.rel.ro      PROGBITS        0000000000000000 0000a8 000040 00  WA  0   0  8
  [ 4] .rela.data.rel.ro RELA            0000000000000000 0001d0 0000a8 18   I 11   3  8
  [ 5] .rodata           PROGBITS        0000000000000000 0000e8 000003 00   A  0   0  1
  [ 6] .comment          PROGBITS        0000000000000000 000278 000016 01  MS  0   0  1
  [ 7] .eh_frame         X86_64_UNWIND   0000000000000000 0000f0 000098 00   A  0   0  8
  [ 8] .rela.eh_frame    RELA            0000000000000000 000290 000060 18   I 11   7  8
  [ 9] .llvm_addrsig     LLVM_ADDRSIG    0000000000000000 0002f0 000005 00   E  0   0  1
  [10] .note.GNU-stack   PROGBITS        0000000000000000 0002f5 000000 00      0   0  1
  [11] .symtab           SYMTAB          0000000000000000 0002f8 0001c8 18     13   8  8
  [12] .shstrtab         STRTAB          0000000000000000 0004c0 00009d 00      0   0  1
  [13] .strtab           STRTAB          0000000000000000 00055d 000088 00      0   0  1

@MaskRay
Copy link
Member Author

MaskRay commented Jun 7, 2024

First, thank you so much for doing this. :)

Np:)

I applied this patch to 17.0.6 to compare behavior. The group sections portion appears to work as expected, but I'm confused by one aspect of the relocation section combining:

You need commit b8dface and 0930f62 to make the .rela.* section name match its relocated section.

Static relocation sections copied to the output due to -r/--emit-relocs cannot be matched by input section descriptions.
.rela.text : { *(.rela.text .rela.text.* .rela.gnu.linkonce.t.*) } is ignored. I think GNU ld ignores them as well.

BTW, you can remove .gnu.linkonce.t.*. It's GNU linkonce sections, which predate COMDAT (available in the generic ABI as of April 2001). They have been obsoleted and unused for a long time.

Created using spr 1.3.5-bogner
Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update LGTM.

Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit 4d9020c into main Jun 7, 2024
5 of 7 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/elf-implement-force-group-allocation branch June 7, 2024 21:19
@justincady
Copy link
Contributor

You need commit b8dface and 0930f62 to make the .rela.* section name match its relocated section.

Thanks, that worked. I should have tested with head of line initially.

LGTM!

Copy link
Contributor

@justincady justincady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

nekoshirro pushed a commit to nekoshirro/Alchemist-LLVM that referenced this pull request Jun 9, 2024
GNU ld's relocatable linking behaviors:

* Sections with the `SHF_GROUP` flag are handled like sections matched
  by the `--unique=pattern` option. They are processed like orphan
  sections and ignored by input section descriptions.
* Section groups' (usually named `.group`) content is updated as the
  section indexes are updated. Section groups can be discarded with
  `/DISCARD/ : { *(.group) }`.

`-r --force-group-allocation` discards section groups and allows
sections with the `SHF_GROUP` flag to be matched like normal sections.
If two section group members are placed into the same output section,
their relocation sections (if present) are combined as well.
This behavior can be useful when -r output is used as a pseudo shared
object (e.g., FreeBSD's amd64 kernel modules, CHERIoT compartments).

This patch implements --force-group-allocation:

* Input SHT_GROUP sections are discarded.
* Input sections do not get the SHF_GROUP flag, so `addInputSec`
  will combine relocation sections if their relocated section group
  members are combined.

The default behavior is:

* Input SHT_GROUP sections are retained.
* Input SHF_GROUP sections can be matched (unlike GNU ld)
* Input SHF_GROUP sections keep the SHF_GROUP flag, so `addInputSec`
  will create different OutputDesc copies.

GNU ld provides the `FORCE_GROUP_ALLOCATION` command, which is not
implemented.

Pull Request: llvm#94704

Signed-off-by: Hafidz Muzakky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants