Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1617559670
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Mon, 3 Jul 2023 07:24:56 GMT, Daniel Jeliński wrote: >> Yes, I think that may have been the original intent. I'll add const on these >> functions. > > ...actually these methods are static, and static functions can't be > const-qualified. Ah okay :) - PR Review Comment: https://git.openjdk.org/jdk/pull/14674#discussion_r1250393159
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Mon, 3 Jul 2023 06:47:04 GMT, Daniel Jeliński wrote: >> src/hotspot/cpu/aarch64/aarch64.ad line 2288: >> >>> 2286: >>> //= >>> 2287: >>> 2288: const bool Matcher::match_rule_supported(int opcode) { >> >> Have to wonder if these were all meant to be `bool Match:xxx() const {`? > > Yes, I think that may have been the original intent. I'll add const on these > functions. ...actually these methods are static, and static functions can't be const-qualified. - PR Review Comment: https://git.openjdk.org/jdk/pull/14674#discussion_r1250385599
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Mon, 3 Jul 2023 00:19:56 GMT, David Holmes wrote: >> Please review this attempt to fix ignored-qualifiers warning. >> >> Example warnings: >> >> src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier >> on return type has no effect [-Wignored-qualifiers] >>CompiledMethod* volatile code() const; >>^ >> >> >> src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type >> qualifiers ignored on cast result type [-Wignored-qualifiers] >> 65 | event.set_source((const ModuleEntry* const)from_module); >>|^ >> >> >> The proposed fix removes the ignored qualifiers. >> In a few AD files I replaced `const` with `constexpr` where I noticed that >> the method is returning a compile-time constant, and other platforms use >> `constexpr` on the same method. >> >> Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux >> complete without errors. Cross-compile GHA builds also pass. > > src/hotspot/cpu/aarch64/aarch64.ad line 2288: > >> 2286: >> //= >> 2287: >> 2288: const bool Matcher::match_rule_supported(int opcode) { > > Have to wonder if these were all meant to be `bool Match:xxx() const {`? Yes, I think that may have been the original intent. I'll add const on these functions. - PR Review Comment: https://git.openjdk.org/jdk/pull/14674#discussion_r1250340625
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. I will approve this as-is but have to wonder whether many of these cases of const return types were intending to declare const functions? P.S. Forgot to say thanks for dealing with this! src/hotspot/cpu/aarch64/aarch64.ad line 2288: > 2286: > //= > 2287: > 2288: const bool Matcher::match_rule_supported(int opcode) { Have to wonder if these were all meant to be `bool Match:xxx() const {`? - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14674#pullrequestreview-1510051989 PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1617042549 PR Review Comment: https://git.openjdk.org/jdk/pull/14674#discussion_r1249926982
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. Can I get another +1 on this? or should I proceed with splitting? - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1614738114
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. I don't think that was the intention. There's a comment on `Method::_code` stating that the pointer can change at any point (so the pointer was supposed to be volatile), and `class CompiledMethod` does not have any `volatile` - qualified methods. There are similar cases where `const` pointer to a class was returned involving `JavaThread`, `ReferenceProcessor`, `PSCardTable` and `ShenandoahHeapRegion`; I suppose we could review if `const` could be added to the returned class, but that's outside of the scope of this PR. - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1612435102
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. In the example, was the intention perhaps `volatile CompiledMethod*` instead of `CompiledMethod* volatile`? - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1612090389
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. David: I think this part of the spec is relevant here: > A non-class non-array prvalue cannot be > [cv-qualified](https://en.cppreference.com/w/cpp/language/cv), [...]. (Note: > a function call or cast expression may result in a prvalue of non-class > cv-qualified type, but the cv-qualifier is generally immediately stripped > out.) [source](https://en.cppreference.com/w/cpp/language/value_category) given that the cv qualifiers are immediately stripped by the compiler, there's no point in providing them. In the particular volatile pointer case: the function performs a volatile read to get the pointer value (address). That address can then be used in a non-volatile manner. Kim: I realize that it's a big change, so thank you very much for reviewing it anyway! I was prepared to split it up, just wanted to know if this warning is something we want to fix. - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1611905364
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > Please review this attempt to fix ignored-qualifiers warning. > > Example warnings: > > src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier > on return type has no effect [-Wignored-qualifiers] >CompiledMethod* volatile code() const; >^ > > > src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type > qualifiers ignored on cast result type [-Wignored-qualifiers] > 65 | event.set_source((const ModuleEntry* const)from_module); >|^ > > > The proposed fix removes the ignored qualifiers. > In a few AD files I replaced `const` with `constexpr` where I noticed that > the method is returning a compile-time constant, and other platforms use > `constexpr` on the same method. > > Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete > without errors. Cross-compile GHA builds also pass. Looks good. Though mind-numbingly large; breaking something like this up into easier to digest chunks can make reviewing easier. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14674#pullrequestreview-1503384263
Re: RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
On Tue, 27 Jun 2023 12:22:43 GMT, Daniel Jeliński wrote: > warning: 'volatile' type qualifier on return type has no effect Can't say that I understand that warning. If I pass in a volatile pointer and return the same pointer then I would not expect to lose the volatile aspect of it. @kbarrett can you explain this one to me? - PR Comment: https://git.openjdk.org/jdk/pull/14674#issuecomment-1611299893
RFR: 8310948: Fix ignored-qualifiers warning in Hotspot
Please review this attempt to fix ignored-qualifiers warning. Example warnings: src/hotspot/share/oops/method.hpp:413:19: warning: 'volatile' type qualifier on return type has no effect [-Wignored-qualifiers] CompiledMethod* volatile code() const; ^ src/hotspot/share/jfr/periodic/jfrModuleEvent.cpp:65:20: warning: type qualifiers ignored on cast result type [-Wignored-qualifiers] 65 | event.set_source((const ModuleEntry* const)from_module); |^ The proposed fix removes the ignored qualifiers. In a few AD files I replaced `const` with `constexpr` where I noticed that the method is returning a compile-time constant, and other platforms use `constexpr` on the same method. Release, debug and slowdebug builds on Aarch64 / x64 and Mac / Linux complete without errors. Cross-compile GHA builds also pass. - Commit messages: - Fix other platforms - Fix shenandoah - Fix ignored-qualifiers warning Changes: https://git.openjdk.org/jdk/pull/14674/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14674&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310948 Stats: 223 lines in 74 files changed: 0 ins; 0 del; 223 mod Patch: https://git.openjdk.org/jdk/pull/14674.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14674/head:pull/14674 PR: https://git.openjdk.org/jdk/pull/14674