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
