Bug 67002

That came out of a GCC version with has a known wrong-code bug (PR 66930)
Please try again with the patch https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66930#c10

I'm testing gcc-5_5.2.1-12 (r226105) now as suggested by Matthias. Unfortunately, my SH7785LCR board crashed earlier today, so I had to restart the build.

Once the build is finished, I will attached the stage2/3 versions of gcc/real.o to this bug report.

The snapshot I am currently building does not the suggested patch from (PR 66930), so that we are sure to still run into the stage2/3 comparison failure for further analysis. After that, I will do another build attempt with the patch applied.

Adrian

Created attachment 36085 [details]
gcc/real.o from different compiler stages

Here are the files, copied as follows:

root@tirpitz:/home/glaubitz> cd gcc-5-test_5.2.1-12/
root@tirpitz:..glaubitz/gcc-5-test_5.2.1-12> find . -name "real.o"
./gcc-5-5.2.1/build/gcc/real.o
./gcc-5-5.2.1/build/stage1-gcc/real.o
./gcc-5-5.2.1/build/prev-gcc/real.o
root@tirpitz:..glaubitz/gcc-5-test_5.2.1-12> mkdir ~/gcc-5-files
root@tirpitz:..glaubitz/gcc-5-test_5.2.1-12> cp -av ./gcc-5-5.2.1/build/gcc/real.o ~/gcc-5-files/real_stage3.o
‘./gcc-5-5.2.1/build/gcc/real.o’ -> ‘/root/gcc-5-files/real_stage3.o’
root@tirpitz:..glaubitz/gcc-5-test_5.2.1-12> cp -av ./gcc-5-5.2.1/build/stage1-gcc/real.o ~/gcc-5-files/real_stage1.o
‘./gcc-5-5.2.1/build/stage1-gcc/real.o’ -> ‘/root/gcc-5-files/real_stage1.o’
root@tirpitz:..glaubitz/gcc-5-test_5.2.1-12> cp -av ./gcc-5-5.2.1/build/prev-gcc/real.o ~/gcc-5-files/real_stage2.o
‘./gcc-5-5.2.1/build/prev-gcc/real.o’ -> ‘/root/gcc-5-files/real_stage2.o’
root@tirpitz:..glaubitz/gcc-5-test_5.2.1-12> cd
root@tirpitz:~> tar czf gcc-5-files.tgz gcc-5-files/
root@tirpitz:~>

Attaching this tarball.

Adrian

Some observations:

glaubitz@z6:~/gcc-5-files> sh4-linux-gnu-strip real_stage2.o 
glaubitz@z6:~/gcc-5-files> ls -l
total 800
-rw-r--r-- 1 glaubitz glaubitz 376020 Jul 26 20:09 real_stage1.o
-rw-r--r-- 1 glaubitz glaubitz  33792 Jul 29 09:15 real_stage2.o
-rw-r--r-- 1 glaubitz glaubitz 401656 Jul 29 05:40 real_stage3.o
glaubitz@z6:~/gcc-5-files> sh4-linux-gnu-strip real_stage3.o 
glaubitz@z6:~/gcc-5-files> ls -l
total 440
-rw-r--r-- 1 glaubitz glaubitz 376020 Jul 26 20:09 real_stage1.o
-rw-r--r-- 1 glaubitz glaubitz  33792 Jul 29 09:15 real_stage2.o
-rw-r--r-- 1 glaubitz glaubitz  33792 Jul 29 09:15 real_stage3.o
glaubitz@z6:~/gcc-5-files> sh4-linux-gnu-strip real_stage1.o 
glaubitz@z6:~/gcc-5-files> ls -l
total 152
-rw-r--r-- 1 glaubitz glaubitz 80768 Jul 29 09:16 real_stage1.o
-rw-r--r-- 1 glaubitz glaubitz 33792 Jul 29 09:15 real_stage2.o
-rw-r--r-- 1 glaubitz glaubitz 33792 Jul 29 09:15 real_stage3.o
glaubitz@z6:~/gcc-5-files> diff real_stage2.o real_stage3.o 
Binary files real_stage2.o and real_stage3.o differ
glaubitz@z6:~/gcc-5-files>

This was done on my main desktop, using a cross-toolchain.

And here is the diff from the disassembly:

glaubitz@z6:~/gcc-5-files> diff -u real*asm
--- real2.asm	2015-07-29 09:17:42.806123211 +0200
+++ real3.asm	2015-07-29 09:17:47.310013352 +0200
@@ -1,5 +1,5 @@
 
-real_stage2.o:     file format elf32-sh-linux
+real_stage3.o:     file format elf32-sh-linux
 
 
 Disassembly of section .text:
@@ -9328,7 +9328,7 @@
     4976:	03 a2       	bra	0x4d80
     4978:	09 00       	nop	
     497a:	5c d0       	mov.l	0x4aec,r0	! 4da8
-    497c:	5c db       	mov.l	0x4af0,r11	! 56e4
+    497c:	5c dc       	mov.l	0x4af0,r12	! 56e4
     497e:	0b 40       	jsr	@r0
     4980:	01 e4       	mov	#1,r4
     4982:	02 62       	mov.l	@r0,r2
@@ -9347,18 +9347,18 @@
     499c:	f3 60       	mov	r15,r0
     499e:	40 70       	add	#64,r0
     49a0:	20 10       	mov.l	r2,@(0,r0)
-    49a2:	0b 4b       	jsr	@r11
+    49a2:	0b 4c       	jsr	@r12
     49a4:	31 10       	mov.l	r3,@(4,r0)
     49a6:	a8 2a       	tst	r10,r10
     49a8:	02 8d       	bt.s	0x49b0
     49aa:	83 6a       	mov	r8,r10
     49ac:	ae a0       	bra	0x4b0c
     49ae:	2d e1       	mov	#45,r1
-    49b0:	50 dc       	mov.l	0x4af4,r12	! d80
+    49b0:	50 db       	mov.l	0x4af4,r11	! d80
     49b2:	f3 65       	mov	r15,r5
     49b4:	f3 64       	mov	r15,r4
     49b6:	30 75       	add	#48,r5
-    49b8:	0b 4c       	jsr	@r12
+    49b8:	0b 4b       	jsr	@r11
     49ba:	18 74       	add	#24,r4
     49bc:	09 e1       	mov	#9,r1
     49be:	16 30       	cmp/hi	r1,r0
@@ -9376,12 +9376,12 @@
     49d6:	13 6e       	mov	r1,r14
     49d8:	01 e5       	mov	#1,r5
     49da:	f3 64       	mov	r15,r4
-    49dc:	0b 4b       	jsr	@r11
+    49dc:	0b 4c       	jsr	@r12
     49de:	18 74       	add	#24,r4
     49e0:	f3 65       	mov	r15,r5
     49e2:	f3 64       	mov	r15,r4
     49e4:	30 75       	add	#48,r5
-    49e6:	0b 4c       	jsr	@r12
+    49e6:	0b 4b       	jsr	@r11
     49e8:	18 74       	add	#24,r4
     49ea:	30 70       	add	#48,r0
     49ec:	00 2e       	mov.b	r0,@r14
glaubitz@z6:~/gcc-5-files>

(In reply to John Paul Adrian Glaubitz from comment #6)
> And here is the diff from the disassembly:

A rare indeterminacy of the register choice.  Both codes are valid.
It seems very hard to find where has this indeterminacy come from.
Please follow Oleg's suggestion #c1.

(In reply to Kazumoto Kojima from comment #7)
> A rare indeterminacy of the register choice.  Both codes are valid.

Ok, that's what I thought as well.

> It seems very hard to find where has this indeterminacy come from.
> Please follow Oleg's suggestion #c1.

Since the patch mentioned in #c1 has already been committed, I'll just have to wait for Matthias to update the SVN revision of the gcc-5 branch in Debian.

Maybe we can add gcc/real.o to the ignore list for the time being?

(In reply to John Paul Adrian Glaubitz from comment #8)
> 
> Maybe we can add gcc/real.o to the ignore list for the time being?

Not sure if this is a good idea.

(In reply to Oleg Endo from comment #9)
> Not sure if this is a good idea.

I actually think it is the best option as chances are dim otherwise that we find what's actually causing this register indeterminacy.

Looking at the build log, it's only gcc/real.o where the comparison fails, all the others (except for the checksum files) are find. I think chances are that, if this is actually a real bug, it might show itself more pronounced when we use gcc-5 to build other packages.

Just look at how many bugs we were able to find in gcc-4.9 while using it as the standard host compiler on Debian sh4.

You are right that ignoring this failure is most certainly not the best on a release architecture that people rely on. But currently, Debian on sh4 is merely just a port and things are expected to break from time to time.

I am currently building now a patched gcc-5_5.2.1-12 which has gcc/real.o added to compare_exclusions, let's see how far we get.

Adrian

(In reply to John Paul Adrian Glaubitz from comment #10)
> 
> Looking at the build log, it's only gcc/real.o where the comparison fails,
> all the others (except for the checksum files) are find. I think chances are
> that, if this is actually a real bug, it might show itself more pronounced
> when we use gcc-5 to build other packages.
> 
> Just look at how many bugs we were able to find in gcc-4.9 while using it as
> the standard host compiler on Debian sh4.

Yeah, for this purpose it can be ignored of course.  However, I guess for that you might also just disable bootstrapping (configure with --disable-bootstrap).

Update:

Michael Karcher, who previously helped smashing some bugs in gcc for the SH target, had a go at this and he discovered that the difference between stage2 and stage3 is the -gtoggle switch. And, indeed, triggering this switch is responsible for the diff:

root@tirpitz:/home/mkarcher> diff -u s2/real-stage2.o s3/real-stage3.o 
root@tirpitz:/home/mkarcher> diff -u s2/real-stage2.o s3-notoggle/real-stage3.o 
Binary files s2/real-stage2.o and s3-notoggle/real-stage3.o differ
root@tirpitz:/home/mkarcher> 

He generated a lot of debug output during build with "-fdump-tree-all" and "-da":

gccroot=/home/glaubitz/gcc-5-test_5.2.1-12/gcc-5-5.2.1
gccbuild=$gccroot/build
$gccbuild/stage1-gcc/xg++ \
  -B$gccbuild/stage1-gcc/ \
  -B/usr/sh4-linux-gnu/bin/ -nostdinc++ \
  -B$gccbuild/stage1-sh4-linux-gnu/libstdc++-v3/src/.libs \
  -B$gccbuild/stage1-sh4-linux-gnu/libstdc++-v3/libsupc++/.libs  \
  -I$gccbuild/stage1-sh4-linux-gnu/libstdc++-v3/include/sh4-linux-gnu \
  -I$gccbuild/stage1-sh4-linux-gnu/libstdc++-v3/include \
  -I$gccroot/src/libstdc++-v3/libsupc++ \
  -c -g -O2 -gtoggle -DIN_GCC \
  -fno-exceptions -fno-rtti -fasynchronous-unwind-tables \
  -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual \
     -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings \
  -DHAVE_CONFIG_H \
  -I$gccbuild/stage2-gcc -I$gccroot/src/gcc -I$gccroot/src/include -I$gccroot/src/libcpp/include  \
  -I$gccroot/src/libdecnumber -I$gccroot/src/libdecnumber/dpd -I$gccbuild/libdecnumber \
  -I$gccroot/src/libbacktrace \
  -da -fdump-tree-all \
  -o real-stage2.o $gccroot/src/gcc/real.c

I'm collecting all the dumped output now and will upload. I will follow up with a download link quickly.

Adrian

(In reply to John Paul Adrian Glaubitz from comment #12)
> I'm collecting all the dumped output now and will upload. I will follow up
> with a download link quickly.

Here we go: http://users.physik.fu-berlin.de/~glaubitz/gcc-pr67002.tgz (approx. 171 MiB)

Created attachment 36117 [details]
reduced test case

> Michael Karcher, who previously helped smashing some bugs in gcc for the SH
> target, had a go at this and he discovered that the difference between stage2
> and stage3 is the -gtoggle switch.

I've confirmed it even with cross 5/6 compilers.  The above reduced
C++ test case fails with "-S -O2 -fcompare-debug" on my 5/6 cross
sh4-linux/sh-elf g++.  It means that the generated insns differ
with/without -g.  It looks to me a target independent issue, though
I could be wrong about it.  With a quick look, rtl_pre pass behaves
differently with -g.  I've found PRs for -fcompare-debug like 63315,
65874, 65980, 66688 and 67043.

Created attachment 36134 [details]
even further reduced test case

I manually reduced the example even more. Interestingly it doesn't contain any C++ specific statements any more, yet it only fails -fcompare-debug with -x c++, but passes with -x c

The bug seems to be quite similar to the infamous "sloth that was dropped on the head as a baby"-bug Linus discovered (https://lkml.org/lkml/2014/7/24/584 , https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61904). -fcompare-debug passes if -fno-var-tracking-assignments is passed as compiler option.

(In reply to Michael Karcher from comment #16)

PR 61904 has been marked as a dup of PR 61801, which has been marked as fixed.  So this must be some other bug.

When compiling the same code as C and as C++ there might be differences because it's two different language front ends.  Sometimes we get cases like PR 56365.

(In reply to Michael Karcher from comment #15)
The first different line of diff of the .pre dumps of Michael's
test case with/without -g is:

< Expression hash table (53 buckets, 12 entries)
---
> Expression hash table (51 buckets, 12 entries)

It means that gcse.c:alloc_hash_table allocates the table with
different number of buckets with -g.
It looks emit_rtl.c:get_max_insn_count returns different values
with/without -g at rtl_pre pass.  This shouldn't happen.
The comment of get_max_insn_count says:

  /* The table size must be stable across -g, to avoid codegen
     differences due to debug insns, and not be affected by
     -fmin-insn-uid, to avoid excessive table size and to simplify
     debugging of -fcompare-debug failures.  */

(In reply to Kazumoto Kojima from comment #18)
In the problematic situation, get_max_insn_count returns the false
value after

  if (MAY_HAVE_DEBUG_INSNS)
    expand_debug_locations ();

in pass_expand::execute (function *fun).  expand_debug_locations
calls sh_recog_treg_set_expr indirectly at this very early stage
to compute rtx costs.  sh_recog_treg_set_expr makes some set insns
and they cause the differences of cur_insn_uid with -g.  Oleg, how
about the patch below?  My first trial was --cur_insn_uid just
after make_insn_raw there but I'm afraid that it will make another
surprise.

diff --git a/config/sh/sh.c b/config/sh/sh.c
index f429193..450d634 100644
--- a/config/sh/sh.c
+++ b/config/sh/sh.c
@@ -14165,6 +14165,12 @@ sh_recog_treg_set_expr (rtx op, machine_mode mode)
   if (!can_create_pseudo_p ())
     return false;
 
+  /* expand_debug_locations may call this to compute rtx costs at
+     very early stage.  In that case, don't make new insns here to
+     avoid codegen differences with -g. */
+  if (currently_expanding_to_rtl)
+    return false;
+
   /* We are going to invoke recog in a re-entrant way and thus
      have to capture its current state and restore it afterwards.  */
   recog_data_d prev_recog_data = recog_data;

(In reply to Kazumoto Kojima from comment #19)
> (In reply to Kazumoto Kojima from comment #18)
> In the problematic situation, get_max_insn_count returns the false
> value after
> 
>   if (MAY_HAVE_DEBUG_INSNS)
>     expand_debug_locations ();
> 
> in pass_expand::execute (function *fun).  expand_debug_locations
> calls sh_recog_treg_set_expr indirectly at this very early stage
> to compute rtx costs.  sh_recog_treg_set_expr makes some set insns
> and they cause the differences of cur_insn_uid with -g.

Thank you very much for tracking this down.

> Oleg, how about the patch below?

Yes, that looks OK.  treg_set_expr-something recog matching is actually only required during combine.  The simpler forms like (reg:SI T_REG) could also be required during expansion.

> My first trial was --cur_insn_uid just
> after make_insn_raw there but I'm afraid that it will make another
> surprise.

Hm ... actually it should work.  The temporary rtx insn is not used for anything else, so that assigned insn uid will never appear anywhere.  However, it's probably better to have one "static rtx_insn" to avoid constructing the temporary rtx over and over again.  Unfortunately the gen_rtx* functions allocate rtx objects always on the GC heap.

(In reply to Oleg Endo from comment #20)
> Yes, that looks OK.  treg_set_expr-something recog matching is actually only
> required during combine.  The simpler forms like (reg:SI T_REG) could also
> be required during expansion.

Thanks.  I'll commit it when the usual test is done.

> Hm ... actually it should work.  The temporary rtx insn is not used for
> anything else, so that assigned insn uid will never appear anywhere. 
> However, it's probably better to have one "static rtx_insn" to avoid
> constructing the temporary rtx over and over again.  Unfortunately the
> gen_rtx* functions allocate rtx objects always on the GC heap.

Yes, it works.  I'm uncomfortable with it because the current use of
crtl->emit.x_cur_insn_uid aka cur_insn_uid is limitted to emit-rtl.c.
I think that the use of that field outside of emit-rtl.c is unexpected.
Also it doesn't sound good to add a new interface to access that field
there.  After all, your "static rtx_insn" is the right thing to do.

(In reply to Kazumoto Kojima from comment #21)
> 
> Yes, it works.  I'm uncomfortable with it because the current use of
> crtl->emit.x_cur_insn_uid aka cur_insn_uid is limitted to emit-rtl.c.
> I think that the use of that field outside of emit-rtl.c is unexpected.
> Also it doesn't sound good to add a new interface to access that field
> there.  After all, your "static rtx_insn" is the right thing to do.

OK.  I will try to implement that "static rtx_insn" thing, after I have finished the patch attachment 36012 [details], which makes some changes to treg_set_expr.

Author: kkojima
Date: Fri Aug  7 20:41:25 2015
New Revision: 226726

URL: https://gcc.gnu.org/viewcvs?rev=226726&root=gcc&view=rev
Log:
PR target/67002
* config/sh/sh.c (sh_recog_treg_set_expr): Return false during
expand phase to avoid codegen differences with -g.


Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/sh/sh.c

Author: kkojima
Date: Fri Aug  7 08:11:45 2015
New Revision: 226715

URL: https://gcc.gnu.org/viewcvs?rev=226715&root=gcc&view=rev
Log:
PR target/67002
* config/sh/sh.c (sh_recog_treg_set_expr): Return false during
expand phase to avoid codegen differences with -g.


Modified:
    branches/trunk/gcc/ChangeLog
    branches/trunk/gcc/config/sh/sh.c

Ok, this seems to have been fixed:

Comparing stages 2 and 3
warning: gcc/cc1objplus-checksum.o differs
warning: gcc/cc1-checksum.o differs
warning: gcc/cc1plus-checksum.o differs
warning: gcc/cc1obj-checksum.o differs
Comparison successful.

gcc-5 is still building though and we might run into another issue, but cross your fingers :).

(In reply to John Paul Adrian Glaubitz from comment #25)
> Ok, this seems to have been fixed:
>

Although this PR could be marked as fixed, I'd like to keep it open.  It will remind me of the "static rtx_insn" mentioned above.

Please open a new PR if there is another boostrapping issue.

(In reply to Oleg Endo from comment #26)
> Please open a new PR if there is another boostrapping issue.

I'm afraid that might actually be the case even though I'm not sure whether it's a bug in gcc or elsewhere.

What happens is that g++ seems to get stuck in stage3 when compiling src/gcc/c-family/c-common.c. After a while, the build daemon will just kill g++ because of inactivity. Interestingly, c-common.o seems to compile fine though.

Should I open a new PR?

Adrian

(In reply to John Paul Adrian Glaubitz from comment #27)
> What happens is that g++ seems to get stuck in stage3 when compiling
> src/gcc/c-family/c-common.c. After a while, the build daemon will just kill
> g++ because of inactivity. Interestingly, c-common.o seems to compile fine
> though.

Ok, it actually went further now. It was just stuck for over an hour. Looking at strace showed that g++ was allocating memory (brk syscalls).

Will report back.

(In reply to John Paul Adrian Glaubitz from comment #28)
> Will report back.

From a current build log (5.3.1-4) [1]:

Comparing stages 2 and 3
warning: gcc/cc1objplus-checksum.o differs
warning: gcc/cc1-checksum.o differs
warning: gcc/cc1obj-checksum.o differs
warning: gcc/cc1plus-checksum.o differs
Comparison successful.

So, I think we can mark this as resolved unless I am overlooking something. You can check the log yourself in any case.

Adrian

> [1] https://people.debian.org/~glaubitz/gcc-5_5.3.1-4_sh4.build

Thanks for confirming.