Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Thu, 12 May 2022 00:26:41 GMT, Yasumasa Suenaga wrote: >> 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 > > 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: 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. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:47:43 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c line 193: > >> 191: #if defined(__GNUC__) && __GNUC__ >= 12 >> 192: #pragma GCC diagnostic pop >> 193: #endif > > 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. I did do that in new commit, and the warning has gone! - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:43:55 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > src/java.base/unix/native/libjli/java_md_common.c line 135: > >> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) >> return 0; >> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, >> cmd); >> 135: #pragma GCC diagnostic pop > > 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. The warning has gone when using return value from `JLI_Snprintf()` in new commit! - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 14:27:27 GMT, Kim Barrett wrote:
>> src/java.base/share/native/libjli/java.c line 1629:
>>
>>> 1627: const char *arg = jargv[i];
>>> 1628: if (arg[0] == '-' && arg[1] == 'J') {
>>> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg +
>>> 2);
>>
>> Wow!
>
> 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).
`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.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:35:43 GMT, Kim Barrett wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid pragma error in before GCC 12
>
> src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 51:
>
>> 49:
>> 50: #define PRAGMA_STRINGOP_OVERFLOW_IGNORED \
>> 51: PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
>
> 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.
I think all of warnings in HotSpot are false-positives, so I added new pragmas
to compilerWarnings.hpp, and use it in HotSpot code.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 19:11:16 GMT, Phil Race wrote: >> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462: >> >>> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits >>> missing-field-initializers strict-aliasing >>> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder >>> delete-non-virtual-dtor strict-overflow \ >>> 462: maybe-uninitialized class-memaccess unused-result extra >>> use-after-free >> >> 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. > > 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 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: - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:35:00 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462: > >> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits >> missing-field-initializers strict-aliasing >> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder >> delete-non-virtual-dtor strict-overflow \ >> 462: maybe-uninitialized class-memaccess unused-result extra >> use-after-free > > 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. 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 - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:56:44 GMT, Kim Barrett wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid pragma error in before GCC 12
>
> src/java.base/share/native/libjli/java.c line 1629:
>
>> 1627: const char *arg = jargv[i];
>> 1628: if (arg[0] == '-' && arg[1] == 'J') {
>> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg +
>> 2);
>
> Wow!
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).
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 08:40:21 GMT, 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.
>>
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> *
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>>
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool*
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Avoid pragma error in before GCC 12
Changes requested by kbarrett (Reviewer).
make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462:
> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits
> missing-field-initializers strict-aliasing
> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder delete-non-virtual-dtor
> strict-overflow \
> 462: maybe-uninitialized class-memaccess unused-result extra
> use-after-free
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.
src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 51:
> 49:
> 50: #define PRAGMA_STRINGOP_OVERFLOW_IGNORED \
> 51: PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
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.
src/java.base/share/native/libjli/java.c line 1629:
> 1627: const char *arg = jargv[i];
> 1628: if (arg[0] == '-' && arg[1] == 'J') {
> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg + 2);
Wow!
src/java.base/unix/native/libjli/java_md_common.c line 135:
> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) return
> 0;
> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR,
> cmd);
> 135: #pragma GCC diagnostic pop
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.
src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c line 193:
> 191: #if defined(__GNUC__) && __GNUC__ >= 12
> 192: #pragma GCC diagnostic pop
> 193: #endif
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.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 12:48:38 GMT, Alan Bateman wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid pragma error in before GCC 12
>
> src/java.base/unix/native/libjli/java_md_common.c line 135:
>
>> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name))
>> return 0;
>> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR,
>> cmd);
>> 135: #pragma GCC diagnostic pop
>
> Can we just replace this code rather than putting pragmas here?
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:
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 08:40:21 GMT, 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. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Avoid pragma error in before GCC 12 src/java.base/unix/native/libjli/java_md_common.c line 135: > 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) return > 0; > 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, > cmd); > 135: #pragma GCC diagnostic pop Can we just replace this code rather than putting pragmas here? - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 08:40:21 GMT, 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. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Avoid pragma error in before GCC 12 It is better to add pragma to `Array::at_put()` in src/hotspot/share/oops/array.hpp , but I couldn't suppress warnings. So I added pragmas to its caller - bytecodeAssembler.cpp, classFileParser.cpp, symbolTable.cpp . - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 11:48:00 GMT, Magnus Ihse Bursie wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > 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... Thanks @magicus for your review! > 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... Agree, so I fixed bugs which were found out by compiler warnings in this PR - they are in libjli. I think we can ignore the others because they are already checked in other methods (e.g. `assert`), or due to structure of `Array` class which has payload in `_data[1]` (and it is also checked in `assert`). - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 08:40:21 GMT, 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. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Avoid pragma error in before GCC 12 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... - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
> 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. > > * -Wstringop-overflow > * src/hotspot/share/oops/array.hpp > * > src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp > > In member function 'void Array::at_put(int, const T&) [with T = unsigned > char]', > inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, > inlined from 'void ConstantPool::method_at_put(int, int, int)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, > inlined from 'ConstantPool* > BytecodeConstantPool::create_constant_pool(JavaThread*) const' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision: Avoid pragma error in before GCC 12 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8646/files - new: https://git.openjdk.java.net/jdk/pull/8646/files/8154f6ea..7f155256 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8646=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8646=00-01 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8646.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646 PR: https://git.openjdk.java.net/jdk/pull/8646
