On Tue, 1 Jul 2025 12:25:04 GMT, Anton Artemov <d...@openjdk.org> wrote:
> 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.
Changes requested by kbarrett (Reviewer).
src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 926:
> 924: JVMCI::CodeInstallResult CodeInstaller::initialize_buffer(JVMCIObject
> compiled_code, CodeBuffer& buffer, HotSpotCompiledCodeStream* stream, u1
> code_flags, JVMCI_TRAPS) {
> 925: JavaThread* thread = stream->thread();
> 926: #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)`.
src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 936:
> 934: __builtin_unreachable();
> 935: }
> 936: #endif
I think this could be replaced with asserting non-null. A failed assert calls
a noreturn reporting
function.
src/hotspot/share/runtime/javaThread.hpp line 1079:
> 1077: }
> 1078: #endif
> 1079: return result;
Similarly here, I think this could be replaced with asserting non-null.
-------------
PR Review: https://git.openjdk.org/jdk/pull/26067#pullrequestreview-2979254552
PR Review Comment: https://git.openjdk.org/jdk/pull/26067#discussion_r2180140733
PR Review Comment: https://git.openjdk.org/jdk/pull/26067#discussion_r2180123147
PR Review Comment: https://git.openjdk.org/jdk/pull/26067#discussion_r2180120768
