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.ld: reinstate 32 byte alignment for .data section #31

Conversation

gregungerer
Copy link
Contributor

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")

@gregungerer
Copy link
Contributor Author

This commit was in with the RISC-V changes originally. The core of the RISC-V changes have been merged into main. So that leaves this as the last piece to add in. I don't see any issues and I think it is the right thing to do.

@floatious
Copy link
Contributor

Perhaps remove the Fixes: tag, since after commit
a934fb4
this patch is no longer a strict fix.

In your commit you changed so that the ALIGN(0x20) is once again performed inside an output section that is being redirected to the flatmem region, thus this patch is no longer strictly necessary.

That said, having the ALIGN attribute on the output section declaration itself, like this patch does:

.data ALIGN(0x20): {

is safer, as it means that the output section will be aligned regardless if it is redirected or not,
so it is safer in case the code is changed once again in the future.

@gregungerer gregungerer force-pushed the gregungerer/reinstate-32-byte-alignment branch from 263e85c to 38f0c3a Compare September 6, 2023 22:52
@gregungerer
Copy link
Contributor Author

Agreed. I updated the commit message with some text (based on your comments) and removed the fixed-by tag.

I don't have any test cases where this change is required. But it is a better solution to aligning the data section.

Copy link
Contributor

@floatious floatious left a comment

Choose a reason for hiding this comment

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

"
That will protect

Signed-off-by: Greg Ungerer [email protected]
"

This final sentence before the SoB seems to end prematurely.

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.

Signed-off-by: Niklas Cassel <[email protected]>

As Niklas points out this patch is no longer strictly necessary.

That being said, having the ALIGN attribute on the output of the data
section declaration itself is safer, as it means that the output section
will be aligned regardless if it is redirected or not.

Signed-off-by: Greg Ungerer <[email protected]>
@gregungerer gregungerer force-pushed the gregungerer/reinstate-32-byte-alignment branch from 38f0c3a to 6cd580f Compare September 7, 2023 07:52
@gregungerer
Copy link
Contributor Author

Doh! Cleaned up now. Thanks.

@gregungerer gregungerer merged commit 679c94a into uclinux-dev:main Sep 12, 2023
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.

2 participants