analyzer: fix false +ve on bitwise binops (PR analyzer/102692)
authorDavid Malcolm <dmalcolm@redhat.com>
Fri, 7 Jan 2022 22:44:23 +0000 (17:44 -0500)
committerDavid Malcolm <dmalcolm@redhat.com>
Tue, 11 Jan 2022 14:16:08 +0000 (09:16 -0500)
commit4f34f8cc1d064bfaaed723312c71e92f495d429b
treed831cf25f32f088e4b260961aed644d0619333db
parent438f2a24a4f02a3128847f7186bfcb56e7a60d83
analyzer: fix false +ve on bitwise binops (PR analyzer/102692)

PR analyzer/102692 reports a false positive at -O2 from
-Wanalyzer-null-dereference on:
  if (!p || q || !p->next)

At the gimple level, -O2 has converted the first || into bitwise or
controlling a jump:
  _4 = _2 | _3;
  if (_4 != 0)
and a recursive call has been converted to iteration.  The analyzer hits
the symbolic value complexity limit whilst analyzing the iteration and
hits a case where _2 is (_Bool)1 (i.e. true) and _3 (i.e. q) is
UNKNOWN(_Bool).

There are two issues leading to the false positive; fixing either issue
fixes the false positive; this patch fixes both for good measure:

(a) The analyzer erronously treats bitwise ops on UNKNOWN(_Bool) as UNKNOWN,
even for case like (1 | UNKNOWN) where we know the result, leading to
bogus edges in the exploded graph.  The patch fixes these cases.

(b) The state-handling code creates "UNKNOWN" symbolic values, as a way
to give up when symbolic expressions get too complicated, and in various
other cases.  This makes sense when building the exploded graph, since
we want the analysis to terminate, but doesn't make sense when checking
the feasibility along a specific path, where we want precision.  The patch
prevents all use of "unknown" symbolic values when performing feasibility
checking of a path (before it merely stopped doing complexity-checking),
by creating a unique placeholder_svalue instead.

This fixes the -Wanalyzer-null-dereference false positive.

Unfortunately, with GCC 12 there's also a false positive from
-Wanalyzer-use-of-uninitialized-value on this code, which is a separate
issue that the patch does not fix.

gcc/analyzer/ChangeLog:
PR analyzer/102692
* diagnostic-manager.cc
(class auto_disable_complexity_checks): Rename to...
(class auto_checking_feasibility): ...this, updating
the calls accordingly.
(epath_finder::explore_feasible_paths): Update for renaming.
* region-model-manager.cc
(region_model_manager::region_model_manager): Update for change from
m_check_complexity to m_checking_feasibility.
(region_model_manager::reject_if_too_complex): Likewise.
(region_model_manager::get_or_create_unknown_svalue): Handle
m_checking_feasibility.
(region_model_manager::create_unique_svalue): New.
(region_model_manager::maybe_fold_binop): Handle BIT_AND_EXPR and
BIT_IOR_EXPRs on booleans where we know the result.
* region-model.cc (test_binop_svalue_folding): Add test coverage
for the above.
* region-model.h (region_model_manager::create_unique_svalue): New
decl.
(region_model_manager::enable_complexity_check): Replace with...
(region_model_manager::begin_checking_feasibility): ...this.
(region_model_manager::disable_complexity_check): Replace with...
(region_model_manager::end_checking_feasibility): ...this.
(region_model_manager::m_check_complexity): Replace with...
(region_model_manager::m_checking_feasibility): ...this.
(region_model_manager::m_managed_dynamic_svalues): New field.

gcc/testsuite/ChangeLog:
PR analyzer/102692
* gcc.dg/analyzer/pr102692.c: New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
gcc/analyzer/diagnostic-manager.cc
gcc/analyzer/region-model-manager.cc
gcc/analyzer/region-model.cc
gcc/analyzer/region-model.h
gcc/testsuite/gcc.dg/analyzer/pr102692.c [new file with mode: 0644]