[Bug fortran/95512] New: gcc/fortran/trans-decl.c:1066: array sanity check after use

[Bug fortran/95512] New: gcc/fortran/trans-decl.c:1066: array sanity check after use

[dcb314 at hotmail dot com]

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

            Bug ID: 95512
           Summary: gcc/fortran/trans-decl.c:1066: array sanity check
                    after use
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com
  Target Milestone: ---

Static analyser cppcheck says:

gcc/fortran/trans-decl.c:1066:11: style: Array index 'dim' is used before
limits check. [arrayIndexThenCheck]

Source code is

      /* Don't try to use the unknown ubound for the last coarray dimension. 
*/
      if (GFC_TYPE_ARRAY_UBOUND (type, dim) == NULL_TREE
          && dim < GFC_TYPE_ARRAY_RANK (type) + GFC_TYPE_ARRAY_CORANK (type) -
1)

Maybe better code:

      /* Don't try to use the unknown ubound for the last coarray dimension. 
*/
      if (dim < GFC_TYPE_ARRAY_RANK (type) + GFC_TYPE_ARRAY_CORANK (type) - 1
          && GFC_TYPE_ARRAY_UBOUND (type, dim) == NULL_TREE)

[Bug fortran/95512] gcc/fortran/trans-decl.c:1066: array sanity check after use

[anlauf at gcc dot gnu.org]

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

anlauf at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-06-05
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |WAITING

--- Comment #1 from anlauf at gcc dot gnu.org ---
This looks like a false positive: dim is the index of the enclosing for loop.

It is also funny that it warns about this statement and not the if preceeding
this one.

[Bug fortran/95512] gcc/fortran/trans-decl.c:1066: array sanity check after use

[dcb314 at hotmail dot com]

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

--- Comment #2 from David Binderman <dcb314 at hotmail dot com> ---
I fail to understand how this can be a false positive.

 if (array[ X] && X < something - 1)

looks like a pretty convincing case of use before sanity checking to me.
It is a standard pattern to sanity check array indexes before use.

The previous if doesn't sanity check the array index, so there is
no chance of getting the sanity check in a non standard place.

[Bug fortran/95512] gcc/fortran/trans-decl.c:1066: array sanity check after use

[anlauf at gcc dot gnu.org]

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

--- Comment #3 from anlauf at gcc dot gnu.org ---
I didn't write the code, but for more context:

trans.h has:

#define GFC_TYPE_ARRAY_LBOUND(node, dim) \
  (TYPE_LANG_SPECIFIC(node)->lbound[dim])
#define GFC_TYPE_ARRAY_UBOUND(node, dim) \
  (TYPE_LANG_SPECIFIC(node)->ubound[dim])

The loop in trans-decl.c:

  for (dim = GFC_TYPE_ARRAY_RANK (type);
       dim < GFC_TYPE_ARRAY_RANK (type) + GFC_TYPE_ARRAY_CORANK (type); dim++)
    {
      if (GFC_TYPE_ARRAY_LBOUND (type, dim) == NULL_TREE)
        {
          GFC_TYPE_ARRAY_LBOUND (type, dim) = create_index_var ("lbound",
nest);
          TREE_NO_WARNING (GFC_TYPE_ARRAY_LBOUND (type, dim)) = 1;
        }
      /* Don't try to use the unknown ubound for the last coarray dimension. 
*/
      if (GFC_TYPE_ARRAY_UBOUND (type, dim) == NULL_TREE
          && dim < GFC_TYPE_ARRAY_RANK (type) + GFC_TYPE_ARRAY_CORANK (type) -
1)
        {
          GFC_TYPE_ARRAY_UBOUND (type, dim) = create_index_var ("ubound",
nest);
          TREE_NO_WARNING (GFC_TYPE_ARRAY_UBOUND (type, dim)) = 1;
        }
    }

So the relevant check is in the loop header, and the current check is there
for the last index.

Maybe it is bad style, but I still consider it a false positive.
cppcheck's view is probably too narrow to understand the range of dim.

[Bug fortran/95512] gcc/fortran/trans-decl.c:1066: array sanity check after use

[dcb314 at hotmail dot com]

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

--- Comment #4 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to anlauf from comment #3)
> I didn't write the code, but for more context:

Thanks for the extra context.

> Maybe it is bad style, but I still consider it a false positive.

If you go back and check what cppcheck is saying, it is merely
mentioning what it thinks is bad style.

> cppcheck's view is probably too narrow to understand the range of dim.

Agreed. AFAIK cppcheck merely searches and reports
on source code patterns like

  if (arrayName[ X] && X < someExpression)

Whether you want to keep the code in bad style is up to you.
I did originally suggest some code that looked IMHO to be better style.

[Bug fortran/95512] gcc/fortran/trans-decl.c:1066: array sanity check after use

[tkoenig at gcc dot gnu.org]

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

Thomas Koenig <tkoenig at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
                 CC|                            |tkoenig at gcc dot gnu.org
             Status|WAITING                     |RESOLVED

--- Comment #5 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
I don't think it is necessary to change the code to avoid false positives
like this.  Hence, closing.

However, it was good to raise the issue so it could be checked.

Thanks!