[Bug tree-optimization/104970] New: ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[Bug tree-optimization/104970] New: ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[marxin at gcc dot gnu.org]

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

            Bug ID: 104970
           Summary: ICE in execute_todo, at passes.cc:2133 since
                    r12-6480-gea19c8f33a3a8d2b
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Keywords: ice-on-valid-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: siddhesh at gcc dot gnu.org
  Target Milestone: ---

The following is reduced from libmsym package:

$ cat linalg.i
__inline void
memset2(void *__dest, int __ch, long __len) {
  long __trans_tmp_1 = __builtin_dynamic_object_size(__dest, 0);
  __builtin___memset_chk(__dest, __ch, __len, __trans_tmp_1);
}

void
mleye(int l, double E[][l]) { memset2(E, 0, sizeof(double)); }

$ gcc linalg.i -c -D_FORTIFY_SOURCE=2 -O
during GIMPLE pass: objsz
linalg.i: In function ‘mleye’:
linalg.i:8:1: internal compiler error: in execute_todo, at passes.cc:2133
    8 | mleye(int l, double E[][l]) { memset2(E, 0, sizeof(double)); }
      | ^~~~~
0x75edfa execute_todo
        /home/marxin/Programming/gcc/gcc/passes.cc:2133
Please submit a full bug report, with preprocessed source (by using
-freport-bug).
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

[Bug tree-optimization/104970] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-03-17
   Target Milestone|---                         |12.0

[Bug tree-optimization/104970] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[siddhesh at gcc dot gnu.org]

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

Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |siddhesh at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #1 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
Looks like a codegen issue with parm_object_size; it's referring to a
non-existent SSA name.  I'll take a look at it first thing on Tuesday.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Not a SSA_NAME, but no longer existing temporary.
If a type has non-constant extents or bit or byte size, the vars (or SSA_NAMEs)
used in those sizes are valid just during gimplification, they can't be
recovered later on when they could have been DCEd or changed in some other way.
For automatic VLAs the only thing that can be done is look up the corresponding
__builtin_alloca_with_align and use its size argument as the size of the whole
object, for VLA parameters it just doesn't exist.
The gimple dump here has:
__attribute__((access ("^1[$ ],$0", )))
void mleye (int l, double[0:D.1990] * E)
{
  int l.0;
  sizetype D.1990;
  bitsizetype D.1991;
  sizetype D.1992;

  l.0 = l;
  _1 = (long int) l.0;
  _2 = _1 + -1;
  _3 = (sizetype) _2;
  D.1990 = _3;
  _4 = (sizetype) l.0;
  _5 = (bitsizetype) _4;
  _6 = _5 * 64;
  D.1991 = _6;
  _7 = (sizetype) l.0;
  _8 = _7 * 8;
  D.1992 = _8;
  memset2 (E, 0, 8);
}
so D.1990 is the TYPE_MAX_VALUE (TYPE_DOMAIN (type)) i.e. l - 1,
D.1991 is TYPE_SIZE (type) (l * 64, i.e. bit size) and D.1992 is TYPE_SIZE_UNIT
(type) (l * 8, i.e. byte size).
The ssa pass turns all those into SSA_NAMEs though (and nothing updates the
TYPE* values) and dse1 removes all those unused stmts.
The objsz1 emergency dump shows though:
  _4 = (sizetype) l_1(D);
  _6 = D.1992 * _4;
  _5 = _6;
  __builtin___memset_chk (E_3(D), 0, 8, _5);
so it strangely wants to multiply D.1992 (which can't be used but as sizeof
(*E)) with l argument, so that is l * l * sizeof (double) if D.1992 could be
used.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
double E[][l]
argument has an unknown size, so it must be an unknown bound.
Seems the get_parm_access function returns size member as NULL, so the only
thing you can look at is the string which encodes it.  For the above it is:
^1[$ ],$0
and on that because one dimension is incomplete one needs to punt.
If the testcase has:
double E[6][l]
instead, the string is
^1[$6],$0
and in that case perhaps l_1(D) * 6 * sizeof(double) could be used.
In any case, parm_object_size needs to be much more careful on when exactly it
can use this stuff safely.
CCing Martin as the author of this stuff.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[msebor at gcc dot gnu.org]

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

--- Comment #4 from Martin Sebor <msebor at gcc dot gnu.org> ---
The VLA bounds were removed from parameter declarations in the fix for pr97172;
there weren't relied on by middle-end warnings then and still aren't today.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[msebor at gcc dot gnu.org]

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

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
Incidentally, __builtin_dynamic_object_size returns the size of a VLA parameter
in both mininum and maximum modes.  In f0 below, the size of the A array is at
least N bytes but it could be more, so based on my reading of the manual, it
seems like f0 is actually supposed return -1 (f1 looks fine).  If my reading is
correct that would seem unfortunate because it would basically makes BDOS
ineffective for _FORTIFY_SOURCE of VLA parameters.

long f0 (int n, char a[static n])
{
  return __builtin_dynamic_object_size (a, 0);   // folded to n, should be -1?
(Clang folds to -1)
}

long f1 (int n, char a[static n])
{
  return __builtin_dynamic_object_size (a, 1);   // folded to n (Clang folds to
-1)
}

Even more unfortunate seems that that without the [static] it's not undefined
to pass an array with fewer elements than the VLA bound indicates to a function
like f0 of f1.  GCC BDOS doesn't seem to consider the [static] notation and
folds the result the same way either way.  So while well-written code will
benefit from the stricter runtime checking made possible by the tighter bound,
it will cause aborts for poorly written code that's strictly valid.  If I'm
right about this, adding a permissive mode to BDOS to accommodate the poorly
written but valid code might be a way out.

There are cases when __builtin_dynamic_object_size could put the VLA bounds to
use, although I suspect they don't mater for _FORTIFY_SOURCE; if they should
matter, the brute force pr97172 fix might need to be revisited and the bounds
somehow preserved  Here are some such use cases:

$ cat c.c && gcc -O -S c.c
long f0 (int n, char a[static n])
{
  return __builtin_dynamic_object_size (a, 1);   // folded to n
}

long f1 (int n, char (*a)[n])
{
  return __builtin_dynamic_object_size (*a, 1);   // folded to -1 (fold to n?)
}

long f2 (int n, char a[1][n])
{
  return __builtin_dynamic_object_size (a[0], 1);   // folded to -1 (fold to
n?)
}

long f3 (int n, char a[static 1][n])
{
  return __builtin_dynamic_object_size (a, 1);   // ICE (fold to n?)
}

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I can't speak for Siddhesh, but at least the testcase coverage in r12-6480
only covered the access attribute.  So perhaps the fix could be to just limit
to that case if it is possible easily to differentiate that from the other
cases.
And certainly if it uses TYPE_SIZE_UNIT for something it needs to punt if it
isn't a constant (or perhaps POLY_INT_CST), because the trees mentioned in
there can't be reliably used during objsz1.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[msebor at gcc dot gnu.org]

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

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
The dollar sign in the internal attr_access string implies a VLA bound and the
attr_access::vla_bounds() function queries the VLA bounds.  That should make it
possible to distinguish the two cases.

Unlike the top-level VLA [N] notation which (unfortunately) implies no size
constraint on the actual argument, attribute access is meant to imply that the
array must have at least N elements (i.e., it's equivalent to [static N]).

As an aside, the tests cases in r12-6480 exercise only a small subset of
possible use cases: BDOS mode 0 results for calls to a function with attribute
access with either a dynamic size (known and unknown result) and a constant
size equal to array size.  There are many more use cases that aren't being
tested  that should be (e.g., larger or smaller arrays of constant size than
the size argument indicates with BDOS modes other than 0).

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[siddhesh at gcc dot gnu.org]

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

--- Comment #8 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #7)
> The dollar sign in the internal attr_access string implies a VLA bound and
> the attr_access::vla_bounds() function queries the VLA bounds.  That should
> make it possible to distinguish the two cases.

I noticed a attr_access.internal_p which has the following comment in
attribs.c:

/* Forms containing the square bracket are internal-only
   (not specified by an attribute declaration), and used
   for various forms of array and VLA parameters.  */   

That should be a good differentiator right?  It fixes the motivating case here.
 This is what I'm testing a bit more extensively.

diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index b0b50774936..1e87739eda6 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -1477,7 +1477,7 @@ parm_object_size (struct object_size_info *osi, tree var)
   tree typesize = TYPE_SIZE_UNIT (TREE_TYPE (TREE_TYPE (parm)));
   tree sz = NULL_TREE;

-  if (access && access->sizarg != UINT_MAX)
+  if (access && access->sizarg != UINT_MAX && !access->internal_p)
     {
       tree fnargs = DECL_ARGUMENTS (fndecl);
       tree arg = NULL_TREE;

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
If that works, please add at least a check that TREE_CODE (typesize) ==
INTEGER_CST (or perhaps also POLY_INT_CST) if it really needs to be multiplied
by the size of what the pointer points to.  In which case the question is also
what should happen for void * parameters, the code right now doesn't multiply
it by anything in that case:
                if (typesize)
                  sz = size_binop (MULT_EXPR, sz, typesize);
but IMHO it should give up for arbitrary incomplete types and just handle the
void * case (so punt on !typesize unless TREE_CODE (TREE_TYPE (TREE_TYPE
(parm))) == VOID_TYPE?).
The documentation says:
"The optional @var{size-index} positional argument denotes a function
argument of integer type that specifies the maximum size of the access.
The size is the number of elements of the type referenced by @var{ref-index},
or the number of bytes when the pointer type is @code{void*}."
so if you have struct S; void foo (struct S *p, int sz) with access attribute
I think you just want to punt.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[msebor at gcc dot gnu.org]

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

--- Comment #10 from Martin Sebor <msebor at gcc dot gnu.org> ---
The purpose of the internal_p flag documented in the attr_access class is more
general than to tell a VLA-like argument from an ordinary array/pointer form
("Set for an attribute added internally rather than by an explicit
declaration") so tying the two together would be fragile.  I expect using
internal_p directly as Siddhesh suggests will probably work now but I would
recommend using the vla_bounds() member function instead in case things change
in the future.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Siddhesh Poyarekar
<siddhesh@gcc.gnu.org>:

https://gcc.gnu.org/g:c1d233e3334df07cfb4f732ace4d93d3cbc28bca

commit r12-7794-gc1d233e3334df07cfb4f732ace4d93d3cbc28bca
Author: Siddhesh Poyarekar <siddhesh@gotplt.org>
Date:   Wed Mar 23 23:09:02 2022 +0530

    tree-optimization/104970: Limit size computation for access attribute

    Limit object size computation only to the simple case where access
    attribute has been explicitly specified.  The object passed to
    __builtin_dynamic_object_size could either be a pointer or a VLA whose
    size has been described using access attribute.

    Further, return a valid size only if the object is a void * pointer or
    points to (or is a VLA of) a type that has a constant size.

    gcc/ChangeLog:

            PR tree-optimization/104970
            * tree-object-size.cc (parm_object_size): Restrict size
            computation scenarios to explicit access attributes.

    gcc/testsuite/ChangeLog:

            PR tree-optimization/104970
            * gcc.dg/builtin-dynamic-object-size-0.c (test_parmsz_simple2,
            test_parmsz_simple3, test_parmsz_extern, test_parmsz_internal,
            test_parmsz_internal2, test_parmsz_internal3): New tests.
            (main): Use them.

    Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[siddhesh at gcc dot gnu.org]

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

Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> changed:

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

--- Comment #12 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
Fixed.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[muecker at gwdg dot de]

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

Martin Uecker <muecker at gwdg dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |muecker at gwdg dot de

--- Comment #13 from Martin Uecker <muecker at gwdg dot de> ---

This fix seem too radical. It now prevents this from working even when there is
an explicit attribute but there is also a VLA bound.  Also I think it would be
nice if it worked without the explicit attribute at least in the simple cases
where this should be now problem.

__attribute__ ((noinline, access (read_only, 2, 1)))
int foo(int n, int buf[n])
{
    buf[n] = 1;
    return __builtin_dynamic_object_size(buf, 0);
}

int main() {
    int n = 10;
    int buf[n];
    return foo(n, buf);
}

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[siddhesh at gcc dot gnu.org]

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

--- Comment #14 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
(In reply to Martin Uecker from comment #13)
> This fix seem too radical. It now prevents this from working even when there
> is an explicit attribute but there is also a VLA bound.  Also I think it
> would be nice if it worked without the explicit attribute at least in the
> simple cases where this should be now problem.

I've cloned this to bug 109334.

[Bug tree-optimization/104970] [12 Regression] ICE in execute_todo, at passes.cc:2133 since r12-6480-gea19c8f33a3a8d2b

[muecker at gwdg dot de]

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

--- Comment #15 from Martin Uecker <muecker at gwdg dot de> ---
Thanks.

I still wonder whether the original example should be added to the test suite?