Bug 36998 - [4.3/4.4 regression] Ada bootstrap broken on i586-*-*
Summary: [4.3/4.4 regression] Ada bootstrap broken on i586-*-*
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.3.2
: P2 normal
Target Milestone: 4.3.2
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-01 13:30 UTC by Eric Botcazou
Modified: 2008-11-29 22:13 UTC (History)
7 users (show)

See Also:
Host:
Target: i586-*-*
Build: gerald@pfeifer.com
Known to work: 4.3.1
Known to fail: 4.3.2
Last reconfirmed: 2008-08-02 15:49:07


Attachments
disable-asserts.patch (339 bytes, patch)
2008-08-02 11:53 UTC, Jakub Jelinek
Details | Diff
sp=reg.patch (1.19 KB, patch)
2008-08-02 11:57 UTC, Jakub Jelinek
Details | Diff
pop.patch (368 bytes, patch)
2008-08-11 16:25 UTC, Jakub Jelinek
Details | Diff
rtl dump (1.59 KB, text/plain)
2008-08-12 00:30 UTC, Kazumoto Kojima
Details
dbr.patch (748 bytes, patch)
2008-08-12 14:48 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Botcazou 2008-08-01 13:30:18 UTC
This is on the 4.3 branch but the mainline is affected too:

/home/eric/build/gcc-4_3-branch/native32/./prev-gcc/xgcc -B/home/eric/build/gcc-4_3-branch/native32/./prev-gcc/ -B/home/eric/install/gcc-4_3-branch/i586-suse-linux/bin/ -c -g -O2 -fomit-frame-pointer      -gnatpg -gnata -nostdinc -I- -I. -Iada -I/home/eric/svn/gcc-4_3-branch/gcc/ada /home/eric/svn/gcc-4_3-branch/gcc/ada/uname.adb -o ada/uname.o
+===========================GNAT BUG DETECTED==============================+
| 4.3.2 20080801 (prerelease) [gcc-4_3-branch revision 138516] (i586-suse-linux-gnu) GCC error:|
| in compute_barrier_args_size_1, at dwarf2out.c:1180                      |
| Error detected around /home/eric/svn/gcc-4_3-branch/gcc/ada/uname.adb:648|
| Please submit a bug report; see http://gcc.gnu.org/bugs.html.            |

Very likely trigged by the fix for PR rtl-optimization/36419.
Comment 1 Jakub Jelinek 2008-08-01 15:10:23 UTC
Weird, I've bootstrapped/regtested 4.3 branch this morning, including ada, on
{i?86,x86_64,ppc,ppc64}-linux just fine, r138443, so the dwarf2out.c changes
were in.
Comment 2 Jakub Jelinek 2008-08-01 16:13:24 UTC
I've managed to reproduce it, only happens with -mtune=i586 apparently.
Without my patch GCC doesn't ICE, just creates bad unwind info.
In *.greg dump we have (in uname__uname_lt):
(insn:HI 10 9 11 3 uname.adb:615 (parallel [
            (set (reg/f:SI 7 sp)
                (plus:SI (reg/f:SI 7 sp)
                    (const_int -12 [0xfffffffffffffff4])))
            (clobber (reg:CC 17 flags))
        ]) 249 {*addsi_1} (nil))

(insn:HI 11 10 12 3 uname.adb:615 (set (mem/i:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A32])
        (reg/v:SI 0 ax [orig:70 left ] [70])) 40 {*pushsi2} (nil))

(call_insn:HI 12 11 13 3 uname.adb:615 (call (mem:QI (symbol_ref:SI ("namet__get_name_string") [flags 0x41] <function_decl 0x7fa5
        (const_int 16 [0x10])) 601 {*call_0} (nil)
    (nil))
                
(insn:HI 13 12 14 3 uname.adb:616 (parallel [
            (set (reg/f:SI 5 di [88])
                (plus:SI (reg/f:SI 7 sp)
                    (const_int 16 [0x10])))
            (clobber (reg:CC 17 flags))
        ]) 249 {*addsi_1} (expr_list:REG_EQUIV (plus:SI (reg/f:SI 7 sp)
            (const_int 16 [0x10]))
        (nil)))

...
(insn:HI 16 15 173 3 uname.adb:616 (parallel [
            (set (reg/f:SI 7 sp)
                (plus:SI (reg/f:SI 7 sp)
                    (const_int 16 [0x10])))
            (clobber (reg:CC 17 flags))
        ]) 249 {*addsi_1} (nil))

...
(insn:HI 26 155 27 3 uname.adb:616 (parallel [
            (set (reg/f:SI 7 sp)
                (plus:SI (reg/f:SI 7 sp)
                    (const_int -4 [0xfffffffffffffffc])))
            (clobber (reg:CC 17 flags))
        ]) 249 {*addsi_1} (nil))

(insn:HI 27 26 28 3 uname.adb:616 (set (mem/i:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A32])
        (reg:SI 1 dx [73])) 40 {*pushsi2} (nil))

(insn:HI 28 27 29 3 uname.adb:616 (set (mem/f/i:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A32])
        (symbol_ref:SI ("namet__name_buffer") [flags 0x40] <var_decl 0x7fa51618a140 namet__name_buffer>)) 40 {*pushsi2} (nil))

(insn:HI 29 28 30 3 uname.adb:616 (set (mem/f/i:SI (pre_dec:SI (reg/f:SI 7 sp)) [0 S4 A32])
        (reg/f:SI 5 di [88])) 40 {*pushsi2} (nil))

(call_insn:HI 30 29 33 3 uname.adb:616 (set (reg:SI 0 ax)
        (call (mem:QI (symbol_ref:SI ("memcpy") [flags 0x41] <function_decl 0x7fa5161dc5b0 memcpy>) [0 S1 A8])
            (const_int 16 [0x10]))) 839 {*call_value_0} (expr_list:REG_EH_REGION (const_int 0 [0x0])
        (nil))
    (nil))

and no stack adjustments in between.  The first insn listed above starts at
args_size 0 level, then the first call is at args_size 16 level, then we pop up
the stack back to args_size 0 level, decrease by 4 bytes again and push 3 * 4 bytes, so again memcpy is at args_size 16 level.
But *.postreload decides to change insn 16 into:
(insn:HI 16 15 173 3 uname.adb:616 (set (reg/f:SI 7 sp)
        (reg/f:SI 5 di [88])) 47 {*movsi_1} (nil))
which is equivalent to the sp = sp + 16, but unfortunately is something
stack_adjust_offset doesn't track for the args_size adjustments.  Which means
that args_size on the second call will be 32 instead of the correct 16, and with my patch it ICEs because label 127 is reachable both from a place where args_size is 0 (jump_insn 8) and from a place where gcc believes it is 16 (after the memcpy call).

Not sure what's the best way to fix this though.  I can surely remove the assert and let it silently generate invalid unwind info, but I don't think that's a good idea.  So, either reload is changed not to do this (pointless) replacement,
or perhaps the dwarf2out.c could assume if sp is set to some register (rather than sp + const), args_size should be reset to 0 (though, I really don't know if
that would be a safe assumption).  Tracking all registers that might ever contain some pointers into the stack would be a nightmare.
Comment 3 Jason Merrill 2008-08-01 20:01:59 UTC
Assuming that (set SP otherreg) resets args_size to 0 seems strictly better than the current situation, and if it turns out to be wrong we ought to see another crash like this one, so it seems fair to try.  Otherwise we'd need to hook into CSE, I guess.
Comment 4 Kazumoto Kojima 2008-08-02 05:48:42 UTC
For SH processors with FPU, 4.3 and 4.4 fail to bootstrap during
building libjava with:

../../../../../../ORIG/trunk/libjava/classpath/native/fdlibm/dtoa.c:885: internal compiler error: in compute_barrier_args_size_1, at dwarf2out.c:1230

It looks that these targets have the similar problem.
Comment 5 Jakub Jelinek 2008-08-02 11:53:30 UTC
Created attachment 15998 [details]
disable-asserts.patch

Quick hack to disable asserts.
Comment 6 Jakub Jelinek 2008-08-02 11:57:53 UTC
Created attachment 15999 [details]
sp=reg.patch

Untested patch to set args_size to 0 when encountering (set (reg sp) (reg di))
etc.

I'll be offline for a week, can somebody please test the latter patch (the former
is just a short term hack to disable the ICEs and keeping the unwind info bogus)?

What perhaps still should be asserted is that on CALL_INSNs current args_size
is equal to INTVAL (XEXP (call, 1)) (probably just in the !after_p case).
Comment 7 Kazumoto Kojima 2008-08-02 15:02:34 UTC
I've tested the 2nd patch on sh4, but it doesn't fix the ICE
on sh4.  Perhaps the problem is slightly different from that
for i586.  Here is a reduced test case for the sh-elf compiler
with the option '-m4 -O -fasynchronous-unwind-tables':

union double_union
{
  double d;
  unsigned int i[2];
};

char *foo (double _d, char *s, char **rve)
{
  union double_union d;

  d.d = _d;
  if ((d.i[1]) & 0x80000000UL)
    (d.i[1]) &= ~0x80000000UL;
  if (((d.i[1]) & 0x7ff00000UL) == 0x7ff00000UL)
    return s;
  if (!d.d)
    {
      s = "0";
      if (rve) *rve = s + 1;
      return s;
    }
}

I've tried to see what is going on in the above test case
with stepping from the top of compute_barrier_args_size_1.
It seems that that function scans insns and updates
cur_args_size as follows:

(insn/f 95 ... (set (reg/f:SI 14 r14) (reg/f:SI 15 r15))...)
...
(insn 91 ... (set (mem/c:SF (pre_dec:SI (reg/f:SI 15 r15))) (reg:SF 68 fr4))...)
cur_args_size is updated to 4.

(insn 92 ... (set (mem/c:SF (pre_dec:SI (reg/f:SI 15 r15))) (reg:SF 69 fr5))...)
cur_args_size is updated to 8.
...
(jump_insn 22 ... (label_ref 77) ...)
...
(insn 86 ... (set (mem/c:DF (pre_dec:SI (reg/f:SI 15 r15))) (reg:DF 6 r6)))
cur_args_size is updated to 16.
...
(jump_insn 33 ... (label_ref 77) ...)
...
;; Exit block
(code_label 77 ...)
...
(insn 102 ... (set (reg/f:SI 15 r15) (reg/f:SI 14 r14)) ...)

Thus label 77 is reachable from (jump_insn 22 ...) where arg_size
is 8 and from (jump_insn 33 ...) where arg_size is 16.
Comment 8 Jakub Jelinek 2008-08-02 15:17:27 UTC
If a label is reachable from different args_size levels, that's a severe bug and should be fixed wherever that is created, likely expander.
Comment 9 Eric Botcazou 2008-08-02 15:49:06 UTC
> Untested patch to set args_size to 0 when encountering (set (reg sp) (reg di))
> etc.

OK on i586-linux (C/C++/Ada) on both the mainline and the 4.3 branch.
Comment 10 Kazumoto Kojima 2008-08-02 22:12:50 UTC
(In reply to comment #8)
> If a label is reachable from different args_size levels, that's a severe bug
> and should be fixed wherever that is created, likely expander.

It seems that arg_size is reset with (set (reg sp) (reg fp))
just after label 77 in #7:

(code_label 77 42 76 10 "" [2 uses])

(note 76 77 61 [bb 9] NOTE_INSN_BASIC_BLOCK)

(insn 61 76 100 bas.c:22 (use (reg/i:SI 0 r0)) -1 (nil))

(note 100 61 101 NOTE_INSN_EPILOGUE_BEG)

(insn 101 100 102 bas.c:22 (unspec_volatile [
            (const_int 0 [0x0])
        ] 0) 284 {blockage} (nil))

(insn 102 101 127 bas.c:22 (set (reg/f:SI 15 r15)
        (reg/f:SI 14 r14)) 175 {movsi_ie} (nil))

Does it look a target bug?  If so, should I file a new
PR target to bugzilla?

Comment 11 Richard Biener 2008-08-08 20:16:27 UTC
This breaks building the kernel as well.
Comment 12 Gerald Pfeifer 2008-08-09 22:20:12 UTC
This also breaks the bootstrap of the 4.3 branch on i386-unknown-freebsd6.3
without Ada being involved.
Comment 13 Richard Biener 2008-08-11 09:36:06 UTC
Testcase for the kernel compile ICE:

./cc1 -quiet -Os -m32 -mpreferred-stack-boundary=2 -fasynchronous-unwind-tables do_mounts.3.i

void panic(const char * fmt, ...) __attribute__ ((noreturn));
int printk(const char * fmt, ...);
extern int strlen(const char *s);
int do_mount_root(char *name, char *fs, int flags, void *data);
void 
mount_block_root(char *name, int flags, char *fs_names, char *root_mount_data)
{
  char *p;
  char b[32];
  for (p = fs_names; *p; p += strlen(p)+1)
    {
      do_mount_root(name, p, flags, root_mount_data);
      panic("VFS: Unable to mount root fs on %s", b);
    }
  for (p = fs_names; *p; p += strlen(p)+1)
    printk(" %s", p);
  panic("VFS: Unable to mount root fs on %s", b);
}
Comment 14 Richard Biener 2008-08-11 09:43:50 UTC
With building the lirc kernel module I am also able to trogger

/usr/src/packages/BUILD/lirc-0.8.3/obj/default/lirc_atiusb//lirc_atiusb.c:433: internal compiler error: in compute_barrier_args_size, at dwarf2out.c:1239
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://bugs.opensuse.org/> for instructions.

which is slightly different.  No testcase yet as the package seems to be
broken in different ways anyway (seems to be the same kind of assert though).
Comment 15 Jakub Jelinek 2008-08-11 10:24:31 UTC
Subject: Bug 36998

Author: jakub
Date: Mon Aug 11 10:23:08 2008
New Revision: 138951

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138951
Log:
	PR rtl-optimization/36998
	* dwarf2out.c (compute_barrier_args_size_1,
	compute_barrier_args_size): Temporarily remove assertions.

	* gcc.dg/pr36998.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr36998.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/dwarf2out.c
    trunk/gcc/testsuite/ChangeLog

Comment 16 pinskia@gmail.com 2008-08-11 10:25:51 UTC
Subject: Re:  [4.3/4.4 regression] Ada bootstrap broken on i586-*-*



Sent from my iPhone

On Aug 11, 2008, at 2:43, "rguenth at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org 
 > wrote:

>
>
> ------- Comment #14 from rguenth at gcc dot gnu dot org  2008-08-11  
> 09:43 -------
> With building the lirc kernel module I am also able to trogger
>
> /usr/src/packages/BUILD/lirc-0.8.3/obj/default/lirc_atiusb// 
> lirc_atiusb.c:433:
> internal compiler error: in compute_barrier_args_size, at  
> dwarf2out.c:1239
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://bugs.opensuse.org/> for instructions.
>
> which is slightly different.  No testcase yet as the package seems  
> to be
> broken in different ways anyway (seems to be the same kind of assert  
> though).

There is a pr about this failure already. It happens because of  
combing noreturn calls.

-- Pinski


>
>
>
> -- 
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36998
>
Comment 17 Jakub Jelinek 2008-08-11 10:27:30 UTC
Subject: Bug 36998

Author: jakub
Date: Mon Aug 11 10:26:08 2008
New Revision: 138952

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138952
Log:
	PR rtl-optimization/36998
	* dwarf2out.c (compute_barrier_args_size_1,
	compute_barrier_args_size): Temporarily remove assertions.

	* gcc.dg/pr36998.c: New test.

Added:
    branches/gcc-4_3-branch/gcc/testsuite/gcc.dg/pr36998.c
Modified:
    branches/gcc-4_3-branch/gcc/ChangeLog
    branches/gcc-4_3-branch/gcc/dwarf2out.c
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog

Comment 18 Eric Weddington 2008-08-11 15:02:07 UTC
For some reason the new test case gcc.dg/pr36998.c is not excluding the AVR, and of course the test is failing on the AVR.
Comment 19 Jakub Jelinek 2008-08-11 15:15:02 UTC
For what reason should it be excluding AVR?  There is nothing target specific in it.
Comment 20 Eric Weddington 2008-08-11 16:00:55 UTC
(In reply to comment #19)
> For what reason should it be excluding AVR?  There is nothing target specific
> in it.
> 

I'm sorry, I got confused with line 4:
/* { dg-options "-Os -mpreferred-stack-boundary=2 -fasynchronous-unwind-tables" { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */

Filing new target bug.


Comment 21 Jakub Jelinek 2008-08-11 16:23:00 UTC
Note that I've committed a temporary removal of the asserts, so we now only silently generate incorrect DW_CFA_GNU_args_size directives, but don't ICE.
Downgrading to P2.
Comment 22 Jakub Jelinek 2008-08-11 16:25:47 UTC
Created attachment 16053 [details]
pop.patch

Additional patch on top of sp=reg.patch patch, which should fix the SH4 case -
the simplistic stack_adjust_offset wouldn't see the adjustments in pop instructions, only push instructions.  I'm not sure if it is enough to do it
this way, or whether for_each_rtx shouldn't be called instead to find all embedded MEMs with {PRE,POST}_{INC,DEC,MODIFY} operands on sp register.
Comment 23 Kazumoto Kojima 2008-08-12 00:27:17 UTC
(In reply to comment #22)
> Created an attachment (id=16053) [edit]
> pop.patch

I've confirmed that this patch gets rid of the ICEs for the test
case in #7 and the original libjava build failure on SH4, though
libjava fails to build at the another place on SH4.
Here is a reduced test case:

typedef union {
  double value;
  struct { unsigned int lsw, msw; } parts;
} ieee_double_shape_type;

double
foo (double x)
{
  unsigned int hx, lx;
  ieee_double_shape_type u;

  u.value = x;
  hx = u.parts.msw;

  if (hx >= 0x40862E42)
    {
      if (hx >= 0x7ff00000)
	{
	  u.value = x;
	  lx = u.parts.lsw;

	  if (((hx & 0xfffff) | lx) != 0)
	    return x+x;
	}
    }

  return x;
}

I'd like to attach .206.shorten dump for this.

Comment 24 Kazumoto Kojima 2008-08-12 00:30:25 UTC
Created attachment 16054 [details]
rtl dump

.shorten rtl dump for the test case in #23 with
-O -fasynchronous-unwind-tables
Comment 25 Jakub Jelinek 2008-08-12 14:48:45 UTC
Created attachment 16059 [details]
dbr.patch

Patch that should cure this testcase.  We weren't handling the dbrs correctly.
For the compute_barrier_args_size* calls when setting destination args_size depth for annulled branches we should only consider the insns from target and for normal sequence only insns from !target.  And in normal args_size computation
we need to ignore the target insns.
Comment 26 Kazumoto Kojima 2008-08-13 03:30:02 UTC
(In reply to comment #25)
> Created an attachment (id=16059) [edit]
> dbr.patch

This fixes the ICE.  Trunk revision 138972 + pop.patch + dbr.patch
with reverting disable-asserts.patch was built successfully on
sh4-unknown-linux-gnu for all languages except ada.  It has passed
regtesting on that target.
Comment 27 Richard Biener 2008-08-18 15:34:06 UTC
This should be fixed by disabling the asserts in question.
Comment 28 Joseph S. Myers 2008-08-27 22:05:18 UTC
4.3.2 is released, changing milestones to 4.3.3.
Comment 29 Eric Botcazou 2008-11-18 20:39:16 UTC
Jakub, what's the status of this PR?
Comment 30 Jakub Jelinek 2008-11-20 21:28:16 UTC
Subject: Bug 36998

Author: jakub
Date: Thu Nov 20 21:26:52 2008
New Revision: 142060

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142060
Log:
	PR rtl-optimization/36998
	* dwarf2out.c (stack_adjust_offset): Add cur_args_size and cur_offset
	arguments.  Handle sp = reg and (set (foo) (mem (pre_inc (reg sp)))).
	(compute_barrier_args_size_1, dwarf2out_frame_debug_expr): Adjust
	stack_adjust_offset callers.
	(dwarf2out_stack_adjust): Likewise.  Handle insns in annulled
	branches properly.
	(compute_barrier_args_size): Handle insns in annulled branches
	properly.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/dwarf2out.c

Comment 31 Jakub Jelinek 2008-11-20 21:51:50 UTC
AFAIK there are 2 bugs in DW_CFA_GNU_args_size handling left.  One is that with
-fdefer-pop DW_CFA_GNU_args_size will be wrong in some cases (either with
-fasynchronous-unwind-tables if noreturn calls with different depth are crossjumped (I think #c13 in this PR is an example), or otherwise when relying
on call's second argument.
The other problem is that alloca with constant argument (possibly discovered
during the optimizations) will be thought as normal stack adjustment and thus
accounted for in DW_CFA_GNU_args_size computations, which is wrong.  See PR37022.

If it would be possible with -fdefer-pop to put into CALL's second argument the
actual cumulative stack difference instead of just the size of the arguments,
I think the first problem could be solved (as long as crossjumping wouldn't merge CALLs with different second argument).  For the second I'm afraid we'll
need some way to mark the alloca stack adjustment, so that dwarf2out.c DW_CFA_GNU_args_size computation would ignore it.

BTW, unless the asserts are reenabled again (they are currently disabled for these reasons), this isn't a regression, I don't think we ever emitted DW_CFA_GNU_args_size correctly in such cases.
Comment 32 Eric Botcazou 2008-11-29 22:13:05 UTC
> BTW, unless the asserts are reenabled again (they are currently disabled for
> these reasons), this isn't a regression, I don't think we ever emitted
> DW_CFA_GNU_args_size correctly in such cases.

OK, thanks for the summary.  Moving this one to fixed then.