From 840a10afaa576f5592e79eda1196ecbee72f6cdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Thu, 21 Jan 2021 08:46:30 -0800 Subject: [PATCH] [pre-analysis] Remove throw/catch handling from preanalysis Summary: D20769039 (https://github.com/facebook/infer/commit/cec8cbeff2535a97625b0fc756340813f930edb7) added a preanalysis step that creates edges from throw nodes to all reachable catch nodes. It intended to fix some deadstore FPs however it caused more damage than the fix itself. In particular, throws were connected irrespective of - the type of the exception - whether the try was surrounded by a catch This in turn caused weird CFGs with dangling and impossible to understand nodes:( This diff reverts this change for now. Instead, the fix should probably be done in the frontend where we have more information about try/catch blocks. Reviewed By: da319 Differential Revision: D25997475 fbshipit-source-id: bbeabfbef --- infer/src/backend/preanal.ml | 39 +------------------ .../cpp/liveness/dead_stores.cpp | 17 +++++++- .../codetoanalyze/cpp/liveness/issues.exp | 1 + 3 files changed, 17 insertions(+), 40 deletions(-) diff --git a/infer/src/backend/preanal.ml b/infer/src/backend/preanal.ml index 91a119476..76f418b05 100644 --- a/infer/src/backend/preanal.ml +++ b/infer/src/backend/preanal.ml @@ -450,49 +450,12 @@ module NoReturn = struct false ) - let has_throw_call node = - Procdesc.Node.get_instrs node - |> Instrs.exists ~f:(fun (instr : Sil.instr) -> - match instr with - | Call (_, Const (Cfun proc_name), _, _, _) -> - String.equal - (Procname.get_method BuiltinDecl.objc_cpp_throw) - (Procname.get_method proc_name) - | _ -> - false ) - - - let get_all_reachable_catch_nodes start_node = - let rec worklist ~todo ~visited result = - if Procdesc.NodeSet.is_empty todo then result - else - let el = Procdesc.NodeSet.choose todo in - let todo = Procdesc.NodeSet.remove el todo in - if Procdesc.NodeSet.mem el visited then worklist ~todo ~visited result - else - let succs = Procdesc.Node.get_succs el |> Procdesc.NodeSet.of_list in - let visited = Procdesc.NodeSet.add el visited in - worklist - ~todo:(Procdesc.NodeSet.union succs todo) - ~visited - (Procdesc.Node.get_exn el @ result) - in - worklist ~todo:(Procdesc.NodeSet.singleton start_node) ~visited:Procdesc.NodeSet.empty [] - - let process tenv proc_desc = let exit_node = Procdesc.get_exit_node proc_desc in Procdesc.iter_nodes (fun node -> if has_noreturn_call tenv node then - Procdesc.set_succs node ~normal:(Some [exit_node]) ~exn:None - else if has_throw_call node then - let catch_nodes = get_all_reachable_catch_nodes node in - let catch_or_exit_nodes = - if List.is_empty catch_nodes then (* throw with no catch *) - [exit_node] else catch_nodes - in - Procdesc.set_succs node ~normal:(Some catch_or_exit_nodes) ~exn:None ) + Procdesc.set_succs node ~normal:(Some [exit_node]) ~exn:None ) proc_desc end diff --git a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp index ca60a04ce..09b7ecc4d 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp +++ b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp @@ -441,8 +441,21 @@ class Exceptions { return 0; } - // the transition to the catch block is set at pre-analysis - int read_in_catch_tricky_ok(bool b1, bool b2) { + int FN_throw_unrelated_catch_bad(int x) { + int i = 5; + throw std::invalid_argument("Positive argument :("); + // the rest is unreachable + try { + throw(0); + } catch (...) { + + return i; + } + } + + // currently, the only transition to the catch block is at the end of the try + // block. See T28898377 + int FP_read_in_catch_tricky_ok(bool b1, bool b2) { int i = 1; try { if (b1) { diff --git a/infer/tests/codetoanalyze/cpp/liveness/issues.exp b/infer/tests/codetoanalyze/cpp/liveness/issues.exp index 71744e129..27ba2fdd9 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/issues.exp +++ b/infer/tests/codetoanalyze/cpp/liveness/issues.exp @@ -1,3 +1,4 @@ +codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::Exceptions::FP_read_in_catch_tricky_ok, 1, DEAD_STORE, no_bucket, ERROR, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::Exceptions::dead_in_catch_bad, 4, DEAD_STORE, no_bucket, ERROR, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::Exceptions::unreachable_catch_bad, 1, DEAD_STORE, no_bucket, ERROR, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::FP_assign_array_tricky2_ok, 3, DEAD_STORE, no_bucket, ERROR, [Write of unused value]