On Tue, 24 Jun 2025 18:34:02 GMT, ExE Boss <d...@openjdk.org> wrote:

>> Unsafe throws IAE for misusing static vs instance fields, and it's revealed 
>> that AtomicXxxFieldUpdaters are using this mechanism to reject static 
>> fields. This is not a good practice, but we can at least document this so we 
>> don't accidentally introduce problems.
>
> src/hotspot/share/prims/unsafe.cpp line 498:
> 
>> 496:       if (fs.access_flags().is_static()) {
>> 497:         offset = -1;
>> 498:       } else {
> 
> Since `offset` is initialised to `-1`, this can simply do:
> Suggestion:
> 
>       if (!fs.access_flags().is_static()) {

True. Meanwhile I think I will add a comment to indicate this intends to fail 
fast when a matching static field is encountered.

> src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1070:
> 
>> 1068:      *
>> 1069:      * @throws NullPointerException if the field is {@code null}
>> 1070:      * @throws IllegalArgumentException if the field is static
> 
> Maybe these checks could be done in **Java** like:
> 
> // implicit null check:
> if ((f.getModifiers() & ACC_STATIC) != 0) {
>       throw new IllegalArgumentException("Field is static");
> }
> 
> and for the `staticField(Offset/Base)` methods:
> 
> // implicit null check:
> if ((f.getModifiers() & ACC_STATIC) == 0) {
>       throw new IllegalArgumentException("Field is not static");
> }

Don't think anyone is willing to change code here...

> src/java.base/share/classes/jdk/internal/misc/Unsafe.java line 1092:
> 
>> 1090:      *         in class {@code c}, i.e., if {@code 
>> c.getDeclaredField(name)}
>> 1091:      *         would throw {@code java.lang.NoSuchFieldException}, or 
>> if such
>> 1092:      *         a field is static
> 
> This is actually not the case, the native code only checks whether the field 
> exists, not whether it’s also not `static`:
> https://github.com/openjdk/jdk/blob/cbcf401170e0600e48ef74770eaa47c84c7e50b0/src/hotspot/share/prims/unsafe.cpp#L492-L503
> 
> Suggestion:
> 
>      *         would throw {@code java.lang.NoSuchFieldException}.

Hmm, didn't know that field stream included static fields

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25945#discussion_r2164666239
PR Review Comment: https://git.openjdk.org/jdk/pull/25945#discussion_r2164623978
PR Review Comment: https://git.openjdk.org/jdk/pull/25945#discussion_r2164623202

Reply via email to