David, Two items I needs answered to get you the patch.
1) The get_field_at_bit_offset function is static in region.cc, so I cannot use it outside of the file (you want me to use it in store.cc) without updating that definition to extern. I am assuming you want this. 2) I am updating my direct access of the tree fields. I am using int_bit_position for the position of a field in a struct as you requested. What do I use for the size of the struct. I had been using ->decl_common.size->int_cst[0]. Brian Sent with ProtonMail Secure Email. ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Saturday, February 13, 2021 9:53 AM, brian.sobulefsky <brian.sobulef...@protonmail.com> wrote: > Hi, answers below. Note, do you want my resubmitted patch to be with > commit --amend, and therefore relative to "real" commits in the history, > or do you want me to chain it onto the last submission? > > I have already sent to assign at gnu for the form. > > I will update the commit message to call it s3 now. > > The case did not arise "in practice." After solving the xfail from the > test suite, I tried dividing the array to have more than one field and > found that this still did not work, so I tracked down why. I would > argue that the case of a struct made of only a single array as its one > field is a lot less likely to arise in practice. For what its worth, > the single field example basically works "for the wrong > reason." > When get_binding_recursive goes up the parent chain, it finds > a binding > in the parent cast region because that cast is a struct made of > only one > field, and so its binding_key is the same as that field. The > "right" > logic would check for a field covering the desired region. > > I'll fix the ChangeLog formatting parentheses. I found the formatting > script, but I did not have one of the imported python files so I just copied others. > > I can add the helper routine you have requested. Where would you like it, > I am thinking analyzer.h so it is visible basically everywhere? > > I can add the check for size matching, which helper routine is easiest. > > Yes, in the event of *p == 'A', *p, having a constant zerop offset > would get sent to get_cast_region, where it would become a cast region. > This defeats the new logic in maybe_fold_sub_svalue. I think in the case > where we are pointing to an array of the requested type, it is much more > correct to return the offset region with a zero offset, as arr[0] should not > be different than arr[1]. Sometimes the 0 tree would be of pointer_type, > and I was not sure if this would defeat it or not, so I made sure it was > of integer_type. This may just be a matter of my being new and not > knowing for sure how everything works and so erring on the side of safety. > > As of now, the routine immediately rejects any case other than where > reg is a field_region and parent is a cast_region. I will think if there > is a C like syntax for the function, really it is checking if the original > form of parent had a field covering the requested field. > > I will remove the first guard, leave the second, and try to reformat > the latters into a similar rejection style. > > Currently, get_field_at_bit_offset is not an externally visible function. > I am taking your instruction to reuse it to mean a prototype should > be added to the relevant header (it would be region.h I think, as long as > both region-model-manager.cc and store.cc include those). > > As you know, I am very new to gcc and so was happy when I could hack my > way to this working at all. I already assumed my direct access was not > correct. Most of what I did is based on "RTFS" since the manual does not > really cover the right way to do these things. I will try the routine > or otherwise macro you say. > > I will change the testfile to leave pre exisiting lines in place. > > Sent with ProtonMail Secure Email. > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Friday, February 12, 2021 4:16 PM, David Malcolm dmalc...@redhat.com wrote: > > > 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 </brian.sobulef...@protonmail.com>