[Bug analyzer/98797] New: Simpler version of the XFAIL in casts-1.c with proposed solution

[Bug analyzer/98797] New: Simpler version of the XFAIL in casts-1.c with proposed solution

[brian.sobulefsky at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98797

            Bug ID: 98797
           Summary: Simpler version of the XFAIL in casts-1.c with
                    proposed solution
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: brian.sobulefsky at protonmail dot com
  Target Milestone: ---

I took some time to investigate the bug concerning the XFAILs in file
gcc/testsuite/gcc.dg/analyzer/casts-1.c and found that there is a simpler
manifestation of this error unrelated to casts. I also have a tentative
solution I can offer. I am new here so I'm sorry if I should be suggesting this
elsewhere, just let me know. In the casts-1.c file, a test is run of the form:


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');


Which should obviously be TRUE but comes out as UNKNOWN. The problem can
actually be replicated with just


char arc[4] = {{'A', 'B', 'C', 'D'}};
char *ptc = arc;

__analyzer_eval(*ptc == 'A');
__analyzer_eval(*(ptc+1) == 'B');


Both of these should obviously be TRUE, and they will print out UNKNOWN. These
do not require any contorted type conversion and suffer the same failure.

Briefly, this problem only occurs if you initialize an entire array and then
point to the array and try to access the individual values by pointer
de-reference. It does not occur if you initialize the array and try to access
the array element directly (you already programmed that case into it), and it
does not happen if you assign to the array elements directly for either case
(array access or pointer de-reference). Because direct assignment to the array
solves the problem, we do not see this problem with integers, as the
initialization of an integer array evidently compiles to individual assignments
to each element.

The easiest issue to solve was the *(ptc+1) version. This case runs as expected
through the whole program, just as arc[1] would, until we get to
region_model_manager::maybe_fold_sub_svalue. In the section of this method
commented  /* Handle getting individual chars from a STRING_CST.  */, there is
only a case for an element_region. By simply adding a analogous case for an
offset_region, modified as necessary for the different class, this can be
handled.

This solves *(ptc+N) for any N except 0. It also does not solve *ptc, which
evidently compiles the same as *(ptc+0). For this case, a modification can be
made to region_model_manager::get_offset_region in for section commented /* If
BYTE_OFFSET is zero, return PARENT.  */. Before checking the zerop condition
and calling get_cast_region, we check a special case where argument region
*parent is of type ARRAY_TYPE and the type of that ARRAY_TYPE is the same as
argument tree type. In this case, we allow the normal method conclusion of
building an offset_region with a byte_offset of 0. After this, the fix
described in the prior paragraph will work for this case.

Finally, for the original issue involving converting between structs, an
analogous case can again be added again to the /* Handle getting individual
chars from a STRING_CST.  */ section of
region_model_manager::maybe_fold_sub_svalue for a field_reg.

This will solve the three manifestations described here. It is known to not
solve the case where struct s2 was defined as {char arr1[2]; char arr2[2];},
which I am actually close to solving already. I have posted the current status
of my solution to GitHub. In the code, I left descriptive comments where I made
changes so that it is obvious. If you want to keep the changes, of course
remove the comments. In the ChangeLog, I put an entry that is a bit more
appropriate.

It is currently the only branch, but I have the branch named
xfail-casts-1-soln0.
https://github.com/poisson-aerohead/gcc

[Bug analyzer/98797] Simpler version of the XFAIL in casts-1.c with proposed solution

[brian.sobulefsky at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98797

--- Comment #1 from Brian Sobulefsky <brian.sobulefsky at protonmail dot com> ---
I have updated the repository I linked previously so that it has current gcc
sources and also includes a solution for the case where struct s2 is not a
single field, that is, we have something like:

struct s1
{
  char a;
  char b;
  char c;
  char d;
};

struct s2
{
  char arr1[2];
  char arr1[2];
};

struct s2 x = {{'A', 'B'}, {'C', 'D'}};
struct s1 *p = (struct s1 *)&x;

__analyzer_eval (p->a == 'A');

The solution I submitted basically has the logic right, but I put everything
into binding_cluster:get_binding_recursive instead of creating a new method for
class binding_cluster, which is the right way to do it. I did this just to have
a quick outline of the solution without altering the class members, but in my
view get_binding_recursive is too cluttered this way and the code in the case I
added should be moved to a new function.

[Bug analyzer/98797] Simpler version of the XFAIL in casts-1.c with proposed solution

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98797

--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:2ac7b19f1e9219f46ccf55f25d8acb3e02e9a2d4

commit r12-7184-g2ac7b19f1e9219f46ccf55f25d8acb3e02e9a2d4
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Wed Feb 9 19:06:15 2022 -0500

    analyzer: handle more casts of string literals [PR98797]

    gcc/analyzer/ChangeLog:
            PR analyzer/98797
            * region-model-manager.cc
            (region_model_manager::maybe_fold_sub_svalue): Generalize getting
            individual chars of a STRING_CST from element_region to any
            subregion which is a concrete access of a single byte from its
            parent region.
            * region.cc (region::get_relative_concrete_byte_range): New.
            * region.h (region::get_relative_concrete_byte_range): New decl.

    gcc/testsuite/ChangeLog:
            PR analyzer/98797
            * gcc.dg/analyzer/casts-1.c: Mark xfails as fixed; add further
            test coverage for casts of string literals.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/98797] Simpler version of the XFAIL in casts-1.c with proposed solution

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98797

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
The branch in comment #0 now gives a 404, but in any case I had to rewrite the
store code in gcc 12 to support detection of uses of uninitialized values, so
any patch is likely bit-rotted.  The new store implementation allows for a
simpler solution that avoids special-casing, which I've committed as the patch
above, fixing the xfails.

[Bug analyzer/98797] Simpler version of the XFAIL in casts-1.c with proposed solution

[brian.sobulefsky at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98797

--- Comment #4 from Brian Sobulefsky <brian.sobulefsky at protonmail dot com> ---
(In reply to David Malcolm from comment #3)
> The branch in comment #0 now gives a 404, but in any case I had to rewrite
> the store code in gcc 12 to support detection of uses of uninitialized
> values, so any patch is likely bit-rotted.  The new store implementation
> allows for a simpler solution that avoids special-casing, which I've
> committed as the patch above, fixing the xfails.

Hi David. As you requested last year I provided an actual patch file generated
by git. If you remember, I did not have a job then and was just sort of looking
around for projects to get involved in. I was new to the distributed
development thing, as I was mostly just a hobby hacker, and so initially was
not sure how to get you the fix. I submitted the patch in an email chain with
subject "Patch for PR analyzer/98797" around February 9, 20201.

A while after, I deleted the repository, as it was both not the preferred way
of submitting gcc patches anyway and also I had not done any gcc work in a
while, so it was just a severely out of date mirror.

I have not looked at your codebase in a while, so if things have changed that
is fine. The solution I tracked down last year was based on the way you had
everything structured at the time. It was a fun project to try to trace someone
else's project like that anyway.

I know we had also had some discussions regarding constraints on another
thread. I had come up with a preliminary way for the constraints to resolve
addition and some other operations, but that problem became a bit more complex
as we needed to decide how to handle the possibility of overflow and I never
saw a final answer on that question. I had gone down that road due to a bug
found in a run from the people at openssl where the constraint manager was not
able to follow a loop correctly. This had involved your "widening_svalue", and
I was not clear on how that worked, but for starters I got it to understand
basic operations like "svalue+3" or whatever, and then that led us to ask how
to handle the possibility of overflow.

If you would like some targeted assistance with development and bug fixing, let
me know.