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

[Thomas Stüfe]

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.

--

+  // 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?

--

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

--

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.

--

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

--

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