[Bug middle-end/98366] New: [11 Regression] wrong-code with memcmp and -m32

[Bug middle-end/98366] New: [11 Regression] wrong-code with memcmp and -m32

[mpolacek at gcc dot gnu.org]

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

            Bug ID: 98366
           Summary: [11 Regression] wrong-code with memcmp and -m32
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mpolacek at gcc dot gnu.org
  Target Milestone: ---

struct S {
  int b;
  int alignment;
  int c : 7;
  int d : 8;
} const e[] = {{.0, 3, '(', 2.0}};
typedef struct S S;

int memcmp (const void *, const void *, __SIZE_TYPE__);

int
main ()
{
  _Bool b = (!memcmp(e, (S[]){{.alignment = 3, '(', 2.0}}, sizeof (S)));
  if (!b)
    __builtin_abort ();
}

with -m32 -O2 generates a binary that aborts.  Started with r11-2709.

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0
                 CC|                            |msebor at gcc dot gnu.org
           Priority|P3                          |P1

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[mpolacek at gcc dot gnu.org]

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

--- Comment #1 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Seems like since that revision we completely elide the memcmp check.

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The question be asked if (S[]){{.alignment = 3, '(', 2.0}} has a defined S[0].b
to be 0 or is uninitialized?

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
                 CC|                            |marxin at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-12-18

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think .b is not a problem, that should be 0, but there are 17 bits of padding
at the end of the structure, and that has unspecified bits.
So, I think the testcase is invalid and normally succeeds only because the
gimplifier decides to zero the whole structure instead of initializing .b to 0.

Of course, it would be nice to find out what the r11-2709 code does exactly...

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
But
struct S {
  int b;
  int alignment;
  int c : 7;
  int d : 8;
  int e : 17;
} const e[] = {{0, 3, '(', 2, 0}};
typedef struct S S;

int memcmp (const void *, const void *, __SIZE_TYPE__);

int
main ()
{
  _Bool b = (!memcmp(e, (S[]){{.alignment = 3, '(', 2, 0}}, sizeof (S)));
  if (!b)
    __builtin_abort ();
}
should be valid because there is no padding but it aborts anyway, I think the
code mishandles bitfields.

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I don't understand why convert_to_bytes function has been added, that is
basically a bad reimplementation of native_encode_initializer, except that it
doesn't handle all the details native_encode_initializer handles.

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 49799
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49799&action=edit
gcc11-pr98366.patch

Untested fix.

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:69165332a914f1167c3077fa1f57afc64fd8a667

commit r11-6271-g69165332a914f1167c3077fa1f57afc64fd8a667
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Dec 19 22:24:10 2020 +0100

    expr: Fix up constant_byte_string bitfield handling [PR98366]

    constant_byte_string now uses a convert_to_bytes function, which doesn't
    handle bitfields at all (don't punt on them, just puts them into wrong bits
    or bytes).  Furthermore, I don't see a reason why that function should
exist
    at all, it duplicates native_encode_initializer functionality.
    Except that native_encode_initializer punted on flexible array members and
2
    tests in the testsuite relied on constant_byte_string handling those.
    So, this patch throws away convert_to_bytes, uses native_encode_initializer
    instead, but teaches it to handle flexible array members (only in the
    non-mask mode with off == -1 for now), furthermore, it adds various corner
    case checks that the old implementation was missing (like that STRING_CSTs
    use int as length and therefore we shouldn't try to build larger than that
    strings, or that native_encode*/native_interpret* APIs require sane
    host and target bytes (8-bit on both).

    2020-12-19  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/98366
            * fold-const.c (native_encode_initializer): Don't try to
            memset more than total_bytes with off == -1 even if len is large.
            Handle flexible array member initializers if off == -1 and mask is
            NULL.
            * expr.c (convert_to_bytes): Remove.
            (constant_byte_string): Use native_encode_initializer instead of
            convert_to_bytes.  Remove extraneous semicolon.  Punt on various
            corner-cases the APIs don't handle, like sizes > INT_MAX,
            BITS_PER_UNIT != 8, CHAR_BIT != 8.

            * gcc.c-torture/execute/pr98366.c: New test.

[Bug middle-end/98366] [11 Regression] wrong-code with memcmp and -m32

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.