Exceptions: Difference between revisions

From MozillaWiki
Jump to navigation Jump to search
m (fixed header levels)
(edit & wikify s3)
Line 83: Line 83:
   } except (nserror e) {}
   } except (nserror e) {}


3. tested nsresults
== Making call sites exception safe ==


This is all the nsresult call sites not already covered. Finally, the hard one. My first thought is that we can look at the success and failure paths through the code and rearrange them to use exceptions. In most cases, the test should just be an if that branches on NS_FAILED or NS_SUCCEEDED, so that we can easily identify the success path and the failure path. Then, we can (logically) rewrite the code so it has this form:
Once the nothrows are identified (and optionally, call sites that ignore errors are rewritten), we are left with the harder job of rewriting all other call sites to be exception safe.


  nsresult rv = callSomeMethod();
We'll start with the assumption that call sites adhere to a simple pattern that's relatively easy to rewrite, namely:
  maybe some_junk();
  if (NS_FAILED(rv)) {
      maybe fixup();
      maybe return rv;
      maybe return NS_ERROR_FOO;
      maybe return 0;
  } else {
      maybe do_stuff();
  }


Having some_junk() would be very annoying--hopefully it is rare and we can just flag it for manual fixing. Otherwise, we rewrite the code to use exceptions. I think there are several distinct rewrite patterns depending on the return values and the presence of the various fixups. Hopefully they can be fit into a small number of basic categories.
  nsresult rv = callSomeMethod();
  // note: no code between call & rv test
  // note: also accept NS_SUCCEEDED(rv) with code in opposite then/else branches
  if (NS_FAILED(rv)) {
    maybe fixup();
    return rv OR return NS_ERROR_FOO or return 0;
  } else {
    maybe do_stuff();
  }   


- Case 1: return rv in orig code
This requires a static analysis that finds all call sites that don't match this pattern so they can be flagged for manual rewrite.


      FixupMgr f;
Given the simple pattern, the rewrite looks like this:
      callSomeMethod();
      maybe do_stuff();
 
- Case 2: all others


       try {
       try {
Line 113: Line 108:
       } except (nserror e) {
       } except (nserror e) {
         maybe fixup();
         maybe fixup();
         maybe throw NS_ERROR_FOO;
         maybe throw NS_ERROR_FOO; // if original returns new error code
         maybe return;
         maybe return;             // if original returns 0
       }
       }


In case 2 it is also an option to use a FixupMgr.
But there are at least two big practical problems with this rewrite.
 
One significant glitch is see is when the error handling block uses constructs like


X1.    NS_FAILED(rv) || foo
First, when the original code failure case ends with 'return rv', the natural thing to do with exceptions is let the exception propagate. (If we use the rewrite above, we're cutting off the stack trace, which is bad for debugging.) If there is no 'fixup' code, then this is easy: just have no try block. If there is 'fixup' code, then we should rewrite to use a compensator object:
X2.   NS_FAILED(rv) && foo
X3.   NS_FAILED(rv1) || NS_FAILED(rv2)
X4.   NS_FAILED(rv1) && NS_SUCCEEDED(rv2)


These seem to be pretty common. Some thoughts on handling them:
  // class created to run fixup
  class SomeCompensator {
    ~SomeCompensator() { fixup(); }
  };


X1. Here we can duplicate the failure code. The code to be dup'd should be small if we use FixupMgrs.
  // call site
  SomeCompensator();
  callSomeMethod();
 
This is the style recommended by C++ authorities. The problem here is that the code is no longer equivalent to the original nsresult code: the fixup is run on success as well as failure. We still might be able to produce this rewriting automatically, but we need to know more about how the fixups appear in our code and how they can be identified.


X2. Here we can guard the catch block actions with the additional condition foo.
The second problem is that the if condition is often a little more complicated than what our pattern allows. We also have:


X3 & X4 seem harder, maybe not worth thinking about right now. Hopefully there are not too many. X1 and X2 are common, though, I think.
* NS_FAILED(rv) || foo
* NS_FAILED(rv) && foo
* NS_FAILED(rv1) || NS_FAILED(rv2)
* NS_FAILED(rv1) && NS_SUCCEEDED(rv2)


Dave
Above, we said we'd flag these for manual fix, but some of them, like the first two, are pretty common and we might want to fix them automatically. The first two cases seem fairly easy to handle, but we need more information on how they're being used.

Revision as of 23:12, 20 December 2007

This is a discussion document for ideas about how to use static analysis to help refactor Mozilla to use C++ exceptions instead of nsresults. In particular, this is about how to modify the call sites of methods so that the code uses exception handling and is exception safe.

nothrow methods

Methods that never fail are the easiest to handle: exception safety at call sites is free. These are called nothrow methods following the Effective C++ terminology. If we identify nothrow methods and annotate them, then we can easily refactor their sites.

nothrow analysis

There are three ways a method can fail:

  • By returning a non-zero (non-NS_OK) value,
  • By calling another nsresult method that can fail (i.e., is not nothrow),
  • By using a C++ feature that can throw an exception. The primary example is allocating memory with new.

The static analysis must be designed to detect any of these conditions. Because it's hard to know what method will be called in C++, a good starting point for the analysis would be to say that a method is nothrow if it can only return 0, does not invoke new, and does not call any other method. If this doesn't pick up enough nothrow methods, we can either do it by hand or augment the analysis with a simple call graph construction.

Rewriting nothrow call sites

Some nothrow call sites may already ignore the return value. These call sites can be left alone (for exception rewriting--but outparamdel will rewrite many of them).

Otherwise, the call site should look like this:

 nsresult rv = callMethod();
 if (NS_FAILED(rv)) {
   stuff;
 }

Logically, this should be rewritten to:

 callMethod();
 nsresult rv = 0;
 if (NS_FAILED(rv)) {
   stuff;
 }

At this point, a standard branch folding and dead code elimination should be able to clean up the code to the desired:

 callMethod();

Call sites that ignore return values

Once nothrows are identified and taken care of, all the remaining call sites need to be made exception safe. Static analysis has no way of knowing the safety requirements, so we can't create a static analysis that just makes everything exception safe. But it seems reasonable to assume that existing call sites that test the nsresult do the right thing in the presence of errors, and then we can make a rewriting tool that will ensure the same thing is done with exceptions.

But first we need to take care of existing call sites that just ignore errors, because (a) that looks like a bug, and (b) if they are supposed to ignore errors, they'll need to be rewritten to ignore exceptions.

Analysis to detect ignored return values

Roughly, the analysis should flag all call sites where:

  • the method is not nothrow
  • there is no branch on NS_FAILED(rv) or NS_SUCCEEDED(rv)

The easy way is to look for if statements where the condition contains NS_FAILED(rv) or NS_SUCCEEDED(rv). But that's not perfect, because of things like:

  • Calling NS_FAILED directly on the function without a variable
  • Assigning the rv or the NS_FAILED test result to a variable then branching later
  • Overwriting the value of rv before the test (so the rv isn't actually branched on)

The easy way might be good enough for our purposes. If not, we can design an abstract interpretation that is more precise.

Rewriting ignored return values

If it turns out most of these are bugs, we can just flag them for manual repair.

On the other hand, if almost all of these sites are correct code, then we can handle an ignore site like this:

 callMethod();

by rewriting it to this:

 nsresult rv = callMethod();
 if (NS_FAILED(rv)) {}

and letting the full rewriting algorithm rewrite this to use exceptions. Or, we could rewrite it directly as:

 try {
   callMethod();
 } except (nserror e) {}

Making call sites exception safe

Once the nothrows are identified (and optionally, call sites that ignore errors are rewritten), we are left with the harder job of rewriting all other call sites to be exception safe.

We'll start with the assumption that call sites adhere to a simple pattern that's relatively easy to rewrite, namely:

 nsresult rv = callSomeMethod();
 // note: no code between call & rv test
 // note: also accept NS_SUCCEEDED(rv) with code in opposite then/else branches
 if (NS_FAILED(rv)) {
   maybe fixup();
   return rv OR return NS_ERROR_FOO or return 0;
 } else {
   maybe do_stuff();
 }    

This requires a static analysis that finds all call sites that don't match this pattern so they can be flagged for manual rewrite.

Given the simple pattern, the rewrite looks like this:

     try {
         callSomeMethod();
         maybe do_stuff();
     } except (nserror e) {
        maybe fixup();
        maybe throw NS_ERROR_FOO; // if original returns new error code
        maybe return;             // if original returns 0
     }

But there are at least two big practical problems with this rewrite.

First, when the original code failure case ends with 'return rv', the natural thing to do with exceptions is let the exception propagate. (If we use the rewrite above, we're cutting off the stack trace, which is bad for debugging.) If there is no 'fixup' code, then this is easy: just have no try block. If there is 'fixup' code, then we should rewrite to use a compensator object:

 // class created to run fixup
 class SomeCompensator {
   ~SomeCompensator() { fixup(); }
 };
 // call site
 SomeCompensator();
 callSomeMethod();
 

This is the style recommended by C++ authorities. The problem here is that the code is no longer equivalent to the original nsresult code: the fixup is run on success as well as failure. We still might be able to produce this rewriting automatically, but we need to know more about how the fixups appear in our code and how they can be identified.

The second problem is that the if condition is often a little more complicated than what our pattern allows. We also have:

  • NS_FAILED(rv) || foo
  • NS_FAILED(rv) && foo
  • NS_FAILED(rv1) || NS_FAILED(rv2)
  • NS_FAILED(rv1) && NS_SUCCEEDED(rv2)

Above, we said we'd flag these for manual fix, but some of them, like the first two, are pretty common and we might want to fix them automatically. The first two cases seem fairly easy to handle, but we need more information on how they're being used.