-
Notifications
You must be signed in to change notification settings - Fork 165
Relax check-blobs check #331
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
Turns out rustc has an unstable symbol order between Linux and macOS, but only if `-Clinker-plugin-lto` is not used, so let's only check those archives for binary changes for now. Closes #329 Signed-off-by: Daniel Egger <daniel@eggers-club.de>
(rust-highfive has picked a reviewer for you, use r? to override) |
I'm not totally happy allowing anything in the non-lto blobs (though I guess I'd probably re-run xtask before a release anyway); maybe we could have a check similar to previous interations which dumps the object file to text that might be easier to compare in a stable ordering? |
The blobs are automatically regenerated when check-blobs is run. I would expect those files to change as well when new blobs are required so in essence this should end up checking the very same thing as we do now but hopefully won't cause troubles for macOS users. If we want to compare the disassembly, then we'd have to check that in as well. |
Before
After
|
Before someone asks, yes, I've verified that it actually still does the comparison (turns out
|
The old CI script didn't have them checked in: it dumped the checked-in archives to asm, generated new archives, dumped them to asm, and compared the two asm dumps. I sort of expect the asm dumps would also be in the wrong order so it would be some effort to sort them, perhaps arm-none-eabi-gcc didn't change ordering between MacOS and Linux the same way llvm apparently does?
It only checks half the things we do now - what if the MacOS build is generating bad non-LTO binaries now or starts doing it in the future? What if the ordering makes some difference we don't know about yet? We don't even know why the lto-plugin archives remain the same ordering while the normal ones don't... it seems to warrant a bit more investigation before deciding to just not check half the blobs. |
I just picked up @jonas-schievink's idea of relaxing the checks: #329 (comment) I'm also fine leaving as-is; it's a bit contributor-unfriendly but also not the end of the world if you need to the build the blobs under Linux. 🤷🏻♂️ |
I'd assume that NB: Objdump doesn't work on the |
For the -lto files we can continue to compare directly anyway. They don't really contain thumb assembly anyway, right? Do the non-lto file dumps compare exactly the same between Linux and MacOS? That is, dumping the Linux build archive on Linux vs a Mac built archive on Mac? If they do it might be workable... |
Yes, the GNU binutils
|
335: Prepare for v0.7.2 release of cortex-m r=thalesfragoso a=adamgreig We've had #328 merged for a while and it fixes a pretty annoying bug which is still in the wild. Can anything think of anything else to get in before a release? If there's nothing coming up (I think #282 needs some more discussion and #331 won't affect the published crate itself) we could get this released. Co-authored-by: Adam Greig <adam@adamgreig.com>
Closing this as no longer relevant now we've got rid of all the pre-compiled blobs. |
Turns out rustc has an unstable symbol order between Linux and macOS,
but only if
-Clinker-plugin-lto
is not used, so let's only check thosearchives for binary changes for now.
Closes #329
Signed-off-by: Daniel Egger daniel@eggers-club.de