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
