[Bug c/111722] New: gcc generates wrong code with

[Bug c/111722] New: gcc generates wrong code with

[zfigura at codeweavers dot com]

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

            Bug ID: 111722
           Summary: gcc generates wrong code with
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zfigura at codeweavers dot com
  Target Milestone: ---

[Bug c/111722] gcc generates wrong code with

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-10-08
             Status|UNCONFIRMED                 |WAITING

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is nothing in this bug except saying there is wrong code happening (not
even with what options or with anything else).

[Bug target/111722] manually defined memcpy() and memmove() incorrectly handle overlap with -O2 -m32 -march=bdver2

[zfigura at codeweavers dot com]

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

Zeb Figura <zfigura at codeweavers dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|unknown                     |13.2.0
           Keywords|                            |wrong-code
             Target|                            |i686-linux-gnu
          Component|c                           |target
            Summary|gcc generates wrong code    |manually defined memcpy()
                   |with                        |and memmove() incorrectly
                   |                            |handle overlap with -O2
                   |                            |-m32 -march=bdver2

--- Comment #2 from Zeb Figura <zfigura at codeweavers dot com> ---
Really sorry about that, I managed to accidentally hit the Enter key halfway
through writing the title. Here is the actual bug description:

--

Wine provides freestanding libraries, including manual definitions of memcpy()
and memmove() [1].

Those are defined in C, and while our definitions are *technically*
non-compliant C (violating the requirement that the pointers must point to the
same object), they should be fine for our targets, and anyway, the case I'm
running into is failure to handle overlap where the pointers *do* in fact point
into the same object. I can't find fault with the definitions themselves,
although I may be missing something.

We also, contrary to standards, give memcpy() the semantics of memmove(),
because some Windows programs are buggy and make that assumption. We do this by
copy-pasting the definition (I'm not sure why we do this rather than just
calling one function from the other, but it is what it is).

I recently started compiling with -march=native, and found that gcc was failing
to correctly handle overlap in memmove. Further investigation revealed that,
somehow, memmove() was being incorrectly optimized to *not* check for overlap,
while memcpy() remained in its unoptimized form.

I ran into this originally with the i686-w64-mingw32 target, but I've adjusted
the target to i686-linux-gnu since it happens there too. It does *not* happen
on x86_64.

[1] https://source.winehq.org/git/wine.git/blob/HEAD:/dlls/ntdll/string.c#l98

[Bug target/111722] manually defined memcpy() and memmove() incorrectly handle overlap with -O2 -m32 -march=bdver2

[zfigura at codeweavers dot com]

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

--- Comment #3 from Zeb Figura <zfigura at codeweavers dot com> ---
Created attachment 56072
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56072&action=edit
testcase

Attaching a reduced-ish testcase, that contains the unmodified code of memcpy()
and memmove(), plus two callers. The callers seem to be necessary to trigger
the incorrect optimization.

Compile with '-c -O2 -march=bdver2 -m32'.

[Bug target/111722] manually defined memcpy() and memmove() incorrectly handle overlap with -O2 -m32 -march=bdver2

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |INVALID

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is no bug here.
ICF finds that your definition of memcpy is the same as memmove and merges the
2 and then calls memcpy from your memmove and then inlines the normal memcpy
because well it says it is the same.

You can just use -fno-builtin to fix the issue by saying memcpy and memmove are
not builtins and treat them like normal functions.

That fixes the issue by not inlining the target defined memcpy.

[Bug target/111722] manually defined memcpy() and memmove() incorrectly handle overlap with -O2 -m32 -march=bdver2

[zfigura at codeweavers dot com]

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

--- Comment #5 from Zeb Figura <zfigura at codeweavers dot com> ---
(In reply to Andrew Pinski from comment #4)
> There is no bug here.
> ICF finds that your definition of memcpy is the same as memmove and merges
> the 2 and then calls memcpy from your memmove and then inlines the normal
> memcpy because well it says it is the same.

I suppose I understand this explanation, but it does not feel like a very
intuitive behaviour.

The ICF part makes sense. The choice to optimize a builtin memcpy/memmove call
into a different instruction sequence (which doesn't match the original) also
makes sense. I would not really expect these two to be combined in this manner,
though. memmove() is not calling builtin memcpy(), it is calling our
implementation of memcpy(), which doesn't have the same semantics as builtin
memcpy().

[It also seems odd to me that func2() would be replaced with a builtin memcpy()
rather than a builtin memmove()?]

> You can just use -fno-builtin to fix the issue by saying memcpy and memmove
> are not builtins and treat them like normal functions.
> 
> That fixes the issue by not inlining the target defined memcpy.

Fair enough, I guess. I suppose that's the right thing to do anyway...