RFR: 8221477: Inject os/cpu-specific constants into Unsafe from JVM

[Andrew Dinn]

On 28/03/2019 12:17, Thomas Stüfe wrote:
> this looks fine, nits only:

Thank you for the review, Thomas. I'll post a follow up webrev to
address the comments. Responses are inline.

> UnsafeConstantsFixup::do_field()
> 
> you could shrink that coding a bit by factoring out the checks, e.g.:
> . . .

Yes, that was egregious cut-and-paste coding.

Actually, the idea is that all fields of the class are final static and
all of them are expected to be injected (which is why the final else
clause asserts).

So, I could just move the asserts up to precede the if-elseif cascade
and also include checks that the field is final and static.

  void do_field(fieldDescriptor* fd) {
    oop mirror = fd->field_holder()->java_mirror();
    assert(mirror != NULL, "UnsafeConstants must have mirror already");
    assert(fd->field_holder() ==
SystemDictionary::UnsafeConstants_klass(), "Should be UnsafeConstants");
    assert(fd->is_final(), "fields of UnsafeConstants must be final");
    assert(fd->is_static() "fields of UnsafeConstants must be static");
    if (fd->name() == vmSymbols::address_size_name()) {
      mirror->int_field_put(fd->offset(), _address_size);
    } else if (fd->name() == vmSymbols::page_size_name()) {
      mirror->int_field_put(fd->offset(), _page_size);
    } else if (fd->name() == vmSymbols::big_endian_name()) {
      . . .
    } else {
      assert(false, "unexpected UnsafeConstants field");
    }

> Also, is there a reason that code from thread.cpp:
> 
>     jint address_size = sizeof(void*);
>     jint page_size = os::vm_page_size();
>     jboolean is_big_endian = LITTLE_ENDIAN_ONLY(false)
> BIG_ENDIAN_ONLY(true);
>     jint use_unaligned_access = UseUnalignedAccesses;
> 
> could not just happen right in UnsafeConstantsFixup::do_field()? You
> would not have to pass all these arguments around, param list could be
> empty.

No good reason. Your suggestion is better.

> src/hotspot/share/classfile/vmSymbols.hpp
> 
> Alignment of trailing '\' broken

Oops!

> src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java
> 
> s/intitialize/initialize
> s/iff/if
The second of those was actually intended to be iff. This is a common
abbreviation used by English/US mathematicians and logicians to write
'if and only if' (it is also sometimes written as <=>). Since you didn't
recognize it I guess I really need to write it out in full.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander