RFR (S): 8211394: CHECK_ must be used in the rhs of an assignment statement within a block

[Ioi Lam]

I am not sure if this change is really required:

Symbol* MethodFamily::generate_no_defaults_message(TRAPS) const {
- return SymbolTable::new_symbol("No qualifying defaults found", THREAD);
   + Symbol* s = SymbolTable::new_symbol("No qualifying defaults found", 
CHECK_NULL);
   + return s;

I think the rationale is "since SymbolTable is a different class, we 
don't know what value it returns when an exception is thrown."

However, the return value is a pointer type. Can we except anything 
other than NULL in case of an exception? I think using the CHECK_NULL 
here will make the code bigger and slower, and also harder to read.

(BTW, we have at 150+ occurrencesof 'return.*THREAD[)];' in the code.)

Thanks
- Ioi

On 10/7/18 3:08 PM, David Holmes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8211394
> webrev: http://cr.openjdk.java.net/~dholmes/8211394/webrev/
>
> If a CHECK_ macro is used on a function call that is part of a return 
> statement i.e.
>
> return foo(CHECK_NULL);
>
> then it expands into an unreachable if-statement that checks the 
> exception state:
>
> return foo();
> if (EXCEPTION_OCCURRED)
>   return NULL;
>
> This is obviously a programming error, but unfortunately not something 
> our often pedantic compilers complain about.
>
> There are two ways to fix:
>
> 1. Convert to assignment:
>
> T* t = foo(CHECK_NULL);
> return t;
>
> 2. If the method is local and its exception behaviour easily 
> discernible and matches the expected behaviour, then change CHECK_ to 
> THREAD
>
> return foo(THREAD);
>
> Both fixes are applied as appropriate. As per the bug report I also 
> revisited an earlier fix in this area - JDK-8062808 - and made 
> adjustments.
>
> Thanks,
> David