On Thu, 15 Oct 2020 09:02:35 GMT, Bernhard Urban-Forster <[email protected]> wrote:
>> Changes requested by ihse (Reviewer). > > @theRealAph I prototyped changing the argument of `bang_stack_with_offset()` > from `int` to `size_t` here: > https://github.com/lewurm/openjdk/commit/85a8f655aa0cb69ef13a2de44dd64c60caf19852. > In that approach casting is > essentially pushed down to `bang_stack_with_offset` because the assembler > instruction of most (all) architectures that > is eventually consuming that offset needs a signed integer anyway. Doesn't > seem like a win to me to be honest. I would > rather prefer to go with what we have in this patch (similar to what x86 is > doing today): > --- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp > +++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp > @@ -1524,7 +1524,7 @@ nmethod* > SharedRuntime::generate_native_wrapper(MacroAssembler* masm, > > // Generate stack overflow check > if (UseStackBanging) { > - __ bang_stack_with_offset(JavaThread::stack_shadow_zone_size()); > + __ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size()); > } else { > Unimplemented(); > } > and leave it with that. What do you think? > @theRealAph I prototyped changing the argument of `bang_stack_with_offset()` > from `int` to `size_t` here: > [lewurm@85a8f65](https://github.com/lewurm/openjdk/commit/85a8f655aa0cb69ef13a2de44dd64c60caf19852). > In that approach > casting is essentially pushed down to `bang_stack_with_offset` because the > assembler instruction of most (all) > architectures that is eventually consuming that offset needs a signed integer > anyway. Doesn't seem like a win to me to > be honest. I would rather prefer to go with what we have in this patch > (similar to what x86 is doing today): ```diff > --- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp > +++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp > @@ -1524,7 +1524,7 @@ nmethod* > SharedRuntime::generate_native_wrapper(MacroAssembler* masm, > > // Generate stack overflow check > if (UseStackBanging) { > - __ bang_stack_with_offset(JavaThread::stack_shadow_zone_size()); > + __ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size()); > } else { > Unimplemented(); > } > ``` > > and leave it with that. What do you think? Fine, but please assert `JavaThread::stack_shadow_zone_size() == (int)JavaThread::stack_shadow_zone_size()`. If all this sounds a bit paranoid, that's because I am. Adding casts to shut up compilers is a very risky business, because often (if not in this case) the programmer doesn't understand the code well, and just sprinkles casts everywhere. But those casts disable compile-time type checking, and this leads to risks for future maintainability. I wonder if we should fix this in a better way, and use this in the future: template <typename T1, typename T2> T1 checked_cast(T2 thing) { guarantee(static_cast<T1>(thing) == thing, "must be"); return static_cast<T1>(thing); } Then I promise we'll never need to have this conversation again. ------------- PR: https://git.openjdk.java.net/jdk/pull/530
