On Wed, 2021-02-10 at 19:42 +0000, brian.sobulefsky wrote: Hi Brian
Thanks for the patch. The patch is large enough to count as legally significant; I've sent you information on copyright assignment separately; the patch can't be committed until that paperwork is in place. In the meantime, I've added some review notes inline below throughout: > Address the bug found in test file > gcc/testsuite/gcc.dg/analyzer/casts-1.c, as > recorded by the XFAIL, and some simpler and more complex versions found > during > the investigation of it. PR analyzer/98797 was opened for these bugs. > > The simplest manifestation of this bug can be replicated with: > > char arr[] = {'A', 'B', 'C', 'D'}; > char *pt = ar; > __analyzer_eval(*(pt + 0) == 'A'); > __analyzer_eval(*(pt + 1) == 'B'); > //etc > > The result of the eval call is "UNKNOWN" because the analyzer is unable to > determine the value at the pointer. Note that array access (such as > arr[0]) is > correctly handled. The code responsible for this is in file > region-model-manager.cc, function > region_model_manager::maybe_fold_sub_svalue. > The relevant section is commented /* Handle getting individual chars from > STRING_CST */. This section only had a case for an element_region. A case > needed to be added for an offset_region. > > Additionally, when the offset was 0, such as in *pt or *(pt + 0), the > function > region_model_manager::get_offset_region was failing to make the needed > offset > region at all. This was due to the test for a constant 0 pointer that was > then > returning get_cast_region. A special case is needed for when PARENT is of > type > array_type whose type matches TYPE. In this case, get_offset_region is > allowed > to continue to normal conclusion. > > The original bug noted in gcc/testsuite/gcc.dg/analyzer/casts-1.c was for > the > case: > > struct s1 > { > char a; > char b; > char c; > char d; > }; > > struct s2 > { > char arr[4]; > }; > > struct s2 x = {{'A', 'B', 'C', 'D'}}; > struct s1 *p = (struct s1 *)&x; > __analyzer_eval (p->a == 'A'); > //etc > > This requires a case added to region_model_manager::maybe_fold_sub_svalue > in > the individual characters from string constant section for a field region. > > Finally, the prior only works for the case where struct s2 was a single > field > struct. The more general case is: > > struct s2 > { > char arr1[2]; > char arr2[2]; > }; > > struct s2 x = {{'A', 'B'}, {'C', 'D'}}; This is s3 in the testcase, rather than s2; looks like this commit message should be updated accordingly to match your change to the testcase. BTW, did this case arise in practice? The existing cases are rather artificial; IIRC I created them whilst experimenting with various casts whilst prototypeing the code, in the hope of generating coverage, but without any specific real-world examples in mind. (kind of "kicking the tires", if you will). Am I right in thinking that this new one arose in a similar way? > Here the array will never be found in the get_any_binding routines. A new > routine is added to class binding_cluster that checks if the region being > searched is a field_region of a cast_region, and if so, tries to find a > field > of the original region that contains the region under examination. This > new > function is named binding_cluster::get_parent_cast_binding. It is called > from > get_binding_recursive. > > gcc/analyzer/ChangeLog: > PR analyzer/98797 > * region-model-manager.cc > region_model_manager::get_offset_region: Add > check for a PARENT array whose type matches TYPE, and have this > skip > returning get_cast_region and rather conclude the function > normally. > * region-model-manager.cc > region_model_manager::maybe_fold_sub_svalue > Update the get character from string_cst section to include cases > for > an offset_region and a field_region. > * store.cc binding_cluster::get_binding_recursive: Add case for > call > to new function get_parent_cast_binding. > * store.cc binding_cluster::get_parent_cast_binding: New function. > * store.h class binding_cluster: Add declaration for new member > function get_parent_class_binding and macros for bit to byte > conversion. Formatting nit: the items in the ChangeLog within each file should be enclosed by parentheses. We have a git commit hook that verifies the format. In theory there's a way to run it ahead of time, but I don't know of the top of my head where it is. > gcc/testsuite/ChangeLog: > PR analyzer/98797 > * gcc.dg/analyzer/casts-1.c: Update file to no longer expect > failures > and add test cases for additional bugs solved. > > diff --git a/gcc/analyzer/region-model-manager.cc > b/gcc/analyzer/region-model-manager.cc > index dfd2413e914..1fd6b94f20a 100644 > --- a/gcc/analyzer/region-model-manager.cc > +++ b/gcc/analyzer/region-model-manager.cc > @@ -602,16 +602,46 @@ region_model_manager::maybe_fold_sub_svalue (tree type, > /* Handle getting individual chars from a STRING_CST. */ > if (tree cst = parent_svalue->maybe_get_constant ()) > if (TREE_CODE (cst) == STRING_CST) > - if (const element_region *element_reg > + { > + if (const element_region *element_reg > = subregion->dyn_cast_element_region ()) > - { > - const svalue *idx_sval = element_reg->get_index (); > - if (tree cst_idx = idx_sval->maybe_get_constant ()) > - if (const svalue *char_sval > - = maybe_get_char_from_string_cst (cst, cst_idx)) > - return get_or_create_cast (type, char_sval); > - } > - > + { > + const svalue *idx_sval = element_reg->get_index (); > + if (tree cst_idx = idx_sval->maybe_get_constant ()) > + if (const svalue *char_sval > + = maybe_get_char_from_string_cst (cst, cst_idx)) > + return get_or_create_cast (type, char_sval); > + } > + else if (const offset_region *offset_reg > + = subregion->dyn_cast_offset_region ()) > + { > + const svalue *offset_sval = offset_reg->get_byte_offset(); > + if (tree cst_offset = offset_sval->maybe_get_constant ()) > + if (const svalue *char_sval > + = maybe_get_char_from_string_cst (cst, cst_offset)) > + return get_or_create_cast (type, char_sval); > + } > + else if (const field_region *field_reg > + = subregion->dyn_cast_field_region ()) > + { > + region_offset field_offset = field_reg->get_offset (); > + if (!field_offset.symbolic_p ()) > + { > + bit_offset_t field_bit_offset = field_offset.get_bit_offset (); > + HOST_WIDE_INT field_offset_bits > + = field_bit_offset.get_val ()[0]; > + if(!(field_offset_bits & BYTE_BOUNDARY_MASK)) > + { > + tree cst_offset > + = build_int_cst (integer_type_node, > + field_offset_bits >> BYTE_BIT_CONVERT); The patch adds logic in two places to mask a HOST_WIDE_INT against BYTE_BOUNDARY_MASK and if zero, shift by BYTE_BIT_CONVERT, and then to build_int_cst from the result. The mask and shift feel like premature optimization to me. Please can you instead introduce a helper subroutine to do this, something like: /* If offset_bits is a multiple of BITS_PER_UNIT, return an INTEGER_CST for the offset expressed in bytes. Otherwise, return NULL_TREE. */ tree maybe_convert_bit_to_byte_offset (HOST_WIDE_INT offset_bits) { if (offset_bits % BITS_PER_UNIT) return NULL_TREE; return build_int_cst (offset_bits / BITS_PER_UNIT); } > + if (const svalue *char_sval > + = maybe_get_char_from_string_cst (cst, cst_offset)) > + return get_or_create_cast (type, char_sval); Doesn't this need to also check that the size of the field is sizeof(char)? Consider: void test_4 () { const char *s = "ABCD"; __analyzer_eval (*(short *)s == 'A'); } With your patch this evaluates as TRUE, as it's erroneously slicing off the 2nd byte of "AB". It ought to be FALSE (and endianness concerns would thwart TRUE also). > + } > + } > + } > + } > /* SUB(INIT(r)).FIELD -> INIT(r.FIELD) > i.e. > Subvalue(InitialValue(R1), FieldRegion(R2, F)) > @@ -867,8 +897,19 @@ region_model_manager::get_offset_region (const region > *parent, > { > /* If BYTE_OFFSET is zero, return PARENT. */ > if (tree cst_offset = byte_offset->maybe_get_constant ()) > - if (zerop (cst_offset)) > - return get_cast_region (parent, type); > + { > + /* Special case: PARENT type is array_type whose type matches TYPE */ > + if (parent->get_type () > + && TREE_CODE(parent->get_type ()) == ARRAY_TYPE > + && TREE_TYPE(parent->get_type ()) == type) > + { > + tree offset_int > + = build_int_cst (integer_type_node, cst_offset->int_cst.val[0]); It took me a while to get the logic here. Am I right in thinking that this is to avoid returning (TYPE)PARENT for the 0 offset case when it's a matching array, since: PARENT[0] is better for that case? If so, I think the comments could be updated. I'm not quite seeing the purpose of constructing offset_int from cst_offset here. Is it to get a value of integer_type_node, or is it redundant? > + byte_offset = get_or_create_constant_svalue (offset_int); > + } > + else if (zerop (cst_offset)) > + return get_cast_region (parent, type); > + } > > /* Fold OFFSET_REGION(OFFSET_REGION(REG, X), Y) > to OFFSET_REGION(REG, (X + Y)). */ > diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc > index abdb336da91..3afba55ce90 100644 > --- a/gcc/analyzer/store.cc > +++ b/gcc/analyzer/store.cc > @@ -996,14 +996,129 @@ binding_cluster::get_binding_recursive (store_manager > *mgr, > return sval; > if (reg != m_base_region) > if (const region *parent_reg = reg->get_parent_region ()) > - if (const svalue *parent_sval > - = get_binding_recursive (mgr, parent_reg, kind)) > + { > + if (const svalue *parent_sval > + = get_binding_recursive (mgr, parent_reg, kind)) > + { > + /* Extract child svalue from parent svalue. */ > + region_model_manager *rmm_mgr = mgr->get_svalue_manager (); > + return rmm_mgr->get_or_create_sub_svalue (reg->get_type (), > + parent_sval, reg); > + } > + else if (const svalue *parent_cast_field_sval > + = get_parent_cast_binding (mgr, reg, kind)) > + { > + /* PARENT_REG is a cast_region and we found a covering binding > + in the original_region */ > + return parent_cast_field_sval; > + } > + } > + return NULL; > +} > + > +/* Get a binding for access to a field of a cast_region. Is it always a cast_region? "reg" is a "const region *". Is it possible to express in C-like syntax what this function does, in the most general case. Is it something like: ((TYPE)EXPR).FIELD ? > + Note the original motivation for this was access of the form: > + struct s1 x = {{'A', 'B'}; {'C', 'D'}}; struct s2 p = (struct s2) &x; > + __analyzer_eval (p->a == 'A'); I think the "s2" needs to be "s2 *" here. > + et al. for the other fields. get_binding_recursive fails because it is > + unidirectional (from the field_region p->a up the chain of parents). The > + routine can probably be expanded, and even further broken out, as other > cases > + are discovered and understood. */ > +const svalue * > +binding_cluster::get_parent_cast_binding (store_manager *mgr, > + const region *reg, > + enum binding_kind kind) const > +{ > + > + /* If we are sure this will never be called incorrectly, we can remove */ > + if (!(mgr && reg)) > + return NULL; I'm fairly sure that mgr will always be non-NULL. I'd prefer to remove the checks; as far as I can tell, reg is always non-NULL. > + const region *parent_reg = reg->get_parent_region (); > + > + /* If it is impossible for a region's parent to be NULL, remove guard */ > + if(!parent_reg) > + return NULL; Some regions do have NULL parents e.g. symbolic regions. I'm not sure whether or not it can be called for them, so I'd keep this defensive check. > + const cast_region *parent_cast = parent_reg->dyn_cast_cast_region (); > + const field_region *field_reg = reg->dyn_cast_field_region (); > + > + /* The first two guards are just safety, this is the real condition */ > + if (!(parent_cast && field_reg)) > + return NULL; > + > + const region *original_reg = parent_cast->get_original_region (); > + region_offset reg_offset = field_reg->get_offset (); > + bit_size_t reg_bit_size; > + tree original_tree = original_reg->get_type (); > + tree original_field; > + > + /* Note assignment to ORIGINAL_FIELD in compound test to > + save some indentation space (80 comes quickly) */ I find it's often easiest to rewrite a series of nested conditionals: if (foo) if (bar) if (baz) { /* etc, where did all the columns go??? */ } into: if (!foo) return; if (!bar) return; if (!baz) return; /* Now we still have plenty of room. */ assuming that the suite being guarded is all of the rest of the function. > + if (original_tree && TREE_CODE(original_tree) == RECORD_TYPE > + && (original_field = TYPE_FIELDS(original_tree)) > + && !reg_offset.symbolic_p() && field_reg->get_bit_size(®_bit_size)) so the above would probably be best rewritten. That said, for future reference, with the compound conditional, you correctly put the "&&" at the start of the line, but every such clause needs to be at the start of a new line, so the above should read: if (original_tree && TREE_CODE (original_tree) == RECORD_TYPE && (original_field = TYPE_FIELDS (original_tree)) && !reg_offset.symbolic_p () && field_reg->get_bit_size (®_bit_size)) to avoid conditions hiding from a casual reader. ...but inverting the conditions and bailing is probably better for the column-width thing as noted above. > + { > + > + bit_offset_t reg_bit_offset = reg_offset.get_bit_offset (); > + HOST_WIDE_INT start_offset = reg_bit_offset.get_val ()[0]; > + HOST_WIDE_INT end_offset = start_offset + reg_bit_size.get_val ()[0] - > 1; > + tree covering_field = NULL_TREE; > + > + /* get_field_at_bit_offset in region.cc has a test for RECORD_TYPE, > maybe > + this should be here too? The test is probably moot, since we have to It would need the test (which is to handle methods and nested types in C++ classes), but I believe this code can simply reuse get_field_at_bit_offset, rather than duplicating the logic. > + fully cover and survive the get_binding_recursive below. Also > + consdider making the function in region.cc usable outside the file? */ > + for (covering_field = original_field; > + (original_field = TREE_CHAIN (original_field));) > + { > + if (original_field->field_decl.bit_offset->int_cst.val[0] > + <= start_offset) > + covering_field = original_field; > + else > + break; /* Is the list guaranteed to be in order? */ > + } > + > + if (covering_field && end_offset > + <= covering_field->field_decl.bit_offset->int_cst.val[0] > + + covering_field->decl_common.size->int_cst.val[0] - 1) Linebreak before &&; probably needs parentheses, so it ought to be formatted something like this: if (covering_field && (end_offset <= (covering_field->field_decl.bit_offset->int_cst.val[0] + covering_field->decl_common.size->int_cst.val[0] - 1))) But the direct access of tree fields seems incorrect to me. Fixing that would mean: if (covering_field && (end_offset <= (DECL_FIELD_BIT_OFFSET (covering_field)->int_cst.val[0] + DECL_SIZE (covering_field)->int_cst.val[0] - 1))) but the usage of int_cst.val[0] seem incorrect to me (what if it's a really large value for which the low entry is 0?); does int_bit_position do what you need? > { > - /* Extract child svalue from parent svalue. */ > + /* We found a field that entirely covers REG. The following code > + should probably be generalized to more cases, as of now it will > + basically handle the case where a char array has been initialized > + into the original struct type. Further work might be to handle when > + a field to a struct is itself a struct, which is likely recursive.*/ > + > region_model_manager *rmm_mgr = mgr->get_svalue_manager (); > - return rmm_mgr->get_or_create_sub_svalue (reg->get_type (), > - parent_sval, reg); > + const region *covering_field_reg = rmm_mgr > + ->get_field_region (original_reg, covering_field); > + > + if (const svalue *parent_sval = get_binding_recursive (mgr, > + covering_field_reg, kind)) Probably best to put the newline before the "=" in this; see examples elsewhere in the analyzer sources. > + { > + > + HOST_WIDE_INT covering_field_offset = covering_field > + ->field_decl.bit_offset->int_cst.val[0]; I don't think the code should be accessing int_cst.val[0]. Does int_bit_position do what you need? > + if (TREE_CODE(covering_field_reg->get_type()) == ARRAY_TYPE > + && !((start_offset - covering_field_offset) > + & BYTE_BOUNDARY_MASK)) > + { > + const svalue *arr_index = rmm_mgr > + ->get_or_create_constant_svalue ( > + build_int_cst (integer_type_node, > + (start_offset - covering_field_offset) > + >> BYTE_BIT_CONVERT)); See notes above about introducing a subroutine for part of this. > + const region *elmt_reg = rmm_mgr > + ->get_element_region (covering_field_reg, > + TREE_TYPE (covering_field_reg->get_type ()), > + arr_index); Probably best to have the newline before the "=" here. > + return rmm_mgr->get_or_create_sub_svalue ( > + elmt_reg->get_type (), parent_sval, elmt_reg); > + } > + } > } > + } > return NULL; > } > > diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h > index 2bcef6c398a..9ddc57fea01 100644 > --- a/gcc/analyzer/store.h > +++ b/gcc/analyzer/store.h > @@ -416,6 +416,9 @@ public: > const svalue *get_binding_recursive (store_manager *mgr, > const region *reg, > enum binding_kind kind) const; > + const svalue *get_parent_cast_binding (store_manager *mgr, > + const region *reg, > + enum binding_kind kind) const; > const svalue *get_any_binding (store_manager *mgr, > const region *reg) const; > const svalue *maybe_get_compound_binding (store_manager *mgr, > @@ -640,4 +643,7 @@ private: > > } // namespace ana > > +#define BYTE_BIT_CONVERT 3 > +#define BYTE_BOUNDARY_MASK ((1 << BYTE_BIT_CONVERT) - 1) As noted above, please lose these in favor of a subroutine. > #endif /* GCC_ANALYZER_STORE_H */ > diff --git a/gcc/testsuite/gcc.dg/analyzer/casts-1.c b/gcc/testsuite/gcc.dg/analyzer/casts-1.c > index 15cd85f77cf..9340615b20f 100644 > --- a/gcc/testsuite/gcc.dg/analyzer/casts-1.c > +++ b/gcc/testsuite/gcc.dg/analyzer/casts-1.c > @@ -13,6 +13,21 @@ struct s2 > char arr[4]; > }; > > +struct s3 > +{ > + char arr1[2]; > + char arr2[2]; > +}; > + > +void test_0 () > +{ > + char ar[] = {'A', 'B', 'C', 'D'}; > + char *pt = ar; > + __analyzer_eval (*(pt + 0) == 'A'); /* { dg-warning "TRUE" } */ > + __analyzer_eval (*(pt + 1) == 'B'); /* { dg-warning "TRUE" } */ > + __analyzer_eval (*(pt + 2) == 'C'); /* { dg-warning "TRUE" } */ > + __analyzer_eval (*(pt + 3) == 'D'); /* { dg-warning "TRUE" } */ > +} Probably best to append new tests, rather than to insert them, to avoid changing the line numbers of existing tests (which can help when comparing DejaGnu results). > void test_1 () > { > struct s1 x = {'A', 'B', 'C', 'D'}; > @@ -38,12 +53,22 @@ void test_2 () > __analyzer_eval (x.arr[2] == 'C'); /* { dg-warning "TRUE" } */ > __analyzer_eval (x.arr[3] == 'D'); /* { dg-warning "TRUE" } */ > struct s1 *p = (struct s1 *)&x; > - __analyzer_eval (p->a == 'A'); /* { dg-warning "TRUE" "true" { xfail *-*-* } } */ > - /* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ > - __analyzer_eval (p->b == 'B'); /* { dg-warning "TRUE" "true" { xfail *-*-* } } */ > - /* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ > - __analyzer_eval (p->c == 'C'); /* { dg-warning "TRUE" "true" { xfail *-*-* } } */ > - /* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ > - __analyzer_eval (p->d == 'D'); /* { dg-warning "TRUE" "true" { xfail *-*-* } } */ > - /* { dg-bogus "UNKNOWN" "unknown" { xfail *-*-* } .-1 } */ > + __analyzer_eval (p->a == 'A'); /* { dg-warning "TRUE" "true" } */ > + __analyzer_eval (p->b == 'B'); /* { dg-warning "TRUE" "true" } */ > + __analyzer_eval (p->c == 'C'); /* { dg-warning "TRUE" "true" } */ > + __analyzer_eval (p->d == 'D'); /* { dg-warning "TRUE" "true" } */ > +} It's great to see xfails become passes. > +void test_3 () > +{ > + struct s3 x = {{'A', 'B'}, {'C', 'D'}}; > + __analyzer_eval (x.arr1[0] == 'A'); /* { dg-warning "TRUE" } */ > + __analyzer_eval (x.arr1[1] == 'B'); /* { dg-warning "TRUE" } */ > + __analyzer_eval (x.arr2[0] == 'C'); /* { dg-warning "TRUE" } */ > + __analyzer_eval (x.arr2[1] == 'D'); /* { dg-warning "TRUE" } */ > + struct s1 *p = (struct s1 *)&x; > + __analyzer_eval (p->a == 'A'); /* { dg-warning "TRUE" "true" } */ > + __analyzer_eval (p->b == 'B'); /* { dg-warning "TRUE" "true" } */ > + __analyzer_eval (p->c == 'C'); /* { dg-warning "TRUE" "true" } */ > + __analyzer_eval (p->d == 'D'); /* { dg-warning "TRUE" "true" } */ > } Thanks again for the patch; hope the above makes sense and is constructive. Dave