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

Hohensee, Paul hohensee at amazon.com
Tue Oct 9 17:50:18 UTC 2018


The new patch looks fine to me too. I don't have a problem with replacing CHECK_* with TRAPS as long as the programmer understands the callee method interface.

I pretty much always come down on the side of extreme defensive coding. Seen too many unlikely combinations result in app crashes or data corruption. Given hundreds of millions of deployments, "unlikely" becomes "certain".

Thanks,

Paul

On 10/8/18, 11:40 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:

    On 9/10/2018 12:41 PM, Ioi Lam wrote:
    > Hi David,
    > 
    > The new changes look good to me.
    
    Thanks Ioi.
    
    > 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:
    
    Yes it does. The called method has to do the wrong thing and the caller 
    of the middle method that just returns the called method, also has to do 
    the wrong thing.
    
    > 
    >     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:
    
    I'm not quite seeing how it hangs together - specifically where the 
    Traps instance is created and where it would get destructed. But if 
    think it worth pursuing ...
    
    Cheers,
    David
    
    > ================ 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