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

Universal abi used in amm functions, no need for OS differentiation #1869

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pittma
Copy link
Contributor

@pittma pittma commented Sep 20, 2024

During the review process, I mistakenly believed that the @_6_arg_universal_ABI did not account for Windows, as it conspicuously matches the Linux calling convention. Additionally, Windows does not have 6 registers in its calling convention, only 4—args 5 and 6 would be pushed onto the stack, so what I had here was wrong in two ways.

While debugging, I could see that this was segfaulting during a read of a pointer in %r11; this was what tipped me off.

Reading symbols from .\crypto\crypto_test.exe...
(gdb) run
Starting program: C:\Users\sdp\src\aws-lc\build\crypto\crypto_test.exe "--gtest_filter=BNTest.ModExp2TestVectors"
[New Thread 29840.0x700c]
[New Thread 29840.0x1b54]
[New Thread 29840.0x64b8]
Debugger detected. Disabling unwind tests.
Note: Google Test filter = BNTest.ModExp2TestVectors
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BNTest
[ RUN      ] BNTest.ModExp2TestVectors
[New Thread 29840.0x64b4]

Thread 1 received signal SIGSEGV, Segmentation fault.
0x00007ff6cb416280 in L$loop5 ()
(gdb) disassemble 0x00007ff6cb416280
Dump of assembler code for function L$loop5:
=> 0x00007ff6cb416280 <+0>:     mov    (%r11),%r13

Running crypto_test was successful locally on:

Microsoft Windows Server
Version 21H2 (OS Build 20348.405)

On an SPR-based system. I have not tested this change on Linux locally yet, but I will in the next few hours.


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.

@pittma pittma requested a review from a team as a code owner September 20, 2024 22:28
@dkostic
Copy link
Contributor

dkostic commented Sep 20, 2024

Hey Dan, thanks for looking into this! Could you please re-enable the code path (https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/cpucap/internal.h#L144) to test the change in the CI?

@pittma
Copy link
Contributor Author

pittma commented Sep 20, 2024

Hey Dan, thanks for looking into this! Could you please re-enable the code path (https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/cpucap/internal.h#L144) to test the change in the CI?

2e8d424 does include that change!

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.45%. Comparing base (f89c9be) to head (1bdcc49).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1869      +/-   ##
==========================================
- Coverage   78.45%   78.45%   -0.01%     
==========================================
  Files         585      585              
  Lines       99458    99460       +2     
  Branches    14237    14236       -1     
==========================================
- Hits        78032    78030       -2     
- Misses      20791    20794       +3     
- Partials      635      636       +1     

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

justsmth added a commit that referenced this pull request Sep 25, 2024
### Description of changes: 
This extends the work done on #1869.
* Updates to latest version of Intel SDE;
* Fixes minor bug in Go test script.
* Disables use of AVX512 IFMA on Windows.

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.

---------

Co-authored-by: Dan Pittman <dan@dpitt.me>
Co-authored-by: Justin Smith <justsmth@amazon.com>
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.

3 participants