| Message ID | 20200814225157.2523-1-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] analyzer: fix initialization from constant pool [PR96609, PR96616] | expand |
On Sat, 15 Aug 2020 at 00:52, David Malcolm <dmalcolm@redhat.com> wrote: > > PR testsuite/96609 and PR analyzer/96616 report various testsuite > failures seen on powerpc64, aarch64, and arm in new tests added by > r11-2694-g808f4dfeb3a95f50f15e71148e5c1067f90a126d. > > Some of these failures (in gcc.dg/analyzer/init.c, and on arm > in gcc.dg/analyzer/casts-1.c) relate to initializations from var_decls > in the constant pool. I wrote the tests assuming that the gimplified > stmts would initialize the locals via a gassign of code CONSTRUCTOR, > whereas on these targets some of the initializations are gassign from > a VAR_DECL e.g.: > c = *.LC0; > where "*.LC0" is a var_decl with DECL_IN_CONSTANT_POOL set. > > For example, in test_7: > struct coord c[2] = {{3, 4}, {5, 6}}; > __analyzer_eval (c[0].x == 3); /* { dg-warning "TRUE" } */ > after the initialization, the store was simply recording: > cluster for: c: INIT_VAL(*.LC0) > when I was expecting the cluster for c to have: > cluster for: c > key: {kind: direct, start: 0, size: 32, next: 32} > value: 'int' {(int)3} > key: {kind: direct, start: 32, size: 32, next: 64} > value: 'int' {(int)4} > key: {kind: direct, start: 64, size: 32, next: 96} > value: 'int' {(int)5} > key: {kind: direct, start: 96, size: 32, next: 128} > value: 'int' {(int)6} > The test for c[0].x == 3 would then generate: > cluster for: _2: (SUB(SUB(INIT_VAL(*.LC0), c[(int)0]), c[(int)0].x)==(int)3) > which is UNKNOWN, leading to the test failing. > > This patch fixes the init.c and casts-1.c failures by special-casing > reads from a var_decl with DECL_IN_CONSTANT_POOL set, so that they build > a compound_svalue containing the bindings implied by the CONSTRUCTOR > node for DECL_INITIAL. > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > Manually verified the fixes to init.c and casts-1.c on > aarch64-unknown-linux-gnu, arm-unknown-eabi, and powerpc64-linux-gnu > (-m32 and -m64). > > Pushed to master as r11-2708-g2867118ddda9b56d991c16022f7d3d634ed08313. > Hi David, Thanks for fixing this. However, this patch is causing 2 ICEs on arm: gcc.dg/analyzer/data-model-1.c (internal compiler error) gcc.dg/analyzer/pr94639.c (internal compiler error) Christophe > This doesn't address the bogus -Wanalyzer-too-complex messages > for pr93032-mztools.c reported in the bugs, which seem to be a > separate issue that I'm now investigating. > > gcc/analyzer/ChangeLog: > PR testsuite/96609 > PR analyzer/96616 > * region-model.cc (region_model::get_store_value): Call > maybe_get_constant_value on decl_regions first. > * region-model.h (decl_region::maybe_get_constant_value): New decl. > * region.cc (decl_region::get_stack_depth): Likewise. > (decl_region::maybe_get_constant_value): New. > * store.cc (get_subregion_within_ctor): New. > (binding_map::apply_ctor_to_region): New. > * store.h (binding_map::apply_ctor_to_region): New decl. > --- > gcc/analyzer/region-model.cc | 5 +++ > gcc/analyzer/region-model.h | 2 ++ > gcc/analyzer/region.cc | 27 +++++++++++++++++ > gcc/analyzer/store.cc | 59 ++++++++++++++++++++++++++++++++++++ > gcc/analyzer/store.h | 3 ++ > 5 files changed, 96 insertions(+) > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc > index 649e20438e4..3c7ea40e8d8 100644 > --- a/gcc/analyzer/region-model.cc > +++ b/gcc/analyzer/region-model.cc > @@ -1192,6 +1192,11 @@ region_model::get_rvalue (tree expr, region_model_context *ctxt) > const svalue * > region_model::get_store_value (const region *reg) const > { > + /* Special-case: handle var_decls in the constant pool. */ > + if (const decl_region *decl_reg = reg->dyn_cast_decl_region ()) > + if (const svalue *sval = decl_reg->maybe_get_constant_value (m_mgr)) > + return sval; > + > const svalue *sval > = m_store.get_any_binding (m_mgr->get_store_manager (), reg); > if (sval) > diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h > index 33aa3461611..3d044bf8d6c 100644 > --- a/gcc/analyzer/region-model.h > +++ b/gcc/analyzer/region-model.h > @@ -1869,6 +1869,8 @@ public: > tree get_decl () const { return m_decl; } > int get_stack_depth () const; > > + const svalue *maybe_get_constant_value (region_model_manager *mgr) const; > + > private: > tree m_decl; > }; > diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc > index f3f577c43de..afe416b001b 100644 > --- a/gcc/analyzer/region.cc > +++ b/gcc/analyzer/region.cc > @@ -874,6 +874,33 @@ decl_region::get_stack_depth () const > return 0; > } > > +/* If the underlying decl is in the global constant pool, > + return an svalue representing the constant value. > + Otherwise return NULL. */ > + > +const svalue * > +decl_region::maybe_get_constant_value (region_model_manager *mgr) const > +{ > + if (TREE_CODE (m_decl) == VAR_DECL > + && DECL_IN_CONSTANT_POOL (m_decl) > + && DECL_INITIAL (m_decl) > + && TREE_CODE (DECL_INITIAL (m_decl)) == CONSTRUCTOR) > + { > + tree ctor = DECL_INITIAL (m_decl); > + gcc_assert (!TREE_CLOBBER_P (ctor)); > + > + /* Create a binding map, applying ctor to it, using this > + decl_region as the base region when building child regions > + for offset calculations. */ > + binding_map map; > + map.apply_ctor_to_region (this, ctor, mgr); > + > + /* Return a compound svalue for the map we built. */ > + return mgr->get_or_create_compound_svalue (get_type (), map); > + } > + return NULL; > +} > + > /* class field_region : public region. */ > > /* Implementation of region::dump_to_pp vfunc for field_region. */ > diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc > index 950a7784542..232920019e0 100644 > --- a/gcc/analyzer/store.cc > +++ b/gcc/analyzer/store.cc > @@ -366,6 +366,65 @@ binding_map::dump (bool simple) const > pp_flush (&pp); > } > > +/* Get the child region of PARENT_REG based upon INDEX within a > + CONSTRUCTOR. */ > + > +static const region * > +get_subregion_within_ctor (const region *parent_reg, tree index, > + region_model_manager *mgr) > +{ > + switch (TREE_CODE (index)) > + { > + default: > + gcc_unreachable (); > + case INTEGER_CST: > + { > + const svalue *index_sval > + = mgr->get_or_create_constant_svalue (index); > + return mgr->get_element_region (parent_reg, > + TREE_TYPE (parent_reg->get_type ()), > + index_sval); > + } > + break; > + case FIELD_DECL: > + return mgr->get_field_region (parent_reg, index); > + } > +} > + > +/* Bind values from CONSTRUCTOR to this map, relative to > + PARENT_REG's relationship to its base region. */ > + > +void > +binding_map::apply_ctor_to_region (const region *parent_reg, tree ctor, > + region_model_manager *mgr) > +{ > + gcc_assert (parent_reg); > + gcc_assert (TREE_CODE (ctor) == CONSTRUCTOR); > + gcc_assert (!CONSTRUCTOR_NO_CLEARING (ctor)); > + > + unsigned ix; > + tree index; > + tree val; > + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), ix, index, val) > + { > + if (!index) > + index = build_int_cst (integer_type_node, ix); > + const region *child_reg > + = get_subregion_within_ctor (parent_reg, index, mgr); > + if (TREE_CODE (val) == CONSTRUCTOR) > + apply_ctor_to_region (child_reg, val, mgr); > + else > + { > + gcc_assert (CONSTANT_CLASS_P (val)); > + const svalue *cst_sval = mgr->get_or_create_constant_svalue (val); > + const binding_key *k > + = binding_key::make (mgr->get_store_manager (), child_reg, > + BK_direct); > + put (k, cst_sval); > + } > + } > +} > + > /* class binding_cluster. */ > > /* binding_cluster's copy ctor. */ > diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h > index 4f251d6420f..16bad030b36 100644 > --- a/gcc/analyzer/store.h > +++ b/gcc/analyzer/store.h > @@ -340,6 +340,9 @@ public: > void dump_to_pp (pretty_printer *pp, bool simple, bool multiline) const; > void dump (bool simple) const; > > + void apply_ctor_to_region (const region *parent_reg, tree ctor, > + region_model_manager *mgr); > + > private: > map_t m_map; > }; > -- > 2.26.2 >
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 649e20438e4..3c7ea40e8d8 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -1192,6 +1192,11 @@ region_model::get_rvalue (tree expr, region_model_context *ctxt) const svalue * region_model::get_store_value (const region *reg) const { + /* Special-case: handle var_decls in the constant pool. */ + if (const decl_region *decl_reg = reg->dyn_cast_decl_region ()) + if (const svalue *sval = decl_reg->maybe_get_constant_value (m_mgr)) + return sval; + const svalue *sval = m_store.get_any_binding (m_mgr->get_store_manager (), reg); if (sval) diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 33aa3461611..3d044bf8d6c 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -1869,6 +1869,8 @@ public: tree get_decl () const { return m_decl; } int get_stack_depth () const; + const svalue *maybe_get_constant_value (region_model_manager *mgr) const; + private: tree m_decl; }; diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc index f3f577c43de..afe416b001b 100644 --- a/gcc/analyzer/region.cc +++ b/gcc/analyzer/region.cc @@ -874,6 +874,33 @@ decl_region::get_stack_depth () const return 0; } +/* If the underlying decl is in the global constant pool, + return an svalue representing the constant value. + Otherwise return NULL. */ + +const svalue * +decl_region::maybe_get_constant_value (region_model_manager *mgr) const +{ + if (TREE_CODE (m_decl) == VAR_DECL + && DECL_IN_CONSTANT_POOL (m_decl) + && DECL_INITIAL (m_decl) + && TREE_CODE (DECL_INITIAL (m_decl)) == CONSTRUCTOR) + { + tree ctor = DECL_INITIAL (m_decl); + gcc_assert (!TREE_CLOBBER_P (ctor)); + + /* Create a binding map, applying ctor to it, using this + decl_region as the base region when building child regions + for offset calculations. */ + binding_map map; + map.apply_ctor_to_region (this, ctor, mgr); + + /* Return a compound svalue for the map we built. */ + return mgr->get_or_create_compound_svalue (get_type (), map); + } + return NULL; +} + /* class field_region : public region. */ /* Implementation of region::dump_to_pp vfunc for field_region. */ diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index 950a7784542..232920019e0 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -366,6 +366,65 @@ binding_map::dump (bool simple) const pp_flush (&pp); } +/* Get the child region of PARENT_REG based upon INDEX within a + CONSTRUCTOR. */ + +static const region * +get_subregion_within_ctor (const region *parent_reg, tree index, + region_model_manager *mgr) +{ + switch (TREE_CODE (index)) + { + default: + gcc_unreachable (); + case INTEGER_CST: + { + const svalue *index_sval + = mgr->get_or_create_constant_svalue (index); + return mgr->get_element_region (parent_reg, + TREE_TYPE (parent_reg->get_type ()), + index_sval); + } + break; + case FIELD_DECL: + return mgr->get_field_region (parent_reg, index); + } +} + +/* Bind values from CONSTRUCTOR to this map, relative to + PARENT_REG's relationship to its base region. */ + +void +binding_map::apply_ctor_to_region (const region *parent_reg, tree ctor, + region_model_manager *mgr) +{ + gcc_assert (parent_reg); + gcc_assert (TREE_CODE (ctor) == CONSTRUCTOR); + gcc_assert (!CONSTRUCTOR_NO_CLEARING (ctor)); + + unsigned ix; + tree index; + tree val; + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), ix, index, val) + { + if (!index) + index = build_int_cst (integer_type_node, ix); + const region *child_reg + = get_subregion_within_ctor (parent_reg, index, mgr); + if (TREE_CODE (val) == CONSTRUCTOR) + apply_ctor_to_region (child_reg, val, mgr); + else + { + gcc_assert (CONSTANT_CLASS_P (val)); + const svalue *cst_sval = mgr->get_or_create_constant_svalue (val); + const binding_key *k + = binding_key::make (mgr->get_store_manager (), child_reg, + BK_direct); + put (k, cst_sval); + } + } +} + /* class binding_cluster. */ /* binding_cluster's copy ctor. */ diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h index 4f251d6420f..16bad030b36 100644 --- a/gcc/analyzer/store.h +++ b/gcc/analyzer/store.h @@ -340,6 +340,9 @@ public: void dump_to_pp (pretty_printer *pp, bool simple, bool multiline) const; void dump (bool simple) const; + void apply_ctor_to_region (const region *parent_reg, tree ctor, + region_model_manager *mgr); + private: map_t m_map; };
PR testsuite/96609 and PR analyzer/96616 report various testsuite failures seen on powerpc64, aarch64, and arm in new tests added by r11-2694-g808f4dfeb3a95f50f15e71148e5c1067f90a126d. Some of these failures (in gcc.dg/analyzer/init.c, and on arm in gcc.dg/analyzer/casts-1.c) relate to initializations from var_decls in the constant pool. I wrote the tests assuming that the gimplified stmts would initialize the locals via a gassign of code CONSTRUCTOR, whereas on these targets some of the initializations are gassign from a VAR_DECL e.g.: c = *.LC0; where "*.LC0" is a var_decl with DECL_IN_CONSTANT_POOL set. For example, in test_7: struct coord c[2] = {{3, 4}, {5, 6}}; __analyzer_eval (c[0].x == 3); /* { dg-warning "TRUE" } */ after the initialization, the store was simply recording: cluster for: c: INIT_VAL(*.LC0) when I was expecting the cluster for c to have: cluster for: c key: {kind: direct, start: 0, size: 32, next: 32} value: 'int' {(int)3} key: {kind: direct, start: 32, size: 32, next: 64} value: 'int' {(int)4} key: {kind: direct, start: 64, size: 32, next: 96} value: 'int' {(int)5} key: {kind: direct, start: 96, size: 32, next: 128} value: 'int' {(int)6} The test for c[0].x == 3 would then generate: cluster for: _2: (SUB(SUB(INIT_VAL(*.LC0), c[(int)0]), c[(int)0].x)==(int)3) which is UNKNOWN, leading to the test failing. This patch fixes the init.c and casts-1.c failures by special-casing reads from a var_decl with DECL_IN_CONSTANT_POOL set, so that they build a compound_svalue containing the bindings implied by the CONSTRUCTOR node for DECL_INITIAL. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Manually verified the fixes to init.c and casts-1.c on aarch64-unknown-linux-gnu, arm-unknown-eabi, and powerpc64-linux-gnu (-m32 and -m64). Pushed to master as r11-2708-g2867118ddda9b56d991c16022f7d3d634ed08313. This doesn't address the bogus -Wanalyzer-too-complex messages for pr93032-mztools.c reported in the bugs, which seem to be a separate issue that I'm now investigating. gcc/analyzer/ChangeLog: PR testsuite/96609 PR analyzer/96616 * region-model.cc (region_model::get_store_value): Call maybe_get_constant_value on decl_regions first. * region-model.h (decl_region::maybe_get_constant_value): New decl. * region.cc (decl_region::get_stack_depth): Likewise. (decl_region::maybe_get_constant_value): New. * store.cc (get_subregion_within_ctor): New. (binding_map::apply_ctor_to_region): New. * store.h (binding_map::apply_ctor_to_region): New decl. --- gcc/analyzer/region-model.cc | 5 +++ gcc/analyzer/region-model.h | 2 ++ gcc/analyzer/region.cc | 27 +++++++++++++++++ gcc/analyzer/store.cc | 59 ++++++++++++++++++++++++++++++++++++ gcc/analyzer/store.h | 3 ++ 5 files changed, 96 insertions(+)