Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Sun, 22 May 2022 03:15:20 GMT, Kim Barrett wrote:
>> Like the others, it is caused by `Array::at_put()`.
>>
>>
>> In file included from
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/annotations.hpp:28,
>> from
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/instanceKlass.hpp:29,
>> from
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/javaClasses.hpp:30,
>> from
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/precompiled/precompiled.hpp:35:
>> 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::symbol_at_put(int, Symbol*)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:362:15,
>> inlined from 'void
>> ClassFileParser::mangle_hidden_class_name(InstanceKlass*)' at
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/classFileParser.cpp:5966:21:
>
> `Array::_data` is a pseudo flexible array member. "Pseudo" because C++
> doesn't have flexible array members. The compiler is completely justified in
> complaining about the apparently out-of-bounds accesses.
>
> There is a "well-known" (though moderately ugly) approach to doing flexible
> array members in C++. Something like this:
>
>
> T* data() {
> return reinterpret_cast(
> reinterpret_cast(this) + data_offset());
> }
>
>
> where `data_offset()` is new and private:
>
>
> static size_t data_offset() {
> return offset_of(Array, _data);
> }
>
>
> Use `data()` everywhere instead of using `_data` directly.
>
> There are other places in HotSpot that use this kind of approach.
Thanks @kimbarrett for your advice! Warnings from array.hpp have gone with your
suggestion.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v8]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge remote-tracking branch 'upstream/master' into gcc12-warnings - Use getter function to access "_data" - Revert changes for bytecodeAssembler.cpp, classFileParser.cpp, symbolTable.cpp - revert changes for memnode.cpp and type.cpp - Add assert to check the range of BasicType - Merge remote-tracking branch 'upstream/master' into HEAD - Revert change for java.c , parse_manifest.c , LinuxPackage.c - Calculate char offset before realloc() - Use return value from JLI_Snprintf - Avoid pragma error in before GCC 12 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/c156bcc5...042c1c70 - Changes: https://git.openjdk.java.net/jdk/pull/8646/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=07 Stats: 42 lines in 7 files changed: 26 ins; 1 del; 15 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
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Sun, 22 May 2022 05:00:21 GMT, Kim Barrett wrote: >> 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: > > 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... It might be GCC bug... https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578 This issue is for integer literal, but [Comment 29](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c29) 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`). - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Sun, 22 May 2022 08:35:54 GMT, Yasumasa Suenaga wrote:
>> `Array::_data` is a pseudo flexible array member. "Pseudo" because C++
>> doesn't have flexible array members. The compiler is completely justified in
>> complaining about the apparently out-of-bounds accesses.
>>
>> There is a "well-known" (though moderately ugly) approach to doing flexible
>> array members in C++. Something like this:
>>
>>
>> T* data() {
>> return reinterpret_cast(
>> reinterpret_cast(this) + data_offset());
>> }
>>
>>
>> where `data_offset()` is new and private:
>>
>>
>> static size_t data_offset() {
>> return offset_of(Array, _data);
>> }
>>
>>
>> Use `data()` everywhere instead of using `_data` directly.
>>
>> There are other places in HotSpot that use this kind of approach.
>
> Thanks @kimbarrett for your advice! Warnings from array.hpp have gone with
> your suggestion.
Good. I see you found and used the existing `base_offset_in_bytes` (which I'd
overlooked), rather than using my suggested `data_offset`.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Sun, 22 May 2022 08:45:48 GMT, Yasumasa Suenaga wrote: >> 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... > > It might be GCC bug... > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578 > > This issue is for integer literal, but [Comment > 29](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c29) 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`). 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. - PR: https://git.openjdk.java.net/jdk/pull/8646
pre-submit tests for github PRs
Why is it that the vast majority of PRs are recording spurious looking failures of github pre-submit tests ? https://github.com/openjdk/jdk/pulls?q=type%3Apr+is%3Aopen+label%3Arfr There seems to be so much noise in these that I pay no attention to them any more (except for jcheck) but they seem to cause concern in some contributors who can't tell what they might have broken Even weirder is that we test Linux x86 there ?? When it isn't a mainstream platform. Why ? Especially since it seem to be the most common failure. -phil.
Re: pre-submit tests for github PRs
On 23/05/2022 8:22 am, Philip Race wrote: Why is it that the vast majority of PRs are recording spurious looking failures of github pre-submit tests ? https://github.com/openjdk/jdk/pulls?q=type%3Apr+is%3Aopen+label%3Arfr There seems to be so much noise in these that I pay no attention to them any more (except for jcheck) but they seem to cause concern in some contributors who can't tell what they might have broken Even weirder is that we test Linux x86 there ?? When it isn't a mainstream platform. Why ? Especially since it seem to be the most common failure. GHA tests a range of OpenJDK ports, not just the "mainstream platforms". The existing linux-86 failures (and others) are due to the Loom integration which only fully supports a couple of platforms and which broke all the other ports upon initial integration. Some folks in the community have been working through things to fix that. David -phil.
Re: pre-submit tests for github PRs
Yet right "at the top" of the list is Linux 86 ... do we have no way to put it on some 2ndary list ? -phil. On 5/22/22 3:58 PM, David Holmes wrote: On 23/05/2022 8:22 am, Philip Race wrote: Why is it that the vast majority of PRs are recording spurious looking failures of github pre-submit tests ? https://github.com/openjdk/jdk/pulls?q=type%3Apr+is%3Aopen+label%3Arfr There seems to be so much noise in these that I pay no attention to them any more (except for jcheck) but they seem to cause concern in some contributors who can't tell what they might have broken Even weirder is that we test Linux x86 there ?? When it isn't a mainstream platform. Why ? Especially since it seem to be the most common failure. GHA tests a range of OpenJDK ports, not just the "mainstream platforms". The existing linux-86 failures (and others) are due to the Loom integration which only fully supports a couple of platforms and which broke all the other ports upon initial integration. Some folks in the community have been working through things to fix that. David -phil.
Re: RFR: 8286262: Windows: Cleanup deprecation warning suppression
On Sun, 15 May 2022 20:50:25 GMT, Kim Barrett wrote: > Please review this cleanup of deprecation warning suppression when building > for Windows. > > This change consists of several parts. > > (1) Remove the global deprecation warning suppression when building HotSpot > for Windows. > > (2) Add macro definitions requesting suppression of selected sets of > deprecation warnings when building HotSpot for Windows. > > (3) Remove unnecessary forwarding macros for various POSIX functions in > globalDefinitions_visCPP.hpp. These were provided to avoid deprecation > warnings (that were previously also being suppressed by the global request). > They are now covered by the new macros provided by change (2) above. > > An alternative to item (3) is to not define _CRT_NONSTDC_NO_DEPRECATE (in item > (2)) and either retain the forwarding macros or define os:: wrapper functions > for all of the affected functions. We might eventually do the latter because > of other reasons for avoiding some of these functions, but the approach being > taken here is simpler. > > For documentation of _CRT_NONSTDC_NO_DEPRECATE, see: > https://docs.microsoft.com/en-us/cpp/c-runtime-library/compatibility > https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996 > > Similarly for _CRT_SECURE_NO_WARNINGS. > > Perhaps similarly for _WINSOCK_DEPRECATED_NO_WARNINGS (though I didn't find > any documentation for the latter). But it might be better to not supress the > warnings and instead use the alternatives (JDK-8286781). > > Testing: > mach5 tier1 All seems quite reasonable. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8718
