8319233: AArch64: Build failure with clang due to -Wformat-nonliteral warning #16486

shqking · 2023-11-03T02:50:22Z

The root cause is that an incorrect variant of function VMError::report_and_die() is used. We should introduce another variadic function, just as macos_aarch64 did before.

GCC toolchain:
From [1][2], GCC differs from Clang in flag -Wformat-nonliteral slightly, i.e. GCC may not raise a warning if "the format function takes its fromat arguments as a va_list". That's why this issue is not exposed by GCC toolchain before.

Besides, I suppose platforms ppc and risc-v may have the same issue.

[1] https://gcc.gnu.org/onlinedocs/gcc-11.4.0/gcc/Warning-Options.html
[2] https://releases.llvm.org/14.0.0/tools/clang/docs/DiagnosticsReference.html

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8319233: AArch64: Build failure with clang due to -Wformat-nonliteral warning (Bug - P4)

Reviewers

Kim Barrett (@kimbarrett - Reviewer)
Evgeny Astigeevich (@eastig - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16486/head:pull/16486
$ git checkout pull/16486

Update a local copy of the PR:
$ git checkout pull/16486
$ git pull https://git.openjdk.org/jdk.git pull/16486/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16486

View PR using the GUI difftool:
$ git pr show -t 16486

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16486.diff

Webrev

Link to Webrev Comment

… warning The root cause is that an incorrect variant of function VMError::report_and_die() is used. We should introduce another variadic function, just as macos_aarch64 did before. GCC toolchain: From [1][2], GCC differs from Clang in flag -Wformat-nonliteral slightly, i.e. GCC may **not** raise a warning if "the format function takes its fromat arguments as a va_list". That's why this issue is not exposed by GCC toolchain before. Besides, I suppose platforms ppc and risc-v may have the same issue. [1] https://gcc.gnu.org/onlinedocs/gcc-11.4.0/gcc/Warning-Options.html [2] https://releases.llvm.org/14.0.0/tools/clang/docs/DiagnosticsReference.html

bridgekeeper · 2023-11-03T02:51:35Z

👋 Welcome back haosun! 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 · 2023-11-03T02:53:52Z

@shqking The following label will be automatically applied to this pull request:

hotspot

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

mlbridge · 2023-11-03T02:57:28Z

Webrevs

00: Full (0fd676f6)

shqking · 2023-11-03T02:57:53Z

I think ppc and risc-v have the same issue. however, I don't have the corresponding hardware.

I wonder if @TheRealMDoerr and @RealFYang could help verify:

whether ppc and risc-v have the build failure with clang toolchain (if clang is supported to build ppc/riscv)
if so, whether this patch could fix the build failure.
Thanks in advance.

TheRealMDoerr · 2023-11-03T11:41:20Z

PPC64 only supports clang on AIX. @jkern: Can you answer the 2 questions?

TheRealMDoerr · 2023-11-03T11:42:31Z

Sorry, that was the wrong jkern.
@JoKern65: Can you answer the 2 questions?

kimbarrett

Shared changes look goo. Platform-specific changes also look good to me,
subject to verification by the respective platform maintainers. Happy to see
the elimination of the va_list dummy; stuff.

openjdk · 2023-11-03T12:39:43Z

@shqking 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:

8319233: AArch64: Build failure with clang due to -Wformat-nonliteral warning

Reviewed-by: kbarrett, eastigeevich

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

1a21c1a: 8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
81db172: 8318955: Add ReleaseIntArrayElements in Java_sun_awt_X11_XlibWrapper_SetBitmapShape XlbWrapper.c to early return
be01caf: 8319323: FFM: Harmonize the @throws tags in the javadocs
ec79ab4: 8319268: Build failure with GCC8.3.1 after 8313643

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.

JoKern65 · 2023-11-03T15:13:45Z

During my switch to xlc17 I came across "error: format string is not a string literal" in several places. To fix this, I inserted lines like "DISABLED_WARNINGS_clang_aix_os_posix.cpp := format-nonliteral," in various places in the gmk files. So in principle we have the problem too, but the compiler didn't show up an error in os_aix_ppc.cpp or vmError.cpp.

We will test your PR overnight in our test suite to check for regressions.

eastig

lgtm

JoKern65 · 2023-11-04T07:17:25Z

We consumed your PR overnight. The AIX ppc64 based on clang was fine and without regressions.

RealFYang · 2023-11-04T07:25:20Z

We will test your PR overnight in our test suite to check for regressions.

@shqking : Sorry, I don't have a clang for linux-riscv at hand. I guess @VladimirKempik might want to give it a try :-)

VladimirKempik · 2023-11-04T07:43:14Z

I'll try ~~after the weekend~~

VladimirKempik · 2023-11-04T15:47:33Z

looks better on risc-v now, we still have issues with register storage specifier is deprecated and incompatible with c++17 for vm_version_linux_riscv.cpp line 77. but it's unrelated to this PR.

This issue has gone:

os_linux_riscv.cpp:255:62: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
  255 |         VMError::report_and_die(thread, uc, nullptr, 0, msg, detail_msg, va_dummy);
      |                                                              ^~~~~~~~~~
os_linux_riscv.cpp:255:74: error: variable 'va_dummy' is uninitialized when used here [-Werror,-Wuninitialized]
  255 |         VMError::report_and_die(thread, uc, nullptr, 0, msg, detail_msg, va_dummy);

shqking · 2023-11-06T01:05:03Z

Thanks a lot for your verification on ppc and risc-v. @JoKern65 and @VladimirKempik.
I also tested JDK build with gcc and clang on linux/aarch64, linux/x86 and macos/aarch64.

Hence, I think this PR is ready to go.
I will integrate it tomorrow if there are no more comments.

shqking · 2023-11-07T01:00:33Z

/integrate

openjdk · 2023-11-07T01:01:39Z

Going to push as commit 439ed04.
Since your change was applied there have been 22 commits pushed to the master branch:

e4803e0: 8318580: "javax/swing/MultiMonitor/MultimonVImage.java failing with Error. Can't find library: /open/test/jdk/java/awt/regtesthelpers" after JDK-8316053
cdf3373: 8319316: Clarify text around which layouts a linker supports
1696603: 8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12
b3126b6: 8319455: Test compiler/print/CompileCommandMemLimit.java times out
1c2ea1d: 8319153: Fix: Class is a raw type in ProcessTools
96e6e67: 4365952: Cannot disable JFileChooser
2d4bbf4: 8319465: Typos in javadoc of com.sun.management.OperatingSystemMXBean methods
8fb94fd: 8319379: G1: gc/logging/TestUnifiedLoggingSwitchStress.java crashes after JDK-8318894
b5c863b: 8316533: C2 compilation fails with assert(verify(phase)) failed: missing Value() optimization
377138c: 8318959: C2: define MachNode::fill_new_machnode() statically
... and 12 more: https://git.openjdk.org/jdk/compare/c788160f8acea7b58b54ad857b601bb7ffb53f8e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-07T01:01:45Z

@shqking Pushed as commit 439ed04.

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

openjdk bot added the rfr Pull request is ready for review label Nov 3, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 3, 2023

kimbarrett approved these changes Nov 3, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2023

eastig approved these changes Nov 3, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 7, 2023

openjdk bot closed this Nov 7, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 7, 2023

shqking deleted the jdk-8319233 branch November 7, 2023 01:01

8319233: AArch64: Build failure with clang due to -Wformat-nonliteral warning #16486

8319233: AArch64: Build failure with clang due to -Wformat-nonliteral warning #16486

Uh oh!

Conversation

shqking commented Nov 3, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Nov 3, 2023

Uh oh!

openjdk bot commented Nov 3, 2023

Uh oh!

mlbridge bot commented Nov 3, 2023

Webrevs

Uh oh!

shqking commented Nov 3, 2023

Uh oh!

TheRealMDoerr commented Nov 3, 2023

Uh oh!

TheRealMDoerr commented Nov 3, 2023

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoKern65 commented Nov 3, 2023

Uh oh!

eastig left a comment

Choose a reason for hiding this comment

Uh oh!

JoKern65 commented Nov 4, 2023

Uh oh!

RealFYang commented Nov 4, 2023

Uh oh!

VladimirKempik commented Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VladimirKempik commented Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shqking commented Nov 6, 2023

Uh oh!

shqking commented Nov 7, 2023

Uh oh!

openjdk bot commented Nov 7, 2023

Uh oh!

openjdk bot commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

shqking commented Nov 3, 2023 •

edited by openjdk bot

Loading

openjdk bot commented Nov 3, 2023 •

edited

Loading

VladimirKempik commented Nov 4, 2023 •

edited

Loading

VladimirKempik commented Nov 4, 2023 •

edited

Loading