cortex-m icon indicating copy to clipboard operation
cortex-m copied to clipboard

Relax check-blobs check

Open therealprof opened this issue 3 years ago • 11 comments

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 [email protected]

therealprof avatar Feb 09 '21 21:02 therealprof

r? @thalesfragoso

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

rust-highfive avatar Feb 09 '21 21:02 rust-highfive

r? @jonas-schievink

therealprof avatar Feb 09 '21 21:02 therealprof

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?

adamgreig avatar Feb 09 '21 21:02 adamgreig

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 avatar Feb 09 '21 21:02 therealprof

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 avatar Feb 09 '21 21:02 therealprof

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

therealprof avatar Feb 09 '21 22:02 therealprof

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.

adamgreig avatar Feb 10 '21 01:02 adamgreig

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: https://github.com/rust-embedded/cortex-m/issues/329#issuecomment-776249663

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 avatar Feb 10 '21 01:02 therealprof

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.

therealprof avatar Feb 10 '21 01:02 therealprof

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

adamgreig avatar Feb 10 '21 01:02 adamgreig

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

therealprof avatar Feb 10 '21 01:02 therealprof

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

adamgreig avatar Oct 16 '23 23:10 adamgreig