RFR (S): 8211394: CHECK_ must be used in the rhs of an assignment statement within a block
Ioi Lam
ioi.lam at oracle.com
Tue Oct 9 02:41:26 UTC 2018
Hi David,
The new changes look good to me.
Regarding defensive programming, I am all for it. But it seems to me the
case Paul proposes to defend would require two places to go wrong:
Klass* my_callee(TRAPS) {
do_something_that_can_fail(THREAD); // <-- forgets to check
return a_valid_klass_ptr;
}
Klass* me(TRAPS) {
Klass* k = my_callee(CHECK_NULL); // make sure NULL is returned
return k;
}
my_caller() {
Klass* k = me(THREAD); // <-- forgets to check
use(k);
}
I am not sure if the best way to defend this is to require all the
intermediaries to "do the right thing just in case", especially when
such a rule has no enforcement mechanism. Also, doing this for
non-pointer types seems less useful.
Perhaps we should think about how the TRAPS/CHECK macros can be
improved? I don't have a concrete proposal, but here's an idea. I am
using void return type for simplicity:
================ test.cpp
#include <stdio.h>
#define DEBUG 1// test for now
class Thread {
public:
Thread() : exception(0) {}
int exception;
};
class Traps {
public:
Thread* thread;
#ifdef DEBUG
int pass_count;
int check_count;
#endif
Traps()
#ifdef DEBUG
: pass_count(0), check_count(0)
#endif
{}
#ifdef DEBUG
~Traps() {
if (pass_count != check_count) {
printf("bad bad\n");
}
}
#endif
};
inline Traps pass(Traps& t) {
#ifdef DEBUG
t.pass_count++;
#endif
Traps new_t;
new_t.thread = t.thread;
return new_t;
}
inline void set_checked(Traps& t) {
#ifdef DEBUG
t.check_count ++;
#endif
}
inline void dummy() {}
#define TRAPS Traps t
#define CHECK pass(t)); set_checked(t); if (t.thread->exception)
return; dummy(
#define CHECK_0 pass(t)); set_checked(t); if (t.thread->exception)
return 0; dummy(
#define THREAD pass(t)); dummy(
#define HAS_PENDING_EXCEPTION has_pending_exceptions(t)
inline int has_pending_exceptions(Traps& t) {
set_checked(t);
return t.thread->exception;
}
void bar(TRAPS) {
return;
}
void foo(TRAPS) {
bar(CHECK);
}
int main() {
Thread thread;
Traps t;
t.thread = &thread;
foo(CHECK_0); // Good
foo(THREAD); // bad if you comment out the next line
if (HAS_PENDING_EXCEPTION) {printf("I checked");}
return 1;
}
================
foo(CHECK_0) expands to
foo(pass(t)); set_checked(t); if (t.thread->exception) return 0; dummy();
Then, for the intermediaries who are just passing along the result of a
call, we would use a new macro
#define RETURN_ONLY pass_only(t)
inline Traps pass_only(Traps& t) {
Traps new_t;
new_t.thread = t.thread;
return new_t;
}
Klass* me(TRAPS) {
return my_callee(RETURN_ONLY);
}
(We probably need some 'const' somewhere so C++ can pass the Thread*
directly in non-debug builds).
Thanks
- Ioi
On 10/8/18 6:12 PM, David Holmes wrote:
> Hi Ioi,
>
> tl;dr http://cr.openjdk.java.net/~dholmes/8211394/webrev.v2/
>
> On 9/10/2018 8:58 AM, Ioi Lam wrote:
>> 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.
>
> I agree that all functions should return an "error value" when an
> exception is detected. I also agree that the return value of any
> exception-throwing function should be examined only if there is no
> exception pending.
>
> I was on the fence as to whether one part of the code should assume
> the other part of the code was written correctly or whether it should
> be defensive. Paul (who filed the bug) wanted defensive. You object
> strongly to it. I've gone back to the review thread for JDK-8062808
> which fixed some return CHECK to return THREAD and no concerns were
> raised there about the change. It seems inappropriate for me to
> overrule all the previous reviewers when I'm on the fence anyway.
>
> So version 2:
>
> http://cr.openjdk.java.net/~dholmes/8211394/webrev.v2/
>
> Thanks,
> David
>
>> 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
>>>>
>>
More information about the hotspot-runtime-dev
mailing list