fix: do not allocate space for uninitialized tls variables#1788
Conversation
|
Hi @ostylk. Do you intend to return to this PR? |
|
Hi, thank you for the reminder, this PR slipped my mind. |
|
All good, I was just checking in on PRs that hadn't been touched in a while. I don't think there's any particular hurry on this though. |
|
Ready for review but I have some comments/questions:
|
|
Welcome back! Regarding the CI failure - RexMovIndirectToAbsolute vs MovIndirectToLea, that sounds like something that could be caused by different defaults as to whether the binary is position independent or not. So possibly the difference might go away if you override those defaults - e.g. by passing a flag to make both linkers produce a position-independent binary. The PR in which we started putting zero bytes in the file for .tbss was #489. It unfortunately looks like I didn't add a test for it, so it's not impossible that something around there could have regressed. |
4f37e80 to
37b1060
Compare
|
Okay, so the linker failure seems to be due to lld behaving differently to GNU ld on openSUSE. While I am unsure what the exact reason is, the startup files ( while lld relaxes to this (effectively ignoring pie/no-pie stuff) So ultimately I think it is best to ignore that difference. GNU ld and wild switch to absolute addresses with no-pie files while lld ignores that fact completely. |
davidlattimore
left a comment
There was a problem hiding this comment.
Thanks for working on this. The change looks good, so I'll merge it. Before I do though, how do you want this to appear in the release notes? Can you update the PR title to read as a line from the release notes, then prefix it with "fix:" or "feat:" depending on whether you want it in the bug fixes section or the features section.
|
Done and thank you for your patience with this PR. |
I noticed the tls-common test failed on my machine depending on which environment and versions of compilers I used. It seems like the
TLSprogram header is generated incorrectly for zero initialized tls variables.E.g. in the tls-common we have a variable
tvarthat is in.tbss. However, the TLS program header isfilesz=4,memsz=4which doesn't zero-initialize it but loads bytes from the elf binary. It just so happens that all CIs and your local setup contained zeros in the binary at the exact place for this to nevertheless work. The correct TLS program header would befilesz=0,memsz=4.So this pull request does/will do two things:
//#ExpectProgramHeader:PT_TLS filesz=0,memsz=4and it will only accept if a program header with at least these properties exists. Implemented by fix: Don't count .tbss size towards TLS segment file size #2190 .