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

Ensure main thread's TLS alignment matches .tdata and .tbss #7

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

ian-h-chamberlain
Copy link
Contributor

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 .tbss section (if the size of .tdata isn'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 __KEY has a minimum alignment of 8:

 29020: 00000c44    32 TLS     GLOBAL DEFAULT   16 __KEY
 25674: 00284bf4     0 NOTYPE  GLOBAL DEFAULT   15 __tdata_lma
    10: 00284bf4     0 SECTION LOCAL  DEFAULT   15 .tdata
 32070: 00284c08     0 NOTYPE  GLOBAL DEFAULT   15 __tdata_lma_end
    11: 00284c08     0 SECTION LOCAL  DEFAULT   16 .tbss
 31346: 00286eb8     0 NOTYPE  GLOBAL DEFAULT   21 __tls_start
 26775: 00287b1c     0 NOTYPE  GLOBAL DEFAULT   21 __tls_end

At runtime, this resulted in the address of __KEY being 0x287afc (__tls_start+0xc44), but it should be 8-aligned instead.

The fix

Basically, just copy the alignment exactly from .tdata and .tbss when laying out the main thread's TLS block, i.e. __tls_start and __tls_end (still subtracting 8 for TLS header).

This ensures the main thread's TLS is aligned the same way as the the .tdata template, 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:

 29020: 00000c48    32 TLS     GLOBAL DEFAULT   16 __KEY
 25674: 00284bf8     0 NOTYPE  GLOBAL DEFAULT   15 __tdata_lma
    10: 00284bf8     0 SECTION LOCAL  DEFAULT   15 .tdata
 32070: 00284c0c     0 NOTYPE  GLOBAL DEFAULT   15 __tdata_lma_end
    11: 00284c10     0 SECTION LOCAL  DEFAULT   16 .tbss
 31346: 00286ec0     0 NOTYPE  GLOBAL DEFAULT   21 __tls_start
 26775: 00287b28     0 NOTYPE  GLOBAL DEFAULT   21 __tls_end

With this layout, the address of __KEY is 0x287b08 (__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 .tdata and .tbss sections were using the form:

.tdata 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 attribute form:

.tdata : ALIGN(4)
{
	/* ... */
}

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 .tdata and .tbss sections to have the correct alignment in the first place, allowing us to use ALIGNOF() when laying out the main thread's TLS.

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.
@ian-h-chamberlain ian-h-chamberlain changed the title Ensure main thread "tbss" alignment matches .tbss Ensure main thread's TLS alignment matches .tdata and .tbss Jun 1, 2023
@fincs
Copy link
Member

fincs commented Jun 2, 2023

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.

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