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

[David Holmes]

On 9/10/2018 8:04 AM, Hohensee, Paul wrote:
> Also, patch looks good to me. And typeo: TRAPS s/b THREAD.

Thanks for looking at this Paul.

> On 10/8/18, 6:01 PM, "Hohensee, Paul" <hohensee at amazon.com> wrote:
> 
> In this example, if you want to define the interface to new_symbol
> as returning NULL if an exception is thrown, then you'd replace CHECK_NULL
with TRAPS, as the previous patches have done. You'd document that so
users of new_symbol could do that. Absent that definition (or if you're
a bit lazy!), you use CHECK_NULL.

Given that 99.9% of hotspot API's are not actually specified/documented ...

>      
>      I'd add a comment to exceptions.hpp along the lines of
>      
>      // Because the CHECK_ macros expand to multiple statements,
>      // they must be used in the context of a statement block such
>      // that the exception check is not dead code. So, e.g., they
>      // cannot be used as a return statement expression. Consider
>      // using THREAD instead if part of the method interface is that
>      // the callee does the exception check.

Okay will add that.

Still don't understand why the compilers don't help us out here. :(

Thanks,
David

>      Paul
>      
>      On 10/8/18, 5:21 PM, "hotspot-runtime-dev on behalf of David Holmes" <hotspot-runtime-dev-bounces at openjdk.java.net on behalf of david.holmes at oracle.com> wrote:
>      
>          Hi Ioi,
>          
>          On 9/10/2018 1:55 AM, Ioi Lam wrote:
>          > 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."
>          
>          Yes. Seems important to try and establish some consistency on how to
>          handle this.
>          
>          > 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.
>          
>          We shouldn't expect anything other than an "error return value" if an
>          exception is pending, but that relies on who/how the target code was coded.
>          
>          I don't think the code change will be at all noticeable in terms of size
>          of speed. Readability is subjective.
>          
>          Thanks,
>          David
>          -----
>          
>          > (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
>          >
>          
>      
>      
>