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

[Thomas Stüfe]

On Thu, Mar 28, 2019 at 3:41 PM Andrew Dinn <adinn at redhat.com> wrote:

> 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.
>

Oh, don't worry on my account. I am not a native speaker nor a
mathematician. You could leave iff and add [sic] to make everyone curious
and start googling "iff" :)

..Thomas


>
> 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
>