CHECK at the end of a void function

[David Holmes]

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
>