313
edits
Exceptions: Difference between revisions
no edit summary
(ooms) |
No edit summary |
||
| (9 intermediate revisions by the same user not shown) | |||
| Line 6: | Line 6: | ||
* Remove all current OOM handling code | * Remove all current OOM handling code | ||
* Create contracts for outparams | |||
* Rewrite call sites that ignore nsresults | * Rewrite call sites that ignore nsresults | ||
| Line 11: | Line 13: | ||
* Rewrite call sites that use NS_SUCCEEDED | * Rewrite call sites that use NS_SUCCEEDED | ||
These steps are explained in more detail below. And by the way, we'd love to have community help with any of these. | These steps are explained in more detail below. And by the way, we'd love to have community help with any of these. Look for "Coding Task" below to find small but useful bites of work that you might be interested in starting on. | ||
= Improved OOM Handling = | = Improved OOM Handling = | ||
| Line 60: | Line 62: | ||
the if statement would be deleted. | the if statement would be deleted. | ||
For these tasks, we need a full automatic rewriting system, with a pattern detection component and a patch generation component. The example patterns given here can be detected by looking at ASTs, so [[Dehydra GCC]] would be a great tool to use for detecting them. Once we have the list of patterns (say, as line numbers where the if statements start), we can work on an Elsa-based patch generator. | |||
Coding Task 1: Dehydra GCC script to detect null pointer checks for allocated memory. | |||
Coding Task 2: Dehydra GCC script to detect explicit tests for NS_OUT_OF_MEMORY return value. | |||
= Contracts for Outparams = | |||
== Background == | |||
Some methods ("getters") 'return' a value other than an nsresult to the caller. Generally, the value is returned through a pointer-typed parameter, a.k.a., outparam. As of Mozilla 1.9, it's not always clear whether the returned value can be null if there is not error, or if the value can be changed if there is an error, etc., making things more confusing for callers. | |||
This situation needs to be cleared up for exceptions. With exceptions, we want these methods to return their value directly, which will have performance and developer sanity benefits. In that case, when there is an exception, no value is returned at all to the calling context. Thus, for all methods, '''when the error code is nonzero, outparams should not be modified'''. | |||
Some methods have the property that when the return code is NS_OK, the outparam is non-null (equivalently, when the outparam is non-null, the return code is nonzero). We want to identify these methods, because for them, a check for null is a valid way of checking for an error, and should be treated the same as an nsresult check for the other refactorings discussed here. | |||
== Code Changes == | |||
Coding Task 3: Create an analysis to check for methods that modify outparams when the return a nonzero nsresult. Eventually this analysis should be a gcc plugin so it can be integrated with the build process. | |||
Coding Task 4: Create an analysis to find methods that always return non-null outparams when the nsresult is NS_OK. Eventually, we want to annotate these methods with gcc attributes and enforce the annotation with a gcc plugin. | |||
= Fixing Ignored nsresults = | |||
== Background == | |||
There are a fair number of call sites that ignore nsresult return values. This can be for several reasons, including: | |||
* The caller checks failure using some other condition (e.g., a null return value) | |||
* The function being called always returns NS_OK. | |||
* At this call site, the caller has ensured that the function will succeed. | |||
* The caller doesn't need to respond to errors. | |||
These calls need checking before we can enable exceptions. In general, it won't be possible to ensure that a function doesn't throw an exception, especially if we use exceptions for OOM. Thus, call sites that now ignore nsresults need to be looked at and made exception safe. | |||
== Finding Ignored nsresults == | |||
The key need here is a tool to automatically find call sites that ignore return values. There is a script under development (by dmandelin) that does this, but it needs to be improved to handle all the special cases, such as checking for a null return value. | |||
Coding Task 5: Finish the ignored nsresult analysis. | |||
Once the list is in place, the calls will need manual attention. | |||
= Removing NS_SUCCEEDED = | |||
== Background == | |||
Many call sites check nsresults with NS_SUCCEEDED. For example: | |||
nsresult rv = p->PrepareToUse(); | |||
if (NS_SUCCEEDED(rv)) { | |||
p->Use(); | |||
} | |||
return rv; | |||
This doesn't make sense with exceptions, because with exceptions enabled, if PrepareToUse returns, then it succeeded. | |||
== Investigating NS_SUCCEEDED == | |||
We want to rewrite call sites that use NS_SUCCEEDED to look more like something that will work with exceptions. But it's not clear yet exactly what that looks like. The first step is to analyze some existing uses of NS_SUCCEEDED to get an idea of what patterns exist and how to rewrite them. The example above, with exceptions can look like: | |||
p->PrepareToUse(); | |||
p->Use(); | |||
Before we have exceptions, we will probably want something like: | |||
nsresult rv = p->PrepareToUse(); | |||
if (NS_FAILED(rv)) return rv; | |||
p->Use(); | |||
return NS_OK; | |||
This would be fairly easy to rewrite to the exceptions version, because the NS_FAILED check is easily identified as equivalent to letting the exception propagate to the caller. | |||
= Far Future Stuff = | |||
This was the original plan for implementing exceptions, now categorized as far future. Also hopefully these steps will be easier once we've done the medium-term stuff documented above. | |||
== Benefits of Exceptions == | == Benefits of Exceptions == | ||