Skip to content

Reduce compiler ability to optimise to statisfy gcc 9.5 #1442

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

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

torben-hansen
Copy link
Contributor

@torben-hansen torben-hansen commented Feb 19, 2024

Issues:

P118709392

Description of changes:

This PR is an attempt to fix a transient issue with gcc 9.5 compilation. This is a test workflow, so no hot-path performance regression concerns from me.

gcc 9.5 has a bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95189 that has not been backported. Essentially, the compiler will falsely reason that it's comparing a string and optimise out everything behind a null-character.

In the test fixture SSLVersionTest.DefaultTicketKeyInitialization it appears the gcc 9.5 compiler only compares the first byte and short-circuits there. What was observed:

4a6274:	394243e0 	ldrb	w0, [sp, #144] <-- first byte of ticket_key
  4a6278:	b90043ff 	str	wzr, [sp, #64] <-- zeroise stack (kzerokey presumably)
  4a627c:	b90053e0 	str	w0, [sp, #80] <-- Put w0 back on the stack(?)
  4a6280:	34000240 	cbz	w0, 4a62c8 <-- zero-test register w0, branch to failure path (adr 4a62c8) if true <_ZN4bssl12_GLOBAL__N_150SSLVersionTest_DefaultTicketKeyInitialization_Test8TestBodyEv+0x128>

Dropping static const removes optimisation options for the compiler. In my tests, gcc 9.5 now emits a direct memcmp instead of attempting any optimisations:

  4a89a4:	aa1603e0 	mov	x0, x22
  4a89a8:	d2800602 	mov	x2, #0x30                  	// #48
  4a89ac:	b90043ff 	str	wzr, [sp, #64]
  4a89b0:	910243e1 	add	x1, sp, #0x90
  4a89b4:	97fd915f 	bl	40cf30 <memcmp@plt>

Testing:

Tested on machines with gcc 9.5 runtime. Before failed, after this patch, no failure (even when forcing first byte to 0).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@torben-hansen torben-hansen requested a review from a team as a code owner February 19, 2024 22:54
dkostic
dkostic previously approved these changes Feb 19, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7f76c04) 76.85% compared to head (47a9a78) 76.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1442   +/-   ##
=======================================
  Coverage   76.85%   76.86%           
=======================================
  Files         425      425           
  Lines       71533    71533           
=======================================
+ Hits        54980    54985    +5     
+ Misses      16553    16548    -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

skmcgrail
skmcgrail previously approved these changes Feb 19, 2024
@torben-hansen torben-hansen enabled auto-merge (squash) February 20, 2024 20:51
@torben-hansen torben-hansen merged commit c8d82c7 into aws:main Feb 20, 2024
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Feb 21, 2024
gcc 9.5 has a bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95189 that has not been backported. Essentially, the compiler will falsely reason that it's comparing a string and optimise out everything behind a null-character. Dropping static const removes optimisation options for the compiler. In my tests, gcc 9.5 now emits a direct memcmp instead of attempting any optimisations.

Add X509_STORE_get1_objects

This will be needed for python/cpython#114573.
Along the way, document the various functions that expose "query from
X509_STORE". Most of them unfortunately leak the weird caching thing
that hash_dir does, as well as OpenSSL's generally poor handling of
issuers with the same name and CRL lookup, but I don't think it's really
worth trying to unexport these APIs.

Change-Id: I18137bdc4cbaa4bd20ff55116a18f350df386e4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65787
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>

Revert "Add X509_STORE_get1_objects"

This reverts commit cd5439a827a29320f7005786a26454904e4b12dc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants