Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 16:15:39 GMT, Ioi Lam wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use pointer_delta_as_int for the name that uses pointer_delta, fix
>> negative case to just do checked_cast.
>
> src/hotspot/share/utilities/globalDefinitions.hpp line 529:
>
>> 527: template
>> 528: inline int pointer_delta_as_int(const volatile T* left, const volatile
>> T* right) {
>> 529: return checked_cast(pointer_delta(left, right, sizeof(T)));
>
> For clarity, I think you should add a comment saying the returned value is
> always non-negative.
done, thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245681394
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote: >> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but >> I've added a pointer delta function in globalDefinitions.hpp to use for >> these pointer diff calculations that return int everywhere. If the name is >> agreeable, I'll fix the other cases of this like this. It's better than raw >> casts. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Use pointer_delta_as_int for the name that uses pointer_delta, fix negative > case to just do checked_cast. Thanks Ioi and Fred and Stefan for suggestions. - PR Comment: https://git.openjdk.org/jdk/pull/14675#issuecomment-1612003549
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v5]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Clarify that pointer_delta_as_int() returns a non-negative int. - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/9fed8c89..856cafd4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote: >> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but >> I've added a pointer delta function in globalDefinitions.hpp to use for >> these pointer diff calculations that return int everywhere. If the name is >> agreeable, I'll fix the other cases of this like this. It's better than raw >> casts. >> Tested with tier1-4. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Use pointer_delta_as_int for the name that uses pointer_delta, fix negative > case to just do checked_cast. Looks good to me. - Marked as reviewed by fparain (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14675#pullrequestreview-1503631614
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
On Wed, 28 Jun 2023 13:17:21 GMT, Coleen Phillimore wrote:
>> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but
>> I've added a pointer delta function in globalDefinitions.hpp to use for
>> these pointer diff calculations that return int everywhere. If the name is
>> agreeable, I'll fix the other cases of this like this. It's better than raw
>> casts.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use pointer_delta_as_int for the name that uses pointer_delta, fix negative
> case to just do checked_cast.
LGTM.
src/hotspot/share/utilities/globalDefinitions.hpp line 529:
> 527: template
> 528: inline int pointer_delta_as_int(const volatile T* left, const volatile
> T* right) {
> 529: return checked_cast(pointer_delta(left, right, sizeof(T)));
For clarity, I think you should add a comment saying the returned value is
always non-negative.
-
Marked as reviewed by iklam (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14675#pullrequestreview-1503498625
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245463745
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v4]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Use pointer_delta_as_int for the name that uses pointer_delta, fix negative case to just do checked_cast. - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/858e9cfb..9fed8c89 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=02-03 Stats: 22 lines in 14 files changed: 0 ins; 1 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
On Wed, 28 Jun 2023 07:52:35 GMT, Stefan Karlsson wrote: >> Taking out that cast does work, so I've fixed that. > >> pointer_delta has different semantics > > Right. That was "recently" added to pointer_delta with JDK-8260046. It begs > the question why felt the need to add it there but feel that it is OK to skip > it for delta_as_int? Is there some usage of delta_as_int that gives back > negative values? Could that call site be changed? There are sites where the result is negative but this is a good suggestion because it makes the name more consistent. I can change those to plain check_casts. - PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1245141370
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
On Tue, 27 Jun 2023 15:11:34 GMT, Coleen Phillimore wrote: >> Hm. Maybe this would be ok. Our original idea was to make it T* not T until >> this cast. I don't know how many other cases there are that I haven't >> gotten to yet. But it would eliminate a cast, so that's good (unless these >> aren't the same). Some instances have ptr - constant that gets promoted I >> think. >> The reason we didn't pick pointer_delta_as_int because pointer_delta has >> different semantics. pointer_delta insists on positive results. > > Taking out that cast does work, so I've fixed that. > pointer_delta has different semantics Right. That was "recently" added to pointer_delta with JDK-8260046. It begs the question why felt the need to add it there but feel that it is OK to skip it for delta_as_int? Is there some usage of delta_as_int that gives back negative values? Could that call site be changed? - PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1244829942
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
On Tue, 27 Jun 2023 20:48:27 GMT, Coleen Phillimore wrote:
>> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but
>> I've added a pointer delta function in globalDefinitions.hpp to use for
>> these pointer diff calculations that return int everywhere. If the name is
>> agreeable, I'll fix the other cases of this like this. It's better than raw
>> casts.
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixed the comment.
delta_as_int() could also be: delta2int() ptrdelta2int() ptrdiff2int()
ptrsub2int() in the theme of shorter names.
pointer_delta_as_int() ignoring it does something different than
pointer_delta() (allows negative returns).
Change them all to check_cast(a - b) - losing identifiable name
ptrdiff_cast(a - b) where int ptrdiff_cast(ptrdiff_t val) { return
check(val); }
Brainstorming in a PR. Your suggestions welcome.
-
PR Comment: https://git.openjdk.org/jdk/pull/14675#issuecomment-1610330128
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v3]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fixed the comment. - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/99a15d6e..858e9cfb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=01-02 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v2]
On Tue, 27 Jun 2023 14:54:03 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/code/codeBlob.inline.hpp line 36:
>>
>>> 34: inline const ImmutableOopMap* CodeBlob::oop_map_for_slot(int slot,
>>> address return_address) const {
>>> 35: assert(_oop_maps != nullptr, "nope");
>>> 36: return _oop_maps->find_map_at_slot(slot, delta_as_int((intptr_t)
>>> return_address, (intptr_t) code_begin()));
>>
>> Is this the only usage of `delta_as_int` that operates on `intptr_t`? If we
>> remove the casts then all usages would operate on pointers. Maybe this is an
>> indication that we only need a `pointer_delta_as_int` function, to go hand
>> in hand with the other `pointer_delta` functions?
>
> Hm. Maybe this would be ok. Our original idea was to make it T* not T until
> this cast. I don't know how many other cases there are that I haven't gotten
> to yet. But it would eliminate a cast, so that's good (unless these aren't
> the same). Some instances have ptr - constant that gets promoted I think.
> The reason we didn't pick pointer_delta_as_int because pointer_delta has
> different semantics. pointer_delta insists on positive results.
Taking out that cast does work, so I've fixed that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1243922324
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files. [v2]
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but > I've added a pointer delta function in globalDefinitions.hpp to use for these > pointer diff calculations that return int everywhere. If the name is > agreeable, I'll fix the other cases of this like this. It's better than raw > casts. > Tested with tier1-4. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Make delta_as_int take pointers and fix cast. Missed one in stubCodeGenerator.hpp - Changes: - all: https://git.openjdk.org/jdk/pull/14675/files - new: https://git.openjdk.org/jdk/pull/14675/files/9b7fdef2..99a15d6e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14675&range=00-01 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14675/head:pull/14675 PR: https://git.openjdk.org/jdk/pull/14675
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Tue, 27 Jun 2023 12:59:59 GMT, Stefan Karlsson wrote:
>> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but
>> I've added a pointer delta function in globalDefinitions.hpp to use for
>> these pointer diff calculations that return int everywhere. If the name is
>> agreeable, I'll fix the other cases of this like this. It's better than raw
>> casts.
>> Tested with tier1-4.
>
> src/hotspot/share/code/codeBlob.inline.hpp line 36:
>
>> 34: inline const ImmutableOopMap* CodeBlob::oop_map_for_slot(int slot,
>> address return_address) const {
>> 35: assert(_oop_maps != nullptr, "nope");
>> 36: return _oop_maps->find_map_at_slot(slot, delta_as_int((intptr_t)
>> return_address, (intptr_t) code_begin()));
>
> Is this the only usage of `delta_as_int` that operates on `intptr_t`? If we
> remove the casts then all usages would operate on pointers. Maybe this is an
> indication that we only need a `pointer_delta_as_int` function, to go hand in
> hand with the other `pointer_delta` functions?
Hm. Maybe this would be ok. Our original idea was to make it T* not T until
this cast. I don't know how many other cases there are that I haven't gotten
to yet. But it would eliminate a cast, so that's good (unless these aren't the
same). Some instances have ptr - constant that gets promoted I think.
The reason we didn't pick pointer_delta_as_int because pointer_delta has
different semantics. pointer_delta insists on positive results.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1243892244
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Tue, 27 Jun 2023 12:41:50 GMT, Coleen Phillimore wrote:
> This is another version of PR https://github.com/openjdk/jdk/pull/14659 but
> I've added a pointer delta function in globalDefinitions.hpp to use for these
> pointer diff calculations that return int everywhere. If the name is
> agreeable, I'll fix the other cases of this like this. It's better than raw
> casts.
> Tested with tier1-4.
src/hotspot/share/code/codeBlob.inline.hpp line 36:
> 34: inline const ImmutableOopMap* CodeBlob::oop_map_for_slot(int slot,
> address return_address) const {
> 35: assert(_oop_maps != nullptr, "nope");
> 36: return _oop_maps->find_map_at_slot(slot, delta_as_int((intptr_t)
> return_address, (intptr_t) code_begin()));
Is this the only usage of `delta_as_int` that operates on `intptr_t`? If we
remove the casts then all usages would operate on pointers. Maybe this is an
indication that we only need a `pointer_delta_as_int` function, to go hand in
hand with the other `pointer_delta` functions?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14675#discussion_r1243696287
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Mon, 26 Jun 2023 17:47:25 GMT, Coleen Phillimore wrote:
> More casts for size of address differences in same code space,
> checked_casts<> and type changes to fix -Wconversion warnings. See CR for
> details.
> Tested with tier1-4.
After some offline conversation with Ioi, we thought of a better way to express
these pointer subtractions returning int, so I'm going to close this PR and
open a new one with at least these changed with that new function.
// delta usually called with pointers or intptr_t that returns an int
// used for code buffer and other nearby address calculations.
// This allows the left side to be less than the right side.
template
inline int delta_as_int(const volatile T left, const volatile T right) {
return checked_cast(left - right);
}
-
PR Comment: https://git.openjdk.org/jdk/pull/14659#issuecomment-1608292000
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Mon, 26 Jun 2023 20:19:47 GMT, Frederic Parain wrote: >> More casts for size of address differences in same code space, >> checked_casts<> and type changes to fix -Wconversion warnings. See CR for >> details. >> Tested with tier1-4. > > src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 220: > >> 218: // If caller is interpreted it already made room for the callee >> arguments >> 219: int overlap = caller.is_interpreted_frame() ? >> ContinuationHelper::InterpretedFrame::stack_argsize(hf) : 0; >> 220: const int fsize = >> checked_cast(ContinuationHelper::InterpretedFrame::frame_bottom(hf) - >> hf.unextended_sp() - overlap); > > This line computes the size of the unextended frame minus the overlap that > exists if the callee is using arguments in the caller’s frame. So this value > is lower or equal to the frame’ size which is encoded with an int. I don't > think a checked_cast is required here. This applies to the x86 version too. I'll make this change in the new PR. I'm going to redo the address calculations to call a function rather than the casts. - PR Review Comment: https://git.openjdk.org/jdk/pull/14659#discussion_r1242788203
Re: RFR: 8310906: Fix -Wconversion warnings in runtime, oops and some code header files.
On Mon, 26 Jun 2023 17:47:25 GMT, Coleen Phillimore wrote: > More casts for size of address differences in same code space, > checked_casts<> and type changes to fix -Wconversion warnings. See CR for > details. > Tested with tier1-4. Changes requested by fparain (Reviewer). src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 220: > 218: // If caller is interpreted it already made room for the callee > arguments > 219: int overlap = caller.is_interpreted_frame() ? > ContinuationHelper::InterpretedFrame::stack_argsize(hf) : 0; > 220: const int fsize = > checked_cast(ContinuationHelper::InterpretedFrame::frame_bottom(hf) - > hf.unextended_sp() - overlap); This line computes the size of the unextended frame minus the overlap that exists if the callee is using arguments in the caller’s frame. So this value is lower or equal to the frame’ size which is encoded with an int. I don't think a checked_cast is required here. This applies to the x86 version too. - PR Review: https://git.openjdk.org/jdk/pull/14659#pullrequestreview-1499357905 PR Review Comment: https://git.openjdk.org/jdk/pull/14659#discussion_r1242721648
