Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]
On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee wrote: >> Please review this small patch to enable the OSX build using Xcode 12.0. >> >> Thanks, >> Paul > > Paul Hohensee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull > request contains three additional commits since > the last revision: > - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch > - Merge branch 'master' into JDK-8253375 > - JDK-8253375 I just copied the declaration and oversaw that tiny little '&'. With it, sharedRuntime.cpp compiles fine. Sorry for not being careful enough. Reviewed. - Marked as reviewed by lucy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v7]
> This patch is reorganized after 8252725, which is separated from this patch > to refactor jlink glugin code. The previous > webrev with hg can be found at: > http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 > integrated, the > regeneration of holder classes is simply to call the new added > GenerateJLIClassesHelper.cdsGenerateHolderClasses > function. Tests: tier1-4 Yumin Qi has updated the pull request incrementally with one additional commit since the last revision: Remove trailing word of line which is not used in holder class regeneration. There is a trailing LF (Line Feed) so trim white spaces from both front and end of the line or it will fail method type validation. - Changes: - all: https://git.openjdk.java.net/jdk/pull/193/files - new: https://git.openjdk.java.net/jdk/pull/193/files/9ab52116..9b0f523b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=193=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=193=05-06 Stats: 22 lines in 2 files changed: 10 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193 PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8223347: Integration of Vector API (Incubator)
On Tue, 29 Sep 2020 22:00:04 GMT, Erik Joelsson wrote: >> This pull request is for integration of the Vector API. It was previously >> reviewed under conditions when mercurial was >> used for the source code control system. Review threads can be found here >> (searching for issue number 8223347 in the >> title): >> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-April/thread.html >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/thread.html >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-July/thread.html >> >> If mercurial was still being used the code would be pushed directly, once >> the CSR is approved. However, in this case a >> pull request is required and needs explicit reviewer approval. Between the >> final review and this pull request no code >> has changed, except for that related to merging. > > Build changes look ok. Hi @PaulSandoz , This integration seems to miss https://github.com/openjdk/panama-vector/pull/1, which had fixed crashes on AVX512 machines. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/367
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]
On Tue, 29 Sep 2020 14:08:49 GMT, Monica Beckwith wrote: >> This is a continuation of >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html >> >> Changes since then: >> * We've improved the write barrier as suggested by Andrew [1] >> * The define-guards around R18 have been changed to `R18_RESERVED`. This >> will be enabled for Windows only for now but >> will be required for the upcoming macOS+Aarch64 [2] port as well. >> * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov >> in our PR for now and built the >> Windows-specific CPU feature detection on top of it. >> >> [1] >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html >> [2] https://openjdk.java.net/jeps/8251280 > > Monica Beckwith has updated the pull request incrementally with one > additional commit since the last revision: > > change string representation for r18 to "r18_tls" on every platform Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/212
RE: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]
Hmm. I'm running Xcode 12.0.1 on 10.15.6 and don't see it. Are you sure you have the '&' in front of locs_buf? I.e., buffer.insts()->initialize_shared_locs((relocInfo*)_buf, sizeof(locs_buf) / sizeof(relocInfo)); ^ Thanks, Paul On 9/29/20, 1:07 PM, "build-dev on behalf of Lutz Schmidt" wrote: On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee wrote: >> Please review this small patch to enable the OSX build using Xcode 12.0. >> >> Thanks, >> Paul > > Paul Hohensee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since > the last revision: > - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch > - Merge branch 'master' into JDK-8253375 > - JDK-8253375 The proposed (updated) change does _NOT_ compile on my machine (MacOS 10.15.6, Xcode 12.0): sharedRuntime.cpp:2860:46: error: cannot cast from type 'struct (anonymous struct at sharedRuntime.cpp:2859:7)' to pointer type 'relocInfo *' You will have to use a union (option (2) as proposed by Kim Barrett far above. I proved that variant compiles on my machine. - Changes requested by lucy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8223347: Integration of Vector API (Incubator)
On Fri, 25 Sep 2020 20:14:29 GMT, Paul Sandoz wrote: > This pull request is for integration of the Vector API. It was previously > reviewed under conditions when mercurial was > used for the source code control system. Review threads can be found here > (searching for issue number 8223347 in the > title): > https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-April/thread.html > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/thread.html > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-July/thread.html > > If mercurial was still being used the code would be pushed directly, once the > CSR is approved. However, in this case a > pull request is required and needs explicit reviewer approval. Between the > final review and this pull request no code > has changed, except for that related to merging. Build changes look ok. - PR: https://git.openjdk.java.net/jdk/pull/367
RFR: 8223347: Integration of Vector API (Incubator)
This pull request is for integration of the Vector API. It was previously reviewed under conditions when mercurial was used for the source code control system. Review threads can be found here (searching for issue number 8223347 in the title): https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-April/thread.html https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/thread.html https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-July/thread.html If mercurial was still being used the code would be pushed directly, once the CSR is approved. However, in this case a pull request is required and needs explicit reviewer approval. Between the final review and this pull request no code has changed, except for that related to merging. - Commit messages: - Fix permissions - Fix permissions - Merge master - Vector API new files - Integration of Vector API (Incubator) Changes: https://git.openjdk.java.net/jdk/pull/367/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=367=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8223347 Stats: 295107 lines in 336 files changed: 292957 ins; 1062 del; 1088 mod Patch: https://git.openjdk.java.net/jdk/pull/367.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/367/head:pull/367 PR: https://git.openjdk.java.net/jdk/pull/367
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]
> On Sep 29, 2020, at 4:05 PM, Lutz Schmidt wrote: > > On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee wrote: > >>> Please review this small patch to enable the OSX build using Xcode 12.0. >>> >>> Thanks, >>> Paul >> >> Paul Hohensee has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev >> excludes the unrelated changes brought in by the merge/rebase. The pull >> request contains three additional commits since >> the last revision: >> - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch >> - Merge branch 'master' into JDK-8253375 >> - JDK-8253375 > > The proposed (updated) change does _NOT_ compile on my machine (MacOS > 10.15.6, Xcode 12.0): > > sharedRuntime.cpp:2860:46: error: cannot cast from type 'struct (anonymous > struct at sharedRuntime.cpp:2859:7)' to > pointer type 'relocInfo *’ Did you use the change from the PR, or apply it manually. That looks like the error one would get if only the type of locs_buf were changed, without changing to take the address in the reference. That is, without changing `(relocInfo*)locs_buf` to `(relocInfo*)_buf`. That second change is in the PR. > You will have to use a union (option (2) as proposed by Kim Barrett far > above. I proved that variant compiles on my > machine. > > - > > Changes requested by lucy (Reviewer). > > PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]
On Tue, 29 Sep 2020 14:08:49 GMT, Monica Beckwith wrote: >> This is a continuation of >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html >> >> Changes since then: >> * We've improved the write barrier as suggested by Andrew [1] >> * The define-guards around R18 have been changed to `R18_RESERVED`. This >> will be enabled for Windows only for now but >> will be required for the upcoming macOS+Aarch64 [2] port as well. >> * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov >> in our PR for now and built the >> Windows-specific CPU feature detection on top of it. >> >> [1] >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html >> [2] https://openjdk.java.net/jeps/8251280 > > Monica Beckwith has updated the pull request incrementally with one > additional commit since the last revision: > > change string representation for r18 to "r18_tls" on every platform Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v7]
On Tue, 29 Sep 2020 14:03:57 GMT, Bernhard Urban-Forster wrote: >> Marked as reviewed by aph (Reviewer). > > @theRealAph okay, I've changed the string representation of `r18` to > `"r18_tls"` on every platform. > @plummercj thank you for your feedback. I've updated the copyright in > mentioned files. Changes look good. Thanks. I'm marking as "reviewed" for serviceability changes and copyrights. - PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]
On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee wrote: >> Please review this small patch to enable the OSX build using Xcode 12.0. >> >> Thanks, >> Paul > > Paul Hohensee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull > request contains three additional commits since > the last revision: > - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch > - Merge branch 'master' into JDK-8253375 > - JDK-8253375 The proposed (updated) change does _NOT_ compile on my machine (MacOS 10.15.6, Xcode 12.0): sharedRuntime.cpp:2860:46: error: cannot cast from type 'struct (anonymous struct at sharedRuntime.cpp:2859:7)' to pointer type 'relocInfo *' You will have to use a union (option (2) as proposed by Kim Barrett far above. I proved that variant compiles on my machine. - Changes requested by lucy (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]
On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee wrote: >> Please review this small patch to enable the OSX build using Xcode 12.0. >> >> Thanks, >> Paul > > Paul Hohensee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev > excludes the unrelated changes brought in by the merge/rebase. The pull > request contains three additional commits since > the last revision: > - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch > - Merge branch 'master' into JDK-8253375 > - JDK-8253375 Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]
> Please review this small patch to enable the OSX build using Xcode 12.0. > > Thanks, > Paul Paul Hohensee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch - Merge branch 'master' into JDK-8253375 - JDK-8253375 - Changes: - all: https://git.openjdk.java.net/jdk/pull/348/files - new: https://git.openjdk.java.net/jdk/pull/348/files/5db5edc2..cb08da07 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=348=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=348=00-01 Stats: 27610 lines in 391 files changed: 4691 ins; 21610 del; 1309 mod Patch: https://git.openjdk.java.net/jdk/pull/348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/348/head:pull/348 PR: https://git.openjdk.java.net/jdk/pull/348
Re: [OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
> On Sep 29, 2020, at 10:18 AM, Hohensee, Paul wrote:
> Code that calls initialize_shared_locs is inconsistent even within itself.
> E.g., in c1_Compilation.cpp, we have
Agreed there seems to be a bit of a mess around that function.
> Anyway, I just wanted to make the compiler warning go away, not figure out
> why the code is the way it is. Matthias and Kim would like to separate out
> the CSystemColors.m patch into a separate bug, which is fine by me (see
> separate email).
>
> So, for the sharedRuntime patch, would it be ok to just use
>
> short locs_buf[84];
>
> to account for possible alignment in initialize_shared_locs? I'm using 84
> because 80 is exactly 5 records plus 5 sizeof(relocInfo)s, allowing for
> alignment adds 3, and rounding the total array size up to a multiple of 8
> adds 1.
Your analysis looks plausible. But consider instead
struct { double data[20]; } locs_buf;
and change next line to use _buf
This doesn't change the size or alignment from pre-existing code. I can't
test whether this will suppress the warning, but I'm guessing it will. And the
rest
of below is off if that’s wrong.
Changing the size and type and worrying about alignment changes seems beyond
what's needed "to make the compiler warning go away, not figure out why the
code is the way it is.” I dislike making weird changes to suppress compiler
warnings,
but changing the type and size seems more weird to me here. I mean, 84 looks
like
a number pulled out of a hat, unless you are going to add a bunch of commentary
that probably really belongs in a bug report to look at this stuff more
carefully.
And file an RFE to look at initialize_shared_locs and its callers more
carefully.
RE: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
> Hi, Matthias, Kim. No problem opening a separate issue to fix CSystemColors.m.
Hi Paul, did that :
https://bugs.openjdk.java.net/browse/JDK-8253791
https://github.com/openjdk/jdk/pull/403
Best regards, Matthias
-Original Message-
From: Hohensee, Paul
Sent: Dienstag, 29. September 2020 15:46
To: Kim Barrett ; Matthias Baesken
Cc: 2d-...@openjdk.java.net; awt-...@openjdk.java.net;
build-dev@openjdk.java.net; hotspot-...@openjdk.java.net
Subject: RE: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
Hi, Matthias, Kim. No problem opening a separate issue to fix CSystemColors.m.
Thanks,
Paul
On 9/29/20, 4:23 AM, "hotspot-dev on behalf of Kim Barrett"
wrote:
> On Sep 29, 2020, at 3:14 AM, Matthias Baesken
wrote:
>
> On Fri, 25 Sep 2020 06:06:31 GMT, Kim Barrett
wrote:
>
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>>
>>> Thanks,
>>> Paul
>>
>> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>>
>>> 127: NSColor* result = nil;
>>> 128:
>>> 129: if (colorIndex < ((useAppleColor) ?
sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>>> java_awt_SystemColor_NUM_COLORS)) {
>>
>> This looks like a plain old bug fix, nothing really to do with the
compiler upgrade. The new compiler presumably just
>> has a new warning that brought attention to the problem. As such, I
don't think it belongs in a PR that's about issues
>> related to the compiler upgrade. Someone might want to backport just
this fix, for example.
>
> Hello Kim, Paul - so should we open a separate bug for the
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue
because it might be a general problem just
> uncovered by the new compiler ? Paul , do you want to do this ? Or should
I open a bug and post a separate change for
> the useAppleColor check in CSystemColors.m ?
I think so.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/348
RE: [OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
Thanks for the reviews/discussion, and apologies for the delayed reply: I've
been OOTO.
Kim is correct, initialize_shared_locs specifically adjusts the alignment of
its buffer argument, which is why short works. char would work as well, but
short happens to be the size of a relocInfo. Maybe the author of the other use
in sharedRuntime.cpp at line 2682 used short to remind of that.
Code that calls initialize_shared_locs is inconsistent even within itself.
E.g., in c1_Compilation.cpp, we have
int locs_buffer_size = 20 * (relocInfo::length_limit + sizeof(relocInfo));
char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
locs_buffer_size / sizeof(relocInfo));
relocInfo::length_limit is a count of shorts, while sizeof(relocInfo) is a
count of chars. The units aren’t the same but are added together as if they
were. relocInfo::length_limit is supposed to be the maximum size of a
compressed relocation record, so why add sizeof(relocInfo)?
And then in sharedRuntime.cpp, we have two places. The first:
short buffer_locs[20];
buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
sizeof(buffer_locs)/sizeof(relocInfo));
relocInfo::length_limit is 15 on a 64-bit machine, so with a buffer of 20
shorts, alignment in initialize_shared_locs might take up to of 3 more, which
is uncomfortably close to 20 afaic. And, if you add sizeof(relocInfo) as
happens in c1_Compilation.cpp, you're bang on at 20. The unstated assumption
seems to be that only a single relocation record will be needed.
The second:
double locs_buf[20];
buffer.insts()->initialize_shared_locs((relocInfo*)locs_buf,
sizeof(locs_buf) / sizeof(relocInfo));
This at allocates a buffer that will hold 5 compressed relocation records with
10 bytes left over, and guarantees 8 byte alignment. Perhaps when it was
written, initialize_shared_locs didn't align its buffer argument address. And,
there's that sizeof(relocInfo) padding again: 2 extra bytes per relocation
record.
Anyway, I just wanted to make the compiler warning go away, not figure out why
the code is the way it is. Matthias and Kim would like to separate out the
CSystemColors.m patch into a separate bug, which is fine by me (see separate
email).
So, for the sharedRuntime patch, would it be ok to just use
short locs_buf[84];
to account for possible alignment in initialize_shared_locs? I'm using 84
because 80 is exactly 5 records plus 5 sizeof(relocInfo)s, allowing for
alignment adds 3, and rounding the total array size up to a multiple of 8 adds
1.
Thanks,
Paul
On 9/29/20, 5:03 AM, "2d-dev on behalf of Kim Barrett"
<2d-dev-r...@openjdk.java.net on behalf of kim.barr...@oracle.com> wrote:
> On Sep 29, 2020, at 3:59 AM, Matthias Baesken
wrote:
>
> On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes
wrote:
>
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>>
>>> Thanks,
>>> Paul
>>
>> src/hotspot/share/runtime/sharedRuntime.cpp line 2851:
>>
>>> 2849: if (buf != NULL) {
>>> 2850: CodeBuffer buffer(buf);
>>> 2851: short locs_buf[80];
>>
>> This code is just weird. Why is the locs_buf array not declared to be
the desired type? If the compiler rejects double
>> because it isn't relocInfo* then why does it accept short? And if it
accepts short will it accept int or long long or
>> int64_t? Using int64_t would be a clearer change. Though semantically
this code is awful. :( Should it be intptr_t ?
>
> Currently we have in the existing code various kind of buffers passed
into initialize_shared_locs that compile nicely
> on all supported compilers and on Xcode 12 as well ,see
>
> c1_Compilation.cpp
>
> 326 char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
> 327 code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
>
> sharedRuntime.cpp
>
> 2681 CodeBuffer buffer(buf);
> 2682 short buffer_locs[20];
> 2683 buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
> 2684
sizeof(buffer_locs)/sizeof(relocInfo));
>
> So probably using short would be consistent to what we do already in the
coding at another place (looking into
> relocInfo it would be probably better to use unsigned short or to
typedef unsigned short in the relocInfo class
> and use the typedef).
That’s *not* consistent, because of buffer alignment. The call to
NEW_RESOURCE_ARRAY is going
to return a pointer to memory that is 2*word aligned. (Interesting,
possibly a 32/64 bit “bug” here.)
The existing code in sharedRuntime.cpp is allocating double-aligned.
iniitalize_shared_locs wants a
HeapWordSize-aligned buffer,
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v12]
> This is a continuation of > https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html > > Changes since then: > * We've improved the write barrier as suggested by Andrew [1] > * The define-guards around R18 have been changed to `R18_RESERVED`. This will > be enabled for Windows only for now but > will be required for the upcoming macOS+Aarch64 [2] port as well. > * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov > in our PR for now and built the > Windows-specific CPU feature detection on top of it. > > [1] > https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html > [2] https://openjdk.java.net/jeps/8251280 Monica Beckwith has updated the pull request incrementally with one additional commit since the last revision: change string representation for r18 to "r18_tls" on every platform - Changes: - all: https://git.openjdk.java.net/jdk/pull/212/files - new: https://git.openjdk.java.net/jdk/pull/212/files/398d7645..15466ab2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=212=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=212=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/212.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/212/head:pull/212 PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
On Tue, 29 Sep 2020 07:11:34 GMT, Matthias Baesken wrote:
>> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>>
>>> 127: NSColor* result = nil;
>>> 128:
>>> 129: if (colorIndex < ((useAppleColor) ?
>>> sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>>> java_awt_SystemColor_NUM_COLORS)) {
>>
>> This looks like a plain old bug fix, nothing really to do with the compiler
>> upgrade. The new compiler presumably just
>> has a new warning that brought attention to the problem. As such, I don't
>> think it belongs in a PR that's about issues
>> related to the compiler upgrade. Someone might want to backport just this
>> fix, for example.
>
> Hello Kim, Paul - so should we open a separate bug for the
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue because
> it might be a general problem just
> uncovered by the new compiler ? Paul , do you want to do this ? Or should I
> open a bug and post a separate change for
> the useAppleColor check in CSystemColors.m ?
I created
https://bugs.openjdk.java.net/browse/JDK-8253791
8253791: Issue with useAppleColor check in CSystemColors.m
for this issue (Kim and Paul are fine to have a separate JBS issue for this)
-
PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v7]
On Fri, 25 Sep 2020 12:44:37 GMT, Andrew Haley wrote:
>> Monica Beckwith has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - os_windows: remove duplicated UMA handling
>> - test_safefetch{32,N} works fine on win+aarch64
>
> Marked as reviewed by aph (Reviewer).
@theRealAph okay, I've changed the string representation of `r18` to
`"r18_tls"` on every platform.
-
PR: https://git.openjdk.java.net/jdk/pull/212
RE: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
Hi, Matthias, Kim. No problem opening a separate issue to fix CSystemColors.m.
Thanks,
Paul
On 9/29/20, 4:23 AM, "hotspot-dev on behalf of Kim Barrett"
wrote:
> On Sep 29, 2020, at 3:14 AM, Matthias Baesken
wrote:
>
> On Fri, 25 Sep 2020 06:06:31 GMT, Kim Barrett
wrote:
>
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>>
>>> Thanks,
>>> Paul
>>
>> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>>
>>> 127: NSColor* result = nil;
>>> 128:
>>> 129: if (colorIndex < ((useAppleColor) ?
sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>>> java_awt_SystemColor_NUM_COLORS)) {
>>
>> This looks like a plain old bug fix, nothing really to do with the
compiler upgrade. The new compiler presumably just
>> has a new warning that brought attention to the problem. As such, I
don't think it belongs in a PR that's about issues
>> related to the compiler upgrade. Someone might want to backport just
this fix, for example.
>
> Hello Kim, Paul - so should we open a separate bug for the
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue
because it might be a general problem just
> uncovered by the new compiler ? Paul , do you want to do this ? Or should
I open a bug and post a separate change for
> the useAppleColor check in CSystemColors.m ?
I think so.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
> On Sep 29, 2020, at 3:59 AM, Matthias Baesken
> wrote:
>
> On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes wrote:
>
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>>
>>> Thanks,
>>> Paul
>>
>> src/hotspot/share/runtime/sharedRuntime.cpp line 2851:
>>
>>> 2849: if (buf != NULL) {
>>> 2850: CodeBuffer buffer(buf);
>>> 2851: short locs_buf[80];
>>
>> This code is just weird. Why is the locs_buf array not declared to be the
>> desired type? If the compiler rejects double
>> because it isn't relocInfo* then why does it accept short? And if it accepts
>> short will it accept int or long long or
>> int64_t? Using int64_t would be a clearer change. Though semantically this
>> code is awful. :( Should it be intptr_t ?
>
> Currently we have in the existing code various kind of buffers passed into
> initialize_shared_locs that compile nicely
> on all supported compilers and on Xcode 12 as well ,see
>
> c1_Compilation.cpp
>
> 326 char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
> 327 code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
>
> sharedRuntime.cpp
>
> 2681 CodeBuffer buffer(buf);
> 2682 short buffer_locs[20];
> 2683 buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
> 2684
> sizeof(buffer_locs)/sizeof(relocInfo));
>
> So probably using short would be consistent to what we do already in the
> coding at another place (looking into
> relocInfo it would be probably better to use unsigned short or to typedef
> unsigned short in the relocInfo class
> and use the typedef).
That’s *not* consistent, because of buffer alignment. The call to
NEW_RESOURCE_ARRAY is going
to return a pointer to memory that is 2*word aligned. (Interesting, possibly a
32/64 bit “bug” here.)
The existing code in sharedRuntime.cpp is allocating double-aligned.
iniitalize_shared_locs wants a
HeapWordSize-aligned buffer, and adjusts the pointer it uses accordingly. (I
think with existing code
it could just make the alignment of the buffer a precondition, but I haven’t
looked at all callers.)
Changing the declaration in sharedRuntime.cpp to short[] reduces the alignment
requirement for the
array, possibly requiring alignment adjustment (and hence size reduction) by
initialize_shared_locs.
Since initialize_shared_locs specifically adjusts the alignment, some
downstream use of the buffer
probably actually cares.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
> On Sep 29, 2020, at 3:14 AM, Matthias Baesken
> wrote:
>
> On Fri, 25 Sep 2020 06:06:31 GMT, Kim Barrett wrote:
>
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>>
>>> Thanks,
>>> Paul
>>
>> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>>
>>> 127: NSColor* result = nil;
>>> 128:
>>> 129: if (colorIndex < ((useAppleColor) ?
>>> sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>>> java_awt_SystemColor_NUM_COLORS)) {
>>
>> This looks like a plain old bug fix, nothing really to do with the compiler
>> upgrade. The new compiler presumably just
>> has a new warning that brought attention to the problem. As such, I don't
>> think it belongs in a PR that's about issues
>> related to the compiler upgrade. Someone might want to backport just this
>> fix, for example.
>
> Hello Kim, Paul - so should we open a separate bug for the
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue because
> it might be a general problem just
> uncovered by the new compiler ? Paul , do you want to do this ? Or should I
> open a bug and post a separate change for
> the useAppleColor check in CSystemColors.m ?
I think so.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v11]
On Mon, 28 Sep 2020 19:53:37 GMT, Monica Beckwith wrote: >> This is a continuation of >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html >> >> Changes since then: >> * We've improved the write barrier as suggested by Andrew [1] >> * The define-guards around R18 have been changed to `R18_RESERVED`. This >> will be enabled for Windows only for now but >> will be required for the upcoming macOS+Aarch64 [2] port as well. >> * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov >> in our PR for now and built the >> Windows-specific CPU feature detection on top of it. >> >> [1] >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html >> [2] https://openjdk.java.net/jeps/8251280 > > Monica Beckwith has updated the pull request incrementally with one > additional commit since the last revision: > > r18 only on Linux Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v10]
On 28/09/2020 20:12, Bernhard Urban-Forster wrote:
> The idea is that the naming should suggest that `r18` shouldn't be used on
> that particular platform. Same is true for
> macOS, but the ABI docs suggest a different usage, hence we have something
> like that in our internal branch for macOS:
> Suggestion:
>
> "r17", NOT_R18_RESERVED("r18") WIN64_ONLY("rtls")
> MACOS_ONLY("rplatform"), "r19",
> Are you suggesting it should rather be something like this eventually?
> Suggestion:
>
> "r17", LINUX_ONLY("r18") WIN64_ONLY("rtls") MACOS_ONLY("rplatform"),
> "r19",
This is wrong. We can't use the register in Linux either, except in
Linux-specific code of which there is almost none. It's also a
pointless complication. r18 should be called r18_tls everywhere, as
discussed.
--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd.
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v10]
On 28/09/2020 20:12, Bernhard Urban-Forster wrote:
> The idea is that the naming should suggest that `r18` shouldn't be used on
> that particular platform. Same is true for
> macOS, but the ABI docs suggest a different usage, hence we have something
> like that in our internal branch for macOS:
> Suggestion:
>
> "r17", NOT_R18_RESERVED("r18") WIN64_ONLY("rtls")
> MACOS_ONLY("rplatform"), "r19",
> Are you suggesting it should rather be something like this eventually?
> Suggestion:
>
> "r17", LINUX_ONLY("r18") WIN64_ONLY("rtls") MACOS_ONLY("rplatform"),
> "r19",
This wrong. We can't use the register in Linux only, except in Linux-specific
code of which there is almost none. It should be called r18_tls everywhere,
as discussed.
--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd.
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes wrote:
>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>
>> Thanks,
>> Paul
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 2851:
>
>> 2849: if (buf != NULL) {
>> 2850: CodeBuffer buffer(buf);
>> 2851: short locs_buf[80];
>
> This code is just weird. Why is the locs_buf array not declared to be the
> desired type? If the compiler rejects double
> because it isn't relocInfo* then why does it accept short? And if it accepts
> short will it accept int or long long or
> int64_t? Using int64_t would be a clearer change. Though semantically this
> code is awful. :( Should it be intptr_t ?
Currently we have in the existing code various kind of buffers passed into
initialize_shared_locs that compile nicely
on all supported compilers and on Xcode 12 as well ,see
c1_Compilation.cpp
326 char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
327 code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
sharedRuntime.cpp
2681 CodeBuffer buffer(buf);
2682 short buffer_locs[20];
2683 buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
2684
sizeof(buffer_locs)/sizeof(relocInfo));
So probably using short would be consistent to what we do already in the coding
at another place (looking into
relocInfo it would be probably better to use unsigned short or to typedef
unsigned short in the relocInfo class
and use the typedef).
-
PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)
On Fri, 25 Sep 2020 06:06:31 GMT, Kim Barrett wrote:
>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>
>> Thanks,
>> Paul
>
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>
>> 127: NSColor* result = nil;
>> 128:
>> 129: if (colorIndex < ((useAppleColor) ?
>> sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>> java_awt_SystemColor_NUM_COLORS)) {
>
> This looks like a plain old bug fix, nothing really to do with the compiler
> upgrade. The new compiler presumably just
> has a new warning that brought attention to the problem. As such, I don't
> think it belongs in a PR that's about issues
> related to the compiler upgrade. Someone might want to backport just this
> fix, for example.
Hello Kim, Paul - so should we open a separate bug for the
src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue because
it might be a general problem just
uncovered by the new compiler ? Paul , do you want to do this ? Or should I
open a bug and post a separate change for
the useAppleColor check in CSystemColors.m ?
-
PR: https://git.openjdk.java.net/jdk/pull/348
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v11]
On Mon, 28 Sep 2020 19:53:37 GMT, Monica Beckwith wrote: >> This is a continuation of >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html >> >> Changes since then: >> * We've improved the write barrier as suggested by Andrew [1] >> * The define-guards around R18 have been changed to `R18_RESERVED`. This >> will be enabled for Windows only for now but >> will be required for the upcoming macOS+Aarch64 [2] port as well. >> * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov >> in our PR for now and built the >> Windows-specific CPU feature detection on top of it. >> >> [1] >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html >> [2] https://openjdk.java.net/jeps/8251280 > > Monica Beckwith has updated the pull request incrementally with one > additional commit since the last revision: > > r18 only on Linux Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/212
