-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
x86_64: pass more tests #17788
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
x86_64: pass more tests #17788
Conversation
a4e7be8
to
b4c1579
Compare
self.rdata_section_index = try self.allocateSection(".rdata", file_size, .{ | ||
.CNT_INITIALIZED_DATA = 1, | ||
.MEM_READ = 1, | ||
.MEM_WRITE = 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rdata is supposed to be read-only isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to a linker bug mutable globals are ending up here. I don't understand this enough to open an issue, but @kubkon said to just do this for now.
For reference, the global I ran into this with is lib.test_runner.cmdline_buffer
, where the test program would crash on first write to this buffer, and only when the emitted code reaches a certain size (such as enabling n tests, or making unrelated changes to what code is emitted).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not rdata, it's .data.rel.ro
since we may have pointers inside a struct we lower in that section. The linker code path responsible for selecting output section doesn't do a recursive check for embedded pointers in any linker (macho, elf or coff), and this is important for PIC/PIE targets where the dynamic linker has to slide the address of each pointer within the binary - hence .data.rel.ro
which is only writeable during dynamic relocation startup phase - well, in theory at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyhow, as @jacobly0 mentioned, that was my suggestion to make it writeable to unblock his progress. I am aware of this nuance and will provide a fix in ELF first, and then backport it to other linkers as I start working on them.
This reduces the regression from 0.11.0 by 95%. Closes ziglang#17678
right-o, well, feel free to merge and save the stuff for follow-up changes if you want |
Depends on #17802
Closes #17678
Closes #17618