-
-
Notifications
You must be signed in to change notification settings - Fork 77
Make --update-in-place default and fallback to unlinking on ETXTBSY #1238
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
Conversation
Also allow TestUpdateInPlace to be set for linker-driver tests. Issue #1238
Also allow TestUpdateInPlace to be set for linker-driver tests. Issue #1238
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.
Thanks. The change looks good. I did some additional testing and it looks like there's more places where we're missing writes. I'm happy to keep fixing them, but you're also welcome to have a go.
I did #1239, which makes it a bit easier to see multiple parts of the file that aren't being written. If I add //#TestUpdateInPlace:true to the shared variant of libc-integration.c, then I get the following sections with diffs:
[
.note.gnu.build-id (0xab8..0xae8) 32 bytes differ between 0xac8 and 0xae7,
.dynsym (0xb38..0xe80) [b3c],
.gnu.version_r (0x1198..0x11d8) [11ac, 11ad, 11bc, 11bd, 11cc, 11cd],
.dynamic (0x1cb8..0x1ed8) 112 bytes differ between 0x1e68 and 0x1ed7,
.got (0x1ed8..0x1fc8) 239 bytes differ between 0x1ed8 and 0x1fc7,
]
The build-id depends on the contents of the rest of the file, so it won't match until all the others match, so that one should be dealt with last.
The rough idea would be to find the code responsible for each of these in elf_writer.rs and locate the spot where those bytes are being skipped without being written to, then zero them. If any prove too tricky, feel free to leave them for me. Or, if you'd rather I do them all, that's also fine. We should probably wait until they're all fixed before we merge this PR though.
| /// will fail. | ||
| UpdateInPlace, | ||
| /// will fail. Specified indicates whether the mode was explicitly specified by the user or not. | ||
| UpdateInPlace { specified: bool }, |
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.
Looking at the code, I wonder if adding a third variant Auto might look better. But entirely up to you. I'm happy with either.
…timore#1239) Also allow TestUpdateInPlace to be set for linker-driver tests. Issue davidlattimore#1238
|
Do let me know if you need help with writing to those uninitialised bytes - or would like me to do it. It's entirely possible that some of them could be tricky to figure out. |
|
I got quite busy in the last two weeks and I think I couldn't manage to do it. Feel free to take it, I'll follow the PRs and try to learn from you |
|
Cool. I think I got them all with #1276. Now, if I change the default for |
Closes #1223