On Thu, 20 Nov 2025 15:34:20 GMT, Jorn Vernee <[email protected]> wrote:
>> There were a few other holes in the recent migration of BytecodeDescriptor,
>> most notably:
>>
>> 1. BytecodeDescriptor is missing checks for `.`, `[` characters, leading,
>> trailing, consecutive `/`, or empty name.
>> 2. EnclosingMethod is only validated by hotspot to carry either field or
>> method type. We still need to check for field types
>>
>> I have written up the behavioral changes in the CSR. In addition, I have
>> added a few more tests to ensure the failure case behaviors of the migrated
>> use sites.
>
> src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java line 160:
>
>> 158: | (1L << ('/' - CHECK_OFFSET))
>> 159: | (1L << (';' - CHECK_OFFSET))
>> 160: | (1L << ('[' - CHECK_OFFSET));
>
> Are we sure that these are the only 4 non-identifier chars we can see in the
> string?
Could you add a test for something like `"Ljava#/lang/Object;"`?
> src/java.base/share/classes/sun/invoke/util/BytecodeDescriptor.java line 166:
>
>> 164: int check = str.charAt(index) - CHECK_OFFSET;
>> 165: if ((check & -Long.SIZE) == 0 && (NON_IDENTIFIER_MASK & (1L
>> << check)) != 0) {
>> 166: break;
>
> Maybe this is a little clearer:
> Suggestion:
>
> if (check < 64 && (NON_IDENTIFIER_MASK & (1L << check)) != 0) {
> break;
These generate similar code (`test` vs `cmp` on x64)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28079#discussion_r2546565467
PR Review Comment: https://git.openjdk.org/jdk/pull/28079#discussion_r2546547498
