RFR: 8319747: galoisCounterMode_AESCrypt stack walking broken

[Daniel Jeliński]

On Fri, 10 Nov 2023 03:48:22 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

>> This PR fixes stack walking by removing incorrect RBP change in the middle of a stub.
>> 
>> I also changed the register assignment to avoid saving RSI on Windows.
>> 
>> Testing:
>> - test/hotspot/jtreg/compiler/codegen/aes/TestAESMain.java still passes, both on Windows and Linux
>> - mach5 tier1-2 (including security and TLS-heavy networking tests) still pass
>> - the generated flamegraphs (attached in JBS) show the correct stack trace with this fix
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 243:
> 
>> 241:   const Register state = r13;
>> 242:   const Address subkeyH_mem(rbp, 8 * wordSize);
>> 243:   const Register subkeyHtbl = r11;
> 
> Changing the register from r14 to r11 here doesn't look correct. As aesgcm_encrypt() uses r11 as temporary for windows at line 3046 and overwrites it at line 3074 etc in the original code.  
> 3046 const Register ghash_pos = NOT_WIN64( r14) WIN64_ONLY( r11 );
> 3074   __ movl(ghash_pos, 0); // Pointer for ghash read and store operations

Ah, you're right! I missed that. I was looking for `#ifdef`s, and forgot that we have the `*_ONLY` macros.

The code works correctly because `subkeyHtbl` and `ghash_pos` have non-overlapping lifetimes; `subkeyHtbl` is last used in line:
3072 __ call(GENERATE_HTBL_48_BLKS, relocInfo::none);

I'd like to make this explicit by changing the code like:

  // reuse subkeyHtbl register - the lifetimes do not overlap
  const Register ghash_pos = subkeyHtbl;

but I can also revert the register renaming changes. What's your preference?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16567#discussion_r1389040108