8266773: Release VM is broken with GCC 9 after 8214237

[DamonFool]

Hi all,

Release VM is broken with GCC 9 due to -Werror=format-overflow=.
And if this assert[1] is removed, fastdebug VM can also reproduce the same bug.
The key to reproduce it is to compile with -O3.

IMO, it seems like a false positive gcc bug.
But we'd better fix it since gcc 9 is assumed to be supported to build OpenJDK.

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp#L47

---

Progress

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

Issue

• JDK-8266773: Release VM is broken with GCC 9 after 8214237

Reviewers

• Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 0561a9c
• Leo Korinth (@lkorinth - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3941/head:pull/3941
$ git checkout pull/3941

Update a local copy of the PR:
$ git checkout pull/3941
$ git pull https://git.openjdk.java.net/jdk pull/3941/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3941

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3941.diff

[DamonFool]

/test
/label add hotspot-gc
/cc hotspot-gc

[DamonFool]

/label add hotspot-gc
/cc hotspot-gc

[DamonFool]

/issue add JDK-8266773

[DamonFool]

/label add hotspot-gc
/cc hotspot-gc

[kimbarrett]

No, don't do this. indent never returns NULL. It asserts the given argument is in bounds. The warning is a (gcc9-specific?) false positive, similar to JDK-8235819.

[DamonFool]

No, don't do this. indent never returns NULL. It asserts the given argument is in bounds. The warning is a (gcc9-specific?) false positive, similar to JDK-8235819.

Thanks @kimbarrett for your review.

Yes, I agree that this seems to be false positive with gcc 9.

Fastdebug build wouldn't fail due to the existence of the assert.
But release vm would fail since there is no such an assert.

So any suggestions?
Thanks.

[lkorinth]

Maybe instead use something like:
out->print("%*s%s", 2 * indent_level, "", "indented*s");
or:
out->print("%*c%s", 2 * indent_level, ' ', "indented*c");

And get rid of the "Indents" string array and associated functions.

[lkorinth]

The string version ("%*s") of mine is better as it will correctly indent the zero indentation case.

[DamonFool]

Maybe instead use something like:
out->print("%*s%s", 2 * indent_level, "", "indented*s");
or:
out->print("%*c%s", 2 * indent_level, ' ', "indented*c");

And get rid of the "Indents" string array and associated functions.

Good suggestion!

Thanks @lkorinth .
Will update it later if there is no objection.

[lkorinth]

#3934 solves the same problem via out->sp(indent_level * 2); please coordinate the fix with @tstuefe, and whatever solution you chose to use, please remove all of the old usages of indent().

[DamonFool]

#3934 solves the same problem via out->sp(indent_level * 2); please coordinate the fix with @tstuefe, and whatever solution you chose to use, please remove all of the old usages of indent().

Okay.
I didn't know @tstuefe 's PR before.

I'll close mine if @tstuefe would like to fix this issue.
Thanks.

[lkorinth]

Or do it the other way, yours is correctly linked to 8266773. Sync with Thomas so that we only have one pull request.

[DamonFool]

Or do it the other way, yours is correctly linked to 8266773. Sync with Thomas so that we only have one pull request.

I prefer @tstuefe 's out->sp(indent_level * 2).

Patch has been updated.

Testing:

• test/hotspot/jtreg/gc on Linux/x64

Thanks.

[bridgekeeper]

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

[openjdk]

@DamonFool
The hotspot-gc label was successfully added.

[mlbridge]

Webrevs

• 02: Full - Incremental (11ec4423)
• 01: Full - Incremental (0561a9c4)
• 00: Full (83b844bb)

[openjdk]

@DamonFool The hotspot-gc label was already applied.

[openjdk]

@DamonFool The hotspot-gc label was already applied.

[openjdk]

@DamonFool The hotspot-gc label was already applied.

[openjdk]

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

[openjdk]

@DamonFool The hotspot-gc label was already applied.

[openjdk]

@DamonFool The hotspot-gc label was already applied.

[mgkwill]

FYI: this patch appears to solve the build issue for me:

diff --git a/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp b/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
index 2e52bd9dcc6..846511ae521 100644
--- a/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
+++ b/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
@@ -319,7 +319,9 @@ void G1GCPhaseTimes::details(T* phase, const char* indent_str) const {
   LogTarget(Trace, gc, phases, task) lt;
   if (lt.is_enabled()) {
     LogStream ls(lt);
-    ls.print("%s", indent_str);
+    if (indent_str) {
+      ls.print("%s", indent_str);
+    }
     phase->print_details_on(&ls);
   }
 }
@@ -332,7 +334,7 @@ void G1GCPhaseTimes::log_phase(WorkerDataArray<double>* phase, uint indent_level
   for (uint i = 0; i < phase->MaxThreadWorkItems; i++) {
     WorkerDataArray<size_t>* work_items = phase->thread_work_items(i);
     if (work_items != NULL) {
-      out->print("%s", indent(indent_level + 1));
+      out->sp(indent_level * 2);
       work_items->print_summary_on(out, true);
       details(work_items, indent(indent_level + 1));
     }

Not sure if this is the correct solution.

[DamonFool]

FYI: this patch appears to solve the build issue for me:

diff --git a/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp b/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
index 2e52bd9dcc6..846511ae521 100644
--- a/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
+++ b/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
@@ -319,7 +319,9 @@ void G1GCPhaseTimes::details(T* phase, const char* indent_str) const {
   LogTarget(Trace, gc, phases, task) lt;
   if (lt.is_enabled()) {
     LogStream ls(lt);
-    ls.print("%s", indent_str);
+    if (indent_str) {
+      ls.print("%s", indent_str);
+    }
     phase->print_details_on(&ls);
   }
 }
@@ -332,7 +334,7 @@ void G1GCPhaseTimes::log_phase(WorkerDataArray<double>* phase, uint indent_level
   for (uint i = 0; i < phase->MaxThreadWorkItems; i++) {
     WorkerDataArray<size_t>* work_items = phase->thread_work_items(i);
     if (work_items != NULL) {
-      out->print("%s", indent(indent_level + 1));
+      out->sp(indent_level * 2);
       work_items->print_summary_on(out, true);
       details(work_items, indent(indent_level + 1));
     }

Not sure if this is the correct solution.

Thanks @mgkwill for your info.

It would be better to remove all of the old usages of indent(), which is suggested by @lkorinth .
Thanks.

[DamonFool]

/test

[openjdk]

@DamonFool you need to get approval to run the tests in tier1 for commits up until 0561a9c

[tstuefe]

hi @DamonFool ,

Thanks for tackling this! Feel free to go forward with your patch, I'll withdraw mine.

You patch looks fine. Nit: having a constant for chars-per-indentation would be nice instead of the literal 2 and " ". But I don't have strong emotions.

Cheers, Thomas

[openjdk]

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

8266773: Release VM is broken with GCC 9 after 8214237

Reviewed-by: stuefe, lkorinth

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

• d0daa72: 8266857: PipedOutputStream.sink should be volatile
• 381de0c: 8266753: jdk/test/lib/process/ProcTest.java failed with "Exception: Proc abnormal end"
• 2d2cd78: 8266761: AssertionError in sun.net.httpserver.ServerImpl.responseCompleted
• 9c9c47e: 8266813: Shenandoah: Use shorter instruction sequence for checking if marking in progress
• 0344e75: 8266794: Remove dead code notify_allocation_jvmti_allocation_event
• 9e6e222: 8266892: avoid maybe-uninitialized gcc warnings on linux s390x
• 6575566: 8266787: Potential overflow of pointer arithmetic in G1ArchiveAllocator
• 8468001: 8263452: Javac slow compilation due to algorithmic complexity
• 67cb22a: 8266601: Fix bugs in AddLNode::Ideal transformations
• 18e9d28: 8266676: G1: Remove dead code init_node_id_to_index_map()
• ... and 20 more: https://git.openjdk.java.net/jdk/compare/3af4efdfcfbbb52d38415374083c66c9e7b22604...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.

[DamonFool]

hi @DamonFool ,

Thanks for tackling this! Feel free to go forward with your patch, I'll withdraw mine.

You patch looks fine. Nit: having a constant for chars-per-indentation would be nice instead of the literal 2 and " ". But I don't have strong emotions.

Cheers, Thomas

Thanks @tstuefe .

What do you mean by having a constant for chars-per-indentation would be nice instead of the literal 2 and " "?
Could you please give me an example?
Thanks.

[lkorinth]

Nice!

[lkorinth]

I would prefer the fixed indentation spaces in the format string instead of in an extra argument. Same for the other cases.

[DamonFool]

I would prefer the fixed indentation spaces in the format string instead of in an extra argument. Same for the other cases.

Good.
That make sense to me.
Updated.
Thanks.

[lkorinth]

Looks good to me. Thanks for fixing this!

[tstuefe]

Looks still good.

..Thomas

[DamonFool]

Thanks @lkorinth and @tstuefe .
/integrate

[openjdk]

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

• f6c5a6b: 8266784: java/text/Collator/RuleBasedCollatorTest.java fails with jtreg 6
• 1356116: 8266456: Replace direct TKit.run() calls with jdk.jpackage.test.Annotations.Test annotation
• dfe8833: 8266783: java/lang/reflect/Proxy/DefaultMethods.java fails with jtreg 6
• 995e956: 8266225: jarsigner is using incorrect security property to show weakness of certs
• 0a12605: 8265448: (zipfs): Reduce read contention in ZipFileSystem
• acf02ed: 8208237: Re-examine defmeth tests and update as needed
• ac0287f: 8266770: Clean pending exception before running dynamic CDS dump
• 7a0a57c: 8266820: micro java/nio/SelectorWakeup.java has wrong copyright header
• d0daa72: 8266857: PipedOutputStream.sink should be volatile
• 381de0c: 8266753: jdk/test/lib/process/ProcTest.java failed with "Exception: Proc abnormal end"
• ... and 28 more: https://git.openjdk.java.net/jdk/compare/3af4efdfcfbbb52d38415374083c66c9e7b22604...master

Your commit was automatically rebased without conflicts.

Pushed as commit 974b9f7.

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