[Bug libfortran/112364] New: calloc used incorrectly

[Bug libfortran/112364] New: calloc used incorrectly

[muecker at gwdg dot de]

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

            Bug ID: 112364
           Summary: calloc used incorrectly
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libfortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: muecker at gwdg dot de
  Target Milestone: ---

As detected by -Walloc-size, there are some calls to calloc in libgfortran with
incorrectly ordered arguments.

See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112347#c10


../../../trunk.year/libgfortran/io/async.c:265:24: warning: allocation of
insufficient size ‘1’ for type ‘transfer_queue’ with size ‘80’ [-Walloc-size]
../../../trunk.year/libgfortran/io/async.c:287:24: warning: allocation of
insufficient size ‘1’ for type ‘transfer_queue’ with size ‘80’ [-Walloc-size]
../../../trunk.year/libgfortran/io/async.c:311:24: warning: allocation of
insufficient size ‘1’ for type ‘transfer_queue’ with size ‘80’ [-Walloc-size]
../../../trunk.year/libgfortran/io/async.c:331:24: warning: allocation of
insufficient size ‘1’ for type ‘transfer_queue’ with size ‘80’ [-Walloc-size]

  transfer_queue *tq = calloc (sizeof (transfer_queue), 1);


Note that for the allocated size the order of arguments does not matter, but -
at least according to my understanding - the alignment requirements for the
returned memory may depend on the object size being the second argument.

[Bug libfortran/112364] calloc used incorrectly

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xry111 at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
           Keywords|                            |easyhack
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-11-03

--- Comment #1 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Martin Uecker from comment #0)

> Note that for the allocated size the order of arguments does not matter, but
> - at least according to my understanding - the alignment requirements for
> the returned memory may depend on the object size being the second argument.

No, per the standard we can assign the result of calloc to T* iff T has a
fundamental alignment requirement, i. e. _Alignof (max_align_t) >= _Alignof
(T).  It's not related to the specified size in calloc call.

But anyway in this case the order of arguments is indeed wrong and it should be
fixed.

[Bug libfortran/112364] calloc used incorrectly

[muecker at gwdg dot de]

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

--- Comment #2 from Martin Uecker <muecker at gwdg dot de> ---

I don't think this is correct.  The requirement is "The pointer returned if the
allocation succeeds is suitably aligned so that it may be assigned to a pointer
to any type of object with a fundamental alignment requirement and size less
than or equal to the size requested."

So it only has to take into account fundamental alignments for objects below
the given size and the fundamental alignment requirement differs for different
objects. Note that there is no "fundamental alignment".  max_align_t would have
the "greatest fundamental alignment", but the wording for allocation functions
does not refer to the greatest fundamental alignment.

[Bug libfortran/112364] calloc used incorrectly

[xry111 at gcc dot gnu.org]

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

--- Comment #3 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Martin Uecker from comment #2)
> I don't think this is correct.  The requirement is "The pointer returned if
> the allocation succeeds is suitably aligned so that it may be assigned to a
> pointer to any type of object with a fundamental alignment requirement and
> size less than or equal to the size requested."
> 
> So it only has to take into account fundamental alignments for objects below
> the given size and the fundamental alignment requirement differs for
> different objects. Note that there is no "fundamental alignment". 
> max_align_t would have the "greatest fundamental alignment", but the wording
> for allocation functions does not refer to the greatest fundamental
> alignment.

Hmm, the "and size less than or equal to the size requested" clause does not
exist in N1570 (C11), but it does exist in N3054 (C2x).  It looks like a subtly
backward-incompatible change in the standard...

[Bug libfortran/112364] calloc used incorrectly

[muecker at gwdg dot de]

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

--- Comment #4 from Martin Uecker <muecker at gwdg dot de> ---
Interesting. But independently of alignment, the description of calloc makes it
clear that it allocates an array of nmemb objects of size x.  So in any case I
think the code should be changed accordingly, which seems simple enough.

[Bug libfortran/112364] calloc used incorrectly

[xry111 at gcc dot gnu.org]

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

--- Comment #5 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Martin Uecker from comment #4)
> Interesting. But independently of alignment, the description of calloc makes
> it clear that it allocates an array of nmemb objects of size x.  So in any
> case I think the code should be changed accordingly, which seems simple
> enough.

Yes, it definitely needs a change.

[Bug libfortran/112364] calloc used incorrectly

[muecker at gwdg dot de]

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

--- Comment #6 from Martin Uecker <muecker at gwdg dot de> ---
For reference, this is were the revised wording in C comes. This is
intentional. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm

[Bug libfortran/112364] calloc used incorrectly

[joseph at codesourcery dot com]

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

--- Comment #7 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I believe "size requested" refers to the product nmemb * size in the case 
of calloc, so getting the arguments the "wrong" way round does not affect 
the required alignment.  The point of the change was to override DR#075 
and stop requiring e.g. 1-byte allocations to be suitably aligned for 
larger types, not to make alignment for calloc depend on more than the 
product of the two arguments.

[Bug libfortran/112364] calloc used incorrectly

[kargl at gcc dot gnu.org]

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

kargl at gcc dot gnu.org changed:

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

--- Comment #8 from kargl at gcc dot gnu.org ---
7 comments?

diff --git a/libgfortran/io/async.c b/libgfortran/io/async.c
index 57097438e89..8fa1f0d4ce0 100644
--- a/libgfortran/io/async.c
+++ b/libgfortran/io/async.c
@@ -262,7 +262,7 @@ init_async_unit (gfc_unit *u)
 void
 enqueue_transfer (async_unit *au, transfer_args *arg, enum aio_do type)
 {
-  transfer_queue *tq = calloc (sizeof (transfer_queue), 1);
+  transfer_queue *tq = calloc (1, sizeof (transfer_queue));
   tq->arg = *arg;
   tq->type = type;
   tq->has_id = 0;
@@ -284,7 +284,7 @@ int
 enqueue_done_id (async_unit *au, enum aio_do type)
 {
   int ret;
-  transfer_queue *tq = calloc (sizeof (transfer_queue), 1);
+  transfer_queue *tq = calloc (1, sizeof (transfer_queue));

   tq->type = type;
   tq->has_id = 1;
@@ -308,7 +308,7 @@ enqueue_done_id (async_unit *au, enum aio_do type)
 void
 enqueue_done (async_unit *au, enum aio_do type)
 {
-  transfer_queue *tq = calloc (sizeof (transfer_queue), 1);
+  transfer_queue *tq = calloc (1, sizeof (transfer_queue));
   tq->type = type;
   tq->has_id = 0;
   LOCK (&au->lock);
@@ -328,7 +328,7 @@ enqueue_done (async_unit *au, enum aio_do type)
 void
 enqueue_close (async_unit *au)
 {
-  transfer_queue *tq = calloc (sizeof (transfer_queue), 1);
+  transfer_queue *tq = calloc (1, sizeof (transfer_queue));

   tq->type = AIO_CLOSE;
   LOCK (&au->lock);

[Bug libfortran/112364] calloc used incorrectly

[muecker at gwdg dot de]

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

--- Comment #9 from Martin Uecker <muecker at gwdg dot de> ---
(In reply to joseph@codesourcery.com from comment #7)
> I believe "size requested" refers to the product nmemb * size in the case 
> of calloc, so getting the arguments the "wrong" way round does not affect 
> the required alignment.  The point of the change was to override DR#075 
> and stop requiring e.g. 1-byte allocations to be suitably aligned for 
> larger types, not to make alignment for calloc depend on more than the 
> product of the two arguments.

This may have been the intention but the wording now refers explicitly to the
size.
To me this also makes sense as calloc talks about allocating an array of
objects of size 'size'. The required alignment of the array would not depend on
the number of elements but only their size.

[Bug libfortran/112364] calloc used incorrectly

[joseph at codesourcery dot com]

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

--- Comment #10 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
The wording refers to "the size requested", which I consider to be the 
product of two arguments in the case of calloc - not a particular argument 
to calloc.

[Bug libfortran/112364] calloc used incorrectly

[kargl at gcc dot gnu.org]

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

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4

[Bug libfortran/112364] calloc used incorrectly

[muecker at gwdg dot de]

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

--- Comment #11 from Martin Uecker <muecker at gwdg dot de> ---
This is one possible way to read it.  But as written, I think one can easily
understand it the other way, because calloc never mentions requested total size
but only about space for an array of objects of size 'size'. And then also
7.24.3 continues with "It may then be used to access such an object or an array
of such objects.." which would also imply that the "size" in the sentence
before refers to the size of an individual object.

Anyway, I think it does not really matter for us...  IMHO it is still useful to
make the code have the logical order as documented for calloc. And if it turns
out that the warning annoys too many people in this way, we can still tweak it
before the release.

[Bug libfortran/112364] calloc used incorrectly

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tobias Burnus <burnus@gcc.gnu.org>:

https://gcc.gnu.org/g:17df6ddcf11aef6d200305d35641a7deb2f430e1

commit r14-5148-g17df6ddcf11aef6d200305d35641a7deb2f430e1
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Mon Nov 6 11:34:31 2023 +0100

    libgfortran: Fix calloc call by swapping arg order [PR112364]

    The prototype of calloc is
      void *calloc(size_t nmemb, size_t size);
    denoting "an array of nmemb objects, each of whose size is size." (C23)

    In order to follow the meaning of the argument names and to silence
    a -Walloc-size warning, this commit swaps the order of the two args
    to read now:  calloc (1, sizeof (transfer_queue));

    libgfortran/ChangeLog:

            PR libfortran/112364
            * io/async.c (enqueue_transfer, enqueue_done_id, enqueue_done,
            enqueue_close): Swap 1st and 2nd arg in calloc call.

[Bug libfortran/112364] calloc used incorrectly

[burnus at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #13 from Tobias Burnus <burnus at gcc dot gnu.org> ---
FIXED by committing the obvious fix as obvious (comment 12). (Same as the
obvious change as proposed by Steven in comment 8.)

As the warning is new (since r14-5059-gd880e093d92084) and there is no real
compelling reason to change it, I don't think it makes sense to queue it for
backporting.

Thanks for the report - and to all involved for the insight into the fine print
of the C standards.

[Bug libfortran/112364] calloc used incorrectly

[peter0x44 at disroot dot org]

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

Peter Damianov <peter0x44 at disroot dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |peter0x44 at disroot dot org

--- Comment #14 from Peter Damianov <peter0x44 at disroot dot org> ---
../../gcc/gcc/../libgcc/libgcov-util.c: In function 'void tag_counters(unsigned
int, int)':
../../gcc/gcc/../libgcc/libgcov-util.c:214:59: warning: 'void* calloc(size_t,
size_t)' sizes specified with 'sizeof' in the earlier argument and not in the
later argument [-Wcalloc-transposed-args]
  214 |   k_ctrs[tag_ix].values = values = (gcov_type *) xcalloc (sizeof
(gcov_type),
      |                                                          
^~~~~~~~~~~~~~~~~~
../../gcc/gcc/../libgcc/libgcov-util.c:214:59: note: earlier argument should
specify number of elements, later size of each element

../../gcc/gcc/../libgcc/libgcov-util.c: In function 'void
topn_to_memory_representation(gcov_ctr_info*)':
../../gcc/gcc/../libgcc/libgcov-util.c:529:43: warning: 'void* calloc(size_t,
size_t)' sizes specified with 'sizeof' in the earlier argument and not in the
later argument [-Wcalloc-transposed-args]
  529 |             = (struct gcov_kvp *)xcalloc (sizeof (struct gcov_kvp), n);
      |                                           ^~~~~~~~~~~~~~~~~~~~~~~~
../../gcc/gcc/../libgcc/libgcov-util.c:529:43: note: earlier argument should
specify number of elements, later size of each element

I found a two more instances of this in libgcov-util.c

Personally I don't agree with this warning at all, it's FAR too spammy for what
it is, considering it never actually catches a real defect, but rather a minor
style choice that (IMO) does not affect readability in any case. Whether I see
sizeof in the first or second argument doesn't affect my understanding of the
code.

In a static analyzer, fine, but I think in GCC, no.

But I guess the same solution should be taken for this one, too, if it's an
issue in libgfortran also.