Improper usage of CHECK macros generates unreachable code

[Volker Simonis]

On 10/7/09, Christian Thalinger <Christian.Thalinger at sun.com> wrote:
> On Wed, 2009-10-07 at 20:20 +1000, David Holmes - Sun Microsystems
>  wrote:
>
> > I dont think it sufficient to simply replace CHECK with THREAD. The
>  > problem is that the value being returned may be complete garbage. ie given:
>  >
>  >      return create_string_variable(ns, name, 0, s, CHECK);
>  >
>  > the intent is to return NULL if create_string_variable triggers an
>  > exception, but if we change this to:
>  >
>  >      return create_string_variable(ns, name, 0, s, THREAD);
>  >
>  > we don't know what value will be returned without examining the
>  > internals of create_string_variable. So the replacement would be unsafe
>  > unless we verified that create_string_variable always returns NULL if it
>  > detects an exception is generated, but if that were the case there would
>  > be no need to use the CHECK macro in the first place. We may find that
>  > we are returning a garbage value.
>
>
> That's true.  But the current CHECK actually does what THREAD would do,
>  given the macro expands to:
>
>
>   static PerfStringVariable* create_string_variable(CounterNS ns,
>                                                     const char* name,
>                                                     const char *s, TRAPS) {
>
>     return create_string_variable(ns, name, 0, s, THREAD);
>
>     if (HAS_PENDING_EXCEPTION) return NULL; (0);
>
>   };
>
>  I don't see how we would not return garbage here too.  So we have to
>  store the return value in temporary variable, as suggested by Volker.
>

That's exactly my point. Changing CHECK to THREAD would just keep the
current behaviour without the creation of unreachable code.

If we want or need a more elaborate solution (and this was what I was
actually asking in my first mail), we should store the result of a
call which uses the check macro into a temporary variable and return
that one. This will actually trigger the check in the CHECK macro as
expected.

My impression is that just replacing CHECK with THREAD would work
because usually the caller of the method which does the call with the
CHECK macro also checks for pending exception such that invalid return
values do no harm. At least this is my only explanation for why this
has not lead to any serious problems until now.

But personally I would vote for the much cleaner solution with the
temporary variable because it's not more overhead and there's still a
small chance to catch some undiscovered errors.

Regards,
Volker

>
>  -- Christian
>
>