Skip to content
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

Changes to enable linking with LLVM/lld #173

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

heshamelmatary
Copy link
Contributor

No description provided.

@heshamelmatary
Copy link
Contributor Author

The rv32imac,gcc failure is likely due to old Binutils installed on the test machines. This could either be fixed by upgrading Binutils there, or maybe use "-z nostart-stop-gc" (as per the commit message) to the linker flags.

@@ -24,7 +18,6 @@ SECTIONS
*(.text.start)
}
}
INSERT BEFORE .text;

SECTIONS
{
Copy link
Member

Choose a reason for hiding this comment

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

What is the new section table layout after these changes?

This is what currently seems to be produced:

  0 .start        00000034  85000000  85000000  00010000  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .text         000073c4  85001000  85001000  00011000  2**12
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  2 .rodata       00000ec7  850083c4  850083c4  000183c4  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 .ARM.exidx    00000008  8500928c  8500928c  0001928c  2**2
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  4 .bss          00010048  8500c000  8500c000  00019294  2**14
                  ALLOC
  5 ._archive_cpio 01842a00  8501c048  8501c048  0002c048  2**0
                  CONTENTS, ALLOC, LOAD, DATA
  6 .data         0000002c  8686ea48  8686ea48  0187ea48  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  7 _driver_list  00000030  8686ea74  8686ea74  0187ea74  2**2
                  CONTENTS, ALLOC, LOAD, DATA

Copy link
Contributor Author

@heshamelmatary heshamelmatary Jul 27, 2023

Choose a reason for hiding this comment

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

lld (unlike ld.bfd) does place .rodata before .text, the layout after the changes are:

[ 1] .rodata.str1.1    PROGBITS        0000000040a11000 001000 000b45 01 AMS  0   0  1
 [ 2] .rodata           PROGBITS        0000000040a11b48 001b48 000668 00   A  0   0  8
 [ 3] .start            PROGBITS        0000000040a121b0 0021b0 00002c 00  AX  0   0  1
 [ 4] .text             PROGBITS        0000000040a13000 003000 007214 00  AX  0   0 4096
 [ 5] .bss              NOBITS          0000000040a1b000 00b000 006090 00  WA  0   0 4096
 [ 6] ._archive_cpio    PROGBITS        0000000040a21090 011090 486c00 00  WA  0   0  1
 [ 7] _driver_list      PROGBITS        0000000040ea7c90 497c90 000060 00 WAR  0   0  8
 [ 8] .data             PROGBITS        0000000040ea7cf0 497cf0 000150 00  WA  0   0  8

@@ -12,7 +12,7 @@ SECTIONS
*(.tdata .tdata.* .gnu.linkonce.td.*)
_tdata_end = . ;
}
.tbss :
.tbss (NOLOAD):
Copy link
Member

Choose a reason for hiding this comment

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

This is already implicitly present in the current toolchain right?

  5 .tbss         0000002c  016c3b90  016c3b90  016b3b8f  2**3
                  ALLOC, THREAD_LOCAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's correct for lld too. I just added NOLOAD here for intentionality and to avoid potential bugs with other linkers and/or older ones.

@kent-mcleod
Copy link
Member

Err, I may be mixing up the meaning of (NOLOAD) in the linker script and the meaning of the ALLOC and LOAD flags in the section table, and what they mean about whether contents are allocated in the ELF file data, allocated in the program image and initialized by the loader (or zeroed). And then what the implicit rules are with .bss, .tbss and other special named sections in the GCC toolchains. Would you be able to add a summary @heshamelmatary ?

@heshamelmatary
Copy link
Contributor Author

Err, I may be mixing up the meaning of (NOLOAD) in the linker script and the meaning of the ALLOC and LOAD flags in the section table, and what they mean about whether contents are allocated in the ELF file data, allocated in the program image and initialized by the loader (or zeroed). And then what the implicit rules are with .bss, .tbss and other special named sections in the GCC toolchains. Would you be able to add a summary @heshamelmatary ?

Most linkers heuristically recognise common section names and apply the proper flags to them. .[t]bss sections for example should be recognised and linkers mark them ALLOC, NOLOAD, which means that they don't occupy space in the ELF image and shouldn't be loaded at run-time, but they should be allocated space at run-time in the process image when loaded.

Adding NOLOAD (similar to the seL4's kernel) is good for intentionality, and for linkers that might not automatically flag .[t]bss at NOLOAD. I've come across a bug in lld that actually allocated space in the ELF before, but I believe that got fixed.

I will add that explanation to the commit message.

@kent-mcleod kent-mcleod added hw-build enable all sel4test hardware builds hw-test enable sel4test hardware builds + runs labels Aug 11, 2023
Like .bss, .tbss needs explicit NONLOAD to ensure they are not
allocated/loaded in the ELF image.

Sponsored by: DARPA.

Signed-off-by: Hesham Almatary <[email protected]>
unused attribute does not prevent the section from being
garbage-collected during link-time optimisation. This may trigger
undefined references errors to [__start|__stop]_driver_list symbols
that are expected to be emitted by the linker anyway.

Adding "retain" attribute makes sure that the section and its
associated symbols are kept regardless of linker's garbage
collection. Another fix could be adding "nostart-stop-gc" to the
linker flags, but since it is only one section (_driver_list)
where its __start/__stop symbols are references, adding retain to
it makes more sense. This additional functionality requires
binutils version 2.36 or later.

Sponsored by: DARPA.

Signed-off-by: Hesham Almatary <[email protected]>
Most linkers heuristically recognise common section names and
apply the proper flags to them. .[t]bss sections for example
should be recognised and linkers mark them ALLOC, NOLOAD,
which means that they don't occupy space in the ELF image
and shouldn't be loaded at run-time, but they should be
allocated space at run-time in the process image when loaded.

Adding NOLOAD (similar to the seL4's kernel) is good for
intentionality, and for linkers that might not automatically
flag .[t]bss at NOLOAD.

Sponsored by: DARPA.

Signed-off-by: Hesham Almatary <[email protected]>
Linker's operators precedence [1] which follows C makes "+" higher than
shifting "<<". Thus, ". + CONFIG_MAX_NODES" will be shifted by 12 in this
case, which is wrong and is not the intended behavior.
In lld (which properly implements C's operators precedence, unlike ld.bfd),
the arithmetic results in a significant value.

The outcome is either a huge ELF file size, or a linking error
(e.g., relocation R_AARCH64_ADR_PREL_PG_HI21 out of range).

This commit fixes the arithmetic and uses += which is more portable.

[1] https://sourceware.org/binutils/docs/ld.html

Sponsored by: DARPA.

Signed-off-by: Hesham Almatary <[email protected]>
This allows to use the same file for both Arm and RISC-V.
Furthermore, this new file is more portable and works with
both LLVM/lld and GNU's ld, unlike the removed Arm's one
which contains directives and sections that lld does not
handle (e.g., .interp, INSERT BEFORE .hash).

Signed-off-by: Hesham Almatary <[email protected]>
@heshamelmatary
Copy link
Contributor Author

I dropped custom Arm's linker handling for sections and directives and rather added a new commit that unifies the linker script between Arm and RISC-V to include fixes for both and to make it more portable to build with lld and ld

@heshamelmatary
Copy link
Contributor Author

Also I modified the "retain" attribute to check if the toolchain supports it or not as this may fail on older GCC versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hw-build enable all sel4test hardware builds hw-test enable sel4test hardware builds + runs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants