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

[David Holmes]

On 8/10/2018 7:03 PM, Thomas Stüfe wrote:
> Hi David,
> 
> looks good to me now. I'm surprised compilers do not warn about this though.

Yeah me too! They complain about silly things but let stuff like this go 
unmentioned. :(

Thanks again.

David

> Best Regards, Thomas
> 
> On Mon, Oct 8, 2018 at 9:03 AM David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Thomas,
>>
>> On 8/10/2018 4:48 PM, Thomas Stüfe wrote:
>>> Looks good.
>>
>> Thanks for looking at it.
>>
>>> You missed:
>>>
>>> classfile/verificationType.cpp line 113
>>>
>>>         return comp_this.is_component_assignable_from(comp_from, context,
>>>                                             from_field_is_protected, CHECK_false);
>>
>> Thank you - webrev updated in place with new file.
>>
>>> and possibly more, did not check further. Difficult to grep for when multi line.
>>
>> Yes. I don't know how to exhaustively detect this problem. :(
>>
>> Cheers,
>> David
>>
>>> Cheers, Thomas
>>>
>>>
>>>
>>> On Mon, Oct 8, 2018 at 12:08 AM David Holmes <david.holmes at oracle.com> 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