Debian folks have noted that __builtin_trap is not implemented on SH, which causes some inconveniences when building glibc. https://lists.debian.org/debian-superh/2016/03/msg00000.html Other targets seem to implement __builtin_trap via unconditional trap, e.g. ARM: https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=205749 We could do something similar on SH.
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?
(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?
(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)
Confirmed.
(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?
Created attachment 38083 [details] 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.
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.
(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.
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.
(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...
(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?
(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.
(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?
(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.
(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.
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.
(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.
(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.
(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.
(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 :).
Maybe Segher could extende Oleg's patch and merge it?
? Why me? What do I have to do with this? It's SH code, I'm not an SH maintainer. /confused
(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.
Send it to gcc-patches@? If it is approved, I can commit it, sure.
(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!
(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?
(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.
(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.
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 <adhemerval.zanella@linaro.org> 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.
(In reply to joseph@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.
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 <glaubitz at physik dot fu-berlin.de> --- > (In reply to joseph@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.