Bug 101469 - wrong code with "-O2 -fPIE" for SH
Summary: wrong code with "-O2 -fPIE" for SH
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 10.3.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2021-07-16 09:48 UTC by Rin Okuyama
Modified: 2023-07-18 04:05 UTC (History)
5 users (show)

See Also:
Host:
Target: sh3-none-elf, shle--netbsdelf
Build:
Known to work:
Known to fail: 10.3.0, 5.5.0, 7.5.0, 9.3.0
Last reconfirmed: 2023-07-07 00:00:00


Attachments
also change address register when applying the peephole (613 bytes, patch)
2023-07-07 10:28 UTC, Oleg Endo
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rin Okuyama 2021-07-16 09:48:24 UTC
This Bug is for GCC 10.3 for shle:

----
$ shle--netbsdelf-gcc -v
Using built-in specs.
COLLECT_GCC=/build/gcc10/tools/bin/shle--netbsdelf-gcc
COLLECT_LTO_WRAPPER=/build/gcc10/tools/libexec/gcc/shle--netbsdelf/10.3.0/lto-wrapper
Target: shle--netbsdelf
Configured with: /usr/src/tools/gcc/../../external/gpl3/gcc/dist/configure --target=shle--netbsdelf --enable-long-long --enable-threads --with-bugurl=http://www.NetBSD.org/support/send-pr.html --with-pkgversion='NetBSD nb1 20210411' --with-system-zlib --without-isl --enable-__cxa_atexit --enable-libstdcxx-time=rt --enable-libstdcxx-threads --with-diagnostics-color=auto-if-env --with-default-libstdcxx-abi=new --with-sysroot=/build/gcc10/dest/landisk --with-mpc=/build/gcc10/tools --with-mpfr=/build/gcc10/tools --with-gmp=/build/gcc10/tools --disable-nls --disable-multilib --program-transform-name='s,^,shle--netbsdelf-,' --enable-languages='c c++ objc' --prefix=/build/gcc10/tools
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.3.0 (NetBSD nb1 20210411)
----

GCC miscompile this code with "-O2 -fPIE":

----
typedef struct {
	int pad[16];
	int i;
	int *p;
} struct_t;

struct_t *sp;

void *ptr(void);

void func(void) {
	sp = ptr();
	sp->p = &sp->i;
}
----

The following is objdump with comments:

----
00000000 <func>:
   0:	mov.l	r12,@-r15
   2:	mova	24 <func+0x24>,r0
   4:	mov.l	24 <func+0x24>,r12
   6:	sts.l	pr,@-r15
   8:	add	r0,r12			! r12 = .got
   a:	mov.l	28 <func+0x28>,r1
   c:	bsrf	r1			! r0 = ptr()
   e:	nop
  10:	mov.l	2c <func+0x2c>,r1
  12:	mov	r0,r2			! r2 = r0
  14:	mov	r12,r0
  16:	mov.l	r2,@(r0,r1)		! @(.got, 2c) = sp = r2
  18:	add	#64,r2			! r2 = &sp->i
  1a:	mov.l	r2,@(4,r12)		! XXX
  1c:	lds.l	@r15+,pr
  1e:	rts
  20:	mov.l	@r15+,r12
  22:	nop
  24:	.word 0x0000
  26:	.word 0x0000
  28:	sett
  2a:	.word 0x0000
  2c:	.word 0x0000
----

The problem is marked by XXX in comment; if this line were

----
  1a:	mov.l	r2,@(4,r2)
----

it would make sense, i.e.,

----
  @(4, &sp->i) = sp->p = r2 = &sp->i
----

However, unfortunately, GCC somehow mistakes r12 (= .got) with r2.
As a result, sp->p is not correctly set, and .got gets corrupted.

Note that generated code is almost same for "-Os -fPIE". And the
problem occurs also for GCC 9.3.
Comment 1 Rin Okuyama 2021-07-18 04:18:04 UTC
I've confirmed that the problem also occurs for sh3-none-elf:

----
$ sh3-none-elf-gcc -v
Using built-in specs.
COLLECT_GCC=/usr/pkg/cross-sh3-none-elf/bin/sh3-none-elf-gcc
COLLECT_LTO_WRAPPER=/usr/pkg/cross-sh3-none-elf/libexec/gcc/sh3-none-elf/10.3.0/lto-wrapper
Target: sh3-none-elf
Configured with: /build/pkgsrc/cross/sh3-none-elf-gcc/work.x86_64/gcc-10.3.0/configure --target=sh3-none-elf --enable-languages=c,c++ --with-newlib --disable-nls --disable-libstdcxx-pch --prefix=/usr/pkg/cross-sh3-none-elf --build=x86_64--netbsd --host=x86_64--netbsd --infodir=/usr/pkg/cross-sh3-none-elf/info --mandir=/usr/pkg/cross-sh3-none-elf/man
Thread model: single
Supported LTO compression algorithms: zlib
gcc version 10.3.0 (GCC)
----

So, this is not NetBSD-specific.

Also, for shle--netbsdelf, GCC 7.5 and 5.5 has the same problem; this is
not a recent regression.

Thanks,
rin
Comment 2 Valeriy E. Ushakov 2021-07-18 17:04:57 UTC
I don't have the latest gcc handy, but I see this bug already in an old netbsd tree from about an year ago with gcc 8.4.0

As hgutch@n.o pointed out, this seems to be a problem with this peephole2:

;;	mov	r12,r0
;;	add	#-48,r0     ->	add	#-48,r12
;;	mov.l	r0,@(4,r10)	mov.l	r12,@(4,r10)
;;	(r12 dead)
(define_peephole2
  [(set (match_operand:SI 0 "arith_reg_dest")
	(match_operand:SI 1 "arith_reg_dest"))
   (set (match_dup 0) (plus:SI (match_dup 0)
			       (match_operand:SI 2 "const_int_operand")))
   (set (match_operand:SI 3 "general_movdst_operand") (match_dup 0))]


as far as I can tell, it optimizes

  r0 = r2
  r0 += 64
  *(r0+4) = r0

to

  r2 += 64
  *(r0+4) = r2

failing to notice that the the destination uses r0 too.

Then in the cprop-registers step that r0 is replaced with r12 b/c of r0 = r12 done a bit earlier.
Comment 3 Rin Okuyama 2021-07-19 05:23:01 UTC
Thank you very much for your analysis!

If that peephole is removed, GCC 10.3 generates working codes.

NetBSD/shle built by this compiler works fine as far as I can see.
I'm carrying out full regression tests of NetBSD on this system.
(It takes about a day to finish.)

Is there a better fix than mechanically removing that peephole?

Thanks,
rin
Comment 4 Oleg Endo 2021-07-19 05:55:36 UTC
(In reply to Rin Okuyama from comment #3)
> Thank you very much for your analysis!
> 
> If that peephole is removed, GCC 10.3 generates working codes.
> 
> NetBSD/shle built by this compiler works fine as far as I can see.
> I'm carrying out full regression tests of NetBSD on this system.
> (It takes about a day to finish.)
> 
> Is there a better fix than mechanically removing that peephole?

Thanks for reporting and analyzing this issue.

I was suspecting that to be a broken peephole pattern.  Of course it's better to fix the broken pattern instead of deleting it completely.

I'll try to come up with a patch during this week.
Comment 5 Rin Okuyama 2021-07-19 06:13:08 UTC
Thank you for your kind attention! I look forward to hearing from you.
Comment 6 Rin Okuyama 2021-07-21 16:40:42 UTC
(In reply to Rin Okuyama from comment #3)
> If that peephole is removed, GCC 10.3 generates working codes.
> 
> NetBSD/shle built by this compiler works fine as far as I can see.
> I'm carrying out full regression tests of NetBSD on this system.
> (It takes about a day to finish.)

It took more time due to minor troubles for my side, but it finished.
Fallout from this bug is fixed, whereas no new regressions are observed!

Thanks,
rin
Comment 7 Oleg Endo 2023-07-07 10:28:53 UTC
Created attachment 55498 [details]
also change address register when applying the peephole

The attached patch should fix the issue without removing the peephole optimization.  If the eliminated register is in the memory address operand, then the memory operand will be updated respectively.

Does anybody still has access to the system where this bug happened? I'd appreciate any testing support.
Comment 8 Rin Okuyama 2023-07-10 02:39:56 UTC
(In reply to Oleg Endo from comment #7)

Hi Oleg. Thank you very much for your great work!

I've tested your patch with GCC 10.4.0 in this weekend, and it perfectly worked:
- testcase attached to comment #0 compiled correctly
- full regression tests on NetBSD/shle-current built by patched GCC successfully completed without any regression (compared with system built by GCC with that peephole optimization removed)
- some 3rd party softwares (including perl 5.36.0, ruby 3.10.10, zsh 5.9, ...) self-built on shle host by GCC of same code base

Let me thank you again, Oleg!

rin
Comment 9 Oleg Endo 2023-07-10 02:45:16 UTC
(In reply to Rin Okuyama from comment #8)
> (In reply to Oleg Endo from comment #7)
> 
> Hi Oleg. Thank you very much for your great work!
> 
> I've tested your patch with GCC 10.4.0 in this weekend, and it perfectly
> worked:
> - testcase attached to comment #0 compiled correctly
> - full regression tests on NetBSD/shle-current built by patched GCC
> successfully completed without any regression (compared with system built by
> GCC with that peephole optimization removed)
> - some 3rd party softwares (including perl 5.36.0, ruby 3.10.10, zsh 5.9,
> ...) self-built on shle host by GCC of same code base
> 
> Let me thank you again, Oleg!
> 
> rin

Thanks you so much for getting back!  Sorry for being late for 2 years with this.  I will commit the patch to all open GCC versions.  Unfortunately GCC 10.5 has just been released recently and that was the last version 10.  So the patch will be included from GCC version 11.
Comment 10 Rin Okuyama 2023-07-10 03:36:23 UTC
(In reply to Oleg Endo from comment #9)
> Sorry for being late for 2 years with this.
...
> Unfortunately GCC 10.5 has just been released recently and that was the last version 10.
> So the patch will be included from GCC version 11.

No problem at all! We will probably ship GCC 10.5 with your fix for NetBSD 10.
And we will switch to GCC 12 for NetBSD-current soon.

> I will commit the patch to all open GCC versions.

I'm looking forward to it :)

I'm really excited to know that we have active GCC developer working on SH!

Thanks,
rin
Comment 11 GCC Commits 2023-07-14 01:40:27 UTC
The master branch has been updated by Oleg Endo <olegendo@gcc.gnu.org>:

https://gcc.gnu.org/g:4dbb3af1efe55174a714d15c2994cf2842ef8c28

commit r14-2511-g4dbb3af1efe55174a714d15c2994cf2842ef8c28
Author: Oleg Endo <olegendo@gcc.gnu.org>
Date:   Fri Jul 14 09:54:20 2023 +0900

    SH: Fix PR101496 peephole bug
    
    gcc/ChangeLog:
    
            PR target/101469
            * config/sh/sh.md (peephole2): Handle case where eliminated reg is also
            used by the address of the following memory operand.
Comment 12 GCC Commits 2023-07-14 01:45:30 UTC
The releases/gcc-11 branch has been updated by Oleg Endo <olegendo@gcc.gnu.org>:

https://gcc.gnu.org/g:75b8353e4b61566f7e8ac627204e2bcd6bfe60a6

commit r11-10909-g75b8353e4b61566f7e8ac627204e2bcd6bfe60a6
Author: Oleg Endo <olegendo@gcc.gnu.org>
Date:   Fri Jul 14 09:54:20 2023 +0900

    SH: Fix PR101469 peephole bug
    
    gcc/ChangeLog:
    
            PR target/101469
            * config/sh/sh.md (peephole2): Handle case where eliminated reg
            is also used by the address of the following memory operand.
Comment 13 GCC Commits 2023-07-14 01:51:05 UTC
The releases/gcc-12 branch has been updated by Oleg Endo <olegendo@gcc.gnu.org>:

https://gcc.gnu.org/g:5f20f736c1144dd9f2ae2f99099b7f7b21a3ab4e

commit r12-9772-g5f20f736c1144dd9f2ae2f99099b7f7b21a3ab4e
Author: Oleg Endo <olegendo@gcc.gnu.org>
Date:   Fri Jul 14 09:54:20 2023 +0900

    SH: Fix PR101469 peephole bug
    
    gcc/ChangeLog:
    
            PR target/101469
            * config/sh/sh.md (peephole2): Handle case where eliminated reg
            is also used by the address of the following memory operand.
Comment 14 GCC Commits 2023-07-14 02:03:59 UTC
The releases/gcc-13 branch has been updated by Oleg Endo <olegendo@gcc.gnu.org>:

https://gcc.gnu.org/g:ef4b6d29d9801c970bee1b3e8675b19ef5f61d2a

commit r13-7563-gef4b6d29d9801c970bee1b3e8675b19ef5f61d2a
Author: Oleg Endo <olegendo@gcc.gnu.org>
Date:   Fri Jul 14 09:54:20 2023 +0900

    SH: Fix PR101469 peephole bug
    
    gcc/ChangeLog:
    
            PR target/101469
            * config/sh/sh.md (peephole2): Handle case where eliminated reg
            is also used by the address of the following memory operand.
Comment 15 Oleg Endo 2023-07-14 02:10:36 UTC
Fixed on master, GCC 13, GCC 12, GCC 11.

Should also be applied to older release branches that are maintained elsewhere.
Comment 16 Rin Okuyama 2023-07-18 03:57:10 UTC
The fix has been cherry-picked for NetBSD:
https://mail-index.netbsd.org/source-changes/2023/07/18/msg146078.html

Let me thank you again, Oleg!
Comment 17 Oleg Endo 2023-07-18 04:05:37 UTC
(In reply to Rin Okuyama from comment #16)
> The fix has been cherry-picked for NetBSD:
> https://mail-index.netbsd.org/source-changes/2023/07/18/msg146078.html
> 
> Let me thank you again, Oleg!

Sure, you're welcome.  Let me know if there's anything else.  When you open an SH related PR here in GCC's bugzilla, please add me to the CC list, or else I might not see it timely.