Ensure main thread's TLS alignment matches .tdata and .tbss#7
Merged
fincs merged 1 commit intodevkitPro:masterfrom Jun 2, 2023
Merged
Ensure main thread's TLS alignment matches .tdata and .tbss#7fincs merged 1 commit intodevkitPro:masterfrom
.tdata and .tbss#7fincs merged 1 commit intodevkitPro:masterfrom
Conversation
SIZEOF(.tdata) doesn't account for the extra alignment added to the
start of `.tbss`, but TLS offsets do account for that. As a result, we
can end up with misaligned reads/writes for uninitialized thread-locals.
Additionally, the .tdata and .tbss sections were using the form:
.section ALIGN(4) : { ... }
This forces the section to start at a 4-aligned address, even if the
section should have alignment greater than 4. By changing to the form
.section : ALIGN(4) { ... }
We can ensure a _minimum_ alignment of 4, but the section will still be
aligned properly if it requires a higher alignment.
.tbss.tdata and .tbss
Member
|
Thank you for this PR, and especially the detailed writeup. Regarding section alignment, you are correct. Ideally we should not be forcefully assigning a start address to each section, and instead we should be using the output alignment attribute. I am always interested in reviewing all our linkscripts currently in use in order to identify/fix potential issues with them, however that is probably out of scope here. For the moment, I think the minimal fix proposed in this PR should be sufficient. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hope it's okay I didn't open a separate issue for this, since I was able to figure out a fix while investigating the root cause.
This bug was possibly introduced by my previous PR #6 since that affected the layout / position of the main thread's thread-local storage.
The main thread's TLS block was reserved as
SIZEOF(.tdata) + SIZEOF(.tbss), but this doesn't account for extra alignment that may have been added before the beginning of the.tbsssection (if the size of.tdataisn't perfectly aligned).As a result, we can end up with misaligned reads/writes for thread-locals that are part of
.tbss.Example
Unfortunately, I haven't found a very good minimal reproduction of this issue. The program this first occurred in fails a debug assertion to an unaligned read of a thread-local variable.
Here's an abbreviated view of the relevant symbols in that program, where
__KEYhas a minimum alignment of 8:At runtime, this resulted in the address of
__KEYbeing0x287afc(__tls_start+0xc44), but it should be 8-aligned instead.The fix
Basically, just copy the alignment exactly from
.tdataand.tbsswhen laying out the main thread's TLS block, i.e.__tls_startand__tls_end(still subtracting 8 for TLS header).This ensures the main thread's TLS is aligned the same way as the the
.tdatatemplate, and also accounts for any padding that was added to align.tbss.I have been able to verify that the changes made here resolve it. With the new linker script, the offsets look correct again:
With this layout, the address of
__KEYis0x287b08(__tls_start+0xc48), which is correctly aligned.As far as I know, this PR requires no changes to
libctru, since the appropriate symbols are still defined and the pointer math still works the same way.Section alignment
Separate but related, the
.tdataand.tbsssections were using the form:This forces the section to start at a 4-aligned address, even if the section should have alignment greater than 4. By changing to the attribute form:
We can ensure a minimum alignment of 4, but the section will still be aligned properly if it requires a greater alignment.
I'm not sure if this same change should be applied to all other sections using the
.section [address] :form, but it was necessary to get the.tdataand.tbsssections to have the correct alignment in the first place, allowing us to useALIGNOF()when laying out the main thread's TLS.