On Tue, 12 May 2026 21:06:30 GMT, Volodymyr Paprotski <[email protected]>
wrote:
>> src/hotspot/cpu/x86/stubGenerator_x86_64_sha3.cpp line 96:
>>
>>> 94: StubGenerator *stubgen,
>>> 95: MacroAssembler *_masm) {
>>> 96: bool multiBlock;
>>
>> You have moved the switch statement which verifies the stub id is in the
>> expected range after the call to `load_archive_data`. Please return it to
>> the start of this method to ensure we perform validation before trying to
>> retrieve the stub from the AOT cache.
>>
>> Note that you need to follow the original and provide a case switch for all
>> legitimate cases and a default case which calls `ShouldNotReachHere()`.
>
> I remember removing this because "this function is a local (static) function,
> essentially a private helper in this file.. it is only ever called here.. no
> need to tripple-check every parameter.. and it will just add complexity.. "
> (changed my mind!)
> ```C++
> void StubGenerator::generate_sha3_stubs() {
> bool avx512Available = VM_Version::supports_evex() &&
> VM_Version::supports_avx512bw();
> if (UseSHA3Intrinsics) {
> if (avx512Available) {
> StubRoutines::_sha3_implCompress =
>
> generate_sha3_implCompress_avx512(StubId::stubgen_sha3_implCompress_id, this,
> _masm);
> StubRoutines::_sha3_implCompressMB =
>
> generate_sha3_implCompress_avx512(StubId::stubgen_sha3_implCompressMB_id,
> this, _masm);
> StubRoutines::_double_keccak =
> generate_sha3_implCompress_avx512(StubId::stubgen_double_keccak_id,
> this, _masm);
> StubRoutines::_quad_keccak =
> generate_sha3_implCompress_avx512(StubId::stubgen_quad_keccak_id,
> this, _masm);
> } else {
> StubRoutines::_sha3_implCompress =
> generate_sha3_implCompress_avx2(StubId::stubgen_sha3_implCompress_id,
> this, _masm);
> StubRoutines::_double_keccak =
> generate_sha3_implCompress_avx2(StubId::stubgen_double_keccak_id,
> this, _masm);
> StubRoutines::_sha3_implCompressMB =
>
> generate_sha3_implCompress_avx2(StubId::stubgen_sha3_implCompressMB_id, this,
> _masm);
> }
> }
> }
>
>
> but.. I compacted your version to just:
>
> ```C++
> switch(stub_id) {
> case StubId::stubgen_sha3_implCompress_id:
> case StubId::stubgen_sha3_implCompressMB_id:
> case StubId::stubgen_double_keccak_id:
> case StubId::stubgen_quad_keccak_id:
> break;
> default:
> ShouldNotReachHere();
> }
>
>
> In fact.. its 'self-documentation' on ways to call this helper.. works for
> me, thanks!
The more important point was defensive programming - errors do get injected
during maintenance and the suggested pattern catches them (for me
maintainability is always the ace trump). However, you are right that this also
serves to state the intended use of the generator, another valuable benefit to
maintainers. But there is also a performance consideration to address.
This is one example of a generator that produces code for more than one stub.
For such generators a single up front switch, prior to calling
`load_archive_data`, is normally used to verify the stub id passed as argument
and also to initialize stub-dependent local state. The reason is that this
serves to minimize the amount of branching in the resulting compiled generator
code. If you look at other multi-stub generators (see e.g. the array copy
stubs) you should find that the same code pattern has been followed
consistently.
Of course that does imply a small amount of unnecessary work is done in the
case where `load_archive_data` returns a non-null address and we don't use the
state. The trade-off is
- redundant execution of code that usually does no more than install a few
zero or small constant values in registers/stack slots
vs
- postponing that same execution until we can be sure `load_archive_data` has
returned `nullptr` but then incurring the extra cost of either a repeated
switch or one or more compare and branch tests to perform the same setup.
Your latest patch has selected that second alternative contrary to what is done
elsewhere.
I'll also note here that one could try to finesse this unnecessary cost by
retaining the code pattern you have chosen modulo placing the first
verification switch inside an `ifdef ASSERT`/`#endif` pair. That would only
cost us extra in a debug build. However, that leaves a maintainer with more
code to read and any subsequent reviewer with more code to check. Specifically,
we have two places we need to update if we should extend the multi-stub
generator to include a new case and that's a potential bug injection
opportunity. You might wonder how often this sort of upgrade happens but . . .
well, here we are.
So, to sum up, my request may have seemed to have been based on a somewhat
arbitrary and inconsequential question of coding style. In reality it was a
choice that was considered before this PR was created and one that was applied
(I hope) consistently across the code base. I'm not going to force you to
follow the pattern employed elsewhere if you can provide a good reason to
proceed with the current patch but I still recommend you do so.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31125#discussion_r3234136586
