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

[Hohensee, Paul]

Also, patch looks good to me. And typeo: TRAPS s/b THREAD.

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.
    
    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.
    
    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
        >