[Bug c/100920] New: bogus warn on -Wscalar-storage-order

[Bug c/100920] New: bogus warn on -Wscalar-storage-order

[george.thopas at gmail dot com]

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

            Bug ID: 100920
           Summary: bogus warn on -Wscalar-storage-order
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: george.thopas at gmail dot com
  Target Milestone: ---

/* 
 * trying to use -Wscalar-storage-order on latest git version
 *
 * https://gcc.gnu.org/g:401bd4adcfda9965363b1ac3ba7e1580f15d6883
 *
 * below test exposes what looks like 2 wrong warnings  
 * a warning on an union where everything is big-endian 
 * a warning on type less void pointer
 *
 * gcc -Werror test.c
 *   
 * Thanks
 */
include <stddef.h>

struct s_1 {
    int val;
} __attribute__((scalar_storage_order("big-endian")));

typedef struct s_1 t_1;

struct s_2 {
    char val;
} __attribute__((scalar_storage_order("big-endian")));

typedef struct s_2 t_2;

struct s12 {
    t_1 a[1];
    t_2 b[1]; 
} __attribute__((scalar_storage_order("big-endian")));

typedef struct s12 t_s12;

/* Warning while everything is big-endian */
union u12 {
    t_1 a[1];
    t_2 b[1];
} __attribute__((scalar_storage_order("big-endian")));

typedef union u12 t_u12;

int main(void)
{
    /* warning while assigning from a type less void pointer */
    t_s12 *msg1 = __builtin_alloca(10);
    t_u12 *msg2 = __builtin_alloca(10);
    msg1->a[0].val=0;
    msg2->a[0].val=0;
    return 0;

[Bug c/100920] bogus warn on -Wscalar-storage-order

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-06-05
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
The warning on the union is indeed an oversight, but the other is
conservatively correct, although we may make a special case for
__builtin_alloca.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|bogus warn on               |bogus warnings with
                   |-Wscalar-storage-order      |-Wscalar-storage-order
           Assignee|unassigned at gcc dot gnu.org      |ebotcazou at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
            Version|unknown                     |12.0

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[george.thopas at gmail dot com]

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

--- Comment #2 from George Thopas <george.thopas at gmail dot com> ---
(In reply to Eric Botcazou from comment #1)
> The warning on the union is indeed an oversight, but the other is
> conservatively correct, although we may make a special case for
> __builtin_alloca.

Hi Eric, first of all, thanks for looking into this. 

It may not be immediately obvious from my example but the the builtin_alloca is
not the only problem. 

Any malloc/calloc has the same effect. Even if one requires casts at alloc
time, any free will trigger the same warning.

    t_s12 *msg1 = (t_s12)malloc(10);
    free(msg1);

I don't understand what the incompatibility reported is with void pointers.  To
my understanding they are type-less and can't be dereferenced.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[ebotcazou at gcc dot gnu.org]

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

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> It may not be immediately obvious from my example but the the builtin_alloca
> is not the only problem. 
> 
> Any malloc/calloc has the same effect. Even if one requires casts at alloc
> time, any free will trigger the same warning.
> 
>     t_s12 *msg1 = (t_s12)malloc(10);
>     free(msg1);
> 
> I don't understand what the incompatibility reported is with void pointers. 
> To my understanding they are type-less and can't be dereferenced.

Yes, the compiler treats alloca/malloc/calloc the same for this purpose.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:a589877a0036fc2f66b7a957859940c53efdc7c9

commit r12-1242-ga589877a0036fc2f66b7a957859940c53efdc7c9
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Sun Jun 6 11:37:45 2021 +0200

    Fix thinko in new warning on type punning for storage order purposes

    In C, unlike in Ada, the storage order of arrays is that of their component
    type, so you need to look at it when deciding to warn.  And the PR
complains
    about a bogus warning on the assignment of a pointer returned by alloca or
    malloc, so this also fixes that.

    gcc/c
            PR c/100920
            * c-decl.c (finish_struct): Fix thinko in previous change.
            * c-typeck.c (convert_for_assignment): Do not warn on pointer
            assignment and initialization for storage order purposes if the
            RHS is a call to a DECL_IS_MALLOC function.
    gcc/testsuite/
            * gcc.dg/sso-14.c: New test.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Thanks for reporting the problem.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[george.thopas at gmail dot com]

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

--- Comment #6 from George Thopas <george.thopas at gmail dot com> ---
(In reply to Eric Botcazou from comment #5)
> Thanks for reporting the problem.

Thanks for the swift fix. It nicely resolves it for the malloc/alloca ... 
It doesn't seem to do anything for the rest of the common idioms however
when I extend  the testcase.

int main()
{
    t_s12 *msg1 = __builtin_alloca(10);
    t_u12 *msg2 = __builtin_alloca(10);
    int same;

    msg1 = malloc (sizeof (t_s12));
    msg2 = malloc (sizeof (t_u12));

    memset(msg1, 0, sizeof(t_s12));
    memcpy(msg2, &msg1, sizeof(t_s12));
    same = memcmp(msg2, msg1, sizeof(t_s12));
    free(msg1);

    return same;
}

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[ebotcazou at gcc dot gnu.org]

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

--- Comment #7 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Thanks for the swift fix. It nicely resolves it for the malloc/alloca ... 
> It doesn't seem to do anything for the rest of the common idioms however
> when I extend the testcase.

Yet there is a provision to do that at line 7302, so a bit unexpected.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Eric Botcazou <ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:97d83259b91fb558f5b09bfb09529900f585c4c9

commit r12-1264-g97d83259b91fb558f5b09bfb09529900f585c4c9
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Mon Jun 7 18:17:31 2021 +0200

    Fix old thinko in warning on pointer for storage order purposes

    gcc/c
            PR c/100920
            * c-typeck.c (convert_for_assignment): Test fndecl_built_in_p to
            spot built-in functions.
    gcc/testsuite/
            * gcc.dg/sso-14.c: Adjust.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[george.thopas at gmail dot com]

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

--- Comment #9 from George Thopas <george.thopas at gmail dot com> ---
/*
Hi Eric,

1) I noticed there's a typo in the test, (which is my fault) and may give
   unexpected behavior later on

    memcpy(msg2, &msg1, sizeof(t_s12));
     => should be  memcpy(msg2, msg1, sizeof(t_s12));

2) Trying to get a platform built free of these warning, I still see quite some
which match below example.  
Raising one for these cases is really problematic. Functions using void in
stead of an explicit type are explicitly saying that the type does not matter.
There never is a warning for any other type where you pass them. so
I hope  you can give this one another go.

Thanks again for your patience.

*/


#include <stddef.h>
#include <string.h>

#define SIZE 10

struct be {
    int a;
    int b; 
} __attribute__((scalar_storage_order("big-endian")));

typedef struct be t_be;

struct le {
    int a;
    int b; 
} __attribute__((scalar_storage_order("little-endian")));

typedef struct le t_le;


void memset2(void *s, char c, int n)
{
   char *d=(char *)s;
   for(int i=0;i<n;i++)
      d[n]=c;
}

void init(void *data, int len)
{
    memset(data,0xcf, len);
}

int main(void)
{
    t_be *d0 = __builtin_alloca(SIZE);
    t_le *d1 = __builtin_alloca(SIZE);
    t_be *d2 = __builtin_alloca(SIZE);
    t_le *d3 = __builtin_alloca(SIZE);    
    void *raw  = __builtin_alloca(SIZE);

    init(d0, SIZE);
    init(d1, SIZE);
    init(d2, SIZE);
    init(d3, SIZE);
    init(raw, SIZE);    

    memset2(d0, 0xcf, SIZE);
    memset2(d1, 0xcf, SIZE);
    memset2(d2, 0xcf, SIZE);
    memset2(d3, 0xcf, SIZE);
    memset2(raw, 0xcf, SIZE);

    return 0;
}

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[ebotcazou at gcc dot gnu.org]

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

--- Comment #10 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Raising one for these cases is really problematic. Functions using void in
> stead of an explicit type are explicitly saying that the type does not
> matter. There never is a warning for any other type where you pass them. so
> I hope  you can give this one another go.

No, the warning needs to be conservative.

[Bug c/100920] bogus warnings with -Wscalar-storage-order

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Eric Botcazou
<ebotcazou@gcc.gnu.org>:

https://gcc.gnu.org/g:5be418dc591014b1773d59abca8fba5be34df618

commit r11-8533-g5be418dc591014b1773d59abca8fba5be34df618
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Wed Jun 9 12:37:47 2021 +0200

    Fix old thinko in warning on pointer for storage order purposes

    gcc/c
            PR c/100920
            * c-typeck.c (convert_for_assignment): Test fndecl_built_in_p to
            spot built-in functions. Do not warn on pointer assignment and
            initialization for storage order purposes if the RHS is a call
            to a DECL_IS_MALLOC function.
    gcc/testsuite/
            * gcc.dg/sso-14.c: New test.