[Bug target/94541] New: -mx32 gcc produces wrong code passing structs by value

[Bug target/94541] New: -mx32 gcc produces wrong code passing structs by value

[ibuclaw at gdcproject dot org]

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

            Bug ID: 94541
           Summary: -mx32 gcc produces wrong code passing structs by value
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ibuclaw at gdcproject dot org
  Target Milestone: ---

Created attachment 48250
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48250&action=edit
test case

Attached reduced example.

Compiled with 'g++ -Wall -Wextra -fno-strict-aliasing -fwrapv', the run-time
results of the following combinations:

-O0 -m64:   0  0
-O1 -m64:   0  0
-O2 -m64:   0  0
-O3 -m64:   0  0
-O0 -m32:   0  0
-O1 -m32:   0  0
-O2 -m32:   0  0
-O3 -m32:   0  0
-O0 -mx32:  0  0
-O1 -mx32: -1 22  // munmap failed, EINVAL
-O2 -mx32: -1 22
-O3 -mx32: -1 22

A failure is not observed when:

- The condition is: if (b.ptr && b.length)
- The condition is: if (b.length)
- The ptr and length fields in Array are swapped.
- Array is passed by reference.


Tested with 8.4.0, 9.2.0, and 10.0.1.  All have the same problem.

[Bug target/94541] -mx32 gcc produces wrong code passing structs by value

[ibuclaw at gdcproject dot org]

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

--- Comment #1 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to Iain Buclaw from comment #0)
> A failure is not observed when:
> 
> - The condition is: if (b.ptr && b.length)
> - The condition is: if (b.length)
> - The ptr and length fields in Array are swapped.
> - Array is passed by reference.
> 

- A call to puts("") is inserted before munmap().

[Bug target/94541] -mx32 gcc produces wrong code passing structs by value

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I don't have a -mx32 capable environment, so can't verify, but mmap/munmap
shouldn't be inlined in any way, so can you reproduce if you replace
mmap with foo and munmap with bar and define them like:
static char c;
__attribute__((noipa)) void *
foo (void *addr, size_t length, int prot, int flags, int fd, off_t offset)
{
  return (void *) &c;
}
__attribute__((noipa)) int
bar (void *addr, size_t length)
{
  if (addr != (void *) &c)
    return -1;
  return 0;
}
or so?  If yes, can you also drop errno from the testcase, and possibly drop
unnecessary arguments to foo and/or bar as long as it reproduces?  Once
minimized that way, please preprocess it.

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |hjl.tools at gmail dot com
   Last reconfirmed|                            |2020-04-09
             Status|UNCONFIRMED                 |ASSIGNED
      Known to work|                            |4.9.3
            Summary|-mx32 gcc produces wrong    |[8/9/10 Regression] -mx32
                   |code passing structs by     |gcc produces wrong code
                   |value                       |passing structs by value
     Ever confirmed|0                           |1

--- Comment #3 from H.J. Lu <hjl.tools at gmail dot com> ---
[hjl@gnu-cfl-2 tmp]$ cat bar.c
typedef unsigned int size_t;
struct Array
{
  size_t length;
  void *ptr;
};
extern int munmap (void *__addr, size_t __len);

int
deallocate(struct Array b)
{
  int result = 0;
  if (b.length)
    result = munmap(b.ptr, b.length);
  return result;
}
[hjl@gnu-cfl-2 tmp]$  gcc -S -O bar.c -mx32 
[hjl@gnu-cfl-2 tmp]$ cat bar.s
        .file   "bar.c"
        .text
        .globl  deallocate
        .type   deallocate, @function
deallocate:
.LFB0:
        .cfi_startproc
        movl    $0, %eax
        testl   %edi, %edi
        jne     .L8
        ret
.L8:
        subl    $8, %esp
        .cfi_def_cfa_offset 16
        movq    %rdi, %rsi   <<<<<< It should be movl %edi, %esi
        shrq    $32, %rdi
        call    munmap
        addl    $8, %esp
        .cfi_def_cfa_offset 8
        ret
        .cfi_endproc
.LFE0:
        .size   deallocate, .-deallocate
        .ident  "GCC: (GNU) 9.3.1 20200408 (Red Hat 9.3.1-2)"
        .section        .note.GNU-stack,"",@progbits
[hjl@gnu-cfl-2 tmp]$ 

GCC 4.9.3 works.

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[ibuclaw at gdcproject dot org]

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

--- Comment #4 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to Jakub Jelinek from comment #2)
> I don't have a -mx32 capable environment, so can't verify, but mmap/munmap
> shouldn't be inlined in any way, so can you reproduce if you replace
> mmap with foo and munmap with bar and define them like:
> static char c;
> __attribute__((noipa)) void *
> foo (void *addr, size_t length, int prot, int flags, int fd, off_t offset)
> {
>   return (void *) &c;
> }
> __attribute__((noipa)) int
> bar (void *addr, size_t length)
> {
>   if (addr != (void *) &c)
>     return -1;
>   return 0;
> }
> or so?  If yes, can you also drop errno from the testcase, and possibly drop
> unnecessary arguments to foo and/or bar as long as it reproduces?  Once
> minimized that way, please preprocess it.

Can't reduce it down so there is no munmap().  I suspect this is because going
across the syscall layer means there's some naked asm involved to call the real
munmap().

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[ibuclaw at gdcproject dot org]

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

--- Comment #5 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to H.J. Lu from comment #3)
> .L8:
> 	subl	$8, %esp
> 	.cfi_def_cfa_offset 16
> 	movq	%rdi, %rsi   <<<<<< It should be movl %edi, %esi
> 	shrq	$32, %rdi
> 	call	munmap
> 	addl	$8, %esp
> 	.cfi_def_cfa_offset 8

Confirmed on my end, if I compile with -O1 -mx32 -S, and make this change, the
call succeeds.

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

--- Comment #6 from H.J. Lu <hjl.tools at gmail dot com> ---
It was caused by r5-901:

commit acea91c9012e211283130eb486d83243bcbbb327
Author: Martin Jambor <mjambor@suse.cz>
Date:   Mon May 26 17:36:00 2014 +0200

    ira.c (split_live_ranges_for_shrink_wrap): Remove bailout on subreg uses.

    2014-05-26  Martin Jambor  <mjambor@suse.cz>

            * ira.c (split_live_ranges_for_shrink_wrap): Remove bailout on
            subreg uses.

    From-SVN: r210936

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|hjl.tools at gmail dot com         |unassigned at gcc dot gnu.org
             Status|ASSIGNED                    |NEW

[Bug middle-end/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |middle-end

--- Comment #7 from H.J. Lu <hjl.tools at gmail dot com> ---
After 284r.ira:

(insn 12 11 13 3 (parallel [
            (set (reg:DI 88)
                (lshiftrt:DI (reg/v:DI 85 [ b ])
                    (const_int 32 [0x20])))
            (clobber (reg:CC 17 flags))
        ]) "y.i":14:14 639 {*lshrdi3_1}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (nil)))
(insn 13 12 14 3 (set (reg:SI 4 si)
        (subreg:SI (reg/v:DI 85 [ b ]) 0)) "y.i":14:14 67 {*movsi_internal}
     (expr_list:REG_DEAD (reg/v:DI 85 [ b ])
        (nil)))
(insn 14 13 15 3 (set (reg:DI 5 di)
        (reg:DI 88)) "y.i":14:14 66 {*movdi_internal}
     (expr_list:REG_DEAD (reg:DI 88)
        (nil)))

After 285r.reload:

(insn 32 11 12 3 (set (reg:DI 5 di [88])
        (reg/v:DI 4 si [orig:85 b ] [85])) "y.i":14:14 66 {*movdi_internal}
     (nil))
(insn 12 32 15 3 (parallel [
            (set (reg:DI 5 di [88])
                (lshiftrt:DI (reg:DI 5 di [88])
                    (const_int 32 [0x20])))
            (clobber (reg:CC 17 flags))
        ]) "y.i":14:14 639 {*lshrdi3_1}
     (nil))

[Bug middle-end/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[pinskia at gcc dot gnu.org]

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

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #7)
> After 284r.ira:

That is fine according to the rules as long as TARGET_TRULY_NOOP_TRUNCATION  is
true.

[Bug middle-end/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

--- Comment #9 from H.J. Lu <hjl.tools at gmail dot com> ---
I am testing this:

diff --git a/gcc/lra-spills.c b/gcc/lra-spills.c
index 0caa4acd3b5..bd4ee80245d 100644
--- a/gcc/lra-spills.c
+++ b/gcc/lra-spills.c
@@ -844,9 +844,14 @@ lra_final_code_change (void)

          struct lra_static_insn_data *static_id = id->insn_static_data;
          bool insn_change_p = false;
+         set = single_set (insn);

+         /* NB: Never change SUBREG in the source operand of a SET
+            insn to REG.  */
          for (i = id->insn_static_data->n_operands - 1; i >= 0; i--)
            if ((DEBUG_INSN_P (insn) || ! static_id->operand[i].is_operator)
+               && (set == NULL
+                   || !rtx_equal_p (SET_SRC (set), *id->operand_loc[i]))
                && alter_subregs (id->operand_loc[i], ! DEBUG_INSN_P (insn)))
              {
                lra_update_dup (id, i);
@@ -855,7 +860,7 @@ lra_final_code_change (void)
          if (insn_change_p)
            lra_update_operator_dups (id);

-         if ((set = single_set (insn)) != NULL
+         if (set != NULL
              && REG_P (SET_SRC (set)) && REG_P (SET_DEST (set))
              && REGNO (SET_SRC (set)) == REGNO (SET_DEST (set)))
            {

[Bug middle-end/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

--- Comment #10 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Andrew Pinski from comment #8)
> (In reply to H.J. Lu from comment #7)
> > After 284r.ira:
> 
> That is fine according to the rules as long as TARGET_TRULY_NOOP_TRUNCATION 
> is true.

We can't turn

(insn 13 12 14 3 (set (reg:SI 4 si)
        (subreg:SI (reg/v:DI 85 [ b ]) 0)) "y.i":14:14 67 {*movsi_internal}
     (expr_list:REG_DEAD (reg/v:DI 85 [ b ])
        (nil)))

into


(insn 13 12 14 3 (set (reg:SI 4 si)
        (reg:SI 4 si [orig:85 b ] [85])) "y.i":14:14 67 {*movsi_internal}
     (expr_list:REG_DEAD (reg/v:DI 4 si [orig:85 b ] [85])
        (nil)))

This is simple wrong.

[Bug middle-end/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[pinskia at gcc dot gnu.org]

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

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #10)
> (In reply to Andrew Pinski from comment #8)
> > (In reply to H.J. Lu from comment #7)
> > > After 284r.ira:
> > 
> > That is fine according to the rules as long as TARGET_TRULY_NOOP_TRUNCATION 
> > is true.
> 
> We can't turn
> 
> (insn 13 12 14 3 (set (reg:SI 4 si)
>         (subreg:SI (reg/v:DI 85 [ b ]) 0)) "y.i":14:14 67 {*movsi_internal}
>      (expr_list:REG_DEAD (reg/v:DI 85 [ b ])
>         (nil)))
> 
> into
> 
> 
> (insn 13 12 14 3 (set (reg:SI 4 si)
>         (reg:SI 4 si [orig:85 b ] [85])) "y.i":14:14 67 {*movsi_internal}
>      (expr_list:REG_DEAD (reg/v:DI 4 si [orig:85 b ] [85])
>         (nil)))
> 
> This is simple wrong.

How is that wrong?  What register was 85 assigned to?  4?
Then is fully correct as TARGET_TRULY_NOOP_TRUNCATION is true for those sizes.  

On MIPS,  TARGET_TRULY_NOOP_TRUNCATION is false for SI/DI truncation.
You need TARGET_TRULY_NOOP_TRUNCATION to be false for those sizes and you get a
truncate RTL instead of the subreg.  subreg means the upper bits of the
register are undefined.

[Bug middle-end/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

--- Comment #12 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Andrew Pinski from comment #11)
> (In reply to H.J. Lu from comment #10)
> > (In reply to Andrew Pinski from comment #8)
> > > (In reply to H.J. Lu from comment #7)
> > > > After 284r.ira:
> > > 
> > > That is fine according to the rules as long as TARGET_TRULY_NOOP_TRUNCATION 
> > > is true.
> > 
> > We can't turn
> > 
> > (insn 13 12 14 3 (set (reg:SI 4 si)
> >         (subreg:SI (reg/v:DI 85 [ b ]) 0)) "y.i":14:14 67 {*movsi_internal}
> >      (expr_list:REG_DEAD (reg/v:DI 85 [ b ])
> >         (nil)))
> > 
> > into
> > 
> > 
> > (insn 13 12 14 3 (set (reg:SI 4 si)
> >         (reg:SI 4 si [orig:85 b ] [85])) "y.i":14:14 67 {*movsi_internal}
> >      (expr_list:REG_DEAD (reg/v:DI 4 si [orig:85 b ] [85])
> >         (nil)))
> > 
> > This is simple wrong.
> 
> How is that wrong?  What register was 85 assigned to?  4?
> Then is fully correct as TARGET_TRULY_NOOP_TRUNCATION is true for those
> sizes.  
> 
> On MIPS,  TARGET_TRULY_NOOP_TRUNCATION is false for SI/DI truncation.
> You need TARGET_TRULY_NOOP_TRUNCATION to be false for those sizes and you
> get a truncate RTL instead of the subreg.  subreg means the upper bits of
> the register are undefined.

In this case, upper bits in 85 are undefined:

(insn 13 12 14 3 (set (reg:SI 4 si)
        (subreg:SI (reg/v:DI 4 si [orig:85 b ] [85]) 0)) "y.i":14:14 67
{*movsi_internal}
     (expr_list:REG_DEAD (reg/v:DI 4 si [orig:85 b ] [85])
        (nil)))

LRA has:

         for (i = id->insn_static_data->n_operands - 1; i >= 0; i--)
            if ((DEBUG_INSN_P (insn) || ! static_id->operand[i].is_operator)
                && alter_subregs (id->operand_loc[i], ! DEBUG_INSN_P (insn)))
              {
                lra_update_dup (id, i);
                insn_change_p = true;
              }
          if (insn_change_p)
            lra_update_operator_dups (id);

          if ((set = single_set (insn)) != NULL
              && REG_P (SET_SRC (set)) && REG_P (SET_DEST (set))
              && REGNO (SET_SRC (set)) == REGNO (SET_DEST (set)))
            {
              /* Remove an useless move insn.  IRA can generate move
                 insns involving pseudos.  It is better remove them
                 earlier to speed up compiler a bit.  It is also
                 better to do it here as they might not pass final RTL
                 check in LRA, (e.g. insn moving a control register
                 into itself).  */
              lra_invalidate_insn_data (insn);
              delete_insn (insn);
            }

But

(insn 13 12 14 3 (set (reg:SI 4 si)
        (subreg:SI (reg/v:DI 4 si [orig:85 b ] [85]) 0)) "y.i":14:14 67
{*movsi_internal}
     (expr_list:REG_DEAD (reg/v:DI 4 si [orig:85 b ] [85])
        (nil)))

isn't useless and shouldn't be removed.

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|middle-end                  |target

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #12)
> 
> But
> 
> (insn 13 12 14 3 (set (reg:SI 4 si)
>         (subreg:SI (reg/v:DI 4 si [orig:85 b ] [85]) 0)) "y.i":14:14 67
> {*movsi_internal}
>      (expr_list:REG_DEAD (reg/v:DI 4 si [orig:85 b ] [85])
>         (nil)))
> 
> isn't useless and shouldn't be removed.

It is useless because TARGET_TRULY_NOOP_TRUNCATION is true.  So the problem is
you don't have a zero_extend in the RTL in the first place where you needed it
to be.
THIS IS NOT A BUG in the middle-end.

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

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

--- Comment #14 from H.J. Lu <hjl.tools at gmail dot com> ---
It is an x32 kernel bug.  These system calls:

       int mprotect(void *addr, size_t len, int prot);
       void *mmap(void *addr, size_t length, int prot, int flags,
                  int fd, off_t offset);
       int munmap(void *addr, size_t length);

take both void *, off_t and size_t.  They need special conversions
since they can't use LP64 nor IA32 versions.

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://bugzilla.kernel.org
                   |                            |/show_bug.cgi?id=207187
         Resolution|INVALID                     |MOVED

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|https://bugzilla.kernel.org |https://sourceware.org/bugz
                   |/show_bug.cgi?id=207187     |illa/show_bug.cgi?id=25810

--- Comment #15 from H.J. Lu <hjl.tools at gmail dot com> ---
I will fix it in glibc:

https://sourceware.org/bugzilla/show_bug.cgi?id=25810

[Bug target/94541] [8/9/10 Regression] -mx32 gcc produces wrong code passing structs by value

[ibuclaw at gdcproject dot org]

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

--- Comment #16 from Iain Buclaw <ibuclaw at gdcproject dot org> ---
(In reply to H.J. Lu from comment #15)
> I will fix it in glibc:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=25810

Thanks, that certainly explains why I couldn't get it down any further.