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

Relax check-blobs check #331

Closed
wants to merge 1 commit into from
Closed

Relax check-blobs check #331

wants to merge 1 commit into from

Conversation

therealprof
Copy link
Contributor

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

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
Copy link

r? @thalesfragoso

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Feb 9, 2021
@therealprof
Copy link
Contributor Author

r? @jonas-schievink

@adamgreig
Copy link
Member

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?

@therealprof
Copy link
Contributor Author

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.

@therealprof
Copy link
Contributor Author

Before

[... assembly of blobs omitted ...]
thread 'main' panicked at 'thumbv6m-none-eabi.a is not up-to-date, please run `cargo xtask assemble`', xtask/src/lib.rs:202:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

After

[... assembly of blobs omitted ...]
Blobs identical.

@therealprof
Copy link
Contributor Author

Before someone asks, yes, I've verified that it actually still does the comparison (turns out ends_with() directly on the path has weird behaviour):

# echo "foo" >>bin/thumbv8m.main-none-eabi-lto.a
# cargo xtask check-blobs
[ ... ]
thread 'main' panicked at 'thumbv8m.main-none-eabi-lto.a is not up-to-date, please run `cargo xtask assemble`', xtask/src/lib.rs:204:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@adamgreig
Copy link
Member

If we want to compare the disassembly, then we'd have to check that in as well.

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?

in essence this should end up checking the very same thing as we do now

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.

@therealprof
Copy link
Contributor Author

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. 🤷🏻‍♂️

@therealprof
Copy link
Contributor Author

I sort of expect the asm dumps would also be in the wrong order so it would be some effort to sort them

I'd assume that objdump does the sorting. Usually I'd expect it to be in ascending order of address but since it's an archive, the addresses all start at 0x0. But that doesn't completely seem the case here either, most of the symbols are sorted alphabetically but then there're a few which seem to restart the alphabet, different compilation units maybe?

NB: Objdump doesn't work on the *-lto.a so I don't have any idea how to implement your disassembly idea.

@adamgreig
Copy link
Member

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...

@therealprof
Copy link
Contributor Author

Yes, the GNU binutils objdump -Cd between Linux builds (as checked in in master) is identical to my local Mac build:
cf. https://gist.github.com/therealprof/35b0a24b5aeb3f9e72809ef80c4c27e3

# git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   bin/thumbv6m-none-eabi.a
	modified:   bin/thumbv7em-none-eabi.a
	modified:   bin/thumbv7em-none-eabihf.a
	modified:   bin/thumbv7m-none-eabi.a
	modified:   bin/thumbv8m.base-none-eabi.a
	modified:   bin/thumbv8m.main-none-eabi.a
	modified:   bin/thumbv8m.main-none-eabihf.a

bors bot added a commit that referenced this pull request Mar 7, 2021
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>
@jonas-schievink jonas-schievink removed their assignment Jan 3, 2022
adamgreig added a commit that referenced this pull request Jan 12, 2022
331: Fix CHANGELOG for recent 0.7 release r=thejpster a=adamgreig

:sweat_smile: 

Co-authored-by: Adam Greig <adam@adamgreig.com>
@adamgreig
Copy link
Member

Closing this as no longer relevant now we've got rid of all the pre-compiled blobs.

@adamgreig adamgreig closed this Oct 16, 2023
@adamgreig adamgreig deleted the relax-check-blobs branch October 16, 2023 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS produces different archives from Linux
5 participants