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>
