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

elf2flt: add riscv 64-bits support v2 #24

Closed
wants to merge 4 commits into from

Conversation

floatious
Copy link
Contributor

Hello Mike,

Since I haven't seen any feedback on:
#22

I'm opening a new PR with the latest and greatest.

Kind regards,
Niklas

floatious and others added 3 commits August 12, 2022 11:02
Commit 8a3e744 ("allow to build arm flat binaries") moved the
following commands:
	. = ALIGN(0x20) ;
	@SYMBOL_PREFIX@_etext = . ;
from the .text section to the top level in the SECTIONS node.

The .text output section is being directed to a memory region using the
"> flatmem :text" output section attribute. Commands in the top level in
the SECTIONS node are not.

This means that the ALIGN() command is no longer being appended to the
flatmem memory region, it will simply update the Location Counter.

The heuristic for placing an output section is described here:
https://sourceware.org/binutils/docs-2.38/ld.html#Output-Section-Address

"If an output memory region is set for the section then it is added to this
region and its address will be the next free address in that region."

Since the .data section is being directed to the same memory region as the
.text section, this means that the Location Counter is not used when
assigning an address to the .data output section, it will simply use the
next free address.

No longer directing these commands to the flatmem memory region means that
the .data output section is no longer aligned to a 32 byte boundary.

Before commit 8a3e744 ("allow to build arm flat binaries"):
$ readelf -S busybox_unstripped.gdb | grep data
  [ 3] .data             PROGBITS         0000000000035ac0  00036ac0
$ readelf -s busybox_unstripped.gdb | grep _etext
 19286: 0000000000035ac0     0 NOTYPE  GLOBAL DEFAULT    1 _etext

After commit 8a3e744 ("allow to build arm flat binaries"):
$ readelf -S busybox_unstripped.gdb | grep data
  [ 3] .data             PROGBITS         0000000000035ab0  00036ab0
$ readelf -s busybox_unstripped.gdb | grep _etext
 19287: 0000000000035ac0     0 NOTYPE  GLOBAL DEFAULT    3 _etext

The .data output section has to be aligned to a 32 byte boundary, see the
FLAT_DATA_ALIGN 0x20 macro and its usage in fs/binfmt_flat.c:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/binfmt_flat.c?h=v5.17#n59

Readd an explicit ALIGN attribute on the .data section itself, since the
linker will obey this attribute regardless if being directed to a memory
region or not. Also remove the ALIGN() command before the .data section,
since this misleads the reader to think that the Location Counter is used
when assigning an address to the .data section, when it actually is not.

Fixes: 8a3e744 ("allow to build arm flat binaries")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Add support for riscv 64bits ISA by defining the relocation types
R_RISCV_32_PCREL, R_RISCV_ADD32, R_RISCV_SUB32, R_RISCV_32 and
R_RISCV_64. riscv64 support also needs the __global_pointer$ symbol to
be defined right after the relocation tables in the data section. To
define this symbol, the "RISCV_GP" line prefix is added. The "RISCV_GP"
string is removed if the target CPU type is riscv64 and the definition
line is dropped for other CPU types.

With these changes, buildroot and busybox build and run on riscv NOMMU
systems with Linux kernel including patch 6045ab5fea4c
("binfmt_flat: do not stop relocating GOT entries prematurely on riscv")
fixing the binfmt_flat loader. Tested on QEMU and Canaan Kendryte K210
boards.

This patch is based on earlier work by Christoph Hellwig <hch@lst.de>.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
In order to make the code more maintainable,
move duplicated code to a common helper function.

No functional change intended.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
@cnlohr
Copy link

cnlohr commented Nov 19, 2022

I could really use this (and to add RISC-V 32-bit as well). @floatious , Have you contacted the mailing list, yet uclinux-dev@uclinux.org

Commit ba379d0 ("elf2flt: fix for segfault on some ARM ELFs")
changed the condition of which input sections that should be included
in the .text output section from:
((a->flags & (SEC_DATA | SEC_READONLY)) == (SEC_DATA | SEC_READONLY))
to:
((a->flags & (SEC_DATA | SEC_READONLY | SEC_RELOC)) ==
(SEC_DATA | SEC_READONLY | SEC_RELOC))

On ARM, the .eh_frame input section does not have the SEC_RELOC flag set,
so on ARM, this change caused .eh_frame to move from .text to .data.

However, on e.g. m68k, xtensa and riscv64, the .eh_frame input section
does have the SEC_RELOC flag set, which means that the change in
commit ba379d0 ("elf2flt: fix for segfault on some ARM ELFs")
caused .eh_frame to move in an opposite way, i.e. from .data to .text.

This resulted in a fatal error on m68k, xtensa and riscv64:
ERROR: text=0x3bab8 overlaps data=0x33f60 ?

This is because elf2flt cannot append to .text after .data has been
appended to.

Note that the binutils maintainer says that the correct thing is
to put read-only relocation data sections in .text:
https://sourceware.org/legacy-ml/binutils/2019-10/msg00132.html

So the proper fix is probably to rewrite elf2flt so that it can append
to .text after .data has been appended to (which will require elf2flt
to move/relocate everything that has already been appended to .data,
since the virtual addresses are contiguous).

However, for now, add an exception for input sections which have all
three flags SEC_DATA, SEC_READONLY, and SEC_RELOC set, and which have a
name equal to a problematic input section (.eh_frame, .gcc_except_table).
That way, we get the same behavior as older elf2flt releases for m68k,
xtensa and riscv64, where we put read-only relocation data in .data,
which was working perfectly fine.

This exception will not change any behavior on ARM, as the .eh_frame
input section does not have flag SEC_RELOC set.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
@cnlohr
Copy link

cnlohr commented Mar 25, 2023

@floatious are you up for also pulling in the RV32 changes here?

5330503

@franzflasch
Copy link

This needs a rebase. Anyone?

@JAicewizard
Copy link

This doesn't seem to work compiling a very small program:

$ cat main.c
#include <stdio.h>

void main(){
	printf("HEY");
}
$ riscv64-linux-gnu-gcc main.c -o a.out -r &&  ../elf2flt/elf2flt a.out -v
TEXT -> vma=0x0 len=0x22
DATA -> vma=0x0 len=0x4
../elf2flt/elf2flt: ERROR: text=0x22 overlaps data=0x0 ?

Am I doing anything wrong? I have not worked with elf2flt before. I also tried using it as a linker within GCC, but this produced similar errors (overlapping sections, although different ones).

@JAicewizard
Copy link

For future reference, my problems were 2/3(or even 4) fold:
1: pie is not supported
2: build-id is not supported
(3): glibc is not supported, which was kind of to be expected but part of the problem
(4): to actually get a working binary on my system I needed to provide "-Wl,-elf2flt=-r" instead of just "-Wl,-elf2flt"

@wbx-github
Copy link
Contributor

the following patch might be useful for some applications to compile properly:

diff --git a/elf2flt.c b/elf2flt.c
index 6685bff..6b3bea4 100644
--- a/elf2flt.c
+++ b/elf2flt.c
@@ -850,11 +850,21 @@ output_relocs (
                                default:
                                        goto bad_resolved_reloc;
 #elif defined(TARGET_riscv64)
+                               case R_RISCV_NONE:
                                case R_RISCV_32_PCREL:
+                               case R_RISCV_ADD8:
+                               case R_RISCV_ADD16:
                                case R_RISCV_ADD32:
                                case R_RISCV_ADD64:
+                               case R_RISCV_SUB6:
+                               case R_RISCV_SUB8:
+                               case R_RISCV_SUB16:
                                case R_RISCV_SUB32:
                                case R_RISCV_SUB64:
+                               case R_RISCV_SET6:
+                               case R_RISCV_SET8:
+                               case R_RISCV_SET16:
+                               case R_RISCV_SET32:
                                        continue;
                                case R_RISCV_32:
                                case R_RISCV_64:

arnout pushed a commit to buildroot/buildroot that referenced this pull request Aug 7, 2023
See here the comment:
uclinux-dev/elf2flt#24

Fixes following autobuild failure:
http://autobuild.buildroot.net/results/c16/c161f191d85f9d064626ee6fcfebea61d916e434/

Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
@gregungerer
Copy link
Contributor

With the recent fixes to the main branch of elf2flt I think this needs a refresh.
My testing shows that really only the single RISC-V nommu patch is now required (that was testing with gcc-13.2, bintutils-2.41 and linux-6.4 - though I tried a couple of older versions of all these as well). It definitely needs Waldemar's update as well to add more of the relocation types.

Can anyone comment on the patch that re-instates the 32-byte alignment? I did not need it for the test cases I tried.

@franzflasch
Copy link

@gregungerer I can test this for RISC-V nommu (64 Bit). But I am not sure if I can follow. You mean now this should work with the latest gcc, binutils and linux 6.4? And the only thing missing would be the RISC-V nommu patch from @damien-lemoal?

@floatious
Copy link
Contributor Author

floatious commented Aug 28, 2023

Can anyone comment on the patch that re-instates the 32-byte alignment? I did not need it for the test cases I tried.

Hello Greg,

looking at the commit message:
90d0c81

The problem was that the ALIGN(0x20) was not performed inside an output section that was being redirected to the flatmem region.

Looking at your commit:
a934fb4

You changed so that the ALIGN(0x20) is once again performed inside an output section that is being redirected to the flatmem region, so the patch that adds the 0x20 alignment should no longer be necessary.

I would argue for extreme caution with having align attributes that are not inside an output section that is being redirected though, as that will most likely reintroduce bugs.

An example seems to be:
https://github.com/uclinux-dev/elf2flt/blob/main/elf2flt.ld.in#L197-L201

I'm quite sure that this can lead to the edata symbol being aligned to 0x10,
and then, bss being aligned to 0x4, but since edata wasn't redirected to flatmem,
edata could be assigned an address that is higher than the address assigned to bss.
(i.e. bss could be assigned an address that is lower than the address assigned to the edata symbol.)

@gregungerer
Copy link
Contributor

gregungerer commented Aug 28, 2023 via email

@gregungerer
Copy link
Contributor

RISC-V support has been merged into main in later versions of this PR.

@gregungerer gregungerer closed this Sep 6, 2023
@cnlohr
Copy link

cnlohr commented Sep 18, 2023

Thank you.

citral23 pushed a commit to citral23/buildroot that referenced this pull request Sep 18, 2023
See here the comment:
uclinux-dev/elf2flt#24

Fixes following autobuild failure:
http://autobuild.buildroot.net/results/c16/c161f191d85f9d064626ee6fcfebea61d916e434/

Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants