8320353: Reenable stringop-overflow warnings #26067

toxaart · 2025-07-01T12:25:04Z

Hi, please consider the following changes:

this PR addresses the issue of stringop-overflow warnings produced by GCC. The compiler does think that the thread pointer returned by JavaThread::current() can be null, though it cant. The thread pointer ends up being an argument in __atomic_load, and the compiler reports the warning related to argument of that method.

The patch adds a hint to the compiler by means of __builtin_unreachable() intrinsic, which tells the compiler that certain piece of code will never be reached (case of thread pointer being null). This solves the issue.

Tested in tiers 1-3 and GHA.

Progress

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

Issue

JDK-8320353: Reenable stringop-overflow warnings (Enhancement - P4)

Reviewers

Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26067

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-07-01T12:25:49Z

👋 Welcome back toxaart! 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 · 2025-07-01T12:26:08Z

@toxaart This change is no longer ready for integration - check the PR body for details.

openjdk · 2025-07-01T12:26:41Z

@toxaart The following labels will be automatically applied to this pull request:

build
graal
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 pull request command.

mlbridge · 2025-07-01T15:09:12Z

Webrevs

00: Full (3289ff19)

magicus

Build changes look good. Someone else will have to review the hotspot changes.

/reviewers 2

openjdk · 2025-07-01T19:03:13Z

@magicus
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

dholmes-ora

Sorry but I really dislike seeing this compiler-specific pollution in shared code. It is even worse that you have to put it in two places (what is so special about the jvmci code to require this?) just because gcc is "too dumb" keep track of things.

Also IIUC from JBS the problem was only seen building Zero, so maybe we can do something there that is Zero specific?

Sorry.

dougxc · 2025-07-02T08:56:21Z

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp

+#if defined(__GNUC__) && !defined(__clang__) && !defined(PRODUCT)
+  if (thread == nullptr) {
+    // This is to prevent --stringop-overflow warning from GCC on linux/fastdebug.
+    // GCC does believe that JavaThread::current() can return nullptr,


does believe -> believes

TheShermanTanker · 2025-07-02T09:04:35Z

src/hotspot/share/runtime/javaThread.hpp

@@ -1067,7 +1067,16 @@ class JavaThread: public Thread {
 public:
  // Returns the running thread as a JavaThread
  static JavaThread* current() {
-    return JavaThread::cast(Thread::current());
+    auto result = JavaThread::cast(Thread::current());


Why auto when the type is known?

TheShermanTanker · 2025-07-02T09:09:42Z

A less messy solution could be to use PRAGMA_STRINGOP_OVERFLOW_IGNORED instead. The conditionals for the PRAGMA IGNORED macros are already all handled for you, so there's no need to put conditionalizations at the use sites. As an example, simply do the following:

PRAGMA_DIAG_PUSH
PRAGMA_STRINGOP_OVERFLOW_IGNORED
// Problem code here
PRAGMA_DIAG_POP

The first 2 macros are placed directly in front of the problem code, and the last goes behind.

kimbarrett · 2025-07-02T13:50:26Z

src/hotspot/share/runtime/javaThread.hpp

+    __builtin_unreachable();
+  }
+#endif
+    return result;


Similarly here, I think this could be replaced with asserting non-null.

kimbarrett · 2025-07-02T13:51:24Z

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp

+    // therefore 2nd hint is needed.
+    __builtin_unreachable();
+  }
+#endif


I think this could be replaced with asserting non-null. A failed assert calls a noreturn reporting
function.

kimbarrett · 2025-07-02T13:58:50Z

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp

@@ -923,6 +923,17 @@ int CodeInstaller::estimate_stubs_size(HotSpotCompiledCodeStream* stream, JVMCI_
 // perform data and call relocation on the CodeBuffer
 JVMCI::CodeInstallResult CodeInstaller::initialize_buffer(JVMCIObject compiled_code, CodeBuffer& buffer, HotSpotCompiledCodeStream* stream, u1 code_flags, JVMCI_TRAPS) {
  JavaThread* thread = stream->thread();
+#if defined(__GNUC__) && !defined(__clang__) && !defined(PRODUCT)


Note that !PRODUCT does not imply ASSERT. There is also the "optimized" build type, which
defines neither PRODUCT nor ASSERT. I have no idea whether the false-positive gcc warnings
occur in that build configuration. If so, then my suggestion of using an assertion to give the compiler
the clue it needs won't work. But I'm guessing that isn't a problem, in which case the macro check
should be defined(ASSERT).

toxaart added 5 commits July 1, 2025 10:39

8320353: Reenabled stringop-overflow warning for linux zero/fastdebug

6739a18

8320353: More work

169a773

8320353: More work

db39eb5

8320353: Added debug only case

231480b

8320353: Fiixed build problem

06b3412

openjdk bot added graal graal-dev@openjdk.org build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jul 1, 2025

toxaart added 2 commits July 1, 2025 14:32

8320353: Fixed whitespace error

452d598

8320353: Fixed comment

3289ff1

toxaart marked this pull request as ready for review July 1, 2025 15:04

openjdk bot added the rfr Pull request is ready for review label Jul 1, 2025

magicus approved these changes Jul 1, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 1, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Jul 1, 2025

dholmes-ora reviewed Jul 2, 2025

View reviewed changes

dougxc reviewed Jul 2, 2025

View reviewed changes

TheShermanTanker reviewed Jul 2, 2025

View reviewed changes

kimbarrett suggested changes Jul 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8320353: Reenable stringop-overflow warnings #26067

8320353: Reenable stringop-overflow warnings #26067

toxaart commented Jul 1, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jul 1, 2025

Uh oh!

openjdk bot commented Jul 1, 2025 •

edited

Loading

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

mlbridge bot commented Jul 1, 2025

Uh oh!

magicus left a comment

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

dholmes-ora left a comment •

edited

Loading

Uh oh!

dougxc Jul 2, 2025

Uh oh!

TheShermanTanker Jul 2, 2025

Uh oh!

TheShermanTanker commented Jul 2, 2025 •

edited

Loading

Uh oh!

kimbarrett Jul 2, 2025

Uh oh!

kimbarrett Jul 2, 2025

Uh oh!

kimbarrett Jul 2, 2025

Uh oh!

Uh oh!

8320353: Reenable stringop-overflow warnings #26067

Are you sure you want to change the base?

8320353: Reenable stringop-overflow warnings #26067

Conversation

toxaart commented Jul 1, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 1, 2025

Uh oh!

openjdk bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

mlbridge bot commented Jul 1, 2025

Webrevs

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jul 1, 2025

Uh oh!

dholmes-ora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

TheShermanTanker Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

TheShermanTanker commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimbarrett Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

kimbarrett Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

kimbarrett Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toxaart commented Jul 1, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 1, 2025 •

edited

Loading

dholmes-ora left a comment •

edited

Loading

TheShermanTanker commented Jul 2, 2025 •

edited

Loading