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

Make leb64 encoding fully safe #339

Merged
merged 1 commit into from
Jan 13, 2021
Merged

Make leb64 encoding fully safe #339

merged 1 commit into from
Jan 13, 2021

Conversation

jonas-schievink
Copy link
Contributor

This removes all unchecked indexing as well as the usage of MaybeUninit::uninit().assume_init() from the leb64 encoding logic.

Resulting binary size change for the log binary:

   text	   data	    bss	    dec	    hex	filename
  33708	      0	      8	  33716	   83b4	log-main
  33508	      0	      8	  33516	   82ec	../../target/thumbv7m-none-eabi/release/log

That's right, it's 200 Bytes smaller.

I've also tried keeping MaybeUninit in place, which reduces binary size by like 400 more bytes. That seemed spooky, so I didn't go for that (zeroing 10 bytes shouldn't take 400 Bytes of code space? does it pull in memset? does it miscompile something due to the UB?). Haven't investigated that further.

Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better. Thanks.

That's right, it's 200 Bytes smaller.

was that with opt-level=3? maybe the indexing prevents loop unrolling and that makes the binary smaller? I would have no idea why keeping the assume-uninit makes the program that much smaller though. llvm optimizations are unpredictable IMO

@japaric
Copy link
Member

japaric commented Jan 13, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 13, 2021

Merge conflict.

@jonas-schievink
Copy link
Contributor Author

was that with opt-level=3?

It was cargo build --release in the firmware/qemu directory, which should be opt-level=3, LTO=fat.

@japaric
Copy link
Member

japaric commented Jan 13, 2021

needs a rebase but feel free to submit to bors once that's resolved

@jonas-schievink
Copy link
Contributor Author

bors r=japaric

@bors
Copy link
Contributor

bors bot commented Jan 13, 2021

Build succeeded:

@bors bors bot merged commit d2d5a0d into main Jan 13, 2021
@bors bors bot deleted the safer-leb branch January 13, 2021 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants