diff mbox

[MIPS] Fix PR target/68273, passing args in wrong regs

Message ID 1454538638.3232.125.camel@ubuntu-sellcey
State New
Headers show

Commit Message

Steve Ellcey Feb. 3, 2016, 10:30 p.m. UTC 
Here is a new patch for PR target/68273.  It makes the GCC calling conventions
compatible with LLVM so that the two agree with each other in all the cases
I could think of testing and it fixes the reported defect.

I couldn't get the GCC compatibility test to work (see
https://gcc.gnu.org/ml/gcc/2016-02/msg00017.html) so I wasn't able to use
that for compatibility testing, instead I hand examined routines with various
argument types (structures, ints, complex, etc) to see what registers 
GCC and LLVM were accessing.

This change does introduce an ABI/compatibility change with GCC itself and
that is obviously a concern.  Basically, any type that is passed by value
and has an external alignment applied to it may get passed in different
registers because we strip off that alignment (via TYPE_MAIN_VARIANT) before
determining the alignment of the variable.

If we don't want to break the GCC compatibility then we will continue to have an
incompatibility with LLVM and we will need to find another way to deal
with the aligned int variable that SRA is creating and passing to a function
that expects a 'normal' integer.

One thought I had was that we could compare TYPE_ALIGN (type) and
TYPE_ALIGN (TYPE_MAIN_VARIANT (type) and if they are different, issue
a warning during compilation about a possible incompatibility with
older objects.

Steve Ellcey
sellcey@imgtec.com


2016-02-03  Steve Ellcey  <sellcey@imgtec.com>

	PR target/68273
	* config/mips/mips.c (mips_function_arg_boundary): Fix argument
	alignment.

Comments

Mike Stump Feb. 4, 2016, 12:25 a.m. UTC | #1 
On Feb 3, 2016, at 2:30 PM, Steve Ellcey <sellcey@imgtec.com> wrote:
> Here is a new patch for PR target/68273.

So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc, tilegx, tilepro or xtensa.

:-(  That’s one of the problems by having each port copy and paste swaths of code from other ports to express the same thing instead of ports sharing just one copy of code.  My port is also broken in the same way (currently).

I’m curious why the caller of the hook can’t grab the main variant, if it wants.  If it did this, then all the ports are fixed wrt this issue.
Eric Botcazou Feb. 4, 2016, 11:09 a.m. UTC | #2 
> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
> tilegx, tilepro or xtensa.
> :-(  That’s one of the problems by having each port copy and paste swaths of
> :code from other ports to express the same thing instead of ports sharing
> :just one copy of code.  My port is also broken in the same way
> :(currently).

Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of 
the compatibility with LLVM has IMO little merit since it's a GCC extension.

> I’m curious why the caller of the hook can’t grab the main variant, if it
> wants.  If it did this, then all the ports are fixed wrt this issue.

Or just fix PRE wrt the alignment discrepancy, which looks a lot safer to me.
It's not because this works on x86 that this is necessarily correct for all 
the other architectures, especially the strict-alignment architectures.
Richard Biener Feb. 4, 2016, 12:49 p.m. UTC | #3 
On Thu, Feb 4, 2016 at 12:09 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
>> tilegx, tilepro or xtensa.
>> :-(  That’s one of the problems by having each port copy and paste swaths of
>> :code from other ports to express the same thing instead of ports sharing
>> :just one copy of code.  My port is also broken in the same way
>> :(currently).
>
> Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of
> the compatibility with LLVM has IMO little merit since it's a GCC extension.

True, but the compiler bug is in the backends - it just now gets
exposed more easily
(read: w/o user intervention).

>> I’m curious why the caller of the hook can’t grab the main variant, if it
>> wants.  If it did this, then all the ports are fixed wrt this issue.
>
> Or just fix PRE wrt the alignment discrepancy, which looks a lot safer to me.
> It's not because this works on x86 that this is necessarily correct for all
> the other architectures, especially the strict-alignment architectures.

Doesn't "fix" the same issue popping up in other passes.

Richard.

> --
> Eric Botcazou
Steve Ellcey Feb. 4, 2016, 6:26 p.m. UTC | #4 
On Thu, 2016-02-04 at 12:09 +0100, Eric Botcazou wrote:
> > So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
> > tilegx, tilepro or xtensa.
> > :-(  That’s one of the problems by having each port copy and paste swaths of
> > :code from other ports to express the same thing instead of ports sharing
> > :just one copy of code.  My port is also broken in the same way
> > :(currently).
> 
> Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of 
> the compatibility with LLVM has IMO little merit since it's a GCC extension.

But it is a GCC extension that is implemented in LLVM.  It's just
implemented differently there.  We either have to change GCC, change
LLVM, or live with the difference.

In any of these cases I wonder if we should change GCC so that the code
below generates warnings.  If the two types are going to get passed
differently we shouldn't allow calls with one type to call a function
expecting the other type to happen with no warning or error.  Currently
this code does not warn even with -Wall.


typedef int alignedint __attribute__((aligned(8)));
extern void foo (alignedint a);
extern void bar (int);

int a;
alignedint b;

int main()
{
        foo(a);
        bar(b);
}


Steve Ellcey
sellcey@imgtec.com
Richard Sandiford Feb. 4, 2016, 8:28 p.m. UTC | #5 
Richard Biener <richard.guenther@gmail.com> writes:
> On Thu, Feb 4, 2016 at 12:09 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000, rx, sparc,
>>> tilegx, tilepro or xtensa.
>>> :-(  That’s one of the problems by having each port copy and paste swaths of
>>> :code from other ports to express the same thing instead of ports sharing
>>> :just one copy of code.  My port is also broken in the same way
>>> :(currently).
>>
>> Yes, fixing a compiler bug by changing the ABI is a no-no, and the argument of
>> the compatibility with LLVM has IMO little merit since it's a GCC extension.
>
> True, but the compiler bug is in the backends - it just now gets
> exposed more easily
> (read: w/o user intervention).

I'm still not convinced it's a backend/target bug though.  It seems
like a similar situation to floats being promoted to doubles and
chars to ints when passed to unprototyped functions or varargs.
In those situations it's up to the hook to decide what type the
value actually gets passed as.  Passing an overaligned int as a
plain int is pretty similar and isn't something that each target
should need to check individually.

Thanks,
Richard
Richard Biener Feb. 4, 2016, 10:04 p.m. UTC | #6 
On February 4, 2016 9:28:22 PM GMT+01:00, Richard Sandiford <rdsandiford@googlemail.com> wrote:
>Richard Biener <richard.guenther@gmail.com> writes:
>> On Thu, Feb 4, 2016 at 12:09 PM, Eric Botcazou
><ebotcazou@adacore.com> wrote:
>>>> So this doesn’t fix aarch64, c6x, epiphany, ia64, iq2000, rs6000,
>rx, sparc,
>>>> tilegx, tilepro or xtensa.
>>>> :-(  That’s one of the problems by having each port copy and paste
>swaths of
>>>> :code from other ports to express the same thing instead of ports
>sharing
>>>> :just one copy of code.  My port is also broken in the same way
>>>> :(currently).
>>>
>>> Yes, fixing a compiler bug by changing the ABI is a no-no, and the
>argument of
>>> the compatibility with LLVM has IMO little merit since it's a GCC
>extension.
>>
>> True, but the compiler bug is in the backends - it just now gets
>> exposed more easily
>> (read: w/o user intervention).
>
>I'm still not convinced it's a backend/target bug though.  It seems
>like a similar situation to floats being promoted to doubles and
>chars to ints when passed to unprototyped functions or varargs.
>In those situations it's up to the hook to decide what type the
>value actually gets passed as.  Passing an overaligned int as a
>plain int is pretty similar and isn't something that each target
>should need to check individually.

I agree here.  BUT we are still effectively changing the ABI of each affected target then without actually knowing we do.
I'd happily do such change in the middle-end if we identified all affected targets and verified the changes impact.

That's a lot of work and thus having active targets follow the "arm way" of fixing the issue in the target is very reasonable in my opinion and the only thing I'd accept at this stage.

Richard.

>Thanks,
>Richard
Eric Botcazou Feb. 5, 2016, 11:42 a.m. UTC | #7 
> True, but the compiler bug is in the backends - it just now gets
> exposed more easily (read: w/o user intervention).

As far as I can see there are no problematic types in the source code, i.e. 
it's plain ANSI C, so we shouldn't be creating types with non-canonical 
alignment from that.

> Doesn't "fix" the same issue popping up in other passes.

Which pass does the alignment promotion?  Maybe it's the one to be fixed.
diff mbox

Patch

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 697abc2..4aa215f 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -5644,7 +5644,10 @@  mips_function_arg_boundary (machine_mode mode, const_tree type)
 {
   unsigned int alignment;
 
-  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
+  alignment = type
+		? TYPE_ALIGN (TYPE_MAIN_VARIANT (type))
+		: GET_MODE_ALIGNMENT (mode);
+
   if (alignment < PARM_BOUNDARY)
     alignment = PARM_BOUNDARY;
   if (alignment > STACK_BOUNDARY)




2016-02-03  Steve Ellcey  <sellcey@imgtec.com>

	PR target/68273
	* gcc.c-torture/execute/pr68273-1.c: New test.
	* gcc.c-torture/execute/pr68273-2.c: New test.


diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
index e69de29..3ce07c6 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-1.c
@@ -0,0 +1,74 @@ 
+/* Make sure that the alignment attribute on an argument passed by
+   value does not affect the calling convention and what registers
+   arguments are passed in.  */
+
+extern void exit (int);
+extern void abort (void);
+
+typedef int alignedint __attribute__((aligned(8)));
+
+int  __attribute__((noinline))
+foo1 (int a, alignedint b)
+{ return a + b; }
+
+int __attribute__((noinline))
+foo2 (int a, int b)
+{
+  return a + b;
+}
+
+int __attribute__((noinline))
+bar1 (alignedint x)
+{
+  return foo1 (1, x);
+}
+
+int __attribute__((noinline))
+bar2 (alignedint x)
+{
+  return foo1 (1, (alignedint) 99);
+}
+
+int __attribute__((noinline))
+bar3 (alignedint x)
+{
+  return foo1 (1, x + (alignedint) 1);
+}
+
+alignedint q = 77;
+
+int __attribute__((noinline))
+bar4 (alignedint x)
+{
+  return foo1 (1, q);
+}
+
+
+int __attribute__((noinline))
+bar5 (alignedint x)
+{
+  return foo2 (1, x);
+}
+
+int __attribute__((noinline))
+use_arg_regs (int i, int j, int k)
+{
+  return i+j-k;
+}
+
+int main()
+{
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (foo1 (19,13) != 32) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar1 (-33) != -32) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar2 (1) != 100) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar3 (17) != 19) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar4 (-33) != 78) abort ();
+   if (use_arg_regs (999, 999, 999) != 999) abort ();
+   if (bar5 (-84) != -83) abort ();
+   exit (0);
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c b/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
index e69de29..1661be9 100644
--- a/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68273-2.c
@@ -0,0 +1,109 @@ 
+/* Make sure that the alignment attribute on an argument passed by
+   value does not affect the calling convention and what registers
+   arguments are passed in.  */
+
+extern void exit (int);
+extern void abort (void);
+
+typedef struct s {
+	char c;
+	char d;
+} t1;
+typedef t1 t1_aligned8  __attribute__((aligned(8)));
+typedef t1 t1_aligned16 __attribute__((aligned(16)));
+typedef t1 t1_aligned32 __attribute__((aligned(32)));
+
+int bar1(int a, t1 b)
+{
+	return a + b.c + b.d;
+}
+
+int bar2(int a, t1_aligned8 b)
+{
+	return a + b.c + b.d;
+}
+
+int bar3(int a, t1_aligned16 b)
+{
+	return a + b.c + b.d;
+}
+
+int bar4(int a, t1_aligned32 b)
+{
+	return a + b.c + b.d;
+}
+
+#define FOODEF(n,m,type) \
+int __attribute__((noinline)) \
+foo##n (int i, type b) \
+  { \
+    return bar##m (i, b); \
+  }
+
+FOODEF(1,  1, t1)
+FOODEF(2,  1, t1_aligned8)
+FOODEF(3,  1, t1_aligned16)
+FOODEF(4,  1, t1_aligned32)
+FOODEF(5,  2, t1)
+FOODEF(6,  2, t1_aligned8)
+FOODEF(7,  2, t1_aligned16)
+FOODEF(8,  2, t1_aligned32)
+FOODEF(9,  3, t1)
+FOODEF(10, 3, t1_aligned8)
+FOODEF(11, 3, t1_aligned16)
+FOODEF(12, 3, t1_aligned32)
+FOODEF(13, 4, t1)
+FOODEF(14, 4, t1_aligned8)
+FOODEF(15, 4, t1_aligned16)
+FOODEF(16, 4, t1_aligned32)
+
+int __attribute__((noinline))
+use_arg_regs (int i, int j, int k)
+{
+  return i+j-k;
+}
+
+#define FOOCALL(i) \
+  c = i*11 + 39; \
+  x1.c = i + 5; \
+  x1.d = i*2 + 3; \
+  x2.c = x1.c + 1; \
+  x2.d = x1.d + 1; \
+  x3.c = x2.c + 1; \
+  x3.d = x2.d + 1; \
+  x4.c = x3.c + 1; \
+  x4.d = x3.d + 1; \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x1) != c + x1.c + x1.d) abort (); \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x2) != c + x2.c + x2.d) abort (); \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x3) != c + x3.c + x3.d) abort (); \
+  if (use_arg_regs (999,999,999) != 999) abort (); \
+  if (foo##i (c, x4) != c + x4.c + x4.d) abort ();
+
+int main()
+{
+  int c;
+  t1 x1;
+  t1_aligned8 x2;
+  t1_aligned16 x3;
+  t1_aligned32 x4;
+  FOOCALL(1);
+  FOOCALL(2);
+  FOOCALL(3);
+  FOOCALL(4);
+  FOOCALL(5);
+  FOOCALL(6);
+  FOOCALL(7);
+  FOOCALL(8);
+  FOOCALL(9);
+  FOOCALL(10);
+  FOOCALL(11);
+  FOOCALL(12);
+  FOOCALL(13);
+  FOOCALL(14);
+  FOOCALL(15);
+  FOOCALL(16);
+  exit (0);
+}