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

[Ioi Lam]

On 10/8/18 3:25 PM, David Holmes wrote:
> On 9/10/2018 8:04 AM, Ioi Lam wrote:
>>> On Oct 8, 2018, at 2:19 PM, David Holmes <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.
>>>
>>
>> Do we actually have a use case where the returned value is 
>> significant when an exception is thrown? That would seem pretty 
>> fragile to me.
>
> No it would be an error. The point is that unless you go and check 
> that the called method has correctly handled exceptions, you don't 
> know that. So rather than assume, or make the effort to establish, be 
> conservative.
>

Both the caller and callee have TRAPS in their argument, which means (to me)

        "the returned value is value is valid only if no exception has 
been thrown".

exceptions.hpp says

     // Macro naming conventions: Macros that end with _ require a 
result value to be returned. They
     // are for functions with non-void result type. The result value is 
usually ignored because of
     // the exception and is only needed for syntactic correctness. The 
_0 ending is a shortcut for
     // _(0) since this is a frequent case.


by doing the CHECK_NULL in the caller, all you're ensuring is

     "when this function has encountered an exception, it will always 
return NULL"

but this function's declaration already says the return value should be 
ignored on exception, so you're simply fixing a value that no one should 
read.

So, this seems to me like a pointless ritual.

Thanks
- Ioi


> David
>
>> If not, then returning whatever the caller returns seems good enough, 
>> and less code to write.
>>
>> Thanks
>> Ioi
>>
>>
>>> 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
>>