[C,v2] Add Walloc-size to warn about insufficient size in allocations [PR71219]

Compared to the previous version I changed the name of the
warning to "Walloc-size" which matches "Wanalyzer-allocation-size"
but is still in line with the other -Walloc-something warnings
we have. I also added it to Wextra.

I found PR71219 that requests the warning and points out that 
it is recommended by the C secure coding guidelines and added
the PR to the commit log  (although the version with cast is not
diagnosed so far.)  

I did not have time to implement the extensions suggested
on the list,  i.e. warn when the size is not a multiple
of the size of the type and warn for if the size is not
suitable for a flexible array member. (this is also a bit
more complicated than it seems)

Bootstrapped and regression tested on x86_64.


Martin


Add option Walloc-size that warns about allocations that have
insufficient storage for the target type of the pointer the
storage is assigned to.

	PR c/71219
gcc:
	* doc/invoke.texi: Document -Walloc-size option.

gcc/c-family:

	* c.opt (Walloc-size): New option.

gcc/c:
	* c-typeck.cc (convert_for_assignment): Add warning.

gcc/testsuite:

	* gcc.dg/Walloc-size-1.c: New test.
---
 gcc/c-family/c.opt                   |  4 ++++
 gcc/c/c-typeck.cc                    | 27 +++++++++++++++++++++
 gcc/doc/invoke.texi                  | 10 ++++++++
 gcc/testsuite/gcc.dg/Walloc-size-1.c | 36 ++++++++++++++++++++++++++++
 4 files changed, 77 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/Walloc-size-1.c

Am Montag, dem 18.09.2023 um 23:26 +0200 schrieb Martin Uecker:
> 
> Compared to the previous version I changed the name of the
> warning to "Walloc-size" which matches "Wanalyzer-allocation-size"
> but is still in line with the other -Walloc-something warnings
> we have. I also added it to Wextra.
> 
> I found PR71219 that requests the warning and points out that 
> it is recommended by the C secure coding guidelines and added
> the PR to the commit log  (although the version with cast is not
> diagnosed so far.)  
> 
> I did not have time to implement the extensions suggested
> on the list,  i.e. warn when the size is not a multiple
> of the size of the type and warn for if the size is not
> suitable for a flexible array member. (this is also a bit
> more complicated than it seems)
> 
> Bootstrapped and regression tested on x86_64.
> 
> 
> Martin
> 
> 
> Add option Walloc-size that warns about allocations that have
> insufficient storage for the target type of the pointer the
> storage is assigned to.
> 
> 	PR c/71219
> gcc:
> 	* doc/invoke.texi: Document -Walloc-size option.
> 
> gcc/c-family:
> 
> 	* c.opt (Walloc-size): New option.
> 
> gcc/c:
> 	* c-typeck.cc (convert_for_assignment): Add warning.
> 
> gcc/testsuite:
> 
> 	* gcc.dg/Walloc-size-1.c: New test.
> ---
>  gcc/c-family/c.opt                   |  4 ++++
>  gcc/c/c-typeck.cc                    | 27 +++++++++++++++++++++
>  gcc/doc/invoke.texi                  | 10 ++++++++
>  gcc/testsuite/gcc.dg/Walloc-size-1.c | 36 ++++++++++++++++++++++++++++
>  4 files changed, 77 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/Walloc-size-1.c
> 
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 7348ad42ee0..9ba08a1fb6d 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -319,6 +319,10 @@ Walloca
>  C ObjC C++ ObjC++ Var(warn_alloca) Warning
>  Warn on any use of alloca.
>  
> +Walloc-size
> +C ObjC Var(warn_alloc_size) Warning
> +Warn when allocating insufficient storage for the target type of the assigned pointer.
> +
>  Walloc-size-larger-than=
>  C ObjC C++ LTO ObjC++ Var(warn_alloc_size_limit) Joined Host_Wide_Int ByteSize Warning Init(HOST_WIDE_INT_MAX)
>  -Walloc-size-larger-than=<bytes>	Warn for calls to allocation functions that
> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
> index e2bfd2caf85..c759c6245ed 100644
> --- a/gcc/c/c-typeck.cc
> +++ b/gcc/c/c-typeck.cc
> @@ -7384,6 +7384,33 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type,
>  		    "request for implicit conversion "
>  		    "from %qT to %qT not permitted in C++", rhstype, type);
>  
> +      /* Warn of new allocations that are not big enough for the target
> +	 type.  */
> +      tree fndecl;
> +      if (warn_alloc_size
> +	  && TREE_CODE (rhs) == CALL_EXPR
> +	  && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
> +	  && DECL_IS_MALLOC (fndecl))
> +	{
> +	  tree fntype = TREE_TYPE (fndecl);
> +	  tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
> +	  tree alloc_size = lookup_attribute ("alloc_size", fntypeattrs);
> +	  if (alloc_size)
> +	    {
> +	      tree args = TREE_VALUE (alloc_size);
> +	      int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
> +	      /* For calloc only use the second argument.  */
> +	      if (TREE_CHAIN (args))
> +		idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN (args))) - 1;
> +	      tree arg = CALL_EXPR_ARG (rhs, idx);
> +	      if (TREE_CODE (arg) == INTEGER_CST
> +		  && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
> +		 warning_at (location, OPT_Walloc_size, "allocation of "
> +			     "insufficient size %qE for type %qT with "
> +			     "size %qE", arg, ttl, TYPE_SIZE_UNIT (ttl));
> +	    }
> +	}
> +
>        /* See if the pointers point to incompatible address spaces.  */
>        asl = TYPE_ADDR_SPACE (ttl);
>        asr = TYPE_ADDR_SPACE (ttr);
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 33befee7d6b..a4fbcf5e1b5 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -8086,6 +8086,16 @@ always leads to a call to another @code{cold} function such as wrappers of
>  C++ @code{throw} or fatal error reporting functions leading to @code{abort}.
>  @end table
>  
> +@opindex Wno-alloc-size
> +@opindex Walloc-size
> +@item -Walloc-size
> +Warn about calls to allocation functions decorated with attribute
> +@code{alloc_size} that specify insufficient size for the target type of
> +the pointer the result is assigned to, including those to the built-in
> +forms of the functions @code{aligned_alloc}, @code{alloca},
> +@code{calloc},
> +@code{malloc}, and @code{realloc}.
> +
>  @opindex Wno-alloc-zero
>  @opindex Walloc-zero
>  @item -Walloc-zero
> diff --git a/gcc/testsuite/gcc.dg/Walloc-size-1.c b/gcc/testsuite/gcc.dg/Walloc-size-1.c
> new file mode 100644
> index 00000000000..61806f58192
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/Walloc-size-1.c
> @@ -0,0 +1,36 @@
> +/* Tests the warnings for insufficient allocation size.
> +   { dg-do compile }
> +   { dg-options "-Walloc-size" }
> + * */
> +#include <stdlib.h>
> +#include <alloca.h>
> +
> +struct b { int x[10]; };
> +
> +void fo0(void)
> +{
> +        struct b *p = malloc(sizeof *p);
> +}
> +
> +void fo1(void)
> +{
> +        struct b *p = malloc(sizeof p);		/* { dg-warning "allocation of insufficient size" } */
> +}
> +
> +void fo2(void)
> +{
> +        struct b *p = alloca(sizeof p);		/* { dg-warning "allocation of insufficient size" } */
> +}
> +
> +void fo3(void)
> +{
> +        struct b *p = calloc(1, sizeof p);	/* { dg-warning "allocation of insufficient size" } */
> +}
> +
> +void g(struct b* p);
> +
> +void fo4(void)
> +{
> +        g(malloc(4));				/* { dg-warning "allocation of insufficient size" } */
> +}
> +

On Tue, 31 Oct 2023, Martin Uecker wrote:

> > +	      if (TREE_CODE (arg) == INTEGER_CST
> > +		  && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))

What if TYPE_SIZE_UNIT (ttl) is not an INTEGER_CST?  I don't see any tests 
of the case of assigning to a pointer to a variably sized type.

Am Dienstag, dem 31.10.2023 um 22:19 +0000 schrieb Joseph Myers:
> On Tue, 31 Oct 2023, Martin Uecker wrote:
> 
> > > +	      if (TREE_CODE (arg) == INTEGER_CST
> > > +		  && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
> 
> What if TYPE_SIZE_UNIT (ttl) is not an INTEGER_CST?  I don't see any tests 
> of the case of assigning to a pointer to a variably sized type.
> 

Right. Thanks! Revised version attached.

Martin



    c: Add Walloc-size to warn about insufficient size in allocations [PR71219]
    
    Add option Walloc-size that warns about allocations that have
    insufficient storage for the target type of the pointer the
    storage is assigned to. Added to Wextra.
    
            PR c/71219
    gcc:
            * doc/invoke.texi: Document -Walloc-size option.
    
    gcc/c-family:
    
            * c.opt (Walloc-size): New option.
    
    gcc/c:
            * c-typeck.cc (convert_for_assignment): Add warning.
    
    gcc/testsuite:
    
            * gcc.dg/Walloc-size-1.c: New test.
            * gcc.dg/Walloc-size-2.c: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 44b9c862c14..29d3d789a49 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -331,6 +331,10 @@ Walloca
 C ObjC C++ ObjC++ Var(warn_alloca) Warning
 Warn on any use of alloca.
 
+Walloc-size
+C ObjC Var(warn_alloc_size) Warning LangEnabledBy(C ObjC, Wextra)
+Warn when allocating insufficient storage for the target type of the assigned pointer.
+
 Walloc-size-larger-than=
 C ObjC C++ LTO ObjC++ Var(warn_alloc_size_limit) Joined Host_Wide_Int ByteSize Warning Init(HOST_WIDE_INT_MAX)
 -Walloc-size-larger-than=<bytes>	Warn for calls to allocation functions that
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 0de4662bfc6..16fadfb5468 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -7347,6 +7347,34 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type,
 		    "request for implicit conversion "
 		    "from %qT to %qT not permitted in C++", rhstype, type);
 
+      /* Warn of new allocations that are not big enough for the target
+	 type.  */
+      tree fndecl;
+      if (warn_alloc_size
+	  && TREE_CODE (rhs) == CALL_EXPR
+	  && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
+	  && DECL_IS_MALLOC (fndecl))
+	{
+	  tree fntype = TREE_TYPE (fndecl);
+	  tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
+	  tree alloc_size = lookup_attribute ("alloc_size", fntypeattrs);
+	  if (alloc_size)
+	    {
+	      tree args = TREE_VALUE (alloc_size);
+	      int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
+	      /* For calloc only use the second argument.  */
+	      if (TREE_CHAIN (args))
+		idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN (args))) - 1;
+	      tree arg = CALL_EXPR_ARG (rhs, idx);
+	      if (TREE_CODE (arg) == INTEGER_CST
+		  && INTEGER_CST == TREE_CODE (TYPE_SIZE_UNIT (ttl))
+		  && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
+		 warning_at (location, OPT_Walloc_size, "allocation of "
+			     "insufficient size %qE for type %qT with "
+			     "size %qE", arg, ttl, TYPE_SIZE_UNIT (ttl));
+	    }
+	}
+
       /* See if the pointers point to incompatible address spaces.  */
       asl = TYPE_ADDR_SPACE (ttl);
       asr = TYPE_ADDR_SPACE (ttr);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5a9284d635c..815a33d4b87 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8138,6 +8138,16 @@ always leads to a call to another @code{cold} function such as wrappers of
 C++ @code{throw} or fatal error reporting functions leading to @code{abort}.
 @end table
 
+@opindex Wno-alloc-size
+@opindex Walloc-size
+@item -Walloc-size
+Warn about calls to allocation functions decorated with attribute
+@code{alloc_size} that specify insufficient size for the target type of
+the pointer the result is assigned to, including those to the built-in
+forms of the functions @code{aligned_alloc}, @code{alloca},
+@code{calloc},
+@code{malloc}, and @code{realloc}.
+
 @opindex Wno-alloc-zero
 @opindex Walloc-zero
 @item -Walloc-zero
diff --git a/gcc/testsuite/gcc.dg/Walloc-size-1.c b/gcc/testsuite/gcc.dg/Walloc-size-1.c
new file mode 100644
index 00000000000..61806f58192
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloc-size-1.c
@@ -0,0 +1,36 @@
+/* Tests the warnings for insufficient allocation size.
+   { dg-do compile }
+   { dg-options "-Walloc-size" }
+ * */
+#include <stdlib.h>
+#include <alloca.h>
+
+struct b { int x[10]; };
+
+void fo0(void)
+{
+        struct b *p = malloc(sizeof *p);
+}
+
+void fo1(void)
+{
+        struct b *p = malloc(sizeof p);		/* { dg-warning "allocation of insufficient size" } */
+}
+
+void fo2(void)
+{
+        struct b *p = alloca(sizeof p);		/* { dg-warning "allocation of insufficient size" } */
+}
+
+void fo3(void)
+{
+        struct b *p = calloc(1, sizeof p);	/* { dg-warning "allocation of insufficient size" } */
+}
+
+void g(struct b* p);
+
+void fo4(void)
+{
+        g(malloc(4));				/* { dg-warning "allocation of insufficient size" } */
+}
+
diff --git a/gcc/testsuite/gcc.dg/Walloc-size-2.c b/gcc/testsuite/gcc.dg/Walloc-size-2.c
new file mode 100644
index 00000000000..fbf297302fc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Walloc-size-2.c
@@ -0,0 +1,20 @@
+/* Test that VLA types do not crash with -Walloc-size
+   { dg-do compile }
+   { dg-options "-Walloc-size" }
+ * */
+#include <stdlib.h>
+#include <alloca.h>
+
+struct foo { int x[10]; };
+
+void fo0(int n)
+{
+        struct foo (*p)[n] = malloc(sizeof *p);
+}
+
+void fo1(int n)
+{
+	struct bar { int x[n]; };
+	struct bar *p = malloc(sizeof *p);
+}
+

On Wed, 1 Nov 2023, Martin Uecker wrote:

> Am Dienstag, dem 31.10.2023 um 22:19 +0000 schrieb Joseph Myers:
> > On Tue, 31 Oct 2023, Martin Uecker wrote:
> > 
> > > > +	      if (TREE_CODE (arg) == INTEGER_CST
> > > > +		  && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
> > 
> > What if TYPE_SIZE_UNIT (ttl) is not an INTEGER_CST?  I don't see any tests 
> > of the case of assigning to a pointer to a variably sized type.
> > 
> 
> Right. Thanks! Revised version attached.

This version is OK.

On Mon, Sep 18, 2023 at 11:26:49PM +0200, Martin Uecker via Gcc-patches wrote:
> Add option Walloc-size that warns about allocations that have
> insufficient storage for the target type of the pointer the
> storage is assigned to.
> 
> 	PR c/71219
> gcc:
> 	* doc/invoke.texi: Document -Walloc-size option.
> 
> gcc/c-family:
> 
> 	* c.opt (Walloc-size): New option.
> 
> gcc/c:
> 	* c-typeck.cc (convert_for_assignment): Add warning.
> 
> gcc/testsuite:
> 
> 	* gcc.dg/Walloc-size-1.c: New test.

> --- a/gcc/c/c-typeck.cc
> +++ b/gcc/c/c-typeck.cc
> @@ -7384,6 +7384,33 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type,
>  		    "request for implicit conversion "
>  		    "from %qT to %qT not permitted in C++", rhstype, type);
>  
> +      /* Warn of new allocations that are not big enough for the target
> +	 type.  */
> +      tree fndecl;
> +      if (warn_alloc_size
> +	  && TREE_CODE (rhs) == CALL_EXPR
> +	  && (fndecl = get_callee_fndecl (rhs)) != NULL_TREE
> +	  && DECL_IS_MALLOC (fndecl))
> +	{
> +	  tree fntype = TREE_TYPE (fndecl);
> +	  tree fntypeattrs = TYPE_ATTRIBUTES (fntype);
> +	  tree alloc_size = lookup_attribute ("alloc_size", fntypeattrs);
> +	  if (alloc_size)
> +	    {
> +	      tree args = TREE_VALUE (alloc_size);
> +	      int idx = TREE_INT_CST_LOW (TREE_VALUE (args)) - 1;
> +	      /* For calloc only use the second argument.  */
> +	      if (TREE_CHAIN (args))
> +		idx = TREE_INT_CST_LOW (TREE_VALUE (TREE_CHAIN (args))) - 1;

I wonder if this part isn't too pedantic or more of a code style.
Some packages fail to build with this with -Werror because they do
  struct S *p = calloc (sizeof (struct S), 1);
or similar.  It is true that calloc arguments are documented to be
nmemb, size, but given sufficient alignment (which is not really different
between either order of arguments) isn't it completely valid to allocate
char array with sizeof (struct S) elements and then store a struct S object
into it?
Given
  struct S { int a, b; int flex[]; };
  struct S *a = calloc (sizeof (struct S), 1);
  struct S *b = calloc (1, sizeof (struct S));
  struct S *c = calloc (sizeof (struct S), n);
  struct S *d = calloc (n, sizeof (struct S));
  struct S *e = calloc (n * sizeof (struct S), 1);
  struct S *f = calloc (1, n * sizeof (struct S));
  struct S *g = calloc (offsetof (struct S, flex[0]) + n * sizeof (int), 1);
  struct S *h = calloc (1, offsetof (struct S, flex[0]) + n * sizeof (int));
what from these feels like a potentially dangerous use compared to
just style that some project might want to enforce?
I'd say b, d, h deserve no warning at all, the rest are just style warnings
(e and f might have warnings desirable as a potential security problem
because it doesn't deal with overflows).

So, for -Walloc-size as written, wouldn't it be better to check for calloc
both arguments, if both are constant, either just multiply them and check
the product or check if both are smaller than TYPE_SIZE_UNIT, if only one of
them is constant, check that one with possible exception of 1 (in which case
try say if the other argument is multiple_of_p or something similar)?

> +	      tree arg = CALL_EXPR_ARG (rhs, idx);
> +	      if (TREE_CODE (arg) == INTEGER_CST
> +		  && tree_int_cst_lt (arg, TYPE_SIZE_UNIT (ttl)))
> +		 warning_at (location, OPT_Walloc_size, "allocation of "
> +			     "insufficient size %qE for type %qT with "
> +			     "size %qE", arg, ttl, TYPE_SIZE_UNIT (ttl));
> +	    }
> +	}
> +

	Jakub

On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote:
> I wonder if this part isn't too pedantic or more of a code style.
> Some packages fail to build with this with -Werror because they do
>   struct S *p = calloc (sizeof (struct S), 1);
> or similar.  It is true that calloc arguments are documented to be
> nmemb, size, but given sufficient alignment (which is not really different
> between either order of arguments) isn't it completely valid to allocate
> char array with sizeof (struct S) elements and then store a struct S object
> into it?

In PR112364 Martin Uecker has pointed out the alignment may be different
with the different order of arguments, per C23 (N2293).  With earlier
versions of the standard some people believe the alignment should not be
different, while the other people disagree (as the text is not very
clear).

On Wed, Dec 06, 2023 at 08:31:12PM +0800, Xi Ruoyao wrote:
> On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote:
> > I wonder if this part isn't too pedantic or more of a code style.
> > Some packages fail to build with this with -Werror because they do
> >   struct S *p = calloc (sizeof (struct S), 1);
> > or similar.  It is true that calloc arguments are documented to be
> > nmemb, size, but given sufficient alignment (which is not really different
> > between either order of arguments) isn't it completely valid to allocate
> > char array with sizeof (struct S) elements and then store a struct S object
> > into it?
> 
> In PR112364 Martin Uecker has pointed out the alignment may be different
> with the different order of arguments, per C23 (N2293).  With earlier
> versions of the standard some people believe the alignment should not be
> different, while the other people disagree (as the text is not very
> clear).

I can understand implementations which use smaller alignment based on
allocation size, but are there any which consider for that just the second
calloc argument rather than the product of both arguments?
I think they'd quickly break a lot of real-world code.
Further I think
"size less than or equal to the size requested"
is quite ambiguous in the calloc case, isn't the size requested in the
calloc case actually nmemb * size rather than just size?

	Jakub

Am Mittwoch, dem 06.12.2023 um 13:57 +0100 schrieb Jakub Jelinek:
> On Wed, Dec 06, 2023 at 08:31:12PM +0800, Xi Ruoyao wrote:
> > On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote:
> > > I wonder if this part isn't too pedantic or more of a code style.
> > > Some packages fail to build with this with -Werror because they do
> > >   struct S *p = calloc (sizeof (struct S), 1);
> > > or similar.  It is true that calloc arguments are documented to be
> > > nmemb, size, but given sufficient alignment (which is not really different
> > > between either order of arguments) isn't it completely valid to allocate
> > > char array with sizeof (struct S) elements and then store a struct S object
> > > into it?
> > 
> > In PR112364 Martin Uecker has pointed out the alignment may be different
> > with the different order of arguments, per C23 (N2293).  With earlier
> > versions of the standard some people believe the alignment should not be
> > different, while the other people disagree (as the text is not very
> > clear).
> 
> I can understand implementations which use smaller alignment based on
> allocation size, but are there any which consider for that just the second
> calloc argument rather than the product of both arguments?

Not that I know of.  

> I think they'd quickly break a lot of real-world code.

There are quite a few projects which use calloc with swapped
arguments.

> Further I think
> "size less than or equal to the size requested"
> is quite ambiguous in the calloc case, isn't the size requested in the
> calloc case actually nmemb * size rather than just size?

This is unclear but it can be understood this way.
This was also Joseph's point.

I am happy to submit a patch that changes the code so
that the swapped arguments to calloc do not cause a warning
anymore.

On the other hand, the only feedback I got so far was
from people who were then happy to get this warning.

Martin

Am Mittwoch, dem 06.12.2023 um 14:34 +0100 schrieb Martin Uecker:
> Am Mittwoch, dem 06.12.2023 um 13:57 +0100 schrieb Jakub Jelinek:
> > On Wed, Dec 06, 2023 at 08:31:12PM +0800, Xi Ruoyao wrote:
> > > On Wed, 2023-12-06 at 13:24 +0100, Jakub Jelinek wrote:
> > > > I wonder if this part isn't too pedantic or more of a code style.
> > > > Some packages fail to build with this with -Werror because they do
> > > >   struct S *p = calloc (sizeof (struct S), 1);
> > > > or similar.  It is true that calloc arguments are documented to be
> > > > nmemb, size, but given sufficient alignment (which is not really different
> > > > between either order of arguments) isn't it completely valid to allocate
> > > > char array with sizeof (struct S) elements and then store a struct S object
> > > > into it?
> > > 
> > > In PR112364 Martin Uecker has pointed out the alignment may be different
> > > with the different order of arguments, per C23 (N2293).  With earlier
> > > versions of the standard some people believe the alignment should not be
> > > different, while the other people disagree (as the text is not very
> > > clear).
> > 
> > I can understand implementations which use smaller alignment based on
> > allocation size, but are there any which consider for that just the second
> > calloc argument rather than the product of both arguments?
> 
> Not that I know of.  
> 
> > I think they'd quickly break a lot of real-world code.
> 
> There are quite a few projects which use calloc with swapped
> arguments.
> 
> > Further I think
> > "size less than or equal to the size requested"
> > is quite ambiguous in the calloc case, isn't the size requested in the
> > calloc case actually nmemb * size rather than just size?
> 
> This is unclear but it can be understood this way.
> This was also Joseph's point.
> 
> I am happy to submit a patch that changes the code so
> that the swapped arguments to calloc do not cause a warning
> anymore.
> 
> On the other hand, the only feedback I got so far was
> from people who were then happy to get this warning.

Note that it is now -Wextra.

On Wed, Dec 06, 2023 at 02:34:10PM +0100, Martin Uecker wrote:
> > Further I think
> > "size less than or equal to the size requested"
> > is quite ambiguous in the calloc case, isn't the size requested in the
> > calloc case actually nmemb * size rather than just size?
> 
> This is unclear but it can be understood this way.
> This was also Joseph's point.
> 
> I am happy to submit a patch that changes the code so
> that the swapped arguments to calloc do not cause a warning
> anymore.

That would be my preference because then the allocation size is
correct and it is purely a style warning.
It doesn't follow how the warning is described:
"Warn about calls to allocation functions decorated with attribute
@code{alloc_size} that specify insufficient size for the target type of
the pointer the result is assigned to"
when the size is certainly sufficient.

But wonder what others think about it.

BTW, shouldn't the warning be for C++ as well?  Sure, I know,
people use operator new more often, but still, the <cstdlib>
allocators are used in there as well.

We have the -Wmemset-transposed-args warning, couldn't we
have a similar one for calloc, and perhaps do it solely in
the case where one uses sizeof of the type used in the cast
pointer?
So warn for
(struct S *) calloc (sizeof (struct S), 1)
or
(struct S *) calloc (sizeof (struct S), n)
but not for
(struct S *) calloc (4, 15)
or
(struct S *) calloc (sizeof (struct T), 1)
or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.

	Jakub

On 2023-12-06 09:21, Jakub Jelinek wrote:
> On Wed, Dec 06, 2023 at 02:34:10PM +0100, Martin Uecker wrote:
>>> Further I think
>>> "size less than or equal to the size requested"
>>> is quite ambiguous in the calloc case, isn't the size requested in the
>>> calloc case actually nmemb * size rather than just size?
>>
>> This is unclear but it can be understood this way.
>> This was also Joseph's point.
>>
>> I am happy to submit a patch that changes the code so
>> that the swapped arguments to calloc do not cause a warning
>> anymore.
> 
> That would be my preference because then the allocation size is
> correct and it is purely a style warning.
> It doesn't follow how the warning is described:
> "Warn about calls to allocation functions decorated with attribute
> @code{alloc_size} that specify insufficient size for the target type of
> the pointer the result is assigned to"
> when the size is certainly sufficient.
> 
> But wonder what others think about it.

+1, from a libc perspective, the transposed arguments don't make a 
difference, a typical allocator will produce sufficiently sized 
allocation for the calloc call.

> BTW, shouldn't the warning be for C++ as well?  Sure, I know,
> people use operator new more often, but still, the <cstdlib>
> allocators are used in there as well.
> 
> We have the -Wmemset-transposed-args warning, couldn't we
> have a similar one for calloc, and perhaps do it solely in
> the case where one uses sizeof of the type used in the cast
> pointer?
> So warn for
> (struct S *) calloc (sizeof (struct S), 1)
> or
> (struct S *) calloc (sizeof (struct S), n)
> but not for
> (struct S *) calloc (4, 15)
> or
> (struct S *) calloc (sizeof (struct T), 1)
> or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.

+1, this could be an analyzer warning, since in practice it is just a 
code cleanliness issue.

Thanks,
Sid

On Wed, Dec 06, 2023 at 09:30:32AM -0500, Siddhesh Poyarekar wrote:
> > We have the -Wmemset-transposed-args warning, couldn't we
> > have a similar one for calloc, and perhaps do it solely in
> > the case where one uses sizeof of the type used in the cast
> > pointer?
> > So warn for
> > (struct S *) calloc (sizeof (struct S), 1)
> > or
> > (struct S *) calloc (sizeof (struct S), n)
> > but not for
> > (struct S *) calloc (4, 15)
> > or
> > (struct S *) calloc (sizeof (struct T), 1)
> > or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.
> 
> +1, this could be an analyzer warning, since in practice it is just a code
> cleanliness issue.

We don't do such things in the analyzer, nor it is possible, by the time
analyzer sees the IL all the sizeofs etc. are folded.  Analyzer is for
expensive to compute warnings, code style warnings are normally implemented
in the FEs.

	Jakub

On 2023-12-06 09:41, Jakub Jelinek wrote:
> On Wed, Dec 06, 2023 at 09:30:32AM -0500, Siddhesh Poyarekar wrote:
>>> We have the -Wmemset-transposed-args warning, couldn't we
>>> have a similar one for calloc, and perhaps do it solely in
>>> the case where one uses sizeof of the type used in the cast
>>> pointer?
>>> So warn for
>>> (struct S *) calloc (sizeof (struct S), 1)
>>> or
>>> (struct S *) calloc (sizeof (struct S), n)
>>> but not for
>>> (struct S *) calloc (4, 15)
>>> or
>>> (struct S *) calloc (sizeof (struct T), 1)
>>> or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.
>>
>> +1, this could be an analyzer warning, since in practice it is just a code
>> cleanliness issue.
> 
> We don't do such things in the analyzer, nor it is possible, by the time
> analyzer sees the IL all the sizeofs etc. are folded.  Analyzer is for
> expensive to compute warnings, code style warnings are normally implemented
> in the FEs.

Thanks, understood.  A separate FE warning is fine as well.

Thanks,
Sid

Am Mittwoch, dem 06.12.2023 um 15:21 +0100 schrieb Jakub Jelinek:
> On Wed, Dec 06, 2023 at 02:34:10PM +0100, Martin Uecker wrote:
> > > Further I think
> > > "size less than or equal to the size requested"
> > > is quite ambiguous in the calloc case, isn't the size requested in the
> > > calloc case actually nmemb * size rather than just size?
> > 
> > This is unclear but it can be understood this way.
> > This was also Joseph's point.
> > 
> > I am happy to submit a patch that changes the code so
> > that the swapped arguments to calloc do not cause a warning
> > anymore.
> 
> That would be my preference because then the allocation size is
> correct and it is purely a style warning.
> It doesn't follow how the warning is described:
> "Warn about calls to allocation functions decorated with attribute
> @code{alloc_size} that specify insufficient size for the target type of
> the pointer the result is assigned to"
> when the size is certainly sufficient.

The C standard defines the semantics of to allocate space 
of 'nmemb' objects of size 'size', so I would say
the warning and its description are correct because
if you call calloc with '1' as size argument but
the object size is larger then you specify an 
insufficient size for the object given the semantical
description of calloc in the standard.

If this does not affect alignment, then  this should 
not matter, but it is still not really correct. 
> 
> But wonder what others think about it.
> 
> BTW, shouldn't the warning be for C++ as well?  Sure, I know,
> people use operator new more often, but still, the <cstdlib>
> allocators are used in there as well.

We should, but it I am not familiar with the C++ FE.

> 
> We have the -Wmemset-transposed-args warning, couldn't we
> have a similar one for calloc, and perhaps do it solely in
> the case where one uses sizeof of the type used in the cast
> pointer?
> So warn for
> (struct S *) calloc (sizeof (struct S), 1)
> or
> (struct S *) calloc (sizeof (struct S), n)
> but not for
> (struct S *) calloc (4, 15)
> or
> (struct S *) calloc (sizeof (struct T), 1)
> or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.
> 
> 	Jakub

Yes, although in contrast to -Wmeset-transposed-args
this would be considered a "style" option which then
nobody would activate.  And if we put it into -Wextra
then we have the same situation as today.

Martin

On Wed, Dec 06, 2023 at 03:56:10PM +0100, Martin Uecker wrote:
> > That would be my preference because then the allocation size is
> > correct and it is purely a style warning.
> > It doesn't follow how the warning is described:
> > "Warn about calls to allocation functions decorated with attribute
> > @code{alloc_size} that specify insufficient size for the target type of
> > the pointer the result is assigned to"
> > when the size is certainly sufficient.
> 
> The C standard defines the semantics of to allocate space 
> of 'nmemb' objects of size 'size', so I would say
> the warning and its description are correct because
> if you call calloc with '1' as size argument but
> the object size is larger then you specify an 
> insufficient size for the object given the semantical
> description of calloc in the standard.

1 is sizeof (char), so you ask for an array of sizeof (struct ...)
chars and store the struct into it.

> > We have the -Wmemset-transposed-args warning, couldn't we
> > have a similar one for calloc, and perhaps do it solely in
> > the case where one uses sizeof of the type used in the cast
> > pointer?
> > So warn for
> > (struct S *) calloc (sizeof (struct S), 1)
> > or
> > (struct S *) calloc (sizeof (struct S), n)
> > but not for
> > (struct S *) calloc (4, 15)
> > or
> > (struct S *) calloc (sizeof (struct T), 1)
> > or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.
> 
> Yes, although in contrast to -Wmeset-transposed-args
> this would be considered a "style" option which then
> nobody would activate.  And if we put it into -Wextra
> then we have the same situation as today.

Well, the significant difference would be that users would
know that they got the size for the allocation right, just
that a coding style says it is better to put the type's size
as the second argument rather than first, and they could disable
that warning separately from -Walloc-size and still get warnings
on (struct S *) calloc (1, 1) or (struct S *) malloc (3) if
sizeof (struct S) is 24...

	Jakub

Am Mittwoch, dem 06.12.2023 um 16:01 +0100 schrieb Jakub Jelinek:
> On Wed, Dec 06, 2023 at 03:56:10PM +0100, Martin Uecker wrote:
> > > That would be my preference because then the allocation size is
> > > correct and it is purely a style warning.
> > > It doesn't follow how the warning is described:
> > > "Warn about calls to allocation functions decorated with attribute
> > > @code{alloc_size} that specify insufficient size for the target type of
> > > the pointer the result is assigned to"
> > > when the size is certainly sufficient.
> > 
> > The C standard defines the semantics of to allocate space 
> > of 'nmemb' objects of size 'size', so I would say
> > the warning and its description are correct because
> > if you call calloc with '1' as size argument but
> > the object size is larger then you specify an 
> > insufficient size for the object given the semantical
> > description of calloc in the standard.
> 
> 1 is sizeof (char), so you ask for an array of sizeof (struct ...)
> chars and store the struct into it.

If you use

char *p = calloc(sizeof(struct foo), 1);

it does not warn.

> 
> > > We have the -Wmemset-transposed-args warning, couldn't we
> > > have a similar one for calloc, and perhaps do it solely in
> > > the case where one uses sizeof of the type used in the cast
> > > pointer?
> > > So warn for
> > > (struct S *) calloc (sizeof (struct S), 1)
> > > or
> > > (struct S *) calloc (sizeof (struct S), n)
> > > but not for
> > > (struct S *) calloc (4, 15)
> > > or
> > > (struct S *) calloc (sizeof (struct T), 1)
> > > or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.
> > 
> > Yes, although in contrast to -Wmeset-transposed-args
> > this would be considered a "style" option which then
> > nobody would activate.  And if we put it into -Wextra
> > then we have the same situation as today.
> 
> Well, the significant difference would be that users would
> know that they got the size for the allocation right, just
> that a coding style says it is better to put the type's size
> as the second argument rather than first, and they could disable
> that warning separately from -Walloc-size and still get warnings
> on (struct S *) calloc (1, 1) or (struct S *) malloc (3) if
> sizeof (struct S) is 24...

Ok. 

Note that another limitation of the current version is that it
does not warn for

... = (struct S*) calloc (...)

with the cast (which is non-idiomatic in C).  This is also
something I would like to address in the future and would be
more important for the C++ version.  But for this case it
should probably use the type of the cast and the warning
needs to be added somewhere else in the FE.


Martin

On Wed, Dec 6, 2023 at 10:13 AM Martin Uecker <uecker@tugraz.at> wrote:
>
> Am Mittwoch, dem 06.12.2023 um 16:01 +0100 schrieb Jakub Jelinek:
> > On Wed, Dec 06, 2023 at 03:56:10PM +0100, Martin Uecker wrote:
> > > > That would be my preference because then the allocation size is
> > > > correct and it is purely a style warning.
> > > > It doesn't follow how the warning is described:
> > > > "Warn about calls to allocation functions decorated with attribute
> > > > @code{alloc_size} that specify insufficient size for the target type of
> > > > the pointer the result is assigned to"
> > > > when the size is certainly sufficient.
> > >
> > > The C standard defines the semantics of to allocate space
> > > of 'nmemb' objects of size 'size', so I would say
> > > the warning and its description are correct because
> > > if you call calloc with '1' as size argument but
> > > the object size is larger then you specify an
> > > insufficient size for the object given the semantical
> > > description of calloc in the standard.
> >
> > 1 is sizeof (char), so you ask for an array of sizeof (struct ...)
> > chars and store the struct into it.
>
> If you use
>
> char *p = calloc(sizeof(struct foo), 1);
>
> it does not warn.
>
> >
> > > > We have the -Wmemset-transposed-args warning, couldn't we
> > > > have a similar one for calloc, and perhaps do it solely in
> > > > the case where one uses sizeof of the type used in the cast
> > > > pointer?
> > > > So warn for
> > > > (struct S *) calloc (sizeof (struct S), 1)
> > > > or
> > > > (struct S *) calloc (sizeof (struct S), n)
> > > > but not for
> > > > (struct S *) calloc (4, 15)
> > > > or
> > > > (struct S *) calloc (sizeof (struct T), 1)
> > > > or similar?  Of course check for compatible types of TYPE_MAIN_VARIANTs.
> > >
> > > Yes, although in contrast to -Wmeset-transposed-args
> > > this would be considered a "style" option which then
> > > nobody would activate.  And if we put it into -Wextra
> > > then we have the same situation as today.
> >
> > Well, the significant difference would be that users would
> > know that they got the size for the allocation right, just
> > that a coding style says it is better to put the type's size
> > as the second argument rather than first, and they could disable
> > that warning separately from -Walloc-size and still get warnings
> > on (struct S *) calloc (1, 1) or (struct S *) malloc (3) if
> > sizeof (struct S) is 24...
>
> Ok.
>
> Note that another limitation of the current version is that it
> does not warn for
>
> ... = (struct S*) calloc (...)
>
> with the cast (which is non-idiomatic in C).

Note that -Wc++-compat encourages the cast, for people who are trying
to make their code compilable as both C and C++.

> This is also
> something I would like to address in the future and would be
> more important for the C++ version.  But for this case it
> should probably use the type of the cast and the warning
> needs to be added somewhere else in the FE.
>
>
> Martin
>