CHECK at the end of a void function

[Coleen Phillimore]

My preference is to keep THREAD as an argument if you were going to use 
CHECK for the last statement of a function.  You have to do this if 
you're returning with a function that takes TRAPS. ie:
   return my_fun(THREAD);

For the most part, I don't think it matters if the compiler can optimize 
it away.  Do our compilers optimize away the extra check for pending 
exception?  I don't know if you answered this.

Lastly, please no, I don't want to see yet another macro for this 
special case.

Coleen

On 2/17/21 8:55 PM, David Holmes wrote:
> Hi Ioi,
>
> > CHECK at the end of a void function
>
> This isn't really about void functions, nor check at the end. It is 
> about using CHECK/CHECK_* on a call that is immediately followed by a 
> return from the current function as it degenerates to:
>
> if (EXCEPTION_OCCURRED)
>   return;
> else
>   return;
>
> On 18/02/2021 8:16 am, Ioi Lam wrote:
>> Converting this from a PR discussion 
>> (https://git.openjdk.java.net/jdk/pull/2494) to a regular mail.
>
> Thanks for doing that.
>
>> What are people's opinion of:
>>
>>      void bar(TRAPS);
>>
>>      void foo(TRAPS) {
>>         bar(CHECK);
>>      }
>>
>> vs
>>
>>      void foo(TRAPS) {
>>         bar(THREAD);
>>      }
>>
>> There's no mention of this in 
>> https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
>>
>> Advantage of CHECK:
>>
>> - More readable -- you don't need to ask yourself:
>>    does the callee need a THREAD, or is the callee a TRAPS function?
>
> But you also don't need to ask yourself that because it doesn't matter 
> when the next action is to return anyway.
>
>> - More maintainable. You don't accidentally miss a check if you add new
>>    code below
>>
>>      void foo(TRAPS) {
>>         bar(THREAD);
>>         baz();       // adding a new call ....
>>      }
>
> True but I would argue that you need to think about the behaviour of 
> bar when adding baz() regardless. This might be wrong:
>
> bar(CHECK);
> baz(); // <= critical code that must always be executed no matter what!
>
>> Note that we MUST use THREAD when returning a value (see 
>> https://bugs.openjdk.java.net/browse/JDK-6889002)
>>
>>      int x(TRAPS);
>>
>>      int y(TRAPS) {
>>         return x(THREAD);
>>      }
>
> I think this is an anti-pattern and we should prefer the more explicit:
>
> RetType ret = x(CHECK_*);
> return ret;
>
> if we want to emphasize use of CHECK.
>
>> so there's inconsistency. However, the compiler will given an error 
>> if you add code below the THREAD. So we don't have the maintenance 
>> issue as void functions:
>>
>>      int Y(TRAPS) {
>>         return X(THREAD);
>>         baz();
>>      }
>
> Don't quite follow that as you wouldn't write anything after a return 
> statement anyway.
>
>> Disadvantage of CHECK:
>>
>> - It's not guaranteed that the C compiler will elide it. The code 
>> gets pre-processed to
>>
>>      inlined bool ThreadShadow::has_pending_exception() const {
>>          return _pending_exception != NULL;
>>      }
>>
>>      void foo(Thread*  __the_thread__) {
>>          bar(__the_thread__);
>>          if 
>> (((ThreadShadow*)__the_thread__)->has_pending_exception()) return;
>>      }
>>
>> Is it safe to assume that any C compiler that can efficiently compile 
>> HotSpot will always elide the "if" line?
>
> I've no idea, but if we can rely on it then I'm okay with always using 
> CHECK. It was the redundant code execution that was my concern.
>
>> I am a little worried about the maintenance issue. If we really want 
>> to avoid the CHECK, I would prefer to have a new macro like:
>>
>>      void foo(TRAPS) {
>>         bar(CHECK_AT_RETURN);
>>      }
>>
>> which will be preprocessed to
>>
>>      void foo(....) {
>>         bar(_thread__); return;
>>      }
>>
>> So you can't accidentally add code below it.
>
> I agree that a new macro might be preferable than just using THREAD. 
> Coming up with a short but meaningful name will be a problem. :) The 
> point is we are not actually checking anything in this case.
>
> Thanks,
> David
>
>> Thanks
>>   -Ioi
>>