-
Notifications
You must be signed in to change notification settings - Fork 76
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
Invalidate variables based on annotations of asm statements #715
Changes from all commits
e3af268
92b89d5
d4f9374
18f157b
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 |
---|---|---|
|
@@ -1899,34 +1899,34 @@ struct | |
in | ||
List.concat_map do_exp exps | ||
|
||
let invalidate ?ctx ask (gs:glob_fun) (st:store) (exps: exp list): store = | ||
type invalidate_mode = InvalidateSelf | InvalidateTransitive | InvalidateSelfAndTransitive [@@deriving show] | ||
|
||
let invalidate_core ~ctx inv_globs inv_exps = | ||
let exps_transitive = List.filter_map (function (_, InvalidateSelf) -> None | (exp, _) -> Some exp) inv_exps in | ||
let lvals_self = List.filter_map (function (_, InvalidateTransitive) -> None | (Lval lval, _) -> Some lval | _ -> None) inv_exps in | ||
let exps_self = List.filter_map (function (_, InvalidateTransitive) | (Lval _, _) -> None | (exp, _) -> Some exp) inv_exps in | ||
Comment on lines
+1906
to
+1907
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. What's with the special |
||
let exps_global = if inv_globs then foldGlobals !Cilfacade.current_file (fun acc global -> | ||
match global with | ||
| GVar (vi, _, _) when not (is_static vi) && not (BaseUtil.is_excluded_from_invalidation vi) -> | ||
mkAddrOf (Var vi, NoOffset) :: acc | ||
(* TODO: what about GVarDecl? *) | ||
| _ -> acc | ||
) [] else [] in | ||
let addr_list = collect_funargs (ask_of_ctx ctx) ctx.global ctx.local (exps_transitive @ exps_global) @ (List.map (eval_lv (ask_of_ctx ctx) ctx.global ctx.local) lvals_self) in | ||
if M.tracing then ( | ||
M.trace "invalidate" "invalidate_core transitive: [%a]\n" (d_list "," CilType.Exp.pretty) exps_transitive; | ||
M.trace "invalidate" "invalidate_core self (exp): [%a]\n" (d_list "," CilType.Exp.pretty) exps_self; | ||
M.trace "invalidate" "invalidate_core self (lval): [%a]\n" (d_list "," CilType.Lval.pretty) lvals_self; | ||
M.trace "invalidate" "invalidate_core global: [%a]\n" (d_list "," CilType.Exp.pretty) exps_global; | ||
); | ||
ctx.emit (Events.Invalidate {addr_list}) | ||
|
||
|
||
let invalidate ~ctx ask (gs:glob_fun) (st:store) (exps: exp list): store = | ||
if M.tracing && exps <> [] then M.tracel "invalidate" "Will invalidate expressions [%a]\n" (d_list ", " d_plainexp) exps; | ||
if exps <> [] then M.info ~category:Imprecise "Invalidating expressions: %a" (d_list ", " d_plainexp) exps; | ||
(* To invalidate a single address, we create a pair with its corresponding | ||
* top value. *) | ||
let invalidate_address st a = | ||
let t = AD.get_type a in | ||
let v = get ask gs st a None in (* None here is ok, just causes us to be a bit less precise *) | ||
let nv = VD.invalidate_value ask t v in | ||
(a, t, nv) | ||
in | ||
(* We define the function that invalidates all the values that an address | ||
* expression e may point to *) | ||
let invalidate_exp exps = | ||
let args = collect_funargs ~warn:true ask gs st exps in | ||
List.map (invalidate_address st) args | ||
in | ||
let invalids = invalidate_exp exps in | ||
let is_fav_addr x = | ||
List.exists BaseUtil.is_excluded_from_invalidation (AD.to_var_may x) | ||
in | ||
let invalids' = List.filter (fun (x,_,_) -> not (is_fav_addr x)) invalids in | ||
if M.tracing && exps <> [] then ( | ||
let addrs = List.map (Tuple3.first) invalids' in | ||
let vs = List.map (Tuple3.third) invalids' in | ||
M.tracel "invalidate" "Setting addresses [%a] to values [%a]\n" (d_list ", " AD.pretty) addrs (d_list ", " VD.pretty) vs | ||
); | ||
set_many ?ctx ask gs st invalids' | ||
invalidate_core ~ctx false (List.map (fun exp -> (exp, InvalidateTransitive)) exps); | ||
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. The |
||
ctx.local | ||
|
||
|
||
let make_entry ?(thread=false) (ctx:(D.t, G.t, C.t, V.t) Analyses.ctx) fundec args: D.t = | ||
|
@@ -2083,23 +2083,11 @@ struct | |
let special_unknown_invalidate ctx ask gs st f args = | ||
(if not (CilType.Varinfo.equal f dummyFunDec.svar) && not (LF.use_special f.vname) then M.error ~category:Imprecise ~tags:[Category Unsound] "Function definition missing for %s" f.vname); | ||
(if CilType.Varinfo.equal f dummyFunDec.svar then M.warn "Unknown function ptr called"); | ||
let addrs = | ||
if get_bool "sem.unknown_function.invalidate.globals" then ( | ||
M.info ~category:Imprecise "INVALIDATING ALL GLOBALS!"; | ||
foldGlobals !Cilfacade.current_file (fun acc global -> | ||
match global with | ||
| GVar (vi, _, _) when not (is_static vi) -> | ||
mkAddrOf (Var vi, NoOffset) :: acc | ||
(* TODO: what about GVarDecl? *) | ||
| _ -> acc | ||
) args | ||
) | ||
else | ||
args | ||
in | ||
if get_bool "sem.unknown_function.invalidate.globals" then M.info ~category:Imprecise "INVALIDATING ALL GLOBALS!"; | ||
(* TODO: what about escaped local variables? *) | ||
(* invalidate arguments and non-static globals for unknown functions *) | ||
invalidate ~ctx (Analyses.ask_of_ctx ctx) gs st addrs | ||
invalidate_core ~ctx (get_bool "sem.unknown_function.invalidate.globals") (List.map (fun arg -> (arg, InvalidateTransitive)) args); | ||
ctx.local | ||
|
||
let special ctx (lv:lval option) (f: varinfo) (args: exp list) = | ||
let invalidate_ret_lv st = match lv with | ||
|
@@ -2413,8 +2401,27 @@ struct | |
(* D.join ctx.local @@ *) | ||
ctx.local | ||
|
||
let asm ctx _ an = | ||
let inv_globs, inv_exps = match an with | ||
| None -> | ||
(* asm basic instruction *) | ||
(not (get_bool "sem.asm.basic-preserve-globals"), []) | ||
| Some (outs, ins, clobbers) -> | ||
(* advanced asm instructions *) | ||
( | ||
List.mem "memory" clobbers && not (get_bool "sem.asm.memory-preserve-globals"), | ||
List.append (if get_bool "sem.asm.readonly-inputs" then [] else List.map (fun (_, _, exp) -> exp, InvalidateTransitive) ins) | ||
(List.map (fun (_, c, lval) -> Lval lval, match String.find c "+" with | ||
| _ -> InvalidateSelfAndTransitive | ||
| exception Not_found -> InvalidateSelf) outs) | ||
) | ||
in | ||
(if get_bool "sem.asm.enabled" then invalidate_core ~ctx inv_globs inv_exps); | ||
ctx.local | ||
|
||
let event ctx e octx = | ||
let st: store = ctx.local in | ||
let ask = ask_of_ctx ctx in | ||
match e with | ||
| Events.Lock (addr, _) when ThreadFlag.is_multi (Analyses.ask_of_ctx ctx) -> (* TODO: is this condition sound? *) | ||
if M.tracing then M.tracel "priv" "LOCK EVENT %a\n" LockDomain.Addr.pretty addr; | ||
|
@@ -2430,6 +2437,20 @@ struct | |
| Events.AssignSpawnedThread (lval, tid) -> | ||
(* TODO: is this type right? *) | ||
set ~ctx:(Some ctx) (Analyses.ask_of_ctx ctx) ctx.global ctx.local (eval_lv (Analyses.ask_of_ctx ctx) ctx.global ctx.local lval) (Cilfacade.typeOfLval lval) (`Thread (ValueDomain.Threads.singleton tid)) | ||
| Events.Invalidate {addr_list} -> | ||
let invalidate_address a = | ||
let t = AD.get_type a in | ||
let v = get ask ctx.global st a None in (* None here is ok, just causes us to be a bit less precise *) | ||
let nv = VD.invalidate_value ask t v in | ||
(a, t, nv) | ||
in | ||
let invalids = List.map invalidate_address addr_list in | ||
if M.tracing && addr_list <> [] then ( | ||
let addrs = List.map (Tuple3.first) invalids in | ||
let vs = List.map (Tuple3.third) invalids in | ||
M.tracel "invalidate" "Setting addresses [%a] to values [%a]\n" (d_list ", " AD.pretty) addrs (d_list ", " VD.pretty) vs | ||
); | ||
set_many ~ctx ask ctx.global ctx.local invalids | ||
Comment on lines
+2440
to
+2453
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. What's the reason for converting this into an event instead of simply extracting the functions for all globals invalidation etc inside the base analysis itself? 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.
The idea of making invalidation an event based mechanism was that other analyses could be informed and then do some proper handling. One could e.g. imagine that an relational analysis that handles referenced variables might need to know when some variables are potentially invalidated. 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'm just afraid that in light of #719 with the need to distinguish non-transitive vs transitive accesses also for reads/frees and writes other than invalidation, this won't be general enough. It's also quite base analysis specific by using its 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.
The idea for invalidate events would be that the
One could add a utility function that takes elements from 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.
They don't currently reimplement it either, but just use the We will probably have to see how this plays together with the general non-transitive vs transitive accessing changes from #719 and #720. 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 thought about this more and how it compares to #720 and the
But given that access analysis (and its event) is really about race detection, it might be better to keep it that way. |
||
| _ -> | ||
ctx.local | ||
end | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ type t = | |||||
| AssignSpawnedThread of lval * ThreadIdDomain.Thread.t (** Assign spawned thread's ID to lval. *) | ||||||
| Access of {var_opt: CilType.Varinfo.t option; write: bool} (** Access varinfo (unknown if None). *) | ||||||
| Assign of {lval: CilType.Lval.t; exp: CilType.Exp.t} (** Used to simulate old [ctx.assign]. *) | ||||||
| Invalidate of {addr_list: PreValueDomain.AD.bucket BatMap.Int.t list} | ||||||
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'm guessing this is the actual type without inlining the implementation detail about buckets:
Suggested change
|
||||||
|
||||||
let pretty () = function | ||||||
| Lock m -> dprintf "Lock %a" LockDomain.Lockset.Lock.pretty m | ||||||
|
@@ -19,3 +20,4 @@ let pretty () = function | |||||
| AssignSpawnedThread (lval, tid) -> dprintf "AssignSpawnedThread (%a, %a)" d_lval lval ThreadIdDomain.Thread.pretty tid | ||||||
| Access {var_opt; write} -> dprintf "Access {var_opt=%a, write=%B}" (docOpt (CilType.Varinfo.pretty ())) var_opt write | ||||||
| Assign {lval; exp} -> dprintf "Assugn {lval=%a, exp=%a}" CilType.Lval.pretty lval CilType.Exp.pretty exp | ||||||
| Invalidate{addr_list} -> dprintf "Invalidate {addr_list=%a}" (docList (PreValueDomain.AD.pretty ())) addr_list |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,9 @@ type t = | |
* transferred to the function node! *) | ||
| Test of CilType.Exp.t * bool | ||
(** The true-branch or false-branch of a conditional exp *) | ||
| ASM of string list * asm_out * asm_in | ||
| ASM of string list * (asm_out * asm_in * string list) option | ||
(** Inline assembly statements, and the annotations for output and input | ||
* variables. *) | ||
* variables and clobbers or None if it is an unannotated basic asm statement. *) | ||
| VDecl of CilType.Varinfo.t | ||
(** VDecl edge for the variable in varinfo. Whether such an edge is there for all | ||
* local variables or only when it is not possible to pull the declaration up, is | ||
|
@@ -42,7 +42,7 @@ let pretty () = function | |
| Entry (f) -> Pretty.text "(body)" | ||
| Ret (Some e,f) -> Pretty.dprintf "return %a" dn_exp e | ||
| Ret (None,f) -> Pretty.dprintf "return" | ||
| ASM (_,_,_) -> Pretty.text "ASM ..." | ||
| ASM _ -> Pretty.text "ASM ..." | ||
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. Now that we actually do something better than ignoring asm, it might be a good idea to also have the edge printout reflect the actual data. |
||
| Skip -> Pretty.text "skip" | ||
| VDecl v -> Cil.defaultCilPrinter#pVDecl () v | ||
| SelfLoop -> Pretty.text "SelfLoop" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1108,6 +1108,38 @@ | |
} | ||
}, | ||
"additionalProperties": false | ||
}, | ||
"asm": { | ||
"title": "sem.asm", | ||
"description": "Inline Assembly Analysis options", | ||
"type": "object", | ||
"properties": { | ||
"enabled": { | ||
"title": "sem.asm.enabled", | ||
"description": "", | ||
"type": "boolean", | ||
"default": true | ||
}, | ||
Comment on lines
+1117
to
+1122
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. What would be the use case for ever disabling asm semantics (and whatever that would mean)? |
||
"basic-preserve-globals": { | ||
"title": "sem.asm.basic-preserve-globals", | ||
"description": "If enabled, do preserve globals across basic asm statements", | ||
"type": "boolean", | ||
"default": false | ||
}, | ||
"memory-preserve-globals": { | ||
"title": "sem.asm.memory-preserve-globals", | ||
"description": "If enabled, do preserve globals across advanced asm statements with memory clobber annotation", | ||
"type": "boolean", | ||
"default": false | ||
}, | ||
Comment on lines
+1123
to
+1134
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. For consistency, it might make sense to negate these options since the pre-existing |
||
"readonly-inputs": { | ||
"title": "sem.asm.readonly-inputs", | ||
"description": "If enabled, do not consider changes through pointers given as inputs", | ||
"type": "boolean", | ||
"default": false | ||
} | ||
}, | ||
"additionalProperties": false | ||
} | ||
}, | ||
"additionalProperties": false | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
#include <assert.h> | ||
|
||
int main() | ||
{ | ||
int x; | ||
|
||
asm("nop"); | ||
|
||
asm volatile(""); | ||
|
||
assert(1); | ||
|
||
return 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
#include <assert.h> | ||
|
||
int main() | ||
{ | ||
int x = 0; | ||
|
||
asm("nop"); | ||
|
||
assert(x == 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. The test is named "discard-local", but in what sense is the local being discarded if it isn't invalidated here. |
||
|
||
return 0; | ||
} |
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 more of a note to us than you, but the notion of transitive vs non-transitive invalidation gets a lot more general in light of #719 because it doesn't apply to just invalidation but every form of access.