* [PATCH] ARM: Convert BUG() to use unreachable() @ 2009-12-08 9:55 Uwe Kleine-König 2009-12-08 17:07 ` David Daney ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Uwe Kleine-König @ 2009-12-08 9:55 UTC (permalink / raw) To: linux-arm-kernel; +Cc: linux-kernel, David Daney Use the new unreachable() macro instead of for(;;); Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Cc: David Daney <ddaney@caviumnetworks.com> --- arch/arm/kernel/traps.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c index 3f361a7..25b50c4 100644 --- a/arch/arm/kernel/traps.c +++ b/arch/arm/kernel/traps.c @@ -681,7 +681,7 @@ void __attribute__((noreturn)) __bug(const char *file, int line) *(int *)0 = 0; /* Avoid "noreturn function does return" */ - for (;;); + unreachable(); } EXPORT_SYMBOL(__bug); -- 1.6.5.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-08 9:55 [PATCH] ARM: Convert BUG() to use unreachable() Uwe Kleine-König @ 2009-12-08 17:07 ` David Daney 2009-12-10 17:50 ` Russell King - ARM Linux 2009-12-17 15:01 ` Jamie Lokier 2 siblings, 0 replies; 24+ messages in thread From: David Daney @ 2009-12-08 17:07 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: linux-arm-kernel, linux-kernel Uwe Kleine-König wrote: > Use the new unreachable() macro instead of for(;;); > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Cc: David Daney <ddaney@caviumnetworks.com> This looks fine to me. The comment is somewhat redundant now, but still accurate. Reviewed-by: David Daney <ddaney@caviumnetworks.com> > --- > arch/arm/kernel/traps.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c > index 3f361a7..25b50c4 100644 > --- a/arch/arm/kernel/traps.c > +++ b/arch/arm/kernel/traps.c > @@ -681,7 +681,7 @@ void __attribute__((noreturn)) __bug(const char *file, int line) > *(int *)0 = 0; > > /* Avoid "noreturn function does return" */ > - for (;;); > + unreachable(); > } > EXPORT_SYMBOL(__bug); > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-08 9:55 [PATCH] ARM: Convert BUG() to use unreachable() Uwe Kleine-König 2009-12-08 17:07 ` David Daney @ 2009-12-10 17:50 ` Russell King - ARM Linux 2009-12-10 17:55 ` David Daney 2009-12-17 15:01 ` Jamie Lokier 2 siblings, 1 reply; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-10 17:50 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: linux-arm-kernel, linux-kernel, David Daney On Tue, Dec 08, 2009 at 10:55:38AM +0100, Uwe Kleine-König wrote: > Use the new unreachable() macro instead of for(;;); Have you investigated what effect this has on generated code? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-10 17:50 ` Russell King - ARM Linux @ 2009-12-10 17:55 ` David Daney 2009-12-16 13:58 ` Uwe Kleine-König 0 siblings, 1 reply; 24+ messages in thread From: David Daney @ 2009-12-10 17:55 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Uwe Kleine-König, linux-arm-kernel, linux-kernel Russell King - ARM Linux wrote: > On Tue, Dec 08, 2009 at 10:55:38AM +0100, Uwe Kleine-König wrote: >> Use the new unreachable() macro instead of for(;;); > > Have you investigated what effect this has on generated code? Yes. Pre GCC-4.5 the generated code should be identical as 'unreachable()' just expands to 'for(;;);' in this case. Post GCC-4.5 the generated code should be smaller. I have not tested on ARM, but on x86_64, i686, and mips64 this is the case. The code size reduction is due to not emitting an endless loop after the asm() that never returns. David Daney ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-10 17:55 ` David Daney @ 2009-12-16 13:58 ` Uwe Kleine-König 0 siblings, 0 replies; 24+ messages in thread From: Uwe Kleine-König @ 2009-12-16 13:58 UTC (permalink / raw) To: David Daney; +Cc: Russell King - ARM Linux, linux-arm-kernel, linux-kernel Hallo, On Thu, Dec 10, 2009 at 09:55:51AM -0800, David Daney wrote: > Russell King - ARM Linux wrote: >> On Tue, Dec 08, 2009 at 10:55:38AM +0100, Uwe Kleine-König wrote: >>> Use the new unreachable() macro instead of for(;;); >> >> Have you investigated what effect this has on generated code? > > Yes. > > Pre GCC-4.5 the generated code should be identical as 'unreachable()' > just expands to 'for(;;);' in this case. > > Post GCC-4.5 the generated code should be smaller. I don't have a toolchain using gcc 4.5. What should we do with this patch? I think in theory the patch is OK. And for pre gcc-4.5 it should not make any difference as we have in include/linux/compiler-gcc4.h: #if __GNUC_MINOR__ >= 5 ... #define unreachable() __builtin_unreachable() #endif and in include/linux/compiler.h: #ifndef unreachable # define unreachable() do { } while (1) #endif So the only impact if that do { } while (1) is used instead of for(;;) . My toolchain (based on 4.3.2) produces the same object files with and without the patch. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-08 9:55 [PATCH] ARM: Convert BUG() to use unreachable() Uwe Kleine-König 2009-12-08 17:07 ` David Daney 2009-12-10 17:50 ` Russell King - ARM Linux @ 2009-12-17 15:01 ` Jamie Lokier 2009-12-17 17:09 ` David Daney 2 siblings, 1 reply; 24+ messages in thread From: Jamie Lokier @ 2009-12-17 15:01 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: linux-arm-kernel, linux-kernel, David Daney Uwe Kleine-König wrote: > Use the new unreachable() macro instead of for(;;); > *(int *)0 = 0; > > /* Avoid "noreturn function does return" */ > - for (;;); > + unreachable(); Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it knows the branch of the code leading to unreachable can never be reached? If GCC-4.5 does not, are you sure a future version of GCC will never remove it? In other words, is __builtin_unreachable() _defined_ in such a way that it cannot remove the previous assignment? We have seen problems with GCC optimising away important tests for NULL pointers in the kernel, due to similar propagation of "impossible to occur" conditions, so it's worth checking with GCC people what the effect of this one would be. In C, there is a general theoretical problem with back-propagation of optimisations from code with undefined behaviour. In the case of __builtin_unreachable(), it would depend on all sorts of unclearly defined semantics whether it can remove a preceding *(int *)0 = 0. I'd strongly suggest asking on the GCC list. (I'd have mentioned this earlier, if I'd known about the patch for other architectures). The documentation for __builtin_unreachable() only says the program is undefined if control flow reaches it. In other words, it does not say what effect it can have on previous instructions, and I think it's quite likely that it has not been analysed in a case like this. One thing that would give me a lot more confidence, because the GCC documentation does mention asm(), is this: > *(int *)0 = 0; > /* Ensure unreachableness optimisations cannot propagate back. *I/ > __asm__ volatile(""); > /* Avoid "noreturn function does return" */ > unreachable(); -- Jamie ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 15:01 ` Jamie Lokier @ 2009-12-17 17:09 ` David Daney 2009-12-17 17:17 ` Richard Guenther 0 siblings, 1 reply; 24+ messages in thread From: David Daney @ 2009-12-17 17:09 UTC (permalink / raw) To: Jamie Lokier, gcc; +Cc: Uwe Kleine-König, linux-arm-kernel, linux-kernel Jamie Lokier wrote: > Uwe Kleine-König wrote: >> Use the new unreachable() macro instead of for(;;); >> *(int *)0 = 0; >> >> /* Avoid "noreturn function does return" */ >> - for (;;); >> + unreachable(); > > Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it > knows the branch of the code leading to unreachable can never be reached? > I don't know the definitive answer, so I am sending to gcc@... FYI: #define unreachable() __builtin_unreachable() > If GCC-4.5 does not, are you sure a future version of GCC will never > remove it? In other words, is __builtin_unreachable() _defined_ in > such a way that it cannot remove the previous assignment? > > We have seen problems with GCC optimising away important tests for > NULL pointers in the kernel, due to similar propagation of "impossible > to occur" conditions, so it's worth checking with GCC people what the > effect of this one would be. > > In C, there is a general theoretical problem with back-propagation of > optimisations from code with undefined behaviour. In the case of > __builtin_unreachable(), it would depend on all sorts of unclearly > defined semantics whether it can remove a preceding *(int *)0 = 0. > > I'd strongly suggest asking on the GCC list. (I'd have mentioned this > earlier, if I'd known about the patch for other architectures). > > The documentation for __builtin_unreachable() only says the program is > undefined if control flow reaches it. In other words, it does not say > what effect it can have on previous instructions, and I think it's > quite likely that it has not been analysed in a case like this. > > One thing that would give me a lot more confidence, because the GCC > documentation does mention asm(), is this: > >> *(int *)0 = 0; >> /* Ensure unreachableness optimisations cannot propagate back. *I/ >> __asm__ volatile(""); >> /* Avoid "noreturn function does return" */ >> unreachable(); > > -- Jamie ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 17:09 ` David Daney @ 2009-12-17 17:17 ` Richard Guenther 2009-12-17 18:17 ` Russell King - ARM Linux 2009-12-22 11:33 ` Paolo Bonzini 0 siblings, 2 replies; 24+ messages in thread From: Richard Guenther @ 2009-12-17 17:17 UTC (permalink / raw) To: David Daney Cc: Jamie Lokier, gcc, Uwe Kleine-König, linux-arm-kernel, linux-kernel On Thu, Dec 17, 2009 at 6:09 PM, David Daney <ddaney@caviumnetworks.com> wrote: > Jamie Lokier wrote: >> >> Uwe Kleine-König wrote: >>> >>> Use the new unreachable() macro instead of for(;;); >>> *(int *)0 = 0; >>> /* Avoid "noreturn function does return" */ >>> - for (;;); >>> + unreachable(); >> >> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it >> knows the branch of the code leading to unreachable can never be reached? >> > > I don't know the definitive answer, so I am sending to gcc@... > > FYI: #define unreachable() __builtin_unreachable() It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure use __builtin_trap (); instead for the whole sequence (the unreachable is implied then). GCC choses a size-optimal trap representation for your target then. Richard. > >> If GCC-4.5 does not, are you sure a future version of GCC will never >> remove it? In other words, is __builtin_unreachable() _defined_ in >> such a way that it cannot remove the previous assignment? >> >> We have seen problems with GCC optimising away important tests for >> NULL pointers in the kernel, due to similar propagation of "impossible >> to occur" conditions, so it's worth checking with GCC people what the >> effect of this one would be. >> >> In C, there is a general theoretical problem with back-propagation of >> optimisations from code with undefined behaviour. In the case of >> __builtin_unreachable(), it would depend on all sorts of unclearly >> defined semantics whether it can remove a preceding *(int *)0 = 0. >> >> I'd strongly suggest asking on the GCC list. (I'd have mentioned this >> earlier, if I'd known about the patch for other architectures). >> >> The documentation for __builtin_unreachable() only says the program is >> undefined if control flow reaches it. In other words, it does not say >> what effect it can have on previous instructions, and I think it's >> quite likely that it has not been analysed in a case like this. >> >> One thing that would give me a lot more confidence, because the GCC >> documentation does mention asm(), is this: >> >>> *(int *)0 = 0; >>> /* Ensure unreachableness optimisations cannot propagate back. *I/ >>> __asm__ volatile(""); >>> /* Avoid "noreturn function does return" */ >>> unreachable(); >> >> -- Jamie > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 17:17 ` Richard Guenther @ 2009-12-17 18:17 ` Russell King - ARM Linux 2009-12-17 18:35 ` Joe Buck ` (2 more replies) 2009-12-22 11:33 ` Paolo Bonzini 1 sibling, 3 replies; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-17 18:17 UTC (permalink / raw) To: Richard Guenther Cc: David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Thu, Dec 17, 2009 at 06:17:11PM +0100, Richard Guenther wrote: > On Thu, Dec 17, 2009 at 6:09 PM, David Daney <ddaney@caviumnetworks.com> wrote: > > Jamie Lokier wrote: > >> > >> Uwe Kleine-König wrote: > >>> > >>> Use the new unreachable() macro instead of for(;;); > >>> *(int *)0 = 0; > >>> /* Avoid "noreturn function does return" */ > >>> - for (;;); > >>> + unreachable(); > >> > >> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it > >> knows the branch of the code leading to unreachable can never be reached? > >> > > > > I don't know the definitive answer, so I am sending to gcc@... > > > > FYI: #define unreachable() __builtin_unreachable() > > It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure > use > __builtin_trap (); > instead for the whole sequence (the unreachable is implied then). > GCC choses a size-optimal trap representation for your target then. How is "size-optimal trap" defined? The point of "*(int *)0 = 0;" is to cause a NULL pointer dereference which is trapped by the kernel to produce a full post mortem and backtrace which is easily recognised as a result of this code. Having gcc decide on, maybe, an undefined instruction instead would be confusing. Let me put it another way: I want this function to terminate with an explicit NULL pointer dereference in every case. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 18:17 ` Russell King - ARM Linux @ 2009-12-17 18:35 ` Joe Buck 2009-12-17 19:06 ` Russell King - ARM Linux 2009-12-17 19:04 ` Jamie Lokier 2009-12-21 19:30 ` Richard Henderson 2 siblings, 1 reply; 24+ messages in thread From: Joe Buck @ 2009-12-17 18:35 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Thu, Dec 17, 2009 at 10:17:18AM -0800, Russell King - ARM Linux wrote: > > It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure > > use > > __builtin_trap (); > > instead for the whole sequence (the unreachable is implied then). > > GCC choses a size-optimal trap representation for your target then. > > How is "size-optimal trap" defined? The point of "*(int *)0 = 0;" is > to cause a NULL pointer dereference which is trapped by the kernel to > produce a full post mortem and backtrace which is easily recognised > as a result of this code. With something like __builtin_trap, the compiler knows that your intent is to cause a trap. But it's asking for trouble, and for future flame wars between kernel developers and gcc developers, to put in sequences that happen to do the right thing if the compiler does no optimizations whatsoever, but that might be messed up by optimizations because they are code sequences whose behavior is undefined. Besides, didn't I see a whole bunch of kernel security patches related to null pointer dereferences lately? If page 0 can be mapped, you suddenly won't get your trap. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 18:35 ` Joe Buck @ 2009-12-17 19:06 ` Russell King - ARM Linux 2009-12-17 19:14 ` Joe Buck 0 siblings, 1 reply; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-17 19:06 UTC (permalink / raw) To: Joe Buck Cc: Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > Besides, didn't I see a whole bunch of kernel security patches related > to null pointer dereferences lately? If page 0 can be mapped, you > suddenly won't get your trap. Page 0 can not be mapped on ARM kernels since the late 1990s, and this protection is independent of the generic kernel. Milage may vary on other architectures, but that's not a concern here. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 19:06 ` Russell King - ARM Linux @ 2009-12-17 19:14 ` Joe Buck 2009-12-17 19:33 ` David Daney ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Joe Buck @ 2009-12-17 19:14 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > Besides, didn't I see a whole bunch of kernel security patches related > > to null pointer dereferences lately? If page 0 can be mapped, you > > suddenly won't get your trap. > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > protection is independent of the generic kernel. > > Milage may vary on other architectures, but that's not a concern here. I don't understand, though, why you would want to implement a generally useful facility (make the kernel trap so you can do a post-mortem analysis) in a way that's only safe for the ARM port. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 19:14 ` Joe Buck @ 2009-12-17 19:33 ` David Daney 2009-12-17 19:33 ` Russell King - ARM Linux 2009-12-17 19:38 ` Jamie Lokier 2 siblings, 0 replies; 24+ messages in thread From: David Daney @ 2009-12-17 19:33 UTC (permalink / raw) To: Joe Buck Cc: Russell King - ARM Linux, Richard Guenther, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König Joe Buck wrote: > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: >> On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: >>> Besides, didn't I see a whole bunch of kernel security patches related >>> to null pointer dereferences lately? If page 0 can be mapped, you >>> suddenly won't get your trap. >> Page 0 can not be mapped on ARM kernels since the late 1990s, and this >> protection is independent of the generic kernel. >> >> Milage may vary on other architectures, but that's not a concern here. > > I don't understand, though, why you would want to implement a generally > useful facility (make the kernel trap so you can do a post-mortem > analysis) in a way that's only safe for the ARM port. > Each Linux kernel architecture has in its architecture specific bug.h an implementation that is deemed by the architecture maintainers to work. As far as I know, few if any of these use __builtin_trap(). Some could be converted to __builtin_trap(), others cannot (x86 for example). If we enhanced __builtin_trap() to take an argument for the trap code, MIPS could be converted. But as it stands now __builtin_trap() is not very useful. As more architectures start adding funky tables that get generated by the inline asm (as in x86), __builtin_trap() becomes less useful. David Daney ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 19:14 ` Joe Buck 2009-12-17 19:33 ` David Daney @ 2009-12-17 19:33 ` Russell King - ARM Linux 2009-12-17 19:38 ` Jamie Lokier 2 siblings, 0 replies; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-17 19:33 UTC (permalink / raw) To: Joe Buck Cc: Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Thu, Dec 17, 2009 at 11:14:01AM -0800, Joe Buck wrote: > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > Besides, didn't I see a whole bunch of kernel security patches related > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > suddenly won't get your trap. > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > protection is independent of the generic kernel. > > > > Milage may vary on other architectures, but that's not a concern here. > > I don't understand, though, why you would want to implement a generally > useful facility (make the kernel trap so you can do a post-mortem > analysis) in a way that's only safe for the ARM port. The discussion at hand is about code contained in the ARM supporting files (arch/arm/kernel/traps.c), rather than the generic kernel. As such, it is not relevant to any architecture other than ARM. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 19:14 ` Joe Buck 2009-12-17 19:33 ` David Daney 2009-12-17 19:33 ` Russell King - ARM Linux @ 2009-12-17 19:38 ` Jamie Lokier 2009-12-17 19:48 ` Russell King - ARM Linux 2 siblings, 1 reply; 24+ messages in thread From: Jamie Lokier @ 2009-12-17 19:38 UTC (permalink / raw) To: Joe Buck Cc: Russell King - ARM Linux, Richard Guenther, David Daney, gcc, linux-kernel, linux-arm-kernel, Uwe Kleine-König Joe Buck wrote: > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > Besides, didn't I see a whole bunch of kernel security patches related > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > suddenly won't get your trap. > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > protection is independent of the generic kernel. > > > > Milage may vary on other architectures, but that's not a concern here. It does not trap on at least one ARM-nommu kernel... > I don't understand, though, why you would want to implement a generally > useful facility (make the kernel trap so you can do a post-mortem > analysis) in a way that's only safe for the ARM port. The patch under discussion, which led to these questions and Cc gcc@gcc.gnu.org, is specific to the ARM architecture and that is Russell's focus, as ARM kernel maintainer. But yes, the whole principle of how to trap and whether it's safe to follow a null pointer dereference with __builtin_unreachable() applies to all the other architectures too. -- Jamie ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 19:38 ` Jamie Lokier @ 2009-12-17 19:48 ` Russell King - ARM Linux 2009-12-17 19:58 ` Russell King - ARM Linux 0 siblings, 1 reply; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-17 19:48 UTC (permalink / raw) To: Jamie Lokier Cc: Joe Buck, Richard Guenther, David Daney, gcc, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Thu, Dec 17, 2009 at 07:38:26PM +0000, Jamie Lokier wrote: > Joe Buck wrote: > > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > > Besides, didn't I see a whole bunch of kernel security patches related > > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > > suddenly won't get your trap. > > > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > > protection is independent of the generic kernel. > > > > > > Milage may vary on other architectures, but that's not a concern here. > > It does not trap on at least one ARM-nommu kernel... I was going to say the following in a different reply but discarded it because it wasn't relevant to the GCC list. I regard ARM nommu as highly experimental, especially as the maintainer vanished half way through merging it into mainline. I know that there are some parts of ARM nommu that are highly buggy - such as ARM940 support invalidating the entire data cache on any DMA transaction... say goodbye stacked return addresses. As such, I would not be surprised if the ARM nommu kernel has _lots_ of weird and wonderful bugs. I am not surprised that NULL pointer dereferences don't work - its actually something I'd expect given that they have a protection unit which the kernel doesn't apparently touch. Maybe the protection unit code never got merged? I've no idea. As I say, uclinux support got as far as being half merged and that's roughly the state it's remained in ever since. We don't even have any no-MMU configurations which the kernel builder automatically tests for us. Given the lack of progress/bug reporting on ARM uclinux, the lack of platform support and the lack of configurations, my view is that there is no one actually using it. I know that I don't particularly take any care with respect to uclinux when making changes to the MMU based kernels. Why bother if apparantly no one's using it? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 19:48 ` Russell King - ARM Linux @ 2009-12-17 19:58 ` Russell King - ARM Linux 0 siblings, 0 replies; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-17 19:58 UTC (permalink / raw) To: Jamie Lokier Cc: Joe Buck, gcc, Richard Guenther, David Daney, linux-kernel, Uwe Kleine-König, linux-arm-kernel On Thu, Dec 17, 2009 at 07:48:37PM +0000, Russell King - ARM Linux wrote: > Given the lack of progress/bug reporting on ARM uclinux, the lack of > platform support and the lack of configurations, my view is that there > is no one actually using it. I know that I don't particularly take any > care with respect to uclinux when making changes to the MMU based kernels. > Why bother if apparantly no one's using it? Jamie, As you seem to be a user of uclinux on ARM, could you help ensure that the support there is bug fixed and we at least have a configuration file which kautobuild can use to provide feedback on the build status of uclinux on ARM please? ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 18:17 ` Russell King - ARM Linux 2009-12-17 18:35 ` Joe Buck @ 2009-12-17 19:04 ` Jamie Lokier 2009-12-21 19:30 ` Richard Henderson 2 siblings, 0 replies; 24+ messages in thread From: Jamie Lokier @ 2009-12-17 19:04 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Richard Guenther, David Daney, gcc, linux-kernel, linux-arm-kernel, Uwe Kleine-König Russell King - ARM Linux wrote: > Let me put it another way: I want this function to terminate with an > explicit NULL pointer dereference in every case. __builtin_trap cannot be used because the GCC manual says "The mechanism used may vary from release to release so you should not rely on any particular implementation". It includes calling abort() as a possible implementation - not ideal. This is not related to GCC, but I have an ARM system here where dereferencing NULL does not trap. You guessed it, it doesn't have a regular MMU. But there are other addresses which do trap. They would be a much better choice for BUG(). (Aha! Maybe that's why the kernel just behaves weirdly when it runs out of memory and eventually triggers a watchdog reboot, with no panic message. I'd better try changing BUG() :-) Even with an MMU, sometimes userspace maps page zero. For example, Wine on x86 does that. (It might be possible to change Wine and kernel vm86 to avoid it, but it has not happened). Bug-free userspace behaviour should not stop kernel's BUG() from doing it's basic job of trapping in the kernel! It would be quite messy if userspace maps page zero, and then a kernel BUG() ploughs ahead into __builtin_unreachable() and completely undefined behaviour, possibly leading to worse behaviour than omitting the check which called BUG(). Under those circumstances I'd rather see it use __builtin_trap() if that really does trap :-) The point of the exercise with __builtin_unreachable() is to reduce the kernel size by removing the for(;;) loop. *(int *)0 = 0 isn't the smallest trapping sequence. (When it works :-) So the most compact _and_ reliable sequence for the kernel, on all architectures, is probably: __asm__ volatile("smallest undefined or always-trapping instruction") followed by __builtin_unreachable(), because GCC documentation _does_ say that asm followed by that will execute the asm and assume the asm doesn't return. -- Jamie ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 18:17 ` Russell King - ARM Linux 2009-12-17 18:35 ` Joe Buck 2009-12-17 19:04 ` Jamie Lokier @ 2009-12-21 19:30 ` Richard Henderson 2009-12-21 20:10 ` Russell King - ARM Linux 2 siblings, 1 reply; 24+ messages in thread From: Richard Henderson @ 2009-12-21 19:30 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: > How is "size-optimal trap" defined? E.g. Sparc and MIPS have "tcc" instructions that trap based on the condition codes, and so we eliminate the branch. That's the only optimization we apply with __builtin_trap. > Let me put it another way: I want this function to terminate with an > explicit NULL pointer dereference in every case. Then just use that. r~ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-21 19:30 ` Richard Henderson @ 2009-12-21 20:10 ` Russell King - ARM Linux 2009-12-22 14:09 ` Dave Korn 0 siblings, 1 reply; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-21 20:10 UTC (permalink / raw) To: Richard Henderson Cc: Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: > On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: >> How is "size-optimal trap" defined? > > E.g. Sparc and MIPS have "tcc" instructions that trap based on the > condition codes, and so we eliminate the branch. That's the only > optimization we apply with __builtin_trap. > >> Let me put it another way: I want this function to terminate with an >> explicit NULL pointer dereference in every case. > > Then just use that. That's precisely what we have been using for many years. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-21 20:10 ` Russell King - ARM Linux @ 2009-12-22 14:09 ` Dave Korn 2009-12-22 14:12 ` Russell King - ARM Linux 0 siblings, 1 reply; 24+ messages in thread From: Dave Korn @ 2009-12-22 14:09 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Richard Henderson, Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König Russell King - ARM Linux wrote: > On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: >> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: >>> How is "size-optimal trap" defined? >> E.g. Sparc and MIPS have "tcc" instructions that trap based on the >> condition codes, and so we eliminate the branch. That's the only >> optimization we apply with __builtin_trap. >> >>> Let me put it another way: I want this function to terminate with an >>> explicit NULL pointer dereference in every case. >> Then just use that. > > That's precisely what we have been using for many years. I don't understand. It should still work just fine; the original version posted appears to simply lack 'volatile' on the (int *) cast. cheers, DaveK ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-22 14:09 ` Dave Korn @ 2009-12-22 14:12 ` Russell King - ARM Linux 2009-12-22 14:49 ` Dave Korn 0 siblings, 1 reply; 24+ messages in thread From: Russell King - ARM Linux @ 2009-12-22 14:12 UTC (permalink / raw) To: Dave Korn Cc: Richard Henderson, Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König On Tue, Dec 22, 2009 at 02:09:02PM +0000, Dave Korn wrote: > Russell King - ARM Linux wrote: > > On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: > >> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: > >>> How is "size-optimal trap" defined? > >> E.g. Sparc and MIPS have "tcc" instructions that trap based on the > >> condition codes, and so we eliminate the branch. That's the only > >> optimization we apply with __builtin_trap. > >> > >>> Let me put it another way: I want this function to terminate with an > >>> explicit NULL pointer dereference in every case. > >> Then just use that. > > > > That's precisely what we have been using for many years. > > I don't understand. It should still work just fine; the original version > posted appears to simply lack 'volatile' on the (int *) cast. Neither do I - AFAIK the existing code works fine. I think this is just a noisy thread about people wanting to use the latest and greated compiler "features" whether they make sense to or not, and this thread should probably die until some problem has actually been identified. If it ain't broke, don't fix. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-22 14:12 ` Russell King - ARM Linux @ 2009-12-22 14:49 ` Dave Korn 0 siblings, 0 replies; 24+ messages in thread From: Dave Korn @ 2009-12-22 14:49 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Dave Korn, Richard Henderson, Richard Guenther, David Daney, gcc, Jamie Lokier, linux-kernel, linux-arm-kernel, Uwe Kleine-König Russell King - ARM Linux wrote: > [ ... ] this thread should probably die until some problem has actually > been identified. > > If it ain't broke, don't fix. <g> Couldn't agree more. Happy Christmas! cheers, DaveK ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] ARM: Convert BUG() to use unreachable() 2009-12-17 17:17 ` Richard Guenther 2009-12-17 18:17 ` Russell King - ARM Linux @ 2009-12-22 11:33 ` Paolo Bonzini 1 sibling, 0 replies; 24+ messages in thread From: Paolo Bonzini @ 2009-12-22 11:33 UTC (permalink / raw) To: Richard Guenther Cc: David Daney, Jamie Lokier, gcc, Uwe Kleine-König, linux-arm-kernel, linux-kernel On 12/17/2009 06:17 PM, Richard Guenther wrote: > It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure > use > __builtin_trap (); > instead for the whole sequence (the unreachable is implied then). > GCC choses a size-optimal trap representation for your target then. Agree that it shouldn't but just to be sure I'd use *(volatile int *)0 = 0; unreachable (); Paolo ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2009-12-22 14:33 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-12-08 9:55 [PATCH] ARM: Convert BUG() to use unreachable() Uwe Kleine-König 2009-12-08 17:07 ` David Daney 2009-12-10 17:50 ` Russell King - ARM Linux 2009-12-10 17:55 ` David Daney 2009-12-16 13:58 ` Uwe Kleine-König 2009-12-17 15:01 ` Jamie Lokier 2009-12-17 17:09 ` David Daney 2009-12-17 17:17 ` Richard Guenther 2009-12-17 18:17 ` Russell King - ARM Linux 2009-12-17 18:35 ` Joe Buck 2009-12-17 19:06 ` Russell King - ARM Linux 2009-12-17 19:14 ` Joe Buck 2009-12-17 19:33 ` David Daney 2009-12-17 19:33 ` Russell King - ARM Linux 2009-12-17 19:38 ` Jamie Lokier 2009-12-17 19:48 ` Russell King - ARM Linux 2009-12-17 19:58 ` Russell King - ARM Linux 2009-12-17 19:04 ` Jamie Lokier 2009-12-21 19:30 ` Richard Henderson 2009-12-21 20:10 ` Russell King - ARM Linux 2009-12-22 14:09 ` Dave Korn 2009-12-22 14:12 ` Russell King - ARM Linux 2009-12-22 14:49 ` Dave Korn 2009-12-22 11:33 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).