RFR(S) 8243506 SharedBaseAddress is ignored by -Xshare:dump
Ioi Lam
ioi.lam at oracle.com
Sat May 30 06:06:42 UTC 2020
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
> <mailto: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
>
More information about the hotspot-runtime-dev
mailing list