8286562: GCC 12 reports some compiler warnings #8646
Conversation
|
👋 Welcome back ysuenaga! A progress list of the required criteria for merging this PR into |
|
@YaSuenag The following labels will be automatically applied to this pull request:
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. |
Webrevs
|
There was a problem hiding this comment.
The harfbuzz disabled warning looks good, so build changes are approved. You'll still need approval for the rest of the changes.
While it's not my place really to say about the code changes, I think hiding the warnings with pragmas like this is the least attractive option. But if the code owners are okay with it...
|
/reviewers 2 |
|
@YaSuenag 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: 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 83 new commits pushed to the
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 |
|
@magicus |
|
Thanks @magicus for your review!
Agree, so I fixed bugs which were found out by compiler warnings in this PR - they are in libjli. |
|
It is better to add pragma to |
| JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd); | ||
| #pragma GCC diagnostic pop |
There was a problem hiding this comment.
Can we just replace this code rather than putting pragmas here?
There was a problem hiding this comment.
I tried several patterns, but I couldn't find out a solution other than pragmas. Do you have any ideas?
For example:
if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) < sizeof(name)) {
JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd);
} else {
return 0;
}Compiler warnings:
/home/ysuenaga/github-forked/jdk/src/java.base/unix/native/libjli/java_md_common.c:135:45: error: '%s' directive output may be truncated writing up to 4094 bytes into a region of size between 2 and 4096 [-Werror=format-truncation=]
135 | JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd);
| ^~
In file included from /usr/include/stdio.h:894,
from /home/ysuenaga/github-forked/jdk/src/java.base/share/native/libjli/java.h:29,
from /home/ysuenaga/github-forked/jdk/src/java.base/unix/native/libjli/java_md_common.c:26:
In function 'snprintf',
inlined from 'Resolve' at /home/ysuenaga/github-forked/jdk/src/java.base/unix/native/libjli/java_md_common.c:135:7:
/usr/include/bits/stdio2.h:71:10: note: '__builtin___snprintf_chk' output between 2 and 8190 bytes into a destination of size 4097
71 | return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
72 | __glibc_objsize (__s), __fmt,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
73 | __va_arg_pack ());
| @@ -459,7 +459,7 @@ else | |||
|
|
|||
| HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits missing-field-initializers strict-aliasing | |||
| HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder delete-non-virtual-dtor strict-overflow \ | |||
| maybe-uninitialized class-memaccess unused-result extra | |||
| maybe-uninitialized class-memaccess unused-result extra use-after-free | |||
There was a problem hiding this comment.
Globally disabling use-after-free warnings for this package seems really
questionable. If these are problems in the code, just suppressing the warning
and leaving the problems to bite us seems like a bad idea. And the problems
ought to be reported upstream to the HarfBuzz folks.
There was a problem hiding this comment.
I don't understand what the actual warning is getting at .. can anyone explain it ?
FWIW the code is still the same in upstream harfbuzz
https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc
There was a problem hiding this comment.
I've pasted a part of warning messages to description of this PR, all of messages are here:
In function 'void trampoline_reference(hb_trampoline_closure_t*)',
inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*, hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2368:24:
/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2286:12: error: pointer 'trampoline' used after 'void free(void*)' [-Werror=use-after-free]
2286 | closure->ref_count++;
| ~~~~~~~~~^~~~~~~~~
In function 'void trampoline_destroy(void*)',
inlined from 'void trampoline_destroy(void*)' at /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2290:1,
inlined from 'void hb_font_funcs_set_nominal_glyph_func(hb_font_funcs_t*, hb_font_get_nominal_glyph_func_t, void*, hb_destroy_func_t)' at /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:733:1,
inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*, hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2363:40:
/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2299:8: note: call to 'void free(void*)' here
2299 | free (closure);
| ~~~~~^~~~~~~~~
There was a problem hiding this comment.
So upstream say it is not a problem since the analysis overlooks that it would not reach that free if it were immutable because of a previous check. I guess we disable the false positive warning for now.
| PRAGMA_DISABLE_GCC_WARNING("-Wformat-overflow") | ||
|
|
||
| #define PRAGMA_STRINGOP_OVERFLOW_IGNORED \ | ||
| PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow") |
There was a problem hiding this comment.
Are the reported problems with format/stringop-overflow real? Or are they
false positives? If they are real then I'm not going to approve ignoring them,
unless there is some urgent reason and there are followup bug reports for
fixing them.
There was a problem hiding this comment.
I think all of warnings in HotSpot are false-positives, so I added new pragmas to compilerWarnings.hpp, and use it in HotSpot code.
| JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd); | ||
| #pragma GCC diagnostic pop |
There was a problem hiding this comment.
Wouldn't it be better to just call JLI_Snprintf without the precheck and check the result to determine whether it was successful or was truncated? I think that kind of check is supposed to make gcc's truncation checker happy.
There was a problem hiding this comment.
The warning has gone when using return value from JLI_Snprintf() in new commit!
| strBufNextChar = strNewBufBegin + (strBufNextChar - strBufBegin); | ||
| #if defined(__GNUC__) && __GNUC__ >= 12 | ||
| #pragma GCC diagnostic pop | ||
| #endif |
There was a problem hiding this comment.
Rather than all this warning suppression cruft and the comment explaining why
it's okay, just compute the (strBufNextChar - strBufBegin) offset a few
lines earlier, before the realloc.
There was a problem hiding this comment.
I did do that in new commit, and the warning has gone!
| @@ -1626,7 +1626,7 @@ TranslateApplicationArgs(int jargc, const char **jargv, int *pargc, char ***parg | |||
| for (i = 0; i < jargc; i++) { | |||
| const char *arg = jargv[i]; | |||
| if (arg[0] == '-' && arg[1] == 'J') { | |||
| *nargv++ = ((arg + 2) == NULL) ? NULL : JLI_StringDup(arg + 2); | |||
| *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg + 2); | |||
There was a problem hiding this comment.
I wonder if the client expects NULL strings in the result, or if the NULL value should be an empty string? If empty strings are okay, this would be simpler without the conditional, just dup from arg + 2 to the terminating byte (which might be immediate).
There was a problem hiding this comment.
NULL affects as a loop stopper in ParseArguments() as following:
static jboolean
ParseArguments(int *pargc, char ***pargv,
int *pmode, char **pwhat,
int *pret, const char *jrepath)
{
int argc = *pargc;
char **argv = *pargv;
int mode = LM_UNKNOWN;
char *arg;
*pret = 0;
while ((arg = *argv) != 0 && *arg == '-') {But I'm not sure it is valid, I think it might be discussed as another issue.
|
Mailing list message from Magnus Ihse Bursie on build-dev: On 2022-05-11 16:01, Kim Barrett wrote:
I agree that an upstream report would be nice, but it has been a /Magnus |
|
Thanks for all to review this PR! I think we should separate this issue as following:
I want to include the change of Awt2dLibraries.gmk (HarfBuzz) in this PR because it is 3rd party library. I will separate in above if I do not hear any objections, and this issue (PR) handles "suppress warnings" only. |
|
@YaSuenag From my PoV this sounds like a good suggestion. |
+1 |
|
I will see what upstream thinks about the harfbuzz warning but in the mean time we can just disable it. |
|
@magicus @prrace Thanks for your review! Can I get the review from HotSpot folks? @kimbarrett |
Already working on it. There are some I don't understand yet. |
| *dest = op(bits, *dest); | ||
| PRAGMA_DIAG_POP |
There was a problem hiding this comment.
I see no stringop here. I'm still trying to understand what the compiler is complaining about.
There was a problem hiding this comment.
I guess GCC cannot understand assert(dest != NULL immediately preceding it.
In file included from /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdLoadBarrier.inline.hpp:33,
from /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:30,
from /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:30:
In function 'void set_form(jbyte, jbyte*) [with jbyte (* op)(jbyte, jbyte) = traceid_or]',
inlined from 'void set(jbyte, jbyte*)' at /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:129:23,
inlined from 'static void JfrTraceIdBits::store(jbyte, const T*) [with T = Klass]' at /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:135:6,
inlined from 'static void JfrTraceId::tag_as_jdk_jfr_event(const Klass*)' at /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:106:3,
inlined from 'static void JdkJfrEvent::tag_as(const Klass*)' at /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:176:35:
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:99:9: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
99 | *dest = op(bits, *dest);
| ~~~~~~^~~~~~~~~~~~~~~~~
There was a problem hiding this comment.
I don't think this warning has anything to do with that NULL check. But I'm
still not understanding what it is warning about. The "region of size 0" part
of the warning message seems important, but I'm not (yet?) seeing how it could
be coming up with that. The code involved here is pretty contorted...
There was a problem hiding this comment.
It might be GCC bug...
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
This issue is for integer literal, but Comment 29 mentions address calculation (e.g. NULL + offset) - it is similar the problem in jfrTraceIdBits.inline.hp because dest comes from low_addr() (addr + low_offset).
There was a problem hiding this comment.
I don't see the similarity. That gcc bug is about literals used as addresses,
which get treated (suggested inappropriately) as NULL+offset, with NULL+offset
being a cause of warnings. I don't see that happening in our case. That is,
in our addr + low_offset, addr doesn't seem to be either a literal or NULL
that I can see.
It's hard for me to investigate this any further just by perusing the code, so
I'm in the process of getting set up to build with gcc12.x. That will let me
do some experiments. It may take me a few days to get to that point though.
There was a problem hiding this comment.
I spent some time looking into this one. I agree there is a false positive
here, and there doesn't seem to be a better solution than suppressing the
warning. I would prefer the change below, rather than what's proposed. Also
eliminate the changes to utilities/compilerWarnings files. This is a very
gcc-specific issue; there's no reason to generalize the solution. The reason
for relocating the suppression is to be able to describe the issue in more
detail in a context where that description makes sense.
template <typename T>
inline void JfrTraceIdBits::store(jbyte bits, const T* ptr) {
assert(ptr != NULL, "invariant");
// gcc12 warns "writing 1 byte into a region of size 0" when T == Klass.
// The warning seems to be a false positive. And there is no warning for
// other types that use the same mechanisms. The warning also sometimes
// goes away with minor code perturbations, such as replacing function calls
// with equivalent code directly inlined.
PRAGMA_DIAG_PUSH
PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
set(bits, traceid_tag_byte(ptr));
PRAGMA_DIAG_POP
}
|
In case of stringop-overflow errors (bytecodeAssembler.cpp, classFileParser.cpp, symbolTable.cpp) noted that offset of The warning has gone with following patch to cast diff --git a/src/hotspot/share/oops/array.hpp b/src/hotspot/share/oops/array.hpp
index 428c2e63384..ba146bb7813 100644
--- a/src/hotspot/share/oops/array.hpp
+++ b/src/hotspot/share/oops/array.hpp
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2000, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -113,7 +113,7 @@ protected:
bool contains(const T& x) const { return index_of(x) >= 0; }
T at(int i) const { assert(i >= 0 && i< _length, "oob: 0 <= %d < %d", i, _length); return _data[i]; }
- void at_put(const int i, const T& x) { assert(i >= 0 && i< _length, "oob: 0 <= %d < %d", i, _length); _data[i] = x; }
+ void at_put(const int i, const T& x) { assert(i >= 0 && i< _length, "oob: 0 <= %d < %d", i, _length); _data[static_cast<unsigned int>(i)] = x; }
T* adr_at(const int i) { assert(i >= 0 && i< _length, "oob: 0 <= %d < %d", i, _length); return &_data[i]; }
int find(const T& x) { return index_of(x); }
I think it is better than disable stringop-overflow in cpp files if we cannot disable it in array.hpp (I tried it, but GCC does not seem to disable it with |
| *dest = op(bits, *dest); | ||
| PRAGMA_DIAG_POP |
There was a problem hiding this comment.
I spent some time looking into this one. I agree there is a false positive
here, and there doesn't seem to be a better solution than suppressing the
warning. I would prefer the change below, rather than what's proposed. Also
eliminate the changes to utilities/compilerWarnings files. This is a very
gcc-specific issue; there's no reason to generalize the solution. The reason
for relocating the suppression is to be able to describe the issue in more
detail in a context where that description makes sense.
template <typename T>
inline void JfrTraceIdBits::store(jbyte bits, const T* ptr) {
assert(ptr != NULL, "invariant");
// gcc12 warns "writing 1 byte into a region of size 0" when T == Klass.
// The warning seems to be a false positive. And there is no warning for
// other types that use the same mechanisms. The warning also sometimes
// goes away with minor code perturbations, such as replacing function calls
// with equivalent code directly inlined.
PRAGMA_DIAG_PUSH
PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
set(bits, traceid_tag_byte(ptr));
PRAGMA_DIAG_POP
}
src/hotspot/share/oops/array.hpp
Outdated
| } | ||
| } | ||
|
|
||
| public: | ||
|
|
||
| // standard operations | ||
| int length() const { return _length; } | ||
| T* data() { return _data; } | ||
| T* data() const { return reinterpret_cast<T*>(reinterpret_cast<uintptr_t>(this) + base_offset_in_bytes()); } |
There was a problem hiding this comment.
Adding the const-qualifier to the data() function and then implicitly
casting it away (by casting through intptr_t) is wrong. Either don't
const-qualify (and leave it to some future use that needs such to address
appropriately), or have two functions. Also, the line length is excessive.
So this:
T* data() {
return reinterpret_cast<T*>(
reinterpret_cast<char*>(this) + base_offset_in_bytes());
}
and optionally add this:
const T* data() const {
return reinterpret_cast<const T*>(
reinterpret_cast<const char*>(this) + base_offset_in_bytes());
}
There was a problem hiding this comment.
Thanks a lot @kimbarrett !
I updated around stringop-overflow warning in jfrTraceIdBits.inline.hpp , and added two data() in Array class. They works fine on my GCC 12 on Fedora 36. Could you review again?
| if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 1) > PATH_MAX) return 0; | ||
| JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd); | ||
| snprintf_result = JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd); | ||
| if ((snprintf_result < 0) && (snprintf_result >= (int)sizeof(name))) { |
There was a problem hiding this comment.
Good catch! I fixed it in new commit.
| snprintf_result = JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd); | ||
| if ((snprintf_result < 0) && (snprintf_result >= (int)sizeof(name))) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Pre-existing: It seems odd that this returns 0 above and below, rather than returning NULL. I think there are one or two other places in this file that are similarly using literal 0 for a null pointer, though others are using NULL. Something to report and clean up separately from this change.
There was a problem hiding this comment.
I was wondering about that too.
I use NULL at L134, and have filed it as JDK-8287363. I will work for it after this issue.
|
/integrate |
|
Going to push as commit 410a25d.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
Mailing list message from David Holmes on build-dev: The new assertion in src/hotspot/share/utilities/globalDefinitions.hpp inline const char* type2name(BasicType t) { is failing with test compiler/jvmci/errors/TestInvalidDebugInfo.java I have filed: https://bugs.openjdk.java.net/browse/JDK-8287491 The test will probably need ProblemListing as it is causing major noise David On 28/05/2022 12:12 pm, Yasumasa Suenaga wrote: |
I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on Fedora 36.
As you can see, the warnings spreads several areas. Let me know if I should separate them by area.
Most of warnings can be ignored.
HarfBuzz is a third party library, so I think it is reasonable to disable warnings in Makefile.
However warnings for libjli seem to be bugs. Let me know if they should be filed as another bugs.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646$ git checkout pull/8646Update a local copy of the PR:
$ git checkout pull/8646$ git pull https://git.openjdk.java.net/jdk pull/8646/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8646View PR using the GUI difftool:
$ git pr show -t 8646Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8646.diff