RFR(S) 8243506 SharedBaseAddress is ignored by -Xshare:dump

[Thomas Stüfe]

Hi Ioi,

looks all good to me now.

Thanks, Thomas

On Sat, May 30, 2020 at 8:08 AM Ioi Lam <ioi.lam at oracle.com> wrote:

> Hi Thomas,
>
> Thanks for the review. Delta from the last webrev:
>
>
> http://cr.openjdk.java.net/~iklam/jdk15/8243506-SharedBaseAddress-ignored.v03-delta/
>
> More comments below
>
> On 5/28/20 11:03 PM, Thomas Stüfe wrote:
>
> Hi Ioi,
>
> this all looks quite good to me. So just a bit of bikeshedding from me:
>
> metaspaceShared.cpp:
>
> (I wish we had a version of align_up with an inbuilt overflow check. A
> choice of either assert or tell me if its overflown.)
>
> --
>
> +static char* compute_shared_base(size_t cds_total) {
> +  char* shared_base = (char*)align_up((char*)SharedBaseAddress,
> MetaspaceShared::reserved_space_alignment());
> +  const char* err = NULL;
> +  if (shared_base_too_high(shared_base, cds_total)) {
> +    err = "too high";
> +  } else if (!shared_base_valid(shared_base)) {
> +    err = "is invalid for this platform";
> +  }
>
> you print two "is" when pointer is invalid.
>
>
> Fixed
>
> --
>
> +  // We don't want any valid object to be at the very bottom of the
> archive.
> +  // See ArchivePtrMarker::mark_pointer().
> +  MetaspaceShared::misc_code_space_alloc(16);
>
> Question, this relies on the misc_code_space being the first region in the
> archive space?
>
>
> Yes.
>
> Only one of the regions is allocatable at any time.  So if this changes in
> the future and some other region becomes the first region, it will be
> easily caught by an assert.
>
>
> --
>
> +  if (use_requested_addr && static_mapinfo->requested_base_address() ==
> NULL) {
> +    log_info(cds)("Archive(s) were created with -XX:SharedBaseAddress=0.
> Always map at os-selected address");
> +    return MAP_ARCHIVE_MMAP_FAILURE;
> +  }
> +
>
> Missing a period at the end.
>
>
> Fixed :-).
>
>
> --
>
> Could we rename MetaspaceShared::default_base_address() to something like
> MetaspaceShared::requested_base_address()? I always think of default base
> address as 0x800000000. It also would clash less with
> Arguments::_default_SharedBaseAddress.
>
> That's a good idea. I was not happy with the name either.
>
> --
>
> test/hotspot/jtreg/runtime/cds/SharedBaseAddress.java
>
> Would a test for "0" make sense, since its special? (Just scanning for
> your log line would probably be enough).
>
> Added the test case and check for the "Always map at os-selected address"
> log message.
>
>
> Thanks
> - Ioi
>
> --
>
> Thanks, Thomas
>
> On Fri, May 29, 2020 at 2:15 AM Ioi Lam <ioi.lam at oracle.com> wrote:
>
>> Hi Calvin,
>>
>> Thanks for the review. I've fixed the problems you pointed out below.
>>
>> I also rebased the patch on top of Thomas's JDK-8243392: Remodel
>> CDS/Metaspace storage reservation. I reorganized the validation code of
>> SharedBaseAddress to check for platform validity and address space
>> wrap-around.
>>
>> SharedBaseAddressOption.java is updated to enable WhiteBox which is
>> required since JDK-8243947.
>>
>>
>>
>> http://cr.openjdk.java.net/~iklam/jdk15/8243506-SharedBaseAddress-ignored.v02/
>>
>> http://cr.openjdk.java.net/~iklam/jdk15/8243506-SharedBaseAddress-ignored.v02-delta/
>>
>> Thanks
>> - Ioi
>>
>> On 5/11/20 8:56 PM, Calvin Cheung wrote:
>> > Hi Ioi,
>> >
>> > Just couple of minor comments:
>> >
>> > metaspaceShared.cpp:
>> >
>> > 336   // We don't want any valid object to be at the very of the
>> archive.
>> >
>> > " ... the very of ..." -> "... the very bottom of..." ?
>> >
>> > SharedBaseAddressOption.java:
>> >
>> >   77         // a top archive specified in the base archive position
>> >   78         run2(topArchiveName, baseArchiveName,
>> >
>> > It looks like the order of the top and base archives are not reversed.
>> >
>> > Because the topArchiveName is used for dumpBaseArchive in line 65 and
>> > the order of the archives specified for dump2 (line 70) are the same
>> > as above (line 78). So the topArchiveName actually refers to the base
>> > archive?
>> >
>> > thanks,
>> >
>> > Calvin
>> >
>> > On 5/5/20 11:40 AM, Ioi Lam wrote:
>> >> https://bugs.openjdk.java.net/browse/JDK-8243506
>> >>
>> http://cr.openjdk.java.net/~iklam/jdk15/8243506-SharedBaseAddress-ignored.v01/
>> >>
>> >>
>> >> -XX:SharedBaseAddress was essentially ignored due to a bug in
>> >> JDK-8231610 [1].
>> >>
>> >> This fix will enhance both the flexibility and security of CDS.
>> >>
>> >>
>> >> After this fix, -XX:SharedBaseAddress=N will have the following effect:
>> >>
>> >> N != 0
>> >>
>> >>   CDS archive written with a base address of N. If
>> -XX:SharedBaseAddress
>> >>   is not specified, we will use a default address (0x8_00000000 on
>> >>   64-bit OS).
>> >>
>> >>   At run time, we will first attempt to map the CDS archive at N. If
>> >> this
>> >>   fails (the OS doesn't have enough contiguous free space at N), we
>> will
>> >>   map the CDS archive at a suitable location picked by the OS.
>> >>
>> >>   Specifying a non-default address could be useful:
>> >>   - for diagnosing purposes
>> >>   - if the default address is already occupied for some reason on your
>> >>     environment, but you have another location that's always free.
>> >>
>> >> N == 0
>> >>
>> >>   At run time, the CDS archive will always be mapped to an address
>> >>   selected by the OS.
>> >>
>> >>   This is useful for improved security -- on OSes that support ASLR
>> >> (Address
>> >>   Space Layout Randomization), the address of the archived metaspace
>> >> objects
>> >>   will be randomized on every run to make attacks more difficult.
>> >>
>> >>
>> >> I have added checks to make the code more robust in handling
>> >> arbitrary values
>> >> of N. See new tests in
>> >> cest/hotspot/jtreg/runtime/cds/SharedBaseAddress.java,
>> >> and MetaspaceShared::initialize_dumptime_shared_and_meta_spaces().
>> >>
>> >>
>> >> Thanks
>> >> - Ioi
>> >>
>> >> ------
>> >>
>> >> [1] https://bugs.openjdk.java.net/browse/JDK-8231610
>> >>     Relocate the CDS archive if it cannot be mapped to the requested
>> >> address
>>
>>
>