On Wed, 13 May 2026 23:48:23 GMT, Sandhya Viswanathan <[email protected]> wrote:
>> Volodymyr Paprotski has updated the pull request incrementally with one >> additional commit since the last revision: >> >> comments from Andrew Dinn > > src/hotspot/cpu/x86/assembler_x86.cpp line 3877: > >> 3875: } >> 3876: vex_prefix(dst, 0, src->encoding(), VEX_SIMD_66, VEX_OPCODE_0F, >> &attributes); >> 3877: emit_int8(0x6F); > > Should this not be 0x7F? Thanks for catching this! Swapping source and destination.. this had the potential of being a very hard to debug memory corruption bug in the future. It got me wondering how my fuzzing did not catch it.. and this instruction is only ever used from the macroAssembler.. which is only ever used from the avx2 version.. > src/hotspot/share/opto/library_call.cpp line 603: > >> 601: case vmIntrinsics::_double_keccak: >> 602: case vmIntrinsics::_quad_keccak: >> 603: return inline_double_keccak(intrinsic_id()); > > Could be renamed as inline_keccak() now? much better, thanks. done. > src/hotspot/share/opto/library_call.cpp line 8398: > >> 8396: default: >> 8397: assert(false, "dont call"); >> 8398: } > > Could this be written better with a state[] array? I think so. Just went and did it.. (would had looked even better when I had the Node[8] prototype..) > src/hotspot/share/opto/library_call.cpp line 8400: > >> 8398: } >> 8399: >> 8400: Node* double_keccak; > > Could be Node* keccak. looks better, done. > src/java.base/share/classes/sun/security/provider/SHA3.java line 99: > >> 97: super(name, digestLength, (WIDTH - c)); >> 98: this.suffix = suffix; >> 99: checkBlockSize(); > > Wonder if this should be an assert instead as in ML_KEM.java or the exception > thrown is ProviderException as in other places. Ah. I remember thinking through this. My first inclination was not to even add a check.. "all SHA3 classes are listed here, can trace the calls". But tracing all the ways blockSize gets set, thats a really tricky call graph; so that justified to me even adding a check. The field is final, so in theory only ever set by a constructor, which is why I kept it in the constructor, instead of moving it to the @IntrinsicCandidate callee.. But I wasn't sure about the Exception to throw.. ProviderException seems to be in line with the current file, so changed it to that. But I do wonder if there are any 'internalization'/translation concerns on me creating strings.. (Also, in defense of using a switch: I am hoping the field being final, the compiler will have an easier time with dead-code elimination here, if/when it can prove it is indeed final..) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31125#discussion_r3250508808 PR Review Comment: https://git.openjdk.org/jdk/pull/31125#discussion_r3250507186 PR Review Comment: https://git.openjdk.org/jdk/pull/31125#discussion_r3250507490 PR Review Comment: https://git.openjdk.org/jdk/pull/31125#discussion_r3250507820 PR Review Comment: https://git.openjdk.org/jdk/pull/31125#discussion_r3250508056
