[Bug fortran/54599] New: Issues found in gfortran by the Coverity Scan

[Bug fortran/54599] New: Issues found in gfortran by the Coverity Scan

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

             Bug #: 54599
           Summary: Issues found in gfortran by the Coverity Scan
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: burnus@gcc.gnu.org
                CC: tkoenig@gcc.gnu.org


The following bugs were found by http://scan.coverity.com/ for gcc/fortran. I
walked through the list and extracted the following issues.


--------------------------------------------------------
dependency.c:        LOGICAL BUG (unreachable code)
--------------------------------------------------------
 401      if (l == 0)
 415      else
 416        {
 417          if (l != 0)

I think one of the "l" should be "r".


--------------------------------------------------------
dependency.c: check_data_pointer_types
--------------------------------------------------------
Same line twice:
 946  if (     expr1->expr_type != EXPR_VARIABLE
 947        || expr1->expr_type != EXPR_VARIABLE)


--------------------------------------------------------
interface.c: gfc_compare_derived_types     BUG
--------------------------------------------------------
Same line twice:
 450      if (     !(dt1->ts.type == BT_DERIVED && derived1 ==
dt1->ts.u.derived)
 451            && !(dt1->ts.type == BT_DERIVED && derived1 ==
dt1->ts.u.derived)


--------------------------------------------------------
openmp.c: gfc_resolve_do_iterator
--------------------------------------------------------
Dead code: Line 1705 is unreachable.
1690          for (j = 1; j < i; j++)

1702              if (j < i)
1703                break;
1704              do_code2 = do_code2->block->next;
1705            }



--------------------------------------------------------
cpp.c: print_line        BUG
--------------------------------------------------------
 841      if (loc.sysp == 2)
 843      else if (loc.sysp == 1)
Issue:
  sysp is "bool".


--------------------------------------------------------
interface.c: 
--------------------------------------------------------
CID 722306: Array compared against 0 (NO_EFFECT)
At (1): Comparing an array to null is not useful: "ref->u.ar.as->upper". 
2266      else if (ref->type == REF_ARRAY && ref->u.ar.type == AR_FULL
2267               && ref->u.ar.as->lower && ref->u.ar.as->upper)


--------------------------------------------------------
target-memory.c:
--------------------------------------------------------
At (4): Assigning: unsigned variable "len" = "gfc_target_expr_size(gfc_expr
*)".
643  len = gfc_target_expr_size (e);
CID 722280: Argument cannot be negative (NEGATIVE_RETURNS)
At (5): "len" is passed to a parameter that cannot be negative. 

Here, len = size_t (unsigned); should one use HOST_WIDE_INT + and gcc_assert?


--------------------------------------------------------
error.c: Side effect in assertion
--------------------------------------------------------
 547          gcc_assert (*format++ == '$');
 The containing function might work differently in a non-debug build.


--------------------------------------------------------
resolve.c
--------------------------------------------------------
Loop only executed once. Should break be continue?

7419  for (i = ar->dimen; i < ar->codimen + ar->dimen; i++)
7424          if (i == (ar->dimen + ar->codimen - 1))
7428              goto failure;
7429            }
7430          break;
7433      if (ar->dimen_type[i] == DIMEN_STAR && i == (ar->dimen + ar->codimen
- 1)
7434          && ar->stride[i] == NULL)
7435        break;
7439      goto failure;
7440    }


--------------------------------------------------------
interface.c: gfc_compare_derived_types
--------------------------------------------------------
First, one checks whether "derived1" is NULL, then one uses
it unconditionally. Add an assert - and remove the check?

 402  if (derived1 != NULL && derived2 != NULL
 411  if (strcmp (derived1->name, derived2->name))



RFC: Should we address the following two warnings?

--------------------------------------------------------
resolve.c:
--------------------------------------------------------
Potential buffer overflow?
4015          strcpy (msg, _("COMPLEX quantities cannot be compared at %L"));

Recall, _(...) is gettext() and returns a string from the translator's .po
file; msg is 200 bytes large, i.e. 50 4-byte characters.


--------------------------------------------------------
intrinsic.c: add_sym
--------------------------------------------------------
Possible-buffer-overflow warning.

--- a/gcc/fortran/intrinsic.c
+++ b/gcc/fortran/intrinsic.c
@@ -273,2 +273,3 @@ add_sym (const char *name, gfc_isym_id id, enum klass cl,
int actual_ok, bt type

+      gcc_assert (strlen (name) + 10 < sizeof (buf));
       strcpy (buf, "_gfortran_");

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

Mikael Morin <mikael at gcc dot gnu.org> changed:

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

--- Comment #1 from Mikael Morin <mikael at gcc dot gnu.org> 2012-09-16 15:52:47 UTC ---
dependency.c:

1060          else
1061            {
1062              gfc_symbol *sym1 = expr1->symtree->n.sym;
1063              gfc_symbol *sym2 = expr2->symtree->n.sym;
//At (1): "!sym2->attr.dimension || sym2->as->type == 2" looks like the
original copy.
//CID 721803: Copy-paste error (COPY_PASTE_ERROR)At (2): "sym2" in
//"!sym1->attr.dimension || sym2->as->type == 2" looks like a copy-paste error.
//Should it say "sym1" instead?
1064              if (sym1->attr.target && sym2->attr.target
1065                  && ((sym1->attr.dummy && !sym1->attr.contiguous
1066                       && (!sym1->attr.dimension
1067                           || sym2->as->type == AS_ASSUMED_SHAPE))
1068                      || (sym2->attr.dummy && !sym2->attr.contiguous
1069                          && (!sym2->attr.dimension
1070                              || sym2->as->type == AS_ASSUMED_SHAPE))))
1071                return 1;
1072            }

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

Tobias Burnus <burnus at gcc dot gnu.org> changed:

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

--- Comment #2 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-09-17 12:08:06 UTC ---
I forgot to put the PR in the ChangeLog, when I fixed some of the issues; see:
http://gcc.gnu.org/viewcvs?view=revision&revision=191381

Still to do
- dependency.c (both issues of comment 1 and the one of comment 2)
- interface.c: gfc_compare_derived_types
- openmp.c
- cpp.c

and possibly the two RFC for resolve.c and intrinsic.c

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-09-18 09:28:46 UTC ---
Dump two patches here. First is about a pointless check ("nl->sym->..." is used
all over the place) and the second: If the assert fails, we would use
TREE_TYPE(NULL_TREE) ...

--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -12481 +12481 @@ resolve_fl_namelist (gfc_symbol *sym)
-      if (nl->sym && nl->sym->name)
+      if (nl->sym->name)

--- a/gcc/fortran/trans-array.c
+++ b/gcc/fortran/trans-array.c
@@ -1024,4 +1024,4 @@ gfc_trans_create_temp_array (stmtblock_t * pre,
stmtblock_t * post, gfc_ss * ss,
     {
-      if (POINTER_TYPE_P (TREE_TYPE (initial)))
-       class_expr = build_fold_indirect_ref_loc (input_location, initial);
+      gcc_assert (POINTER_TYPE_P (TREE_TYPE (initial)));
+      class_expr = build_fold_indirect_ref_loc (input_location, initial);
       eltype = TREE_TYPE (class_expr);

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-09-18 10:10:28 UTC ---
And another one: Unreachable code; due to  "if(extremum == NULL) {...;
continue}", last == NULL is always false:

--- a/gcc/fortran/simplify.c
+++ b/gcc/fortran/simplify.c
@@ -4109,4 +4109 @@ simplify_min_max (gfc_expr *expr, int sign)
-      if (last == NULL)
-       expr->value.function.actual = arg->next;
-      else
-       last->next = arg->next;
+      last->next = arg->next;

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

--- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-09-20 20:49:27 UTC ---
Author: burnus
Date: Thu Sep 20 20:49:20 2012
New Revision: 191590

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191590
Log:
2012-09-20  Tobias Burnus  <burnus@net-b.de>

        PR fortran/54599
        * cpp.c (print_line): Properly handle extern C.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/cpp.c

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[tkoenig at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

--- Comment #6 from Thomas Koenig <tkoenig at gcc dot gnu.org> 2012-09-22 10:32:58 UTC ---
Author: tkoenig
Date: Sat Sep 22 10:32:51 2012
New Revision: 191640

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191640
Log:
2012-09-22  Thomas König  <tkoenig@gcc.gnu.org>

    PR fortran/54599
    * dependency.c (gfc_dep_compare_expr):  Clarify logic,
    remove dead code.


Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/dependency.c

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[tkoenig at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-09-22
     Ever Confirmed|0                           |1

--- Comment #7 from Thomas Koenig <tkoenig at gcc dot gnu.org> 2012-09-22 10:34:22 UTC ---
The dependency.c issue is fixed.

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

--- Comment #8 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2012-09-22 21:48:01 UTC ---
> --------------------------------------------------------
> interface.c: gfc_compare_derived_types     BUG
> --------------------------------------------------------
> Same line twice:
>  450      if (     !(dt1->ts.type == BT_DERIVED && derived1 == dt1->ts.u.derived)
>  451            && !(dt1->ts.type == BT_DERIVED && derived1 ==dt1->ts.u.derived)

This is pr46244.

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54599

--- Comment #9 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-09-23 06:48:56 UTC ---
Author: burnus
Date: Sun Sep 23 06:48:48 2012
New Revision: 191649

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191649
Log:
2012-09-23  Tobias Burnus  <burnus@net-b.de>

        * parse.c (parse_derived): Don't set attr.alloc_comp
        for pointer components with allocatable subcomps.

        PR fortran/54599
        * resolve.c (resolve_fl_namelist): Remove superfluous
        NULL check.
        * simplify.c (simplify_min_max): Remove unreachable code.
        * trans-array.c (gfc_trans_create_temp_array): Change
        a condition into an assert.

        PR fortran/54618
        * trans-expr.c (gfc_trans_class_init_assign): Guard
        re-setting of the _data by gfc_conv_expr_present.
        (gfc_conv_procedure_call): Fix INTENT(OUT) handling
        for allocatable BT_CLASS.

2012-09-23  Tobias Burnus  <burnus@net-b.de>

        PR fortran/54618
        * gfortran.dg/class_array_14.f90: New.


Added:
    trunk/gcc/testsuite/gfortran.dg/class_array_14.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/parse.c
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/simplify.c
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[dominiq at lps dot ens.fr]

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

Dominique d'Humieres <dominiq at lps dot ens.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #10 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
What is left of this PR besides pr46244? I think new PRs should be opened for
the remaining issues and this should be closed.

[Bug fortran/54599] Issues found in gfortran by the Coverity Scan

[dominiq at lps dot ens.fr]

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

Dominique d'Humieres <dominiq at lps dot ens.fr> changed:

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

--- Comment #11 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> What is left of this PR besides pr46244? I think new PRs should be opened
> for the remaining issues and this should be closed.

No answer for almost nine months. Closing as FIXED.