Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253375: OSX build fails with Xcode 12.0 (12A7209) #348

Closed
wants to merge 3 commits into from

Conversation

phohensee
Copy link
Member

@phohensee phohensee commented Sep 24, 2020

Please review this small patch to enable the OSX build using Xcode 12.0.

Thanks,
Paul

/issue add JDK-8253375
/summary Replace double array with short array in AdapterHandlerLibrary::create_native_wrapper, add parens around ?: in CSystemColors:getColor


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8253375: OSX build fails with Xcode 12.0 (12A7209)

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/348/head:pull/348
$ git checkout pull/348

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 24, 2020

👋 Welcome back phh! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 24, 2020
@openjdk
Copy link

openjdk bot commented Sep 24, 2020

@phohensee This issue is referenced in the PR title - it will now be updated.

@openjdk
Copy link

openjdk bot commented Sep 24, 2020

@phohensee Setting summary to Replace double array with short array in AdapterHandlerLibrary::create_native_wrapper, add parens around ?: in CSystemColors:getColor

@openjdk
Copy link

openjdk bot commented Sep 24, 2020

@phohensee The following labels will be automatically applied to this pull request: awt hotspot.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label (add|remove) "label" command.

@openjdk openjdk bot added hotspot hotspot-dev@openjdk.org awt client-libs-dev@openjdk.org labels Sep 24, 2020
@phohensee
Copy link
Member Author

phohensee commented Sep 24, 2020

/cc build

@openjdk openjdk bot added the build build-dev@openjdk.org label Sep 24, 2020
@openjdk
Copy link

openjdk bot commented Sep 24, 2020

@phohensee
The build label was successfully added.

@openjdk
Copy link

openjdk bot commented Sep 24, 2020

@phohensee The hotspot label was already applied.

@openjdk openjdk bot added the 2d client-libs-dev@openjdk.org label Sep 24, 2020
@openjdk
Copy link

openjdk bot commented Sep 24, 2020

@phohensee
The 2d label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Sep 24, 2020

Webrevs

prrace
prrace approved these changes Sep 24, 2020
Copy link
Contributor

@prrace prrace left a comment

The awt change looks fine although it would be nice if you could paste the compiler
error message into the bug report, since I'd like to see the compiler's reason why this is ambiguous today and not before - assuming that is the issue.

@openjdk
Copy link

openjdk bot commented Sep 24, 2020

@phohensee This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details.

After integration, the commit message for the final commit will be:

8253375: OSX build fails with Xcode 12.0 (12A7209)

Replace double array with short array in AdapterHandlerLibrary::create_native_wrapper, add parens around ?: in CSystemColors:getColor

Reviewed-by: prr, kbarrett, lucy

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 12 new commits pushed to the master branch:

  • 4622a18: 8253791: Issue with useAppleColor check in CSystemColors.m
  • ac02afe: 8253721: Flag -XX:AVX3Threshold does not accept Zero value
  • 4d9f207: 7151826: [TEST_BUG] [macosx] The test javax/swing/JPopupMenu/4966112/bug4966112.java not for mac
  • 1eca634: 8252000: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jdb
  • a55cde4: 8252002: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jdwp
  • ffc97ba: 8253543: sanity/client/SwingSet/src/ButtonDemoScreenshotTest.java failed with "AssertionError: All pixels are not black"
  • 5310d85: 8252937: Correct "no comment" warnings from javadoc in java.math package
  • 8df3e72: 8218685: jlink --list-plugins needs to be readable and tidy
  • 2fe0a5d: 8253476: TestUseContainerSupport.java fails on some Linux kernels w/o swap limit capabilities
  • fb20690: 8253637: Update EC removal
  • ... and 2 more: https://git.openjdk.java.net/jdk/compare/d5be82948b588d6eb0543d72d94abcd4ddfb9ae5...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 24, 2020
@@ -2848,7 +2848,7 @@ void AdapterHandlerLibrary::create_native_wrapper(const methodHandle& method) {
BufferBlob* buf = buffer_blob(); // the temporary code buffer in CodeCache
if (buf != NULL) {
CodeBuffer buffer(buf);
double locs_buf[20];
short locs_buf[80];
Copy link
Member

@dholmes-ora dholmes-ora Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@MBaesken MBaesken Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@kimbarrett kimbarrett left a comment

No, don't do this. In the original, double is used to obtain the desired
alignmnent. Changing the element type to short reduces the alignment
requirement for the variable. initialize_shared_locs will then adjust the
alignment, potentially shrinking the effective array size. So this is a
real change that I think shouldn't be made.

I think changing the declaration for locs_buf to any of the following gives
equivalent behavior to the current code. I don't know whether any of them
will trigger the new clang warning though. I don't have access to that
version right now, and the documentation for the warning is useless (like so
much of clang's warning options documentation).

(1) alignas(double) char locs_buf[20 * sizeof(double)];
(but alignas is not yet in the "permitted features" list in the Style Guide:
https://bugs.openjdk.java.net/browse/JDK-8252584)

(2) union { char locs_buf[20 * sizeof(double)]; double align; };

(3) std::aligned_storage_t<20 * sizeof(double)> locs_buf;
and change (relocInfo*)locs_buf => (relocInfo*)&locs_buf
and #include <type_traits>
This has the benefit of being explicit that we're just stack allocating a
block of storage.

I'll make a wild guess that (1) and (2) will still warn, though char arrays
are somewhat special in the language so maybe they won't. I'm pretty sure
(3) should do the trick.

@@ -126,7 +126,7 @@ + (void)reloadColors {
+ (NSColor*)getColor:(NSUInteger)colorIndex useAppleColor:(BOOL)useAppleColor {
NSColor* result = nil;

if (colorIndex < (useAppleColor) ? sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS : java_awt_SystemColor_NUM_COLORS) {
if (colorIndex < ((useAppleColor) ? sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS : java_awt_SystemColor_NUM_COLORS)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@MBaesken MBaesken Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@MBaesken MBaesken Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@mlbridge
Copy link

mlbridge bot commented Sep 25, 2020

Mailing list message from Kim Barrett on awt-dev:

On Sep 25, 2020, at 1:49 AM, Kim Barrett <kbarrett at openjdk.java.net> wrote:

On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee <phh at openjdk.org> wrote:

Please review this small patch to enable the OSX build using Xcode 12.0.

Thanks,
Paul

[I tried submitting this comment through the github UI and somehow managed to lose
all the context in the process. In case it?s not (eventually) obvious, this is about the
change to src/hotspot/share/runtime/sharedRuntime.cpp. Still figuring out github?]

@mlbridge
Copy link

mlbridge bot commented Sep 25, 2020

Mailing list message from Kim Barrett on awt-dev:

On Sep 25, 2020, at 1:49 AM, Kim Barrett <kbarrett at openjdk.java.net> wrote:

On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee <phh at openjdk.org> wrote:

Please review this small patch to enable the OSX build using Xcode 12.0.

Thanks,
Paul

[?]

I think changing the declaration for locs_buf to any of the following gives
equivalent behavior to the current code. [?]

[?]

Another variant that probably avoids both the warning and avoids any C++14 features:

(4) union { char data[20 * sizeof(double)]; double align; } locs_buf;
and change (relocInfo*)locs_buf => (relocInfo*)&locs_buf.

@RealLucy
Copy link
Contributor

RealLucy commented Sep 25, 2020

I checked Kim Barrett's proposals (1) and (2) on my machine (MacOS 10.15, Xcode 12.0). Both proposals make the warning go away.

Another note, actually, it's more like a question:
Has anyone had an eye on what happens in initialize_shared_locs(relocInfo* buf, int length)? To my understanding, "buf", which is passed in as "locs_buf", is stored into CodeBuffer fields. Is that a good idea? "locs_buf" points into the stack. This pointer becomes invalid at the end of the "locs_buf" scope. Did I get something wrong here?

@mlbridge
Copy link

mlbridge bot commented Sep 25, 2020

Mailing list message from Kim Barrett on 2d-dev:

On Sep 25, 2020, at 6:22 AM, Lutz Schmidt <lucy at openjdk.java.net> wrote:

On Fri, 25 Sep 2020 05:46:08 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

Another note, actually, it's more like a question:
Has anyone had an eye on what happens in initialize_shared_locs(relocInfo* buf, int length)? To my understanding,
"buf", which is passed in as "locs_buf", is stored into CodeBuffer fields. Is that a good idea? "locs_buf" points into
the stack. This pointer becomes invalid at the end of the "locs_buf" scope. Did I get something wrong here?

It?s ?odd? but I think it?s more or less okay. Here and in other uses we seem to be allocating
dynamically scoped storage for temporary use by the CodeBuffer. I think a more normal
formulation might be to allocate the buffer, then pass it to the CodeBuffer constructor as a
non-transfer of ownership. But I haven?t looked at all the places where this is used, and that?s
perhaps out of scope for the problem at hand.

moqimoqidea added a commit to HeyTool/jdk that referenced this pull request Sep 28, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 29, 2020

Mailing list message from Kim Barrett on 2d-dev:

On Sep 29, 2020, at 3:14 AM, Matthias Baesken <mbaesken at openjdk.java.net> wrote:

On Fri, 25 Sep 2020 06:06:31 GMT, Kim Barrett <kbarrett at openjdk.org> 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.

@mlbridge
Copy link

mlbridge bot commented Sep 29, 2020

Mailing list message from Kim Barrett on 2d-dev:

On Sep 29, 2020, at 3:59 AM, Matthias Baesken <mbaesken at openjdk.java.net> wrote:

On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes <dholmes at openjdk.org> 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.

Copy link
Contributor

@RealLucy RealLucy left a comment

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.

@mlbridge
Copy link

mlbridge bot commented Sep 29, 2020

Mailing list message from Kim Barrett on 2d-dev:

On Sep 29, 2020, at 4:05 PM, Lutz Schmidt <lucy at openjdk.java.net> wrote:

On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee <phh at openjdk.org> 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*)&locs_buf`. That second change is in the PR.

Copy link
Contributor

@RealLucy RealLucy left a comment

I just copied the declaration and oversaw that tiny little '&'. With it, sharedRuntime.cpp compiles fine. Sorry for not being careful enough. Reviewed.

@phohensee
Copy link
Member Author

phohensee commented Sep 30, 2020

/integrate

@openjdk openjdk bot closed this Sep 30, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 30, 2020
@openjdk
Copy link

openjdk bot commented Sep 30, 2020

@phohensee Since your change was applied there have been 17 commits pushed to the master branch:

  • 04775f1: 8253768: Deleting unused pipe_class definitions in adl-file (x86_64.ad).
  • dc3a0f5: 8253183: Fragile memory barrier selection for some weak memory model platforms
  • 8331e63: 8253778: ShenandoahSafepoint::is_at_shenandoah_safepoint should not access VMThread state from other threads
  • 4c65365: 8253792: javax.swing.text.GapContent documentation typos
  • 3078b5f: 6441211: Small Error in API at javax.swing.plaf.synth.Region
  • 4622a18: 8253791: Issue with useAppleColor check in CSystemColors.m
  • ac02afe: 8253721: Flag -XX:AVX3Threshold does not accept Zero value
  • 4d9f207: 7151826: [TEST_BUG] [macosx] The test javax/swing/JPopupMenu/4966112/bug4966112.java not for mac
  • 1eca634: 8252000: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jdb
  • a55cde4: 8252002: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jdwp
  • ... and 7 more: https://git.openjdk.java.net/jdk/compare/d5be82948b588d6eb0543d72d94abcd4ddfb9ae5...master

Your commit was automatically rebased without conflicts.

Pushed as commit f80a606.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@phohensee phohensee deleted the JDK-8253375 branch Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
6 participants