[jdk8u-dev] RFR: 8329826: GCC 12 reports some compiler error when building jdk8

[mirabilos]

On Tue, 7 May 2024 16:56:17 GMT, Simon Tooke <stooke at openjdk.org> wrote:

>> hotspot/src/share/vm/opto/type.cpp line 2560:
>> 
>>> 2558:             ciInstanceKlass* k = o->as_instance()->java_lang_Class_klass()->as_instance_klass();
>>> 2559:             field = k->get_field_by_offset(_offset, true);
>>> 2560:           }
>> 
>> This is useless papering over, not a fix, and not even good because it makes the code less legible at no gain.
>> 
>> Better to convert the two `assert`s to actual checks which abort the control flow of the function if triggered.
>
> (disclaimer: I am not a reviewer)
> This proposed change doesn't really handle the case where o == NULL, although it makes the compiler happy.  I propose something like:
> 
>     assert(o != NULL, "must be constant");
>     if (o != NULL) {
>         ciInstanceKlass* k = o->as_instance()->java_lang_Class_klass()->as_instance_klass();
>         ciField* field = k->get_field_by_offset(_offset, true);
>         assert(field != NULL, "missing field");
>         BasicType basic_elem_type = field->layout_type();
>         _is_ptr_to_narrowoop = UseCompressedOops && (basic_elem_type == T_OBJECT ||
>                                                      basic_elem_type == T_ARRAY);
>      } else {
>         _is_ptr_to_narrowoop = (?? I am not familiar enough with this code to put a suggestion here)
>     }

No, don’t `assert` *and* `if` something. (The compiler may delete one of these two under Undefined Behaviour rules.)

I’ve not looked at the callers, but returning an error or throwing an exception if `o == NULL` would be best. Do **not** rely on `assert`.

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

PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/479#discussion_r1592873417