diff mbox

[Fortran] PR54599 (4/n) FIx issues found by Coverity scann

Message ID 5056D59E.7070303@net-b.de
State New
Headers show

Commit Message

Tobias Burnus Sept. 17, 2012, 7:47 a.m. UTC 
This patch fixes some of the issues collected in the PR.


Some remarks to the changed in the attached patch:


-         gcc_assert (*format++ == '$');

That code gets executed when
  "some error message with value %d for string %s"
gets translated into
  "some error message for string %2$s with value %1$d"

And without the ++ gfortran doesn't skip over the $ sign.


+  gcc_assert (derived1 && derived2);
One of the many places where we first check for non-NULL - and then 
dereference the pointer unconditionally.


-              && ref->u.ar.as->lower && ref->u.ar.as->upper)

"lower" is an array of pointers but not a pointer itself - thus, "lower" 
is always != NULL. Ditto for upper.



-         break;
+         continue;

It makes sense to walk through the loop more than once ;-)

+       size = int_size_in_bytes (type);
+       gcc_assert (size >= 0);

The function returns a HOST_WIDE_INT or  -1 if it doesn't fit  into that 
signed variable. To silence the warning, I now check the result and 
added an assert.


Build and regtested on x86-64-linux.
OK for the trunk?

Tobias

Comments

Mikael Morin Sept. 17, 2012, 9:53 a.m. UTC | #1 
On 17/09/2012 09:47, Tobias Burnus wrote:
> This patch fixes some of the issues collected in the PR.
> 
> 
> Some remarks to the changed in the attached patch:
> 
> 
> -         gcc_assert (*format++ == '$');
> 
> That code gets executed when
>  "some error message with value %d for string %s"
> gets translated into
>  "some error message for string %2$s with value %1$d"
> 
> And without the ++ gfortran doesn't skip over the $ sign.
> 
> 
> +  gcc_assert (derived1 && derived2);
> One of the many places where we first check for non-NULL - and then
> dereference the pointer unconditionally.
> 
> 
> -              && ref->u.ar.as->lower && ref->u.ar.as->upper)
> 
> "lower" is an array of pointers but not a pointer itself - thus, "lower"
> is always != NULL. Ditto for upper.
> 
> 
> 
> -         break;
> +         continue;
> 
> It makes sense to walk through the loop more than once ;-)
> 
> +       size = int_size_in_bytes (type);
> +       gcc_assert (size >= 0);
> 
> The function returns a HOST_WIDE_INT or  -1 if it doesn't fit  into that
> signed variable. To silence the warning, I now check the result and
> added an assert.
> 
> 
> Build and regtested on x86-64-linux.
> OK for the trunk?
> 
> Tobias
OK
diff mbox

Patch

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

	* error.c (error_print): Move increment out of the assert.
	* interface.c (gfc_compare_derived_types): Add assert.
	(get_expr_storage_size): Remove always-true logical condition.
	* resolve.c (resolve_allocate_expr): Fix looping logic.
	* target-memory.c (gfc_target_expr_size): Add assert.

diff --git a/gcc/fortran/error.c b/gcc/fortran/error.c
index 64b9357..4b06156 100644
--- a/gcc/fortran/error.c
+++ b/gcc/fortran/error.c
@@ -536,23 +536,24 @@  error_print (const char *type, const char *format0, va_list argp)
 
       if (ISDIGIT (*format))
 	{
 	  /* This is a position specifier.  For example, the number
 	     12 in the format string "%12$d", which specifies the third
 	     argument of the va_list, formatted in %d format.
 	     For details, see "man 3 printf".  */
 	  pos = atoi(format) - 1;
 	  gcc_assert (pos >= 0);
 	  while (ISDIGIT(*format))
 	    format++;
-	  gcc_assert (*format++ == '$');
+	  gcc_assert (*format == '$');
+	  format++;
 	}
       else
 	pos++;
 
       c = *format++;
 
       if (pos > maxpos)
 	maxpos = pos;
 
       switch (c)
 	{
diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
index b348856..88689aa 100644
--- a/gcc/fortran/interface.c
+++ b/gcc/fortran/interface.c
@@ -388,27 +388,28 @@  gfc_match_end_interface (void)
 /* Compare two derived types using the criteria in 4.4.2 of the standard,
    recursing through gfc_compare_types for the components.  */
 
 int
 gfc_compare_derived_types (gfc_symbol *derived1, gfc_symbol *derived2)
 {
   gfc_component *dt1, *dt2;
 
   if (derived1 == derived2)
     return 1;
 
+  gcc_assert (derived1 && derived2);
+
   /* Special case for comparing derived types across namespaces.  If the
      true names and module names are the same and the module name is
      nonnull, then they are equal.  */
-  if (derived1 != NULL && derived2 != NULL
-      && strcmp (derived1->name, derived2->name) == 0
+  if (strcmp (derived1->name, derived2->name) == 0
       && derived1->module != NULL && derived2->module != NULL
       && strcmp (derived1->module, derived2->module) == 0)
     return 1;
 
   /* Compare type via the rules of the standard.  Both types must have
      the SEQUENCE or BIND(C) attribute to be equal.  */
 
   if (strcmp (derived1->name, derived2->name))
     return 0;
 
   if (derived1->component_access == ACCESS_PRIVATE
@@ -2259,24 +2260,23 @@  get_expr_storage_size (gfc_expr *e)
 		else
 		  return 0;
 	      }
 	    else if (ref->u.ar.as->upper[i]
 		     && ref->u.ar.as->upper[i]->expr_type == EXPR_CONSTANT)
 	      end = mpz_get_si (ref->u.ar.as->upper[i]->value.integer);
 	    else
 	      return 0;
 
 	    elements *= (end - start)/stride + 1L;
 	  }
-      else if (ref->type == REF_ARRAY && ref->u.ar.type == AR_FULL
-	       && ref->u.ar.as->lower && ref->u.ar.as->upper)
+      else if (ref->type == REF_ARRAY && ref->u.ar.type == AR_FULL)
 	for (i = 0; i < ref->u.ar.as->rank; i++)
 	  {
 	    if (ref->u.ar.as->lower[i] && ref->u.ar.as->upper[i]
 		&& ref->u.ar.as->lower[i]->expr_type == EXPR_CONSTANT
 		&& ref->u.ar.as->upper[i]->expr_type == EXPR_CONSTANT)
 	      elements *= mpz_get_si (ref->u.ar.as->upper[i]->value.integer)
 			  - mpz_get_si (ref->u.ar.as->lower[i]->value.integer)
 			  + 1L;
 	    else
 	      return 0;
 	  }
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 6a7b6c9..f67c07f 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -7419,23 +7419,23 @@  check_symbols:
   for (i = ar->dimen; i < ar->codimen + ar->dimen; i++)
     {
       if (ar->dimen_type[i] == DIMEN_ELEMENT
 	  || ar->dimen_type[i] == DIMEN_RANGE)
 	{
 	  if (i == (ar->dimen + ar->codimen - 1))
 	    {
 	      gfc_error ("Expected '*' in coindex specification in ALLOCATE "
 			 "statement at %L", &e->where);
 	      goto failure;
 	    }
-	  break;
+	  continue;
 	}
 
       if (ar->dimen_type[i] == DIMEN_STAR && i == (ar->dimen + ar->codimen - 1)
 	  && ar->stride[i] == NULL)
 	break;
 
       gfc_error ("Bad coarray specification in ALLOCATE statement at %L",
 		 &e->where);
       goto failure;
     }
 
diff --git a/gcc/fortran/target-memory.c b/gcc/fortran/target-memory.c
index bedc668..7a55dcd 100644
--- a/gcc/fortran/target-memory.c
+++ b/gcc/fortran/target-memory.c
@@ -117,25 +117,28 @@  gfc_target_expr_size (gfc_expr *e)
 	}
       else
 	return 0;
 
     case BT_HOLLERITH:
       return e->representation.length;
     case BT_DERIVED:
       {
 	/* Determine type size without clobbering the typespec for ISO C
 	   binding types.  */
 	gfc_typespec ts;
+	HOST_WIDE_INT size;
 	ts = e->ts;
 	type = gfc_typenode_for_spec (&ts);
-	return int_size_in_bytes (type);
+	size = int_size_in_bytes (type);
+	gcc_assert (size >= 0);
+	return size;
       }
     default:
       gfc_internal_error ("Invalid expression in gfc_target_expr_size.");
       return 0;
     }
 }
 
 
 /* The encode_* functions export a value into a buffer, and 
    return the number of bytes of the buffer that have been
    used.  */