Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Marek Polacek]

On Tue, Nov 10, 2015 at 12:40:49PM -0700, Martin Sebor wrote:
> On 11/10/2015 09:36 AM, Marek Polacek wrote:
> >While both C and C++ FEs are able to reject e.g.
> >int a[__SIZE_MAX__ / sizeof(int)];
> >they are accepting code such as
> >int (*a)[__SIZE_MAX__ / sizeof(int)];
> >
> >As Joseph pointed out, any construction of a non-VLA type whose size is half 
> >or
> >more of the address space should receive a compile-time error.
> >
> >Done by moving up the check for the size in bytes so that it checks check 
> >every
> >non-VLA complete array type constructed in the course of processing the
> >declarator.  Since the C++ FE had the same problem, I've fixed it up there as
> >well.  And that's why I had to twek dg-error of two C++ tests; if the size of
> >an array is considered invalid, we give an error message with word "unnamed".
> >
> >(I've removed the comment about crashing in tree_to_[su]hwi since that seems
> >to no longer be the case.)
> 
> Thanks for including me on this. I tested it with C++ references
> to arrays (in addition to pointers) and it works correctly for
> those as well (unsurprisingly). The only thing that bothers me

Good, thanks!

> a bit is that the seemingly  arbitrary inconsistency between
> the diagnostics:
 
> >+p = new char [1][MAX - 99]; // { dg-error "size of unnamed 
> >array" }
> >  p = new char [1][MAX / 2];  // { dg-error "size of array" }
> 
> Would it be possible to make the message issued by the front ends
> the same? I.e., either both "unnamed array" or both just "array?"

Yeah, I was thinking about that, too, but I was also hoping that we can
clean this up as a follow-up.  I think let's drop the "unnamed" word, even
though that means that the changes in new44.C brought with my patch will
essentially have to be reverted...

Oh, and we could also be more informative and print the size of an array,
or the number of elements, as clang does.

Thanks,

Marek

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Marek Polacek]

On Tue, Nov 10, 2015 at 04:48:13PM -0700, Jeff Law wrote:
> Someone (I can't recall who) suggested the overflow check ought to be
> shared, I agree.  Can you factor out that check, shove it into c-family/ and
> call it from the C & C++ front-ends?
> 
> Approved with that change.  Please post it here for archival purposes
> though.

Done, thanks.

Bootstrapped/regtested on x86_64-linux, applying to trunk.

2015-11-11  Marek Polacek  

PR c/68107
PR c++/68266
* c-common.c (valid_array_size_p): New function.
* c-common.h (valid_array_size_p): Declare.

* c-decl.c (grokdeclarator): Call valid_array_size_p.  Remove code
checking the size of an array.

* decl.c (grokdeclarator): Call valid_array_size_p.  Remove code
checking the size of an array.

* c-c++-common/pr68107.c: New test.
* g++.dg/init/new38.C (large_array_char): Adjust dg-error.
(large_array_char_template): Likewise.
* g++.dg/init/new44.C: Adjust dg-error.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 53c1d81..a393b32 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -13110,4 +13110,26 @@ warn_duplicated_cond_add_or_warn (location_t loc, tree 
cond, vec **chain)
 (*chain)->safe_push (cond);
 }
 
+/* Check if array size calculations overflow or if the array covers more
+   than half of the address space.  Return true if the size of the array
+   is valid, false otherwise.  TYPE is the type of the array and NAME is
+   the name of the array, or NULL_TREE for unnamed arrays.  */
+
+bool
+valid_array_size_p (location_t loc, tree type, tree name)
+{
+  if (type != error_mark_node
+  && COMPLETE_TYPE_P (type)
+  && TREE_CODE (TYPE_SIZE_UNIT (type)) == INTEGER_CST
+  && !valid_constant_size_p (TYPE_SIZE_UNIT (type)))
+{
+  if (name)
+   error_at (loc, "size of array %qE is too large", name);
+  else
+   error_at (loc, "size of unnamed array is too large");
+  return false;
+}
+  return true;
+}
+
 #include "gt-c-family-c-common.h"
diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index c825454..bad8d05 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1463,5 +1463,6 @@ extern bool check_no_cilk (tree, const char *, const char 
*,
   location_t loc = UNKNOWN_LOCATION);
 extern bool reject_gcc_builtin (const_tree, location_t = UNKNOWN_LOCATION);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec **);
+extern bool valid_array_size_p (location_t, tree, tree);
 
 #endif /* ! GCC_C_COMMON_H */
diff --git gcc/c/c-decl.c gcc/c/c-decl.c
index a3d8ead..fb4f5ea 100644
--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -6007,6 +6007,9 @@ grokdeclarator (const struct c_declarator *declarator,
TYPE_SIZE_UNIT (type) = size_zero_node;
SET_TYPE_STRUCTURAL_EQUALITY (type);
  }
+
+   if (!valid_array_size_p (loc, type, name))
+ type = error_mark_node;
  }
 
if (decl_context != PARM
@@ -6014,7 +6017,8 @@ grokdeclarator (const struct c_declarator *declarator,
|| array_ptr_attrs != NULL_TREE
|| array_parm_static))
  {
-   error_at (loc, "static or type qualifiers in non-parameter 
array declarator");
+   error_at (loc, "static or type qualifiers in non-parameter "
+ "array declarator");
array_ptr_quals = TYPE_UNQUALIFIED;
array_ptr_attrs = NULL_TREE;
array_parm_static = 0;
@@ -6293,22 +6297,6 @@ grokdeclarator (const struct c_declarator *declarator,
}
 }
 
-  /* Did array size calculations overflow or does the array cover more
- than half of the address-space?  */
-  if (TREE_CODE (type) == ARRAY_TYPE
-  && COMPLETE_TYPE_P (type)
-  && TREE_CODE (TYPE_SIZE_UNIT (type)) == INTEGER_CST
-  && ! valid_constant_size_p (TYPE_SIZE_UNIT (type)))
-{
-  if (name)
-   error_at (loc, "size of array %qE is too large", name);
-  else
-   error_at (loc, "size of unnamed array is too large");
-  /* If we proceed with the array type as it is, we'll eventually
-crash in tree_to_[su]hwi().  */
-  type = error_mark_node;
-}
-
   /* If this is declaring a typedef name, return a TYPE_DECL.  */
 
   if (storage_class == csc_typedef)
diff --git gcc/cp/decl.c gcc/cp/decl.c
index bd3f2bc..a3caa19 100644
--- gcc/cp/decl.c
+++ gcc/cp/decl.c
@@ -9945,6 +9945,9 @@ grokdeclarator (const cp_declarator *declarator,
case cdk_array:
  type = create_array_type_for_decl (dname, type,
 declarator->u.array.bounds);
+ if (!valid_array_size_p (input_location, type, dname))
+   type = error_mark_node;
+
  if (declarator->std_attributes)
/* [dcl.array]/1:
 

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Marek Polacek]

On Tue, Nov 10, 2015 at 06:38:31PM +0100, Paolo Carlini wrote:
>  Hi,
> 
> On 11/10/2015 05:36 PM, Marek Polacek wrote:
> >+
> >+/* Did array size calculations overflow or does the array
> >+   cover more than half of the address-space?  */
> >+if (COMPLETE_TYPE_P (type)
> >+&& TREE_CODE (TYPE_SIZE_UNIT (type)) == INTEGER_CST
> >+&& !valid_constant_size_p (TYPE_SIZE_UNIT (type)))
> >+  {
> >+if (name)
> >+  error_at (loc, "size of array %qE is too large", name);
> >+else
> >+  error_at (loc, "size of unnamed array is too large");
> >+type = error_mark_node;
> >+  }
> >   }
> Obviously "the issue" predates your proposed change, but I don't understand
> why the code implementing the check can't be shared by the front-ends via a
> small function in c-family...

Certainly I'm in favor of sharing code between C and C++ FEs, though in
this case it didn't seem too important/obvious, because of the extra !=
error_mark_node check + I don't really like the new function getting *type
and setting it there.

But I'll submit another version of the patch with a common function.

Marek

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Marek Polacek]

On Wed, Nov 11, 2015 at 01:42:04PM +0100, Bernd Schmidt wrote:
> On 11/11/2015 01:31 PM, Marek Polacek wrote:
> 
> >Certainly I'm in favor of sharing code between C and C++ FEs, though in
> >this case it didn't seem too important/obvious, because of the extra !=
> >error_mark_node check + I don't really like the new function getting *type
> >and setting it there.
> 
> Make it return bool to indicate whether to change type to error_mark.

Yeah, I've done it like so.

Marek

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Bernd Schmidt]

On 11/11/2015 01:31 PM, Marek Polacek wrote:


Certainly I'm in favor of sharing code between C and C++ FEs, though in
this case it didn't seem too important/obvious, because of the extra !=
error_mark_node check + I don't really like the new function getting *type
and setting it there.


Make it return bool to indicate whether to change type to error_mark.


Bernd

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Martin Sebor]

Oh, and we could also be more informative and print the size of an array,
or the number of elements, as clang does.


Yes, that's pretty nice. It helps but the diagnostic must point at
the right dimension. GCC often just points at the whole expression
or some token within it.

void* foo ()
{
enum { I = 65535, J = 65536, K = 65537, L = 65538, M = 65539 };

return new int [I][J][K][L][M];
}
z.c:5:24: error: array is too large (65536 elements)
return new int [I][J][K][L][M];
   ^

It might be even more helpful if it included the size of each element
(i.e., here, K * L * M byes).

Martin

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Joseph Myers]

On Tue, 10 Nov 2015, Marek Polacek wrote:

> While both C and C++ FEs are able to reject e.g.
> int a[__SIZE_MAX__ / sizeof(int)];
> they are accepting code such as
> int (*a)[__SIZE_MAX__ / sizeof(int)];
> 
> As Joseph pointed out, any construction of a non-VLA type whose size is half 
> or
> more of the address space should receive a compile-time error.
> 
> Done by moving up the check for the size in bytes so that it checks check 
> every
> non-VLA complete array type constructed in the course of processing the
> declarator.  Since the C++ FE had the same problem, I've fixed it up there as
> well.  And that's why I had to twek dg-error of two C++ tests; if the size of
> an array is considered invalid, we give an error message with word "unnamed".
> 
> (I've removed the comment about crashing in tree_to_[su]hwi since that seems
> to no longer be the case.)
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

The C front-end changes are OK.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Paolo Carlini]

 Hi,

On 11/10/2015 05:36 PM, Marek Polacek wrote:

+
+   /* Did array size calculations overflow or does the array
+  cover more than half of the address-space?  */
+   if (COMPLETE_TYPE_P (type)
+   && TREE_CODE (TYPE_SIZE_UNIT (type)) == INTEGER_CST
+   && !valid_constant_size_p (TYPE_SIZE_UNIT (type)))
+ {
+   if (name)
+ error_at (loc, "size of array %qE is too large", name);
+   else
+ error_at (loc, "size of unnamed array is too large");
+   type = error_mark_node;
+ }
  }
Obviously "the issue" predates your proposed change, but I don't 
understand why the code implementing the check can't be shared by the 
front-ends via a small function in c-family...


Paolo.

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Martin Sebor]

On 11/10/2015 09:36 AM, Marek Polacek wrote:

While both C and C++ FEs are able to reject e.g.
int a[__SIZE_MAX__ / sizeof(int)];
they are accepting code such as
int (*a)[__SIZE_MAX__ / sizeof(int)];

As Joseph pointed out, any construction of a non-VLA type whose size is half or
more of the address space should receive a compile-time error.

Done by moving up the check for the size in bytes so that it checks check every
non-VLA complete array type constructed in the course of processing the
declarator.  Since the C++ FE had the same problem, I've fixed it up there as
well.  And that's why I had to twek dg-error of two C++ tests; if the size of
an array is considered invalid, we give an error message with word "unnamed".

(I've removed the comment about crashing in tree_to_[su]hwi since that seems
to no longer be the case.)


Thanks for including me on this. I tested it with C++ references
to arrays (in addition to pointers) and it works correctly for
those as well (unsurprisingly). The only thing that bothers me
a bit is that the seemingly  arbitrary inconsistency between
the diagnostics:


+p = new char [1][MAX - 99]; // { dg-error "size of unnamed array" }
  p = new char [1][MAX / 2];  // { dg-error "size of array" }


Would it be possible to make the message issued by the front ends
the same? I.e., either both "unnamed array" or both just "array?"

Martin

Re: [C/C++ PATCH] Reject declarators with huge arrays (PR c/68107, c++/68266)

[Jeff Law]

On 11/10/2015 09:36 AM, Marek Polacek wrote:

While both C and C++ FEs are able to reject e.g.
int a[__SIZE_MAX__ / sizeof(int)];
they are accepting code such as
int (*a)[__SIZE_MAX__ / sizeof(int)];

As Joseph pointed out, any construction of a non-VLA type whose size is half or
more of the address space should receive a compile-time error.

Done by moving up the check for the size in bytes so that it checks check every
non-VLA complete array type constructed in the course of processing the
declarator.  Since the C++ FE had the same problem, I've fixed it up there as
well.  And that's why I had to twek dg-error of two C++ tests; if the size of
an array is considered invalid, we give an error message with word "unnamed".

(I've removed the comment about crashing in tree_to_[su]hwi since that seems
to no longer be the case.)

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-11-10  Marek Polacek  

PR c/68107
PR c++/68266
* c-decl.c (grokdeclarator): Check whether the size of arrays is
valid earlier.

* decl.c (grokdeclarator): Check whether the size of arrays is valid
earlier.

* c-c++-common/pr68107.c: New test.
* g++.dg/init/new38.C (large_array_char): Adjust dg-error.
(large_array_char_template): Likewise.
* g++.dg/init/new44.C: Adjust dg-error.
Someone (I can't recall who) suggested the overflow check ought to be 
shared, I agree.  Can you factor out that check, shove it into c-family/ 
and call it from the C & C++ front-ends?


Approved with that change.  Please post it here for archival purposes 
though.


Your decision as to whether or not the shared routine verifies that type 
!= error_mark_node as is currently done in the C++ front-end.  The C 
front-end merely checks it earlier.  SO it's safe to put that test into 
the shared code if you want.


Jeff