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

8257420: Zero VM build broken with clang after JDK-8256726 due to strlen() is not a constexpr #1518

Closed
wants to merge 4 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Nov 30, 2020

Hi all,

The newly added STATIC_ASSERT [1] breaks the build of Zero VM with clang.
It complains that 'non-type template argument is not a constant expression' since strlen() [2] is not a constexpr.

Any comments?

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp#L1374
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/flags/jvmFlagLookup.hpp#L42


Progress

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

Testing

Linux additional Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (8/8 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8257420: Zero VM build broken with clang after JDK-8256726 due to strlen() is not a constexpr

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 30, 2020

👋 Welcome back jiefu! 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.

@DamonFool
Copy link
Member Author

DamonFool commented Nov 30, 2020

/issue add JDK-8257420
/test
/label add hotspot-runtime
/cc hotspot-runtime

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

openjdk bot commented Nov 30, 2020

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

@openjdk
Copy link

openjdk bot commented Nov 30, 2020

@DamonFool
The hotspot-runtime label was successfully added.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Nov 30, 2020
@openjdk
Copy link

openjdk bot commented Nov 30, 2020

@DamonFool The hotspot-runtime label was already applied.

@mlbridge
Copy link

mlbridge bot commented Nov 30, 2020

Webrevs

@shipilev
Copy link
Member

shipilev commented Nov 30, 2020

Argh. Since I meant to backport the JDK-8257420 to 11u and 8u, and David was questioning the usefulness of STATIC_ASSERT in the original review, probably the simplest solution is to drop the STATIC_ASSERT from there entirely. This would make it a one-liner fix.

@DamonFool
Copy link
Member Author

DamonFool commented Nov 30, 2020

Argh. Since I meant to backport the JDK-8257420 to 11u and 8u, and David was questioning the usefulness of STATIC_ASSERT in the original review, probably the simplest solution is to drop the STATIC_ASSERT from there entirely. This would make it a one-liner fix.

OK. Will do it soon.
Thanks.

const int add_len = 32; \
STATIC_ASSERT(add_len == strlen("Index out of bounds for length ")); \
/* Two integers, the additional message, and the null-terminator */ \
char message[2 * jintAsStringSize + add_len + 1]; \
Copy link
Member

@shipilev shipilev Nov 30, 2020

Choose a reason for hiding this comment

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

Sorry for another nit. Since STATIC_ASSERT is gone, we can inline add_len, and make the whole thing + 33.

Copy link
Member Author

@DamonFool DamonFool Nov 30, 2020

Choose a reason for hiding this comment

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

Fixed. Thanks.

@openjdk
Copy link

openjdk bot commented Nov 30, 2020

@DamonFool 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 details.

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

8257420: Zero VM build broken with clang after JDK-8256726 due to strlen() is not a constexpr

Reviewed-by: shade, stuefe

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 7 new commits pushed to the master branch:

  • 8969069: 8256995: [vector] Improve broadcast operations
  • 6eb25d7: 8254082: AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start, int end) is missing fast-path for String
  • 4c86e46: 8256810: Incremental rebuild broken on Macosx
  • 02ba519: 8255001: Move G1PeriodicGCTask to its own file
  • 8aaee53: 8256187: [TEST_BUG] Automate bug4275046.java test
  • a3e1980: 8256541: Sort out what version of awk is used in the build system
  • e3abe51: 8257418: C2: Rename barrier data member in MemNode and LoadStoreNode

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Nov 30, 2020
Copy link
Member

@tstuefe tstuefe left a comment

Looks fine.

I guess another solution would have been

#define MSGFMT "Index %d out of bounds for length %d"
#define add_len sizeof(MSGFMT)
..
char message[2 * jintAsStringSize + add_len]; 
..
jio_snprintf(message, sizeof(message), MSGFMT, \
...

to spare you the character counting

@DamonFool
Copy link
Member Author

DamonFool commented Nov 30, 2020

Looks fine.

I guess another solution would have been

#define MSGFMT "Index %d out of bounds for length %d"
#define add_len sizeof(MSGFMT)
..
char message[2 * jintAsStringSize + add_len]; 
..
jio_snprintf(message, sizeof(message), MSGFMT, \
...

to spare you the character counting

Nice skill. Got it.
Thanks.

@DamonFool
Copy link
Member Author

DamonFool commented Dec 1, 2020

/integrate

@openjdk openjdk bot closed this Dec 1, 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 Dec 1, 2020
@openjdk
Copy link

openjdk bot commented Dec 1, 2020

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

  • 822ee47: 8257242: [macOS] Java app crashes while switching input methods
  • 7d89852: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files
  • 4356469: 8230501: Class data support for hidden classes
  • 11dad14: 8257445: (zipfs) Add DataProvider to TestLocOffsetFromZip64EF.java
  • 29f86e0: 8256536: Newer AMD 19h (EPYC) Processor family defaults
  • 7f58a8e: 8213719: Both sect163r2 and sect163k1 are default curves for field size 163
  • ae5b526: 8257448: Clean duplicated non-null check in the SunJSSE provider implementation
  • 41dbc13: 8180352: Add Stream.toList() method
  • 8969069: 8256995: [vector] Improve broadcast operations
  • 6eb25d7: 8254082: AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start, int end) is missing fast-path for String
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/c0719605e79f7e91f528dd197dcb953f4ed99169...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0eaf0bb.

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

@DamonFool DamonFool deleted the JDK-8257420 branch Dec 1, 2020
@mlbridge
Copy link

mlbridge bot commented Dec 2, 2020

Mailing list message from David Holmes on hotspot-runtime-dev:

On 1/12/2020 3:11 am, Thomas Stuefe wrote:

On Mon, 30 Nov 2020 15:24:12 GMT, Jie Fu <jiefu at openjdk.org> wrote:

Hi all,

The newly added STATIC_ASSERT [1] breaks the build of Zero VM with clang.
It complains that 'non-type template argument is not a constant expression' since strlen() [2] is not a constexpr.

Any comments?

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp#L1374
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/flags/jvmFlagLookup.hpp#L42

Jie Fu has updated the pull request incrementally with one additional commit since the last revision:

Inline add_len

Looks fine.

I guess another solution would have been

#define MSGFMT "Index %d out of bounds for length %d"
#define add_len sizeof(MSGFMT)

I didn't realize that would work. I thought the use of the string
literal would decay into a "const char * const" and thus sizeof would
give the size of the pointer.

..
char message[2 * jintAsStringSize + add_len];

Except you subtract four for the two %d ocurrences.

David
-----

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
3 participants