-
Notifications
You must be signed in to change notification settings - Fork 279
Use null check annotation everywhere #1229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -909,7 +909,7 @@ void goto_checkt::pointer_validity_check( | |
const exprt &access_ub, | ||
const irep_idt &mode) | ||
{ | ||
if(mode!=ID_java && !enable_pointer_check) | ||
if(!enable_pointer_check) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes: if for some reason you want to use goto-check on Java code (probably in this case you have omitted or customised java-bytecode-instrument in some custom driver program), the checks below that point are still inapplicable to Java. |
||
return; | ||
|
||
const exprt &pointer=expr.op0(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -566,6 +566,38 @@ void java_bytecode_instrumentt::operator()(exprt &expr) | |
instrument_code(expr); | ||
} | ||
|
||
/// Instruments the code attached to `symbol` with | ||
/// runtime exceptions or corresponding assertions. | ||
/// Exceptions are thrown when the `throw_runtime_exceptions` flag is set. | ||
/// Otherwise, assertions are emitted. | ||
/// \par parameters: `symbol_table`: global symbol table (may gain exception type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. line too long: https://travis-ci.org/diffblue/cbmc/jobs/263176469#L455 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Plus this doxy comment appears mangled - can be fixed by removing the back ticks and re running it through the converter |
||
/// stubs and similar) | ||
/// `symbol`: the symbol to instrument | ||
/// `throw_runtime_exceptions`: flag determining whether we instrument the code | ||
/// with runtime exceptions or with assertions. The former applies if this flag | ||
/// is set to true. | ||
/// `max_array_length`: maximum array length; the only reason we need this is | ||
/// in order to be able to call the object factory to create exceptions. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documented one, ditched the other as not needed any longer. |
||
void java_bytecode_instrument( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might consider renaming this function to be |
||
symbol_tablet &symbol_table, | ||
symbolt &symbol, | ||
const bool throw_runtime_exceptions, | ||
message_handlert &message_handler, | ||
const size_t max_array_length, | ||
const size_t max_tree_depth) | ||
{ | ||
java_bytecode_instrumentt instrument( | ||
symbol_table, | ||
throw_runtime_exceptions, | ||
message_handler, | ||
max_array_length, | ||
max_tree_depth); | ||
INVARIANT( | ||
symbol.value.id()==ID_code, | ||
"java_bytecode_instrument expects a code-typed symbol"); | ||
instrument(symbol.value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the overloaded method the symbol is looked up in the symbol table with the ID and then the In any case - should the other function call into this one? The comment below says that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out I overstated this -- at least if https://stackoverflow.com/questions/16781886/can-we-store-unordered-maptiterator can be believed, it's ok to hold the reference but not the iterator. I have therefore changed this to avoid the iteration-while-mutating, but hold the reference. I didn't want to call the single-symbol function to avoid creating and destroying the context class once per symbol. |
||
} | ||
|
||
/// Instruments all the code in the symbol_table with | ||
/// runtime exceptions or corresponding assertions. | ||
/// Exceptions are thrown when the `throw_runtime_exceptions` flag is set. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -746,6 +746,15 @@ void java_bytecode_languaget::replace_string_methods( | |
// Replace body of the function by code generated by string preprocess | ||
symbolt &symbol=context.lookup(id); | ||
symbol.value=generated_code; | ||
// Specifically instrument the new code, since this comes after the | ||
// blanket instrumentation pass called before typechecking. | ||
java_bytecode_instrument( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there was already something relying on this instrumentation pass java_bytecode_convert_method.cpp:527 I was expecting to see a removal to this call as it is moved to later in the process, but it seems we are just adding one? |
||
context, | ||
symbol, | ||
throw_runtime_exceptions, | ||
get_message_handler(), | ||
max_nondet_array_length, | ||
max_nondet_tree_depth); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ Author: Daniel Kroening, [email protected] | |
#include <util/type.h> | ||
#include <util/symbol_table.h> | ||
#include <util/message.h> | ||
#include <util/std_expr.h> | ||
|
||
#include "java_bytecode_parse_tree.h" | ||
|
||
|
@@ -64,4 +65,9 @@ irep_idt resolve_friendly_method_name( | |
const symbol_tablet &symbol_table, | ||
std::string &error); | ||
|
||
/// Dereference an expression and flag it for a null-pointer check | ||
/// \param expr: expression to dereference and check | ||
/// \param type: expected result type (typically expr.type().subtype()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the documentation be in the cpp file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to be normal for that header. Personally I prefer the docs in the header, but we should move or keep en masse. |
||
dereference_exprt checked_dereference(const exprt &expr, const typet &type); | ||
|
||
#endif // CPROVER_JAVA_BYTECODE_JAVA_UTILS_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this line (and others in cbmc-java) being removed? Should the test (ideally) be modified to stop it being trivially simplified?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the simplifier gives a SUCCESSFUL verdict you don't get the specific goal printouts (cf. if it was just complicated enough to use the BMC). Perhaps that ought to change, but I'd like to do that elsewhere if so. I'm unsure exactly how this now passes the simplifier and didn't before (suspect: typecasts, which are now less likely to occur nested?) However a look over the GOTO programs for the tests shows they're still testing what they intended to, so I don't think the tests are any more or less trivial than they were before.