On Tue, 18 Nov 2025 08:53:05 GMT, Aleksey Shipilev <[email protected]> wrote:
>> Jiangli Zhou has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix Whitespace error.
>
> test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 39:
>
>> 37: import javax.crypto.spec.SecretKeySpec;
>> 38:
>> 39: public class TestAesGcmIntrinsic {
>
> This sounds like `TestGCMSplitBound` or some such; it is not a generic test
> for AES/GCM intrinsic.
I renamed to TestAesGcmIntrinsic name, when converting the original test into
the jtreg test. `TestGCMSplitBound` SGTM. Changed.
> test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 93:
>
>> 91: }
>> 92: }
>> 93: for (int messageSize = SPLIT_LEN; messageSize < SPLIT_LEN + 300;
>> messageSize++) {
>
> `[SPLIT_LEN - 300; SPLIT_LEN + 300]` for completeness, perhaps?
Done.
> test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 96:
>
>> 94: byte[] message = randBytes(messageSize);
>> 95: try {
>> 96: byte[] ciphertext = gcmEncrypt(key, message, aad);
>
> I believe it makes sense to check that round-trip is successful, e.g. that
> `decrypt(encrypt(message)) == message`. Currently we implicitly rely on
> exceptions being thrown from the incorrectly executing code, which is IMO too
> weak -- in the boundary conditions like these, there might be bugs that _do
> not_ manifest in visible exceptions, and just the encryption is subtly broken.
That's a good idea. I added decrypt part and the check as suggested.
> test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 109:
>
>> 107: TestAesGcmIntrinsic test = new TestAesGcmIntrinsic();
>> 108: long startTime = System.currentTimeMillis();
>> 109: while (System.currentTimeMillis() - startTime < 60 * 1000) {
>
> I get that you want a stress test. But time-limiting puts the test into weird
> condition: it can have different number of iterations, depending on auxiliary
> load on the machine. These tests are running in parallel with lots of other
> tests, so it is not uncommon. Do you even need to repeat `jitFunc()` call
> multiple times? Looks like it traverses the interesting configurations in one
> go?
I did some testing today. For 200 runs, removing the time-limited loop, there
is 89 runs out of 200 fail. So I changed to use an iteration of three runs, all
200 runs fail without the fix.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544352236
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544352547
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544348398
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544346000
