Bug 68273 - [5 Regression] Wrong code on mips/mipsel due to (invalid?) peeking at alignments in function_arg.
Summary: [5 Regression] Wrong code on mips/mipsel due to (invalid?) peeking at alignme...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 5.2.1
: P2 normal
Target Milestone: 6.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ABI, wrong-code
Depends on:
Blocks:
 
Reported: 2015-11-10 14:46 UTC by Aurelien Jarno
Modified: 2017-10-10 14:56 UTC (History)
11 users (show)

See Also:
Host: mipsel-linux-gnu
Target: mipsel-linux-gnu
Build: mipsel-linux-gnu
Known to work: 6.2.0, 7.0
Known to fail: 6.1.0
Last reconfirmed: 2016-01-20 00:00:00


Attachments
preprocessed source (33.88 KB, text/plain)
2015-11-10 14:46 UTC, Aurelien Jarno
Details
Reduced testcase (443 bytes, text/plain)
2016-01-20 22:26 UTC, Jeffrey A. Law
Details
Testcase for the testsuite (578 bytes, text/x-csrc)
2016-02-05 17:45 UTC, Jeffrey A. Law
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Aurelien Jarno 2015-11-10 14:46:44 UTC
Created attachment 36676 [details]
preprocessed source

gsoap version 2.8.22 does not compile correctly on mips and mipsel with GCC 5.2.1 or trunk, while it compiles correctly on GCC 4.9. The resulting binary segfaults.

A bit of analysis shows this is due to wrong code optimization at -O2 level, and more specifically with the  -fipa-sra option, which calls functions with arguments in the wrong register.

I have not been able to get a simple testcase yet, but it is possible to reproduce the issue with the attached preprocessed code. The problem occurs in this function:

  case 34:
# 436 "soapcpp2_yacc.y"
    { if ((yyvsp[-1].rec).sto & Stypedef)
     { sprintf(errbuf, "invalid typedef qualifier for '%s'", (yyvsp[0].sym)->name);
    semwarn(errbuf);
     }
     printf("%p\n", (yyvsp[0].sym));
     p = enter(sp->table, (yyvsp[0].sym));
     p->info.typ = (yyvsp[-1].rec).typ;
     p->info.sto = (yyvsp[-1].rec).sto;
     p->info.hasval = False;
     p->info.offset = sp->offset;
     if (sp->grow)
    sp->offset += p->info.typ->width;
     else if (p->info.typ->width > sp->offset)
    sp->offset = p->info.typ->width;
     sp->entry = p;
   }
# 2290 "soapcpp2_yacc.c"
    break;

When compiled with -O2, GCC outputs the following corresponding code:

$L380:  
        lw      $25,%call16(__printf_chk)($28)
        lw      $21,%got(sp)($28)
        lui     $5,%hi($LC37)
        move    $6,$7
        addiu   $5,$5,%lo($LC37)
        sw      $7,13452($sp)
        .reloc  1f,R_MIPS_JALR,__printf_chk
1:      jalr    $25
        li      $4,1                    # 0x1

        lw      $28,88($sp)
        lw      $2,0($21)
        lw      $7,13452($sp)
        lw      $4,0($2)
        lw      $25,%call16(enter)($28)
        nop
        .reloc  1f,R_MIPS_JALR,enter
1:      jalr    $25
        move    $6,$7

Note how the second argument is loaded in $6 (ie a2) instead of $5 (ie a1) when calling enter.

When compiled with -O2 -fno-ipa-sra the correct register is used:

-O2 -fno-ipa-sra

$L387:  
        lw      $25,%call16(__printf_chk)($28)
        lw      $21,%got(sp)($28)
        lui     $5,%hi($LC37)
        move    $6,$7
        sw      $7,13500($sp)
        addiu   $5,$5,%lo($LC37)
        .reloc  1f,R_MIPS_JALR,__printf_chk
1:      jalr    $25
        li      $4,1                    # 0x1

        lw      $28,136($sp)
        lw      $2,0($21)
        lw      $7,13500($sp)
        lw      $4,0($2)
        lw      $25,%call16(enter)($28)
        nop
        .reloc  1f,R_MIPS_JALR,enter
1:      jalr    $25
        move    $5,$7

However it is first loaded to $7 for no obvious reason, especially this is not a saved register, so its value is lost after the call. I am note sure it is something related, but loading the value through this intermediate register is due to the use of -O2, this is not the case -O1:

$L370:  
        lw      $2,0($16)
        nop
        sw      $2,13476($sp)
        move    $6,$2
        lui     $5,%hi($LC37)
        addiu   $5,$5,%lo($LC37)
        li      $4,1                    # 0x1
        lw      $25,%call16(__printf_chk)($28)
        nop
        .reloc  1f,R_MIPS_JALR,__printf_chk
1:      jalr    $25
        nop

        lw      $28,136($sp)
        nop
        lw      $17,%got(sp)($28)
        nop
        lw      $2,0($17)
        lw      $5,13476($sp)
        lw      $4,0($2)
        lw      $25,%call16(enter)($28)
        nop
        .reloc  1f,R_MIPS_JALR,enter
1:      jalr    $25
        nop
Comment 1 Richard Biener 2015-12-04 10:46:22 UTC
GCC 5.3 is being released, adjusting target milestone.
Comment 2 Jeffrey A. Law 2016-01-20 19:51:42 UTC
My current theory is ipa-sra is increasing the alignment of an object during its transformations.  As a result, the mechanisms to determine which registers to use for argument passing believe the 2nd arg must be in an aligned register pair.
Comment 3 Jeffrey A. Law 2016-01-20 22:26:53 UTC
Created attachment 37414 [details]
Reduced testcase

Reduced testcase.  Compile with a mipsel-linux-gnu cross compiler with -O2.

It's easiest to see the problem looking at the .expand dump:

;; _17 = enter (_14, prephitmp_23);

(insn 81 80 82 (set (reg/f:SI 262)
        (symbol_ref:SI ("sp")  <var_decl 0x7f289897eab0 sp>)) j.c:58 -1
     (nil))

(insn 82 81 83 (set (reg/f:SI 261)
        (mem/f/c:SI (reg/f:SI 262) [10 sp+0 S4 A32])) j.c:58 -1
     (nil))

(insn 83 82 84 (set (reg:SI 263)
        (mem/f:SI (reg/f:SI 261) [12 sp.0_13->table+0 S4 A32])) j.c:58 -1
     (nil))

(insn 84 83 85 (set (reg:SI 6 $6)
        (reg/f:SI 255 [ prephitmp_23 ])) j.c:58 -1
     (nil))

(insn 85 84 86 (set (reg:SI 4 $4)
        (reg:SI 263)) j.c:58 -1
     (nil))

(call_insn 86 85 87 (parallel [
            (set (reg:SI 2 $2)
                (call (mem:SI (symbol_ref:SI ("enter") [flags 0x41]  <function_decl 0x7f2890d2ca80 enter>) [0 enter S4 A32])
                    (const_int 16 [0x10])))
            (clobber (reg:SI 31 $31))
        ]) j.c:58 -1
     (expr_list:REG_CALL_DECL (symbol_ref:SI ("enter") [flags 0x41]  <function_decl 0x7f2890d2ca80 enter>)
        (nil))
    (expr_list (use (reg:SI 79 $fakec))
        (expr_list:SI (use (reg:SI 4 $4))
            (expr_list:SI (use (reg:SI 6 $6))
                (nil)))))


Note carefully the 2nd argument to the enter call is loaded into $6.  It should have been loaded into $5.  AFAICT, SRA is mucking around with the alignment of the object, which in turn causes the MIPS backend to want to pass the object in an aligned register pair.
Comment 4 Jeffrey A. Law 2016-01-21 06:44:23 UTC
It's starting to look like a problem in tree-ssa-pre.c

Essentially we have these these key statements in various blocks:

  _10 = yyvsp_1->sym;
  _15 = yyvsp_1->sym;
  _17 = enter (_14, _15);
  _22 = MEM[(union YYSTYPE *)yyvsp_1];

Respectively _10 and _22 have the types:

(gdb) p debug_tree (cfun->gimple_df->ssa_names.m_vecdata[10]->typed.type)
 <pointer_type 0x7ffff03c6498
    type <record_type 0x7ffff0384348 Symbol type_0 SI
        size <integer_cst 0x7ffff02dd5e8 constant 32>
        unit size <integer_cst 0x7ffff02dd600 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0x7ffff03842a0
        fields <field_decl 0x7ffff02ee8e8 name type <pointer_type 0x7ffff02ebe70>
            unsigned SI file j.c line 4 col 9 size <integer_cst 0x7ffff02dd5e8 32> unit size <integer_cst 0x7ffff02dd600 4>
            align 32 offset_align 64
            offset <integer_cst 0x7ffff02dd618 constant 0>
            bit offset <integer_cst 0x7ffff02dd678 constant 0> context <record_type 0x7ffff03842a0 Symbol>> context <translation_unit_decl 0x7ffff03c80f0 D.1453>
        pointer_to_this <pointer_type 0x7ffff03c6498> chain <type_decl 0x7ffff02ee850 D.1407>>
    sizes-gimplified public unsigned SI size <integer_cst 0x7ffff02dd5e8 32> unit size <integer_cst 0x7ffff02dd600 4>
    align 32 symtab 0 alias set -1 canonical type 0x7ffff03c6540>


(gdb) p debug_tree (cfun->gimple_df->ssa_names.m_vecdata[22]->typed.type)
 <pointer_type 0x7ffff03cd5e8
    type <record_type 0x7ffff0384498 Tnode type_0 BLK
        size <integer_cst 0x7ffff02dd678 constant 0>
        unit size <integer_cst 0x7ffff02dd618 constant 0>
        align 8 symtab 0 alias set -1 canonical type 0x7ffff03843f0 context <translation_unit_decl 0x7ffff03c80f0 D.1453>
        pointer_to_this <pointer_type 0x7ffff0384c78> chain <type_decl 0x7ffff02eea18 D.1410>>
    sizes-gimplified unsigned SI
    size <integer_cst 0x7ffff02dd5e8 type <integer_type 0x7ffff02d6150 bitsizetype> constant 32>
    unit size <integer_cst 0x7ffff02dd600 type <integer_type 0x7ffff02d60a8 sizetype> constant 4>
    align 64 symtab 0 alias set -1 canonical type 0x7ffff0384d20>

The type of _15 is the same as the type of _10.

For reference, we're referring to instances of this structure:

union YYSTYPE
{
  Symbol *sym;
  Node rec;
};


The key thing to note from the types is they have different alignments.

Anyway, PRE detects the redundant memory reference and creates:

  # prephitmp_23 = PHI <pretmp_26(9), _10(5)>


Where _23 and _26 will have the type with the 64 bit alignment.

That node feeds this statement:

  _17 = enter (_14, prephitmp_23);

Yow!  Remember that statement previously looked like:

  _17 = enter (_14, _15);

So the alignment associated with the second argument to "enter" has changed from 32 to 64.  That in turn may change how the argument gets passed on some ports (mips, epiphany, maybe others).   In this specific instance is causes the MIPS backend to align the parameter, passing it in $6 rather than where the callee expects it ($5), which in turn results in gsoap being mis-compiled on MIPS.

Richi -- you know the PRE code better than anyone that's currently active.  Thoughts?
Comment 5 rguenther@suse.de 2016-01-21 08:11:58 UTC
On Thu, 21 Jan 2016, law at redhat dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68273
> 
> Jeffrey A. Law <law at redhat dot com> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>            Priority|P3                          |P2
>            Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
> 
> --- Comment #4 from Jeffrey A. Law <law at redhat dot com> ---
> It's starting to look like a problem in tree-ssa-pre.c
> 
> Essentially we have these these key statements in various blocks:
> 
>   _10 = yyvsp_1->sym;
>   _15 = yyvsp_1->sym;
>   _17 = enter (_14, _15);
>   _22 = MEM[(union YYSTYPE *)yyvsp_1];
> 
> Respectively _10 and _22 have the types:
> 
> (gdb) p debug_tree (cfun->gimple_df->ssa_names.m_vecdata[10]->typed.type)
>  <pointer_type 0x7ffff03c6498
>     type <record_type 0x7ffff0384348 Symbol type_0 SI
>         size <integer_cst 0x7ffff02dd5e8 constant 32>
>         unit size <integer_cst 0x7ffff02dd600 constant 4>
>         align 32 symtab 0 alias set -1 canonical type 0x7ffff03842a0
>         fields <field_decl 0x7ffff02ee8e8 name type <pointer_type
> 0x7ffff02ebe70>
>             unsigned SI file j.c line 4 col 9 size <integer_cst 0x7ffff02dd5e8
> 32> unit size <integer_cst 0x7ffff02dd600 4>
>             align 32 offset_align 64
>             offset <integer_cst 0x7ffff02dd618 constant 0>
>             bit offset <integer_cst 0x7ffff02dd678 constant 0> context
> <record_type 0x7ffff03842a0 Symbol>> context <translation_unit_decl
> 0x7ffff03c80f0 D.1453>
>         pointer_to_this <pointer_type 0x7ffff03c6498> chain <type_decl
> 0x7ffff02ee850 D.1407>>
>     sizes-gimplified public unsigned SI size <integer_cst 0x7ffff02dd5e8 32>
> unit size <integer_cst 0x7ffff02dd600 4>
>     align 32 symtab 0 alias set -1 canonical type 0x7ffff03c6540>
> 
> 
> (gdb) p debug_tree (cfun->gimple_df->ssa_names.m_vecdata[22]->typed.type)
>  <pointer_type 0x7ffff03cd5e8
>     type <record_type 0x7ffff0384498 Tnode type_0 BLK
>         size <integer_cst 0x7ffff02dd678 constant 0>
>         unit size <integer_cst 0x7ffff02dd618 constant 0>
>         align 8 symtab 0 alias set -1 canonical type 0x7ffff03843f0 context
> <translation_unit_decl 0x7ffff03c80f0 D.1453>
>         pointer_to_this <pointer_type 0x7ffff0384c78> chain <type_decl
> 0x7ffff02eea18 D.1410>>
>     sizes-gimplified unsigned SI
>     size <integer_cst 0x7ffff02dd5e8 type <integer_type 0x7ffff02d6150
> bitsizetype> constant 32>
>     unit size <integer_cst 0x7ffff02dd600 type <integer_type 0x7ffff02d60a8
> sizetype> constant 4>
>     align 64 symtab 0 alias set -1 canonical type 0x7ffff0384d20>
> 
> The type of _15 is the same as the type of _10.
> 
> For reference, we're referring to instances of this structure:
> 
> union YYSTYPE
> {
>   Symbol *sym;
>   Node rec;
> };
> 
> 
> The key thing to note from the types is they have different alignments.
> 
> Anyway, PRE detects the redundant memory reference and creates:
> 
>   # prephitmp_23 = PHI <pretmp_26(9), _10(5)>
> 
> 
> Where _23 and _26 will have the type with the 64 bit alignment.
> 
> That node feeds this statement:
> 
>   _17 = enter (_14, prephitmp_23);
> 
> Yow!  Remember that statement previously looked like:
> 
>   _17 = enter (_14, _15);
> 
> So the alignment associated with the second argument to "enter" has changed
> from 32 to 64.  That in turn may change how the argument gets passed on some
> ports (mips, epiphany, maybe others).   In this specific instance is causes the
> MIPS backend to align the parameter, passing it in $6 rather than where the
> callee expects it ($5), which in turn results in gsoap being mis-compiled on
> MIPS.
> 
> Richi -- you know the PRE code better than anyone that's currently active. 
> Thoughts?

Sounds like the same mistake the ARM port had - deriving calling 
convention from type alignment of (r)_values_.  An rvalue doesn't
have alignment.

Value-numbering (and PRE) unify based on value equivalence, it
doesn't matter whether the type of the value stored in register
_23 or _15 are different, all it matters is if they are the
same, say 42 (with "alignment 8") or 42 (with "alignment 16").

Values don't have alignment.

The ports need to be fixed similar to how ARM AACPS was fixed.
See

2015-07-06  Alan Lawrence  <alan.lawrence@arm.com>

        Backport from mainline r225465
        2015-07-06  Alan Lawrence  <alan.lawrence@arm.com>

        PR target/65956
        * config/arm/arm.c (arm_needs_doubleword_align): Drop any outer
        alignment attribute, exploring one level down for records and 
arrays.
Comment 6 Richard Biener 2016-01-21 08:19:31 UTC
So in

/* Implement TARGET_FUNCTION_ARG_BOUNDARY.  Every parameter gets at
   least PARM_BOUNDARY bits of alignment, but will be given anything up
   to STACK_BOUNDARY bits if the type requires it.  */

static unsigned int
mips_function_arg_boundary (machine_mode mode, const_tree type)
{
  unsigned int alignment;

  alignment = type ? TYPE_ALIGN (type) : GET_MODE_ALIGNMENT (mode);
  if (alignment < PARM_BOUNDARY)
    alignment = PARM_BOUNDARY;
  if (alignment > STACK_BOUNDARY)
    alignment = STACK_BOUNDARY;
  return alignment;
}

drop the TYPE_ALIGN use if mode is not BLKmode (and assert it is not VOIDmode?
I wonder what we get for integer constants here).

Similar to arm I'm curious what the ABI documentation says for

typedef int myint __attribute__((align(1)));

int foo (myint, myint);

calling convention vs. any other alignment of the args and whether it is
really intended to make those incompatible.

With C11 aligning a type is no longer a GCC extension(?), for previous
C versions either GCC shouldn't change the ABI or document the ABI
extension.

But I really doubt this is an intended behavior.

I see another use in va-arg processing (and va-args are usually the argument
why port maintainers say they need to look at the types of the actual
passed values - but I think the mode and its alignment is what matters)

  /* If the actual alignment is less than the alignment of the type,
     adjust the type accordingly so that we don't assume strict alignment
     when dereferencing the pointer.  */
  boundary *= BITS_PER_UNIT;
  if (boundary < TYPE_ALIGN (type))
    {
      type = build_variant_type_copy (type);
      TYPE_ALIGN (type) = boundary;
    }

Aggregates are tricky (but again, don't look at alignment of actual passed
values!  Look at most at the type alignment of the prototype arg type!
[or for values not passed by reference simply ignore it and use the alignment
of the main variant]).
Comment 7 Richard Biener 2016-01-21 08:24:01 UTC
Target bug, not mine.  Might want to review the lengthy discussion about the similar ARM issue on the mailinglist last year (Jan-Apr).
Comment 8 Richard Biener 2016-01-21 14:12:39 UTC
Also consider


typedef int myint __attribute__((align(1)));
void foo (int, int);

void bar()
{
  foo ((myint)1, (myint)2);
  foo (1, 2);
}
Comment 9 jsm-csl@polyomino.org.uk 2016-01-21 18:24:57 UTC
On Thu, 21 Jan 2016, rguenth at gcc dot gnu.org wrote:

> With C11 aligning a type is no longer a GCC extension(?), for previous
> C versions either GCC shouldn't change the ABI or document the ABI
> extension.

C11 doesn't allow under-aligning a type and doesn't allow changing 
alignment of parameters.  "An alignment attribute shall not be specified 
in a declaration of a typedef, or a bit-field, or a function, or a 
parameter, or an object declared with the register storage-class 
specifier.".  (You're meant to be able to have structures with 
over-aligned elements, which of course could be passed as function 
arguments - each structure type would still have a fixed alignment - 
though in fact the syntax fails to allow for this; see DR#444.)
Comment 10 Jeffrey A. Law 2016-01-21 22:42:46 UTC
I understand everything you're saying Richi.  Note however that the MIPS backend explicitly wants to align things based on the object's alignment -- even scalars passed in registers:

/* Advance to an even register if the argument is doubleword-aligned.  */
  if (doubleword_aligned_p)
    info->reg_offset += info->reg_offset & 1;

Which can be tracked back to this commit:
commit 26bcab5a0015a5304899649081f7777d676996b8
Author: rsandifo <rsandifo@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Sat Sep 25 07:42:43 2004 +0000

        * config/mips/mips.h (struct mips_args): Clarify comments.
        * config/mips/mips.c (struct mips_arg_info): Likewise.
        (mips_arg_info): Don't allow fpr_p to affect the register or
        stack alignment.  Remove o64 silliness.
        (function_arg): Deal with the o32 float,float case specially.

Which is the end of a slew of fixes to this code -- enough that tracking down the original source of the aligned register stuff is painful at best.  

I'm nowhere near comfortable to take this further -- my MIPS knowledge is old and limited more to the ISA than the nitty gritty details of argument passing.

One could argue that this kind of alignment hackery is fundamentally broken and can't be baked into the ABI.  But the reality is this code has been in GCC for well over a decade, so it has effectively become part of the ABI.

Anyway, leaving to the MIPS maintainers to sort out.  It ought to be dramatically easier at this point.
Comment 11 Jakub Jelinek 2016-01-22 13:38:47 UTC
(In reply to Jeffrey A. Law from comment #10)
> I understand everything you're saying Richi.  Note however that the MIPS
> backend explicitly wants to align things based on the object's alignment --
> even scalars passed in registers:

And that is the bug, backend should never do that.
Alignment of a scalar object is a property of that object, but if you pass it by value, you just pass the value, so copy it to some other location.
Except for calls to unprototyped C (and Fortran) functions, one could argue that some ABI could be using the alignment of the argument type on the function definition (and require declarations to be matching).  But with calls to unprototyped functions this becomes major problem.
As for ARM, the questions are what the compiler will do say on:
typedef int alignedint __attribute__((aligned(8)));
int  __attribute__((weak)) foo (int a, alignedint b)
{ return b;}
void bar (alignedint x)
{
  foo (1, x);
}

void
bar2 (alignedint x)
{
  foo (1, (alignedint) 1);
}

void
bar3 (alignedint x)
{
  foo (1, x + (alignedint) 1);
}

alignedint q;

void
bar4 (alignedint x)
{
  foo (1, q);
}

extern int baz (int, int);

void
bar5 (alignedint x)
{
  baz (1, x);
}
Comment 12 Bernd Schmidt 2016-01-27 14:29:41 UTC
(In reply to Jeffrey A. Law from comment #10)
> /* Advance to an even register if the argument is doubleword-aligned.  */
>   if (doubleword_aligned_p)
>     info->reg_offset += info->reg_offset & 1;
> 
> Which can be tracked back to this commit:
> commit 26bcab5a0015a5304899649081f7777d676996b8
> Author: rsandifo <rsandifo@138bc75d-0d04-0410-961f-82ee72b054a4>
> Date:   Sat Sep 25 07:42:43 2004 +0000
> 
>         * config/mips/mips.h (struct mips_args): Clarify comments.
>         * config/mips/mips.c (struct mips_arg_info): Likewise.
>         (mips_arg_info): Don't allow fpr_p to affect the register or
>         stack alignment.  Remove o64 silliness.
>         (function_arg): Deal with the o32 float,float case specially.
> 

Cc'ing Richard in case he has input.
Comment 13 Steve Ellcey 2016-01-29 21:35:05 UTC
I have submitted a patch for this defect:

https://gcc.gnu.org/ml/gcc-patches/2016-01/msg02350.html
Comment 14 Richard Biener 2016-02-04 13:06:53 UTC
I am currently testing the following patch to eventually mitigate the issue
somewhat by forcing all "registers" to have non-qualified type (their
qualification does not matter nor does their alignment).  The create_tmp_reg
hunk is more risky (in case we have bogus callers ending up forcing the
result to memory).

It will hide the PRE bug (well, still if the user wrote a 64bit aligned
type and a 32bit aligned type PRE might change one for the other - in
fact it will even end up using a 32bit aligned type if the code consistently
uses 64bit aligned types).

But as said earlier, the GCC implemented ABI is simply seriously broken
if you consider the user can control "alignment" of (register-typed) values.

Index: gcc/gimple-expr.c
===================================================================
--- gcc/gimple-expr.c   (revision 233136)
+++ gcc/gimple-expr.c   (working copy)
@@ -484,9 +484,8 @@ create_tmp_var (tree type, const char *p
 tree
 create_tmp_reg (tree type, const char *prefix)
 {
-  tree tmp;
-
-  tmp = create_tmp_var (type, prefix);
+  gcc_assert (is_gimple_reg_type (type));
+  tree tmp = create_tmp_var (TYPE_MAIN_VARIANT (type), prefix);
   if (TREE_CODE (type) == COMPLEX_TYPE
       || TREE_CODE (type) == VECTOR_TYPE)
     DECL_GIMPLE_REG_P (tmp) = 1;
Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c (revision 233136)
+++ gcc/tree-ssanames.c (working copy)
@@ -286,7 +286,7 @@ make_ssa_name_fn (struct function *fn, t
 
   if (TYPE_P (var))
     {
-      TREE_TYPE (t) = var;
+      TREE_TYPE (t) = TYPE_MAIN_VARIANT (var);
       SET_SSA_NAME_VAR_OR_IDENTIFIER (t, NULL_TREE);
     }
   else
Comment 15 Richard Biener 2016-02-04 13:14:06 UTC
(In reply to Richard Biener from comment #14)
> I am currently testing the following patch to eventually mitigate the issue
> somewhat by forcing all "registers" to have non-qualified type (their
> qualification does not matter nor does their alignment).  The create_tmp_reg
> hunk is more risky (in case we have bogus callers ending up forcing the
> result to memory).
> 
> It will hide the PRE bug (well, still if the user wrote a 64bit aligned
> type and a 32bit aligned type PRE might change one for the other - in
> fact it will even end up using a 32bit aligned type if the code consistently
> uses 64bit aligned types).
> 
> But as said earlier, the GCC implemented ABI is simply seriously broken
> if you consider the user can control "alignment" of (register-typed) values.
> 
> Index: gcc/gimple-expr.c
> ===================================================================
> --- gcc/gimple-expr.c   (revision 233136)
> +++ gcc/gimple-expr.c   (working copy)
> @@ -484,9 +484,8 @@ create_tmp_var (tree type, const char *p
>  tree
>  create_tmp_reg (tree type, const char *prefix)
>  {
> -  tree tmp;
> -
> -  tmp = create_tmp_var (type, prefix);
> +  gcc_assert (is_gimple_reg_type (type));
> +  tree tmp = create_tmp_var (TYPE_MAIN_VARIANT (type), prefix);
>    if (TREE_CODE (type) == COMPLEX_TYPE
>        || TREE_CODE (type) == VECTOR_TYPE)
>      DECL_GIMPLE_REG_P (tmp) = 1;

Ok, the assert doesn't go well here.  The PRE issue should be mitigated
by the tree-ssanames.c hunk already.  Re-testing with a different above
variant.

> Index: gcc/tree-ssanames.c
> ===================================================================
> --- gcc/tree-ssanames.c (revision 233136)
> +++ gcc/tree-ssanames.c (working copy)
> @@ -286,7 +286,7 @@ make_ssa_name_fn (struct function *fn, t
>  
>    if (TYPE_P (var))
>      {
> -      TREE_TYPE (t) = var;
> +      TREE_TYPE (t) = TYPE_MAIN_VARIANT (var);
>        SET_SSA_NAME_VAR_OR_IDENTIFIER (t, NULL_TREE);
>      }
>    else
Comment 16 Richard Biener 2016-02-05 12:13:11 UTC
Testing of the tree-ssanames.c change went ok on x86_64, can somebody see whether this fixes the testcase on mips?
Comment 17 Jeffrey A. Law 2016-02-05 16:04:35 UTC
I've got enough state on this BZ and remember just enough MIPS that I can verify behaviour with a cross.
Comment 18 Jeffrey A. Law 2016-02-05 16:48:25 UTC
Richi,

The patch fixes both the original testcase as well as the reduced testcase.  The difficultly I see is building a reliable test for the regression suite.  

Parsing RTL dumps looking for the right assignments to the argument registers is, umm, painful.
Comment 19 Jeffrey A. Law 2016-02-05 17:45:33 UTC
Created attachment 37599 [details]
Testcase for the testsuite

My best attempt at pulling together the right dg-markers for this test.  mips.exp does some "interesting" stuff.  I think I managed to narrow it down to the case we want to test and reasonable strings to search for in the RTL expansion dump.

Essentially it verifies that we've got a single assignment to $6 (for the 3-argument call to sprintf) and two assignments to $5 (one for the sprintf call one for the enter call).

When we generate the wrong code we have two assignments to $6 and one to $5.

Use as you see fit.
Comment 20 Richard Biener 2016-02-08 10:04:50 UTC
Author: rguenth
Date: Mon Feb  8 10:04:18 2016
New Revision: 233211

URL: https://gcc.gnu.org/viewcvs?rev=233211&root=gcc&view=rev
Log:
2016-02-08  Richard Biener  <rguenther@suse.de>
	Jeff Law  <law@redhat.com>

	PR target/68273
	* tree-ssanames.c (make_ssa_name_fn): Always use unqualified
	types for anonymous SSA names.

	* gcc.target/mips/pr68273.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.target/mips/pr68273.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssanames.c
Comment 21 Richard Biener 2016-02-08 10:46:32 UTC
Testcase fixed.  Leaving it open though as the ABI issue in the target is still present.
Comment 22 Richard Biener 2016-02-10 14:23:43 UTC
If -fno-ipa-sra helps can you try

Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c      (revision 233268)
+++ gcc/tree-sra.c      (working copy)
@@ -4696,7 +4696,7 @@ get_replaced_param_substitute (struct ip
     {
       char *pretty_name = make_fancy_name (adj->base);
 
-      repl = create_tmp_reg (TREE_TYPE (adj->base), "ISR");
+      repl = create_tmp_reg (TYPE_MAIN_VARIANT (TREE_TYPE (adj->base)), "ISR");
       DECL_NAME (repl) = get_identifier (pretty_name);
       obstack_free (&name_obstack, pretty_name);

as well?
Comment 23 Hector Oron 2016-02-11 11:41:09 UTC
On mips LE machine running Debian OS,
system type             : loongson-ls3a-rs780e-1w
cpu model               : ICT Loongson-3 V0.5  FPU V0.1

I have attempted to build GCC 5.3 with the posted patches:

Index: gcc-5-5.3.1/src/gcc/tree-ssanames.c
===================================================================
--- gcc-5-5.3.1.orig/src/gcc/tree-ssanames.c
+++ gcc-5-5.3.1/src/gcc/tree-ssanames.c
@@ -173,7 +173,7 @@ make_ssa_name_fn (struct function *fn, t
 
   if (TYPE_P (var))
     {
-      TREE_TYPE (t) = var;
+      TREE_TYPE (t) = TYPE_MAIN_VARIANT (var);
       SET_SSA_NAME_VAR_OR_IDENTIFIER (t, NULL_TREE);
     }
   else
Index: gcc-5-5.3.1/src/gcc/tree-sra.c
===================================================================
--- gcc-5-5.3.1.orig/src/gcc/tree-sra.c
+++ gcc-5-5.3.1/src/gcc/tree-sra.c
@@ -4547,7 +4547,7 @@ get_replaced_param_substitute (struct ip
     {
       char *pretty_name = make_fancy_name (adj->base);
 
-      repl = create_tmp_reg (TREE_TYPE (adj->base), "ISR");
+      repl = create_tmp_reg (TYPE_MAIN_VARIANT (TREE_TYPE (adj->base)), "ISR");
       DECL_NAME (repl) = get_identifier (pretty_name);
       obstack_free (&name_obstack, pretty_name);


Compiled GCC 5.3.1, then with it compiled graphviz, and using dot application triggers an error (lt-dot: emit.c:3874: bezier_bb: Assertion `bz.size % 3 == 1' failed.)

While if I compile graphviz passing -fno-ipa-sra, then dot works as expected.

If there is something I can post to help solve this issue let me know.
Comment 24 Jakub Jelinek 2016-02-11 11:44:56 UTC
Instead of all the workarounds, the mips backend just should be fixed, that is the only real fix.
Comment 25 Richard Biener 2016-02-11 11:49:24 UTC
Re-adding GCC 6 as regression, though graphviz is not gsoap.

Did you try the patch referenced in comment #13 yet?
Comment 26 Hector Oron 2016-02-11 12:16:15 UTC
(In reply to Richard Biener from comment #25)

> Did you try the patch referenced in comment #13 yet?

Not really, as I understood that was not safe. But I'll proceed with testing that patch which touches gcc/config/mips/mips.c
Comment 27 Hector Oron 2016-02-11 14:16:54 UTC
(In reply to Richard Biener from comment #25)
> Re-adding GCC 6 as regression, though graphviz is not gsoap.

Right, I initially thought it was the same issue, as it only happens on mips, and both have a workaround of using -fno-ipa-sra.
Comment 28 Steve Ellcey 2016-02-11 16:20:56 UTC
(In reply to Hector Oron from comment #27)
> (In reply to Richard Biener from comment #25)
> > Re-adding GCC 6 as regression, though graphviz is not gsoap.
> 
> Right, I initially thought it was the same issue, as it only happens on
> mips, and both have a workaround of using -fno-ipa-sra.

Rather than the original patch from comment #13, I would try this follow-up mips.c change:

https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00250.html

I think it is better but I am still not sure if it is what we want to do as a permanent fix.
Comment 29 Aurelien Jarno 2016-02-11 16:50:37 UTC
(In reply to Richard Biener from comment #25)
> Re-adding GCC 6 as regression, though graphviz is not gsoap.

Indeed the patch that has been committed (or the same one backported to the GCC 5 branch) correctly fixes the gsoap issue. It doesn't fixes the graphviz one, that said while the issue looks similar it might actually be a different one.
Comment 30 Hector Oron 2016-02-12 12:36:07 UTC
(In reply to Steve Ellcey from comment #28)
> (In reply to Hector Oron from comment #27)
> > (In reply to Richard Biener from comment #25)
> > > Re-adding GCC 6 as regression, though graphviz is not gsoap.
> > 
> > Right, I initially thought it was the same issue, as it only happens on
> > mips, and both have a workaround of using -fno-ipa-sra.
> 
> Rather than the original patch from comment #13, I would try this follow-up
> mips.c change:
> 
> https://gcc.gnu.org/ml/gcc-patches/2016-02/msg00250.html
> 
> I think it is better but I am still not sure if it is what we want to do as
> a permanent fix.

That one worked for me. I was able to compile graphviz properly and dot is not aborting anymore.
Comment 31 Richard Biener 2016-02-15 10:05:13 UTC
eipa_sra introduces the remaining SSA name with non-default alignment via

#0  make_ssa_name_fn (fn=0x7ffff68bb5e8, 
    var=<parm_decl 0x7ffff69bc700 ISRA.81>, stmt=<gimple_nop 0x7ffff68bcf50>)
    at /space/rguenther/src/svn/trunk3/gcc/tree-ssanames.c:311
#1  0x0000000000e203d1 in get_or_create_ssa_default_def (fn=0x7ffff68bb5e8, 
    var=<parm_decl 0x7ffff69bc700 ISRA.81>)
    at /space/rguenther/src/svn/trunk3/gcc/tree-dfa.c:360
#2  0x0000000000e6638e in get_reaching_def (
    var=<parm_decl 0x7ffff69bc700 ISRA.81>)
    at /space/rguenther/src/svn/trunk3/gcc/tree-into-ssa.c:1168
#3  0x0000000000e67855 in maybe_replace_use (use_p=0x7ffff6a17810)
    at /space/rguenther/src/svn/trunk3/gcc/tree-into-ssa.c:1753
#4  0x0000000000e67eb9 in rewrite_update_stmt (
    stmt=<gimple_assign 0x7ffff6966f50>, gsi=...)
    at /space/rguenther/src/svn/trunk3/gcc/tree-into-ssa.c:1950
#5  0x0000000000e685f4 in rewrite_update_dom_walker::before_dom_children (
    this=0x7fffffffd580, bb=<basic_block 0x7ffff6491f08 (2)>)
    at /space/rguenther/src/svn/trunk3/gcc/tree-into-ssa.c:2130
#6  0x00000000013e0993 in dom_walker::walk (this=0x7fffffffd580, 
    bb=<basic_block 0x7ffff6491f08 (2)>)
    at /space/rguenther/src/svn/trunk3/gcc/domwalk.c:265

thus SSA rewrite.  ISRA.81 is a PARM_DECL built in ipa_modify_formal_parameters
from an access present in the IL created by early SRA.

Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c      (revision 233418)
+++ gcc/tree-sra.c      (working copy)
@@ -933,7 +933,7 @@ create_access (tree expr, gimple *stmt,
 
   access = create_access_1 (base, offset, size);
   access->expr = expr;
-  access->type = TREE_TYPE (expr);
+  access->type = TYPE_MAIN_VARIANT (TREE_TYPE (expr));
   access->write = write;
   access->grp_unscalarizable_region = unscalarizable_region;
   access->stmt = stmt;
@@ -1088,7 +1088,7 @@ scalarize_elem (tree base, HOST_WIDE_INT
   {
     struct access *access = create_access_1 (base, pos, size);
     access->expr = ref;
-    access->type = type;
+    access->type = TYPE_MAIN_VARIANT (type);
     access->grp_total_scalarization = 1;
     access->reverse = reverse;
     /* Accesses for intraprocedural SRA can have their stmt NULL.  */
@@ -1108,7 +1108,7 @@ create_total_scalarization_access (tree
 
   access = create_access_1 (var, 0, size);
   access->expr = var;
-  access->type = TREE_TYPE (var);
+  access->type = TYPE_MAIN_VARIANT (TREE_TYPE (var));
   access->grp_total_scalarization = 1;
 }
 

fixes the IL checking below and thus hopefully the testcase.

Index: tree-ssa.c
===================================================================
--- tree-ssa.c  (revision 233418)
+++ tree-ssa.c  (working copy)
@@ -936,6 +936,29 @@ verify_ssa (bool check_modified_stmt, bo
                              name, stmt, virtual_operand_p (name)))
                goto err;
            }
+
+         if (TYPE_ALIGN (TREE_TYPE (name))
+             != TYPE_ALIGN (TYPE_MAIN_VARIANT (TREE_TYPE (name))))
+           {
+             error ("type with non-default alignment");
+             goto err;
+           }
+

it would be nice to see this IL checking on trunk and thus results from
testing on affected targets - I'll give it a try in x86_64.
Comment 32 Hector Oron 2016-02-26 00:07:14 UTC
(In reply to Richard Biener from comment #31)
> eipa_sra introduces the remaining SSA name with non-default alignment via

[PATCH]

> it would be nice to see this IL checking on trunk and thus results from
> testing on affected targets - I'll give it a try in x86_64.

I have backported the patch to 5.3.1, which I am using.. and applying the following patch based on yours, dot does not fail here anymore.

I also have a gcc built with tests, which I'll try to submit to relevant mailing list.

Extending already commited patch from comment #20 with:

Index: gcc-5-5.3.1/src/gcc/tree-sra.c
===================================================================
--- gcc-5-5.3.1.orig/src/gcc/tree-sra.c
+++ gcc-5-5.3.1/src/gcc/tree-sra.c
@@ -936,7 +936,7 @@ create_access (tree expr, gimple stmt, b

   access = create_access_1 (base, offset, size);
   access->expr = expr;
-  access->type = TREE_TYPE (expr);
+  access->type = TYPE_MAIN_VARIANT (TREE_TYPE (expr));
   access->write = write;
   access->grp_unscalarizable_region = unscalarizable_region;
   access->stmt = stmt;
@@ -1026,7 +1026,7 @@ completely_scalarize_var (tree var)

   access = create_access_1 (var, 0, size);
   access->expr = var;
-  access->type = TREE_TYPE (var);
+  access->type = TYPE_MAIN_VARIANT (TREE_TYPE (var));
   access->grp_total_scalarization = 1;

   completely_scalarize_record (var, var, 0, var);
Index: gcc-5-5.3.1/src/gcc/tree-ssa.c
===================================================================
--- gcc-5-5.3.1.orig/src/gcc/tree-ssa.c
+++ gcc-5-5.3.1/src/gcc/tree-ssa.c
@@ -965,6 +965,14 @@ verify_ssa (bool check_modified_stmt, bo
                              name, stmt, virtual_operand_p (name)))
                goto err;
            }
+
+         if (TYPE_ALIGN (TREE_TYPE (name))
+             != TYPE_ALIGN (TYPE_MAIN_VARIANT (TREE_TYPE (name))))
+           {
+             error ("type with non-default alignment");
+             goto err;
+           }
+
        }
     }
Comment 33 Aurelien Jarno 2016-04-25 16:53:52 UTC
(In reply to Hector Oron from comment #32)
> (In reply to Richard Biener from comment #31)
> > eipa_sra introduces the remaining SSA name with non-default alignment via
> 
> [PATCH]
> 

For the record, Debian now have the patch from comment #32 in its version of GCC 5 and 6. This fixes a few more issues than reported here, in addition to graphviz, it also fixes subversion and jq.
Comment 34 Richard Biener 2016-06-03 10:06:04 UTC
GCC 5.4 is being released, adjusting target milestone.
Comment 35 Matthew Fortune 2016-06-03 10:43:44 UTC
(In reply to Aurelien Jarno from comment #33)
> (In reply to Hector Oron from comment #32)
> > (In reply to Richard Biener from comment #31)
> > > eipa_sra introduces the remaining SSA name with non-default alignment via
> > 
> > [PATCH]
> > 
> 
> For the record, Debian now have the patch from comment #32 in its version of
> GCC 5 and 6. This fixes a few more issues than reported here, in addition to
> graphviz, it also fixes subversion and jq.

Richard: Is the updated patch from comment #32 in-keeping with your original changes in comment #20? If so then I'll post it on gcc-patches.

I do accept that there are MIPS fixes to be made in this area but I'd like to get the existing code to 'work' again first if possible.
Comment 36 rguenther@suse.de 2016-06-03 11:16:55 UTC
On Fri, 3 Jun 2016, matthew.fortune at imgtec dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68273
> 
> --- Comment #35 from Matthew Fortune <matthew.fortune at imgtec dot com> ---
> (In reply to Aurelien Jarno from comment #33)
> > (In reply to Hector Oron from comment #32)
> > > (In reply to Richard Biener from comment #31)
> > > > eipa_sra introduces the remaining SSA name with non-default alignment via
> > > 
> > > [PATCH]
> > > 
> > 
> > For the record, Debian now have the patch from comment #32 in its version of
> > GCC 5 and 6. This fixes a few more issues than reported here, in addition to
> > graphviz, it also fixes subversion and jq.
> 
> Richard: Is the updated patch from comment #32 in-keeping with your original
> changes in comment #20? If so then I'll post it on gcc-patches.
> 
> I do accept that there are MIPS fixes to be made in this area but I'd like to
> get the existing code to 'work' again first if possible.

Note that the tree-ssa.c changes from comment#32 do not work as-is
(on the branch you don't notice that because checking is disabled).
Even the relaxed variant that we don't have over-aligned types in the IL
breaks AFAIR.

Given the analysis in comment #31 I'd rather patch up either
turn_representatives_into_adjustments or ipa_modify_formal_parameters
(which already seems to have code to avoid under-aligned types).

That is, does

Index: gcc/ipa-prop.c
===================================================================
--- gcc/ipa-prop.c      (revision 237053)
+++ gcc/ipa-prop.c      (working copy)
@@ -3910,7 +3909,7 @@ ipa_modify_formal_parameters (tree fndec
              if (is_gimple_reg_type (ptype))
                {
                  unsigned malign = GET_MODE_ALIGNMENT (TYPE_MODE 
(ptype));
-                 if (TYPE_ALIGN (ptype) < malign)
+                 if (TYPE_ALIGN (ptype) != malign)
                    ptype = build_aligned_type (ptype, malign);
                }
            }

fix the issue as well?
Comment 37 Matthew Fortune 2016-08-05 11:09:43 UTC
(In reply to rguenther@suse.de from comment #36)
> On Fri, 3 Jun 2016, matthew.fortune at imgtec dot com wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68273
> > 
> > --- Comment #35 from Matthew Fortune <matthew.fortune at imgtec dot com> ---
> > (In reply to Aurelien Jarno from comment #33)
> > > (In reply to Hector Oron from comment #32)
> > > > (In reply to Richard Biener from comment #31)
> > > > > eipa_sra introduces the remaining SSA name with non-default alignment via
> > > > 
> > > > [PATCH]
> > > > 
> > > 
> > > For the record, Debian now have the patch from comment #32 in its version of
> > > GCC 5 and 6. This fixes a few more issues than reported here, in addition to
> > > graphviz, it also fixes subversion and jq.
> > 
> > Richard: Is the updated patch from comment #32 in-keeping with your original
> > changes in comment #20? If so then I'll post it on gcc-patches.
> > 
> > I do accept that there are MIPS fixes to be made in this area but I'd like to
> > get the existing code to 'work' again first if possible.
> 
> Note that the tree-ssa.c changes from comment#32 do not work as-is
> (on the branch you don't notice that because checking is disabled).
> Even the relaxed variant that we don't have over-aligned types in the IL
> breaks AFAIR.
> 
> Given the analysis in comment #31 I'd rather patch up either
> turn_representatives_into_adjustments or ipa_modify_formal_parameters
> (which already seems to have code to avoid under-aligned types).
> 
> That is, does
> 
> Index: gcc/ipa-prop.c
> ===================================================================
> --- gcc/ipa-prop.c      (revision 237053)
> +++ gcc/ipa-prop.c      (working copy)
> @@ -3910,7 +3909,7 @@ ipa_modify_formal_parameters (tree fndec
>               if (is_gimple_reg_type (ptype))
>                 {
>                   unsigned malign = GET_MODE_ALIGNMENT (TYPE_MODE 
> (ptype));
> -                 if (TYPE_ALIGN (ptype) < malign)
> +                 if (TYPE_ALIGN (ptype) != malign)
>                     ptype = build_aligned_type (ptype, malign);
>                 }
>             }
> 
> fix the issue as well?

Sorry for the very slow reply, got busy with libjava bugs.

This does fix jq as well so it looks good. I haven't done a GCC testsuite run with the change in place yet though.
Comment 38 Richard Biener 2016-08-09 07:38:44 UTC
Author: rguenth
Date: Tue Aug  9 07:38:13 2016
New Revision: 239273

URL: https://gcc.gnu.org/viewcvs?rev=239273&root=gcc&view=rev
Log:
2016-08-09  Richard Biener  <rguenther@suse.de>

	PR ipa/68273
	* ipa-prop.c (ipa_modify_formal_parameters): Build
	parameter types with natural alignment also for the
	over-aligned case.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-prop.c
Comment 39 Richard Biener 2016-08-09 10:50:08 UTC
Author: rguenth
Date: Tue Aug  9 10:49:36 2016
New Revision: 239277

URL: https://gcc.gnu.org/viewcvs?rev=239277&root=gcc&view=rev
Log:
2016-08-09  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2016-08-09  Richard Biener  <rguenther@suse.de>

	PR ipa/68273
	* ipa-prop.c (ipa_modify_formal_parameters): Build
	parameter types with natural alignment also for the
	over-aligned case.

	2016-07-15  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/71881
	* tree-loop-distribution.c (destroy_loop): Remove blocks in
	reverse DOM order to make debug temp generation happy.

	* gcc.dg/torture/pr71881.c: New testcase.

Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/torture/pr71881.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/ipa-prop.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
    branches/gcc-6-branch/gcc/tree-loop-distribution.c
Comment 40 Richard Biener 2016-08-09 10:55:51 UTC
Mitigated on trunk and the GCC 6 branch (correct me if I am wrong).
Comment 41 Jakub Jelinek 2017-10-10 14:16:31 UTC
GCC 5 branch has been closed, should be fixed in GCC 6.2 and later.