-
Notifications
You must be signed in to change notification settings - Fork 273
Don't phi-merge uninitialised objects #2168
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 all 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 |
---|---|---|
@@ -0,0 +1,54 @@ | ||
public class PhiMergeUninitialized { | ||
|
||
public int dynamicAllocationUninitialized(Boolean trigger) { | ||
|
||
Ephemeral obj; | ||
if (trigger) { | ||
obj = new Ephemeral(42); | ||
} else { | ||
obj = new Aetherial(20); | ||
} | ||
|
||
assert obj.val == 20; | ||
return obj.val; | ||
} | ||
|
||
private Ephemeral field; | ||
|
||
public int fieldUninitialized(Boolean trigger) { | ||
if (trigger) { | ||
field = new Ephemeral(42); | ||
} | ||
|
||
assert field.val == 42; | ||
return field.val; | ||
} | ||
|
||
private static Ephemeral staticField; | ||
|
||
public int staticFieldUninitialized(Boolean trigger) { | ||
if (trigger) { | ||
staticField = new Ephemeral(42); | ||
} else { | ||
staticField = new Aetherial(76); | ||
} | ||
|
||
assert staticField.val == 76; | ||
return staticField.val; | ||
} | ||
|
||
class Ephemeral { | ||
Ephemeral(int value) { | ||
val = value; | ||
} | ||
|
||
int val; | ||
} | ||
|
||
class Aetherial extends Ephemeral { | ||
Aetherial(int value) { | ||
super(value); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
CORE | ||
PhiMergeUninitialized.class | ||
--function PhiMergeUninitialized.fieldUninitialized --show-vcc | ||
activate-multi-line-match | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^.*:\s+(dynamic_object|new_tmp)[0-9]+(@[0-9]+)?#0\) | ||
^.*\?\s+(dynamic_object|new_tmp)[0-9]+(@[0-9]+)?#0\s+: | ||
-- | ||
These regexes are making sure that a variable of generation 0 dosen't appear in a phi merge, so the below | ||
statement: | ||
|
||
(guard ? dynamic_object2@3#1 : dynamic_object3@3#0) | ||
|
||
Should not appear. First regex deals with when the gen zero is the latter position, second when it's in the former. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
CORE | ||
PhiMergeUninitialized.class | ||
--function PhiMergeUninitialized.dynamicAllocationUninitialized --show-vcc | ||
activate-multi-line-match | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^.*:\s+(dynamic_object|new_tmp)[0-9]+(@[0-9]+)?#0\) | ||
^.*\?\s+(dynamic_object|new_tmp)[0-9]+(@[0-9]+)?#0\s+: | ||
-- | ||
These regexes are making sure that a variable of generation 0 dosen't appear in a phi merge, so the below | ||
statement: | ||
|
||
(guard ? dynamic_object2@3#1 : dynamic_object3@3#0) | ||
|
||
Should not appear. First regex deals with when the gen zero is the latter position, second when it's in the former. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
CORE | ||
PhiMergeUninitialized.class | ||
--function PhiMergeUninitialized.staticFieldUninitialized --show-vcc | ||
activate-multi-line-match | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^.*:\s+(dynamic_object|new_tmp)[0-9]+(@[0-9]+)?#0\) | ||
^.*\?\s+(dynamic_object|new_tmp)[0-9]+(@[0-9]+)?#0\s+: | ||
-- | ||
These regexes are making sure that a variable of generation 0 dosen't appear in a phi merge, so the below | ||
statement: | ||
|
||
(guard ? dynamic_object2@3#1 : dynamic_object3@3#0) | ||
|
||
Should not appear. First regex deals with when the gen zero is the latter position, second when it's in the former. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
CORE | ||
test.c | ||
--function dynamicAllocationUninitialized --show-vcc | ||
activate-multi-line-match | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^.*:\s+dynamic_object[0-9]+(@[0-9]+)?#0\) | ||
^.*\?\s+dynamic_object[0-9]+(@[0-9]+)?#0\s+: | ||
-- | ||
These regexes are making sure that a variable of generation 0 dosen't appear in a phi merge, so the below | ||
statement: | ||
|
||
(guard ? dynamic_object2@3#1 : dynamic_object3@3#0) | ||
|
||
Should not appear. First regex deals with when the gen zero is the latter position, second when it's in the former. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
CORE | ||
test.c | ||
--function globalUninitialized --show-vcc | ||
activate-multi-line-match | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^.*:\s+global(@[0-9]+)?#0\) | ||
^.*\?\s+global(@[0-9]+)?#0\s+: | ||
-- | ||
These regexes are making sure that a variable of generation 0 dosen't appear in a phi merge, so the below | ||
statement: | ||
|
||
(guard ? global#1 : global#0) | ||
|
||
Should not appear. First regex deals with when the gen zero is the latter position, second when it's in the former. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
CORE | ||
test.c | ||
--function localUninitialized --show-vcc | ||
activate-multi-line-match | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^.*:\s+local(@[0-9]+)?#0\) | ||
^.*\?\s+local(@[0-9]+)?#0\s+: | ||
-- | ||
These regexes are making sure that a variable of generation 0 dosen't appear in a phi merge, so the below | ||
statement: | ||
|
||
(guard ? local#1 : local#0) | ||
|
||
Should not appear. First regex deals with when the gen zero is the latter position, second when it's in the former. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
CORE | ||
test.c | ||
--function staticLocalUninitialized --show-vcc | ||
activate-multi-line-match | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^.*:\s+staticLocal(@[0-9]+)?#0\) | ||
^.*\?\s+staticLocal(@[0-9]+)?#0\s+: | ||
-- | ||
These regexes are making sure that a variable of generation 0 dosen't appear in a phi merge, so the below | ||
statement: | ||
|
||
(guard ? staticLocal#1 : dynamic_object#0) | ||
|
||
Should not appear. First regex deals with when the gen zero is the latter position, second when it's in the former. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
#include <assert.h> | ||
#include <stdlib.h> | ||
|
||
void dynamicAllocationUninitialized(int trigger) | ||
{ | ||
int *obj; | ||
if(trigger) | ||
{ | ||
obj = (int *)malloc(sizeof(int)); | ||
*obj = 42; | ||
} | ||
else | ||
{ | ||
obj = (int *)malloc(sizeof(int)); | ||
*obj = 76; | ||
} | ||
|
||
assert(*obj == 42); | ||
} | ||
|
||
int global; | ||
void globalUninitialized(int trigger) | ||
{ | ||
if(trigger) | ||
{ | ||
global = 44; | ||
} | ||
else | ||
{ | ||
global = 20; | ||
} | ||
|
||
assert(global == 44); | ||
} | ||
|
||
void staticLocalUninitialized(int trigger) | ||
{ | ||
static int staticLocal; | ||
if(trigger) | ||
{ | ||
staticLocal = 43; | ||
} | ||
|
||
assert(staticLocal == 43); | ||
} | ||
|
||
void localUninitialized(int trigger) | ||
{ | ||
int local; | ||
if(trigger) | ||
{ | ||
local = 24; | ||
} | ||
|
||
assert(local == 24); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,12 @@ void goto_symext::symex_allocate( | |
else | ||
throw "failed to zero initialize dynamic object"; | ||
} | ||
else | ||
{ | ||
exprt nondet = build_symex_nondet(object_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. Point of interest: you've copied the case for 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. I can't see any major deviations - the code isn't identical but the intentions seem the same and most of the code in decl is run in assign at some point too. One thing to note is that assign propagates the nondet as a constant, while the decl code flat-out disables any propagation, would that be an issue? 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. It'll only disable it for the time being, but it's interesting. I'm not quite well informed enough to give an authoritative answer -- I suggest running this by @peterschrammel and post his answer here. 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. I'd argue that this is what #35 is about, and you're doing parts of that here, even though the full value only unfolds once you break down nondet symbols into their individual fields. |
||
code_assignt assignment(value_symbol.symbol_expr(), nondet); | ||
symex_assign(state, assignment); | ||
} | ||
|
||
exprt rhs; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -425,10 +425,22 @@ void goto_symext::phi_function( | |
|
||
exprt rhs; | ||
|
||
// Don't add a conditional to the assignment when: | ||
// 1. Either guard is false, so we can't follow that branch. | ||
// 2. Either identifier is of generation zero, and so hasn't been | ||
// initialized and therefor an invalid target. | ||
if(dest_state.guard.is_false()) | ||
rhs=goto_state_rhs; | ||
else if(goto_state.guard.is_false()) | ||
rhs=dest_state_rhs; | ||
else if(goto_state.level2_current_count(l1_identifier) == 0) | ||
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. Add comments briefly explaining why these checks are here 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. Added a comment block at the top of the if. 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. Thank you! |
||
{ | ||
rhs = dest_state_rhs; | ||
} | ||
else if(dest_state.level2.current_count(l1_identifier) == 0) | ||
{ | ||
rhs = goto_state_rhs; | ||
} | ||
else | ||
{ | ||
rhs=if_exprt(diff_guard.as_expr(), goto_state_rhs, dest_state_rhs); | ||
|
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.
This is great, but could it please be put in a separate commit? The explanation in the PR is absolutely necessary to understand this, but will never make it to the commit history at this stage.
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.
Thanks for factoring it out into a separate commit. Could you please also add the text from the PR to body of the commit message? It's really informative, and should be preserved!
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.
Ah, sorry, fixed. Still getting fully used to git.