[Bug target/70216] [SH] Implement __builtin_trap

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #31 from joseph at codesourcery dot com  ---
On Wed, 6 Dec 2017, glaubitz at physik dot fu-berlin.de wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216
> 
> --- Comment #30 from John Paul Adrian Glaubitz  fu-berlin.de> ---
> (In reply to jos...@codesourcery.com from comment #29)
> > I don't see the issue building glibc with build-many-glibcs.py any more, 
> > hence it no longer uses -fno-isolate-erroneous-paths-dereference 
> > -fno-isolate-erroneous-paths-attribute for SH.
> 
> Is this particular patch part of glibc_2.25?

No, d40dbe7 is first in 2.26.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #30 from John Paul Adrian Glaubitz  ---
(In reply to jos...@codesourcery.com from comment #29)
> I don't see the issue building glibc with build-many-glibcs.py any more, 
> hence it no longer uses -fno-isolate-erroneous-paths-dereference 
> -fno-isolate-erroneous-paths-attribute for SH.

Is this particular patch part of glibc_2.25?

We still need the __builtin_trap() for the kernel in any case which is why the
issue was recently brought up again. Without the patch, it's not possible to
build the kernel with gcc-7.

[Bug target/70216] [SH] Implement __builtin_trap

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #29 from joseph at codesourcery dot com  ---
I don't see the issue building glibc with build-many-glibcs.py any more, 
hence it no longer uses -fno-isolate-erroneous-paths-dereference 
-fno-isolate-erroneous-paths-attribute for SH.

commit c89721e25d609ec4f3366a3956b2f3e5acd76e77
Author: Adhemerval Zanella 
Date:   Mon Mar 13 08:04:22 2017 -0300

build-many-glibcs: Remove no_isolate from SH config

Now with d40dbe7 SH build does not require more the no_isolate gcc
options to correct build glibc (since SH build now does not generate
a trap anymore).  This patch removes the unrequired options from
SH config.

Checked with a build for sh3-linux-gnu, sh3eb-linux-gnu, sh4-linux-gnu,
and sh4eb-linux-gnu.

* scripts/build-many-glibcs.py (Context.add_all_configs): Remove
no_isolate usage for SH.

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #28 from Oleg Endo  ---
(In reply to John Paul Adrian Glaubitz from comment #27)
> 
> The problem is that with gcc-7 as the default compiler in Debian, this issue
> always results in glibc and the Linux kernel failing to build from source.

That's not my fault, is it?  If you want to hotfix the debian build something,
then you already have the patch and it's working for you...

I'll try to get this issue done by the end of this month.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #27 from John Paul Adrian Glaubitz  ---
(In reply to Oleg Endo from comment #26)
> What's the matter anyway?  This issue has been around for like
> 2 years and now it can't wait a week or two?

The problem is that with gcc-7 as the default compiler in Debian, this issue
always results in glibc and the Linux kernel failing to build from source.

For gcc-5 and gcc-6, it was enough to add "extra_cflags +=
-fno-delete-null-pointer-checks" for glibc, but that no longer works with gcc-7
and the glibc build will always fail. For the kernel, there is no workaround I
know of.

Sorry, I did not mean to pressure anyone.

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #26 from Oleg Endo  ---
(In reply to John Paul Adrian Glaubitz from comment #25)
> (In reply to Segher Boessenkool from comment #24)
> > Send it to gcc-patches@?  If it is approved, I can commit it, sure.
> 
> Ok, thanks! Will do!

Thanks, I can apply my own patch.  No worries.  But like I wrote in comment #10
and in comment #19, I would like to extend the patch.  What's the matter
anyway?  This issue has been around for like 2 years and now it can't wait a
week or two?

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #25 from John Paul Adrian Glaubitz  ---
(In reply to Segher Boessenkool from comment #24)
> Send it to gcc-patches@?  If it is approved, I can commit it, sure.

Ok, thanks! Will do!

[Bug target/70216] [SH] Implement __builtin_trap

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #24 from Segher Boessenkool  ---
Send it to gcc-patches@?  If it is approved, I can commit it, sure.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #23 from John Paul Adrian Glaubitz  ---
(In reply to Segher Boessenkool from comment #22)
> ?
> 
> Why me?  What do I have to do with this?  It's SH code, I'm not
> an SH maintainer.  /confused

I was wondering whether you could help with merging Oleg's patch from comment
#6 as Oleg said he doesn't have much time at the moment.

[Bug target/70216] [SH] Implement __builtin_trap

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #22 from Segher Boessenkool  ---
?

Why me?  What do I have to do with this?  It's SH code, I'm not
an SH maintainer.  /confused

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #21 from John Paul Adrian Glaubitz  ---
Maybe Segher could extende Oleg's patch and merge it?

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #20 from John Paul Adrian Glaubitz  ---
(In reply to Oleg Endo from comment #19)
> (In reply to John Paul Adrian Glaubitz from comment #18)
> 
> > I can confirm that the patch from comment #6 resolves the problem for me.
> 
> Thanks for checking.

Just for completeness sake, the patch also fixes the build of glibc with gcc-7.

So, yes, it should be merged :).

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #19 from Oleg Endo  ---
(In reply to John Paul Adrian Glaubitz from comment #18)

> I can confirm that the patch from comment #6 resolves the problem for me.

Thanks for checking.

> 
> Can we get it merged in one form or another?
> 

Like I said in comment #10, it's OK to add this to GCC 7.  However, if so, then
I'd like to implement the -mbuiltin-trap=insn-FFFD and make it the default for
Linux as discussed above with Rich.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #18 from John Paul Adrian Glaubitz  ---
(In reply to John Paul Adrian Glaubitz from comment #17)
> I'm testing the patch right now. Already rebuild gcc with the patch and I'm
> now building the kernel with that gcc.

I can confirm that the patch from comment #6 resolves the problem for me.

Can we get it merged in one form or another?

I'll file a bug report against the gcc-7 package in Debian so the patch gets
added in Debian already.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #17 from John Paul Adrian Glaubitz  ---
(In reply to Rich Felker from comment #16)
> The kernel build regression is just a gratuitous unresolved symbol; the code
> path where is happens should not be reachable or the kernel would crash. So
> I think the patch as-is is fine for fixing that issue.

I'm testing the patch right now. Already rebuild gcc with the patch and I'm now
building the kernel with that gcc.

[Bug target/70216] [SH] Implement __builtin_trap

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #16 from Rich Felker  ---
The kernel build regression is just a gratuitous unresolved symbol; the code
path where is happens should not be reachable or the kernel would crash. So I
think the patch as-is is fine for fixing that issue. The need for an
instruction that generates SIGILL is just for ensuring that apps that reach
__builtin_trap() or equivalent actually trap rather than doing something
wrong/random.

I think -mbuiltin-trap=insn-FFFD and making it default for Linux (and possibly
other hosted targets? NetBSD?) would fully solve that problem.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #15 from John Paul Adrian Glaubitz  ---
(In reply to Oleg Endo from comment #14)
> (In reply to John Paul Adrian Glaubitz from comment #13)
> > 
> > What about glibc which originally resulted in this bug report?
> 
> I have no idea about it.

I'll just give it a try then now.

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #14 from Oleg Endo  ---
(In reply to John Paul Adrian Glaubitz from comment #13)
> 
> What about glibc which originally resulted in this bug report?

I have no idea about it.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #13 from John Paul Adrian Glaubitz  ---
(In reply to Oleg Endo from comment #12)
> I don't think the patch will be immediately useful for a linux config.  It
> will require more work.

What about glibc which originally resulted in this bug report?

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #12 from Oleg Endo  ---
(In reply to John Paul Adrian Glaubitz from comment #11)
> > 
> > It's OK to add __builtin_trap to GCC 7.
> > Could you have a look and try the patch in Comment 6?  I don't have so much
> > time for SH stuff these days...
> 
> I will test the patch.
> 
> Will it be enough to just add the patch, rebuild gcc-7 and then use this
> version of gcc-7 to rebuild the kernel? Or do we need to pass extra options
> during the kernel build?

I don't think the patch will be immediately useful for a linux config.  It will
require more work.

[Bug target/70216] [SH] Implement __builtin_trap

[glaubitz at physik dot fu-berlin.de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #11 from John Paul Adrian Glaubitz  ---
(In reply to Oleg Endo from comment #10)
> > FYI this issue is currently a regression that prevents building Linux with
> > gcc7, since gcc7 introduced an optimization that transforms x/0 to
> > __builtin_trap() rather than calling the libgcc div function.
> 
> It's OK to add __builtin_trap to GCC 7.
> Could you have a look and try the patch in Comment 6?  I don't have so much
> time for SH stuff these days...

I will test the patch.

Will it be enough to just add the patch, rebuild gcc-7 and then use this
version of gcc-7 to rebuild the kernel? Or do we need to pass extra options
during the kernel build?

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #10 from Oleg Endo  ---
(In reply to Rich Felker from comment #9)
> From a Linux standpoint, there is no trapa trap number defined that would
> cause a fatal signal. The ones that are defined are for syscalls and debug
> breakpoints. On the other hand, a permanently-undefined opcode would cause
> SIGILL. So for Linux targets with aim of generating code that does what you
> intend for it to do on both existing and future kernels, I think it makes
> much more sense to use FFFD. This also aligns with the behavior of
> __builtin_trap() on other targets, which is almost always to execute an
> instruction that causes SIGILL (or, if unavailable, SIGSEGV).
> 
> I'm not opposed to supporting a -m option to choose a trapa trap instead,
> and/or making something with trapa the default for freestanding/non-linux
> targets.

So my patch from Comment 6 could be extended to handle the option
  -mbuiltin-trap=insn-FFFD

and define that as the default for linux configs.

Would that do the job?


> FYI this issue is currently a regression that prevents building Linux with
> gcc7, since gcc7 introduced an optimization that transforms x/0 to
> __builtin_trap() rather than calling the libgcc div function.

It's OK to add __builtin_trap to GCC 7.
Could you have a look and try the patch in Comment 6?  I don't have so much
time for SH stuff these days...

[Bug target/70216] [SH] Implement __builtin_trap

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #9 from Rich Felker  ---
>From a Linux standpoint, there is no trapa trap number defined that would cause
a fatal signal. The ones that are defined are for syscalls and debug
breakpoints. On the other hand, a permanently-undefined opcode would cause
SIGILL. So for Linux targets with aim of generating code that does what you
intend for it to do on both existing and future kernels, I think it makes much
more sense to use FFFD. This also aligns with the behavior of __builtin_trap()
on other targets, which is almost always to execute an instruction that causes
SIGILL (or, if unavailable, SIGSEGV).

I'm not opposed to supporting a -m option to choose a trapa trap instead,
and/or making something with trapa the default for freestanding/non-linux
targets.

FYI this issue is currently a regression that prevents building Linux with
gcc7, since gcc7 introduced an optimization that transforms x/0 to
__builtin_trap() rather than calling the libgcc div function.

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #8 from Oleg Endo  ---
(In reply to Rich Felker from comment #7)
> Is there a reason we don't use an undefined instruction that will trap?
> 0xfffd is mentioned as permanently undefined here on page 85:
> 
> http://documentation.renesas.com/doc/products/mpumcu/rej09b0003_sh4a.pdf
> 
> I actually had trouble getting Linux to trap this; it's one thing I need to
> investigate on the kernel side, which is why I didn't reply earlier. I'll
> report my findings later.

I think it's a bit odd and obscure to use 0xFFFD for this purpose.  There are
256 trap numbers to choose from.  Unless there's some clear benefit of using
0xFFFD, I'd rather avoid it.


> One other thing I'm wondering about is the contract for clobbers on the
> function call version.

I believe I've mentioned this in the docs section of the patch.  The compiler
would assume that only the PR is clobbered by the function call.

> Is __builtin_trap supposed to be able to return? I
> was under the impression that it's noreturn, in which case there would be no
> requirements on what it can clobber,

That's true.  Although it's technically possible for some trap to return, I
can't think of any practical case where it would be required.  If the function
implementation wants to return it has to take care of any other clobbers.

My plan was to re-use the same concept for trapping arithmetic (PR 54272),
where the trap function could do something like throwing an exception.  In any
case, I don't think it would want to return to the original code either.

On a second thought, it might be better to emit the function call like:

 sts.l   pr,@-r15
 jsr @r...
 nop

and officially not clobber PR either.  This might be useful when injecting an
exception, because the original PR value can be restored.

> but there may be calling-convention
> issues still: Is it callable via PLT? On FDPIC targets, does the function
> receive a valid GOT pointer in r12 so that it can make further calls/data
> access?

My idea was to have it like any other special support function in libgcc, which
are often implemented in asm directly.

> Also, if the expectation is that the trap will not return, does the
> trapa need the bug workaround? Does the bug (requiring the 5x or) take place
> at the time the trapa is executed, or at the time of return to the
> instruction after it? This is not well-documented and I remember looking
> into it before but I don't remember the answer.

I only know tnsh7456ae.pdf which doesn't explain what exactly goes wrong at
which stage in the CPU.  From what I understand it somehow seems to be related
to the instruction prefetching and decoding of the trapa instruction and what
follows it.

[Bug target/70216] [SH] Implement __builtin_trap

[bugdal at aerifal dot cx]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #7 from Rich Felker  ---
Is there a reason we don't use an undefined instruction that will trap? 0xfffd
is mentioned as permanently undefined here on page 85:

http://documentation.renesas.com/doc/products/mpumcu/rej09b0003_sh4a.pdf

I actually had trouble getting Linux to trap this; it's one thing I need to
investigate on the kernel side, which is why I didn't reply earlier. I'll
report my findings later.

One other thing I'm wondering about is the contract for clobbers on the
function call version. Is __builtin_trap supposed to be able to return? I was
under the impression that it's noreturn, in which case there would be no
requirements on what it can clobber, but there may be calling-convention issues
still: Is it callable via PLT? On FDPIC targets, does the function receive a
valid GOT pointer in r12 so that it can make further calls/data access? Also,
if the expectation is that the trap will not return, does the trapa need the
bug workaround? Does the bug (requiring the 5x or) take place at the time the
trapa is executed, or at the time of return to the instruction after it? This
is not well-documented and I remember looking into it before but I don't
remember the answer.

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #6 from Oleg Endo  ---
Created attachment 38083
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=38083=edit
Proposed patch

This patch adds two new target options:
  -msh4-trapa-sleep-bug
  -mbuiltin-trap=

and two configure options:
  --with-builtin-trap
  --enable-sh4-trapa-sleep-bug / --disable-sh4-trapa-sleep-bug

On sh*-linux it should configure itself to use -mbuiltin-trap=trapa,imm=84 by
default.

I'm not sure if the -msh4-trapa-sleep-bug should be enabled by default on
sh*-linux to always emit the workaround insn sequence (trap + 5x or r0,r0).  If
--enable-sh4-trapa-sleep-bug is not specified, it will only be enabled when
compiling code for SH4.

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

Oleg Endo  changed:

   What|Removed |Added

 CC||bugdal at aerifal dot cx

--- Comment #5 from Oleg Endo  ---
(In reply to Oleg Endo from comment #3)
> (In reply to Kazumoto Kojima from comment #2)
> > 
> > I think that # less than 0x20 are reserved by kernel, gdb uses 0x20
> > and 0xc3 and gcc uses 0x33 for profiling.  Perhaps 0x54 (ascii 'T')
> > or something?
> 
> If there are no other opinions, I guess I'll just do that.  I don't have any
> better idea.

Rich, since you've been doing something related to trap handling in the linux
kernel, maybe you have a comment?

[Bug target/70216] [SH] Implement __builtin_trap

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

Andrew Pinski  changed:

   What|Removed |Added

   Keywords||missed-optimization
 Status|UNCONFIRMED |NEW
   Last reconfirmed||2016-03-13
 Ever confirmed|0   |1
   Severity|normal  |enhancement

--- Comment #4 from Andrew Pinski  ---
Confirmed.

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #3 from Oleg Endo  ---
(In reply to Kazumoto Kojima from comment #2)
> 
> I think that # less than 0x20 are reserved by kernel, gdb uses 0x20
> and 0xc3 and gcc uses 0x33 for profiling.  Perhaps 0x54 (ascii 'T')
> or something?

If there are no other opinions, I guess I'll just do that.  I don't have any
better idea.

Basically it's the same question as in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54272#c2

Emitting a function call into libgcc for __builtin_trap could also be an option
(-mbuiltin-trap=libcall)

[Bug target/70216] [SH] Implement __builtin_trap

[kkojima at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

--- Comment #2 from Kazumoto Kojima  ---
(In reply to Oleg Endo from comment #1)
> For sh4-linux this option should be enabled by default with some useful trap
> number value.  Which trap number should be used in this case?

I think that # less than 0x20 are reserved by kernel, gdb uses 0x20
and 0xc3 and gcc uses 0x33 for profiling.  Perhaps 0x54 (ascii 'T')
or something?

[Bug target/70216] [SH] Implement __builtin_trap

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70216

Oleg Endo  changed:

   What|Removed |Added

 CC||aurelien at aurel32 dot net,
   ||glaubitz at physik dot 
fu-berlin.d
   ||e, kkojima at gcc dot gnu.org

--- Comment #1 from Oleg Endo  ---
I'm thinking to add a new -m option to enable the generation of the SH trap
insn for __builtin_trap.

For example: -mbuiltin_trap=123 would emit "trap #123".

For sh4-linux this option should be enabled by default with some useful trap
number value.  Which trap number should be used in this case?