Re: [PATCH] [Bug target/35504] incorrect code generated on i386 for C++ multiple inheritance, large return structures and regparm or fastcall calling conventions

Re: [PATCH] [Bug target/35504] incorrect code generated on i386 for C++ multiple inheritance, large return structures and regparm or fastcall calling conventions

[Uros Bizjak]

Hello!

> This is a proposed patch for bug 35504. The testcase is in bugzilla.

Huh, the attachment is scrambled. See [1].

Please add the testcase to the patch. Also, send a ChangeLog entry for
gcc/ChangeLog and gcc/patches/ChangeLog, separate from the patch. It
would be nice, if you describe the rationale for the thange together
with the patch submission (Your comment #0 from PR will do). Please
read http://gcc.gnu.org/contribute.html (Submitting Patches section in
particular) for further instructions.

You didn't say how you test the patch. At least you should bootstrap
gcc with your patch and run the testsuite for defult languages. I can
help you with this step if you are unable to do it.

[1] http://gcc.gnu.org/ml/gcc-patches/2008-03/msg00610.html

Thanks for your contribution,
Uros.

Re: [PATCH] [Bug target/35504] incorrect code generated on i386 for C++ multiple inheritance, large return structures and regparm or fastcall calling conventions

[Mikulas Patocka]

Hi

> Hello!
> 
> > This is a proposed patch for bug 35504. The testcase is in bugzilla.
> 
> Huh, the attachment is scrambled. See [1].

Hmm, I see, I don't know why. So use the attachment in bugzilla, there is 
also a testcase.

I am not a gcc developer, you can take it and commit it to gcc.

> Please add the testcase to the patch. Also, send a ChangeLog entry for
> gcc/ChangeLog and gcc/patches/ChangeLog, separate from the patch. It
> would be nice, if you describe the rationale for the thange together
> with the patch submission (Your comment #0 from PR will do). Please
> read http://gcc.gnu.org/contribute.html (Submitting Patches section in
> particular) for further instructions.
> 
> You didn't say how you test the patch. At least you should bootstrap
> gcc with your patch and run the testsuite for defult languages. I can
> help you with this step if you are unable to do it.

Bootstrap doesn't use non-standard calling conventions in C++, so the 
patch should have no effect on it. The testcase is in bugzilla. You can 
add it to your regression testsuite.

> [1] http://gcc.gnu.org/ml/gcc-patches/2008-03/msg00610.html
> 
> Thanks for your contribution,
> Uros.

Mikulas

[PATCH, i386]: Fix target/35504: incorrect code generated on i386 for C++ multiple inheritance, large return structures and regparm or fastcall calling conventions

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

Mikulas Patocka wrote:

> Hmm, I see, I don't know why. So use the attachment in bugzilla, there is 
> also a testcase.
>
> I am not a gcc developer, you can take it and commit it to gcc.
>   

Attached patch correctly calculates location of "this" pointer when 
regparm or fastcall is in effect.

2008-03-18  Mikulas Patocka  <mikulas@artax.karlin.mff.cuni.cz>

        PR target/35504
        * config/i386/i386.c (x86_this_parameter): Calculate correct "this"
        pointer location when "regparm = N" or "fastcall" is in effect.

testsuite/ChangeLog

2008-03-18  Mikulas Patocka  <mikulas@artax.karlin.mff.cuni.cz>

        PR target/35504
        * g++.dg/other/pr35504.C: New test.


The patch was bootstrapped and regression tested on x86_64-pc-linux-gnu 
{,-m32}.

Patch is committed to SVN, it will be committed to 4.3 and 4.2 in a 
couple of days.

Thanks,
Uros.


[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 4311 bytes --]

Index: testsuite/g++.dg/other/pr35504.C
===================================================================
--- testsuite/g++.dg/other/pr35504.C	(revision 0)
+++ testsuite/g++.dg/other/pr35504.C	(revision 0)
@@ -0,0 +1,147 @@
+// { dg-do run { target { { i?86-*-* x86_64-*-* } && ilp32 } } }
+
+#define ATTR0 __attribute__((__regparm__(0)))
+#define ATTR1 __attribute__((__regparm__(1)))
+#define ATTR2 __attribute__((__regparm__(2)))
+#define ATTR3 __attribute__((__regparm__(3)))
+#define ATTR4 __attribute__((__fastcall__))
+#define ATTR5 __attribute__((__stdcall__))
+#define ATTR6 __attribute__((__cdecl__))
+#define ATTR7
+
+extern "C" void abort (void);
+
+struct long_struct
+{
+  int a[3];
+};
+
+struct long_struct ret;
+
+class c3 *this3;
+
+class c1
+{
+  int val1;
+public:
+    virtual void foo () { }
+};
+
+class c2
+{
+public:
+  virtual ATTR0 struct long_struct method0 ()
+  {
+    return ret;
+  }
+
+  virtual ATTR1 struct long_struct method1 ()
+  {
+    return ret;
+  }
+
+  virtual ATTR2 struct long_struct method2 ()
+  {
+    return ret;
+  }
+
+  virtual ATTR3 struct long_struct method3 ()
+  {
+    return ret;
+  }
+
+  virtual ATTR4 struct long_struct method4 ()
+  {
+    return ret;
+  }
+
+  virtual ATTR5 struct long_struct method5 ()
+  {
+    return ret;
+  }
+
+  virtual ATTR6 struct long_struct method6 ()
+  {
+    return ret;
+  }
+
+  virtual ATTR7 struct long_struct method7 ()
+  {
+    return ret;
+  }
+};
+
+class c3:c1, public c2
+{
+public:
+  c3 ()
+  {
+    this3 = this;
+  }
+
+  struct long_struct check_this (int a)
+  {
+    if (this3 != this)
+      abort ();
+
+    return ret;
+  }
+
+  virtual ATTR0 struct long_struct method0 ()
+  {
+    return check_this (0);
+  }
+
+  virtual ATTR1 struct long_struct method1 ()
+  {
+    return check_this (1);
+  }
+
+  virtual ATTR2 struct long_struct method2 ()
+  {
+    return check_this (2);
+  }
+
+  virtual ATTR3 struct long_struct method3 ()
+  {
+    return check_this (3);
+  }
+
+  virtual ATTR4 struct long_struct method4 ()
+  {
+    return check_this (4);
+  }
+
+  virtual ATTR5 struct long_struct method5 ()
+  {
+    return check_this (5);
+  }
+
+  virtual ATTR6 struct long_struct method6 ()
+  {
+    return check_this (6);
+  }
+
+  virtual ATTR7 struct long_struct method7 ()
+  {
+    return check_this (7);
+  }
+};
+
+class c3 c3_instance;
+class c2 *c2_ptr = &c3_instance;
+
+int
+main ()
+{
+  c2_ptr->method0 ();
+  c2_ptr->method1 ();
+  c2_ptr->method2 ();
+  c2_ptr->method3 ();
+  c2_ptr->method4 ();
+  c2_ptr->method5 ();
+  c2_ptr->method6 ();
+  c2_ptr->method7 ();
+
+  return 0;
+}
Index: stor-layout.c
===================================================================
--- stor-layout.c	(revision 133318)
+++ stor-layout.c	(working copy)
@@ -699,6 +699,9 @@ update_alignment_for_field (record_layou
 		 && ! integer_zerop (DECL_SIZE (rli->prev_field)))))
 	{
 	  unsigned int type_align = TYPE_ALIGN (type);
+	  unsigned int type_size
+	    = tree_low_cst (TYPE_SIZE (type), 1);
+	  type_align = MAX (type_align, type_size);
 	  type_align = MAX (type_align, desired_align);
 	  if (maximum_field_alignment != 0)
 	    type_align = MIN (type_align, maximum_field_alignment);
Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 133318)
+++ config/i386/i386.c	(working copy)
@@ -22698,6 +22698,7 @@ x86_this_parameter (tree function)
 {
   tree type = TREE_TYPE (function);
   bool aggr = aggregate_value_p (TREE_TYPE (type), type) != 0;
+  int nregs;
 
   if (TARGET_64BIT)
     {
@@ -22710,11 +22711,25 @@ x86_this_parameter (tree function)
       return gen_rtx_REG (DImode, parm_regs[aggr]);
     }
 
-  if (ix86_function_regparm (type, function) > 0 && !stdarg_p (type))
+  nregs = ix86_function_regparm (type, function);
+
+  if (nregs > 0 && !stdarg_p (type))
     {
-      int regno = AX_REG;
+      int regno;
+
       if (lookup_attribute ("fastcall", TYPE_ATTRIBUTES (type)))
-	regno = CX_REG;
+	regno = aggr ? DX_REG : CX_REG;
+      else
+        {
+	  regno = AX_REG;
+	  if (aggr)
+	    {
+	      regno = DX_REG;
+	      if (nregs == 1)
+		return gen_rtx_MEM (SImode,
+				    plus_constant (stack_pointer_rtx, 4));
+	    }
+	}
       return gen_rtx_REG (SImode, regno);
     }
 

[PATCH] [Bug target/35504] incorrect code generated on i386 for C++ multiple inheritance, large return structures and regparm or fastcall calling conventions

[Mikulas Patocka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 504 bytes --]

Hi

This is a proposed patch for bug 35504. The testcase is in bugzilla.

Mikulas

> ------- Comment #3 from rguenth at gcc dot gnu dot org  2008-03-08 10:58 -------
> Interesting ;)
> 
> Please send patches to gcc-patches@gcc.gnu.org indicating how you tested it.
>            Keywords|                            |ABI, wrong-code
> 
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35504
> 
> ------- You are receiving this mail because: -------
> You reported the bug, or are watching the reporter.
> 

[-- Attachment #2: c++-thunks.patch --]
[-- Type: TEXT/x-diff, Size: 370 bytes --]

\x1f‹\b\0\0\0\0\0\0\x03QŰn‚0\x18ž–§ř慁`ľâaˆqĂMłxa˛¨KśŤ†!\x10"\x02’¸,žű
ŹnŃ,Y/ú÷đú—\x10‚Ŕuťn\x12űaĐ\rű樚:.k\x18”š„ö	5A‡–1´č°Cĺ€N{”*şŽ˙Áż ÷\rŤ\x7f{Eˇm\x10Ă\x18\r\x06í1ôzŃ^[Ăś\x15(\0B\x1fjx4GĚ/b—‡IĚ2/Hě\0•\x7f¤^^[ň\Ă\x1d(Z-Üä|çd\x01Kk„Ś•2Ŕ§BŞŠ0ć\x10\x1aq‚)fŻl˝xš(úĹŐ¤ćÔöQ’ě‹”9œgá{Á=¨MßÉšëDQłíŰó‚ÍśŰőňáeťŘHSM!\réň(]Î'N\x10d¸Çźş€őƒ¨]˝(÷äşLŽ7p\x1dšQ§+Ľ´j'‘ĺčů/´|Ď?Ű9˘W
‹ĐźČb\x04ž\0ó#[-VP7ËC˛\x13đ4*r&>>çŽč*Šťgi"\x1aée%şŚIű“¨'ŮŮ\vŐ˛\x0fgŐ*˝öý	‚Ą|\x01AX”„Ł\x02\0\0