* Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet [not found] <200707060939.l669drk2019988@imap1.linux-foundation.org> @ 2007-07-06 17:21 ` Linus Torvalds 2007-07-06 18:40 ` Derek M Jones 2007-07-06 19:53 ` Josh Triplett 0 siblings, 2 replies; 5+ messages in thread From: Linus Torvalds @ 2007-07-06 17:21 UTC (permalink / raw) To: akpm, Josh Triplett, Al Viro, linux-sparse; +Cc: adobriyan, ak This would be a sparse bug, I think. Returning a void-valued expression in a void-valued function is proper C99 (and gcc has allowed it for longer). And it often makes for nicer-looking code, with no real downsides. This seems to be a new bug introduced by Al Viro in sparse: -Wall should _not_ enable -Wreturn-void, I think (and I don't think Al meant it to: his commit message says "Conditional on -Wreturn-void", and he probably didn't think about the fact that -Wall will force them all on. Josh, Al and sparse mailing list added to participants list, and patch dropped. I think -Wreturn-void is fine, but it just shouldn't be part of -Wall, and thus shouldn't trigger for the kernl. Linus On Fri, 6 Jul 2007, akpm@linux-foundation.org wrote: > > From: Alexey Dobriyan <adobriyan@sw.ru> > > sparse now warns about > arch/x86_64/mm/mmap.c:15:3: warning: returning void-valued expression > > Generated code looks correct: there is jump to the end of > arch_pick_mmap_layout() after ia32_pick_mmap_layout(), but this should be > fixed regardless. > > Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru> > Cc: Andi Kleen <ak@suse.de> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > --- > > arch/x86_64/mm/mmap.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff -puN arch/x86_64/mm/mmap.c~x86_64-arch_pick_mmap_layout-fixlet arch/x86_64/mm/mmap.c > --- a/arch/x86_64/mm/mmap.c~x86_64-arch_pick_mmap_layout-fixlet > +++ a/arch/x86_64/mm/mmap.c > @@ -11,8 +11,10 @@ > void arch_pick_mmap_layout(struct mm_struct *mm) > { > #ifdef CONFIG_IA32_EMULATION > - if (current_thread_info()->flags & _TIF_IA32) > - return ia32_pick_mmap_layout(mm); > + if (current_thread_info()->flags & _TIF_IA32) { > + ia32_pick_mmap_layout(mm); > + return; > + } > #endif > mm->mmap_base = TASK_UNMAPPED_BASE; > if (current->flags & PF_RANDOMIZE) { > _ > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet 2007-07-06 17:21 ` [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet Linus Torvalds @ 2007-07-06 18:40 ` Derek M Jones 2007-07-06 18:48 ` Linus Torvalds 2007-07-06 19:53 ` Josh Triplett 1 sibling, 1 reply; 5+ messages in thread From: Derek M Jones @ 2007-07-06 18:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: akpm, Josh Triplett, Al Viro, linux-sparse, adobriyan, ak Linus, > Returning a void-valued expression in a void-valued function is > proper C99 (and gcc has allowed it for longer). And it often makes for No such change was made in C99. Sentence 1785 http://c0x.coding-guidelines.com/6.8.6.4.html I don't recall seeing a proposal for this in C9X. At the last WG14 meeting the gates were opened for C1X proposals. Perhaps somebody should propose this change. As you say there is prior art in gcc. -- Derek M. Jones tel: +44 (0) 1252 520 667 Knowledge Software Ltd mailto:derek@knosof.co.uk Applications Standards Conformance Testing http://www.knosof.co.uk ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet 2007-07-06 18:40 ` Derek M Jones @ 2007-07-06 18:48 ` Linus Torvalds 0 siblings, 0 replies; 5+ messages in thread From: Linus Torvalds @ 2007-07-06 18:48 UTC (permalink / raw) To: Derek M Jones; +Cc: akpm, Josh Triplett, Al Viro, linux-sparse, adobriyan, ak On Fri, 6 Jul 2007, Derek M Jones wrote: > > > Returning a void-valued expression in a void-valued function is proper C99 > > (and gcc has allowed it for longer). And it often makes for > > No such change was made in C99. > Sentence 1785 http://c0x.coding-guidelines.com/6.8.6.4.html Oops. Ok, my bad. It's apparently just a long-time gcc feature. It's one that I was initially surprised by, but it's certainly a sensible one from a type standpoint, and a lot better thought out than some gcc extensions ;) So I do actually like it. It's much nicer exactly for code like if (somecase) return that_case_handler(); where the type of the function to handle the case matches the type of the calling function (regardless of whether that type is void or not, but void obviously is a special case). Linus ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet 2007-07-06 17:21 ` [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet Linus Torvalds 2007-07-06 18:40 ` Derek M Jones @ 2007-07-06 19:53 ` Josh Triplett 2007-07-06 21:39 ` Pavel Roskin 1 sibling, 1 reply; 5+ messages in thread From: Josh Triplett @ 2007-07-06 19:53 UTC (permalink / raw) To: Linus Torvalds; +Cc: akpm, Josh Triplett, Al Viro, linux-sparse, adobriyan, ak On Fri, 2007-07-06 at 10:21 -0700, Linus Torvalds wrote: > This would be a sparse bug, I think. > > Returning a void-valued expression in a void-valued function is > proper C99 (and gcc has allowed it for longer). And it often makes for > nicer-looking code, with no real downsides. > > This seems to be a new bug introduced by Al Viro in sparse: -Wall should > _not_ enable -Wreturn-void, I think (and I don't think Al meant it to: his > commit message says "Conditional on -Wreturn-void", and he probably didn't > think about the fact that -Wall will force them all on. > > Josh, Al and sparse mailing list added to participants list, and patch > dropped. I think -Wreturn-void is fine, but it just shouldn't be part of > -Wall, and thus shouldn't trigger for the kernl. The kernel does not seem to use sparse -Wall by default. Did the patch submitter use CF=-Wall or CF=-Wreturn-void, or otherwise change the set of warnings emitted by Sparse? I do think the sparse warning logic needs some work. -Wall logically should mean "all warnings", but gcc historically uses it to mean "some reasonable set of warnings". However, since Sparse exists solely to give warnings, it defaults to "some reasonable set of warnings", and uses -Wall for "all warnings". Perhaps for gcc compatibility, that should change. I'd love to hear suggestions for how Sparse should behave. A few options: * Leave the current behavior, and tell people annoyed by Sparse warnings they don't care about that they should either avoid -Wall or use -Wall -Wno-warning-i-do-not-care-about. * Ignore -Wall since we already give a reasonable default set of warnings, and possibly introduce a -Weverything or similar that really does mean "turn on every Sparse warning". * Change -Wall to mean "turn on some additional warnings, but not all of them". We then have to decide which warnings -Wall should enable, and again we might want to add a -Weverything. * Turn off all warnings by default unless you give -Wall. This seems entirely broken, given the purpose of Sparse. > On Fri, 6 Jul 2007, akpm@linux-foundation.org wrote: > > From: Alexey Dobriyan <adobriyan@sw.ru> > > > > sparse now warns about > > arch/x86_64/mm/mmap.c:15:3: warning: returning void-valued expression - Josh Triplett ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet 2007-07-06 19:53 ` Josh Triplett @ 2007-07-06 21:39 ` Pavel Roskin 0 siblings, 0 replies; 5+ messages in thread From: Pavel Roskin @ 2007-07-06 21:39 UTC (permalink / raw) To: Josh Triplett Cc: Linus Torvalds, akpm, Josh Triplett, Al Viro, linux-sparse, adobriyan, ak On Fri, 2007-07-06 at 12:53 -0700, Josh Triplett wrote: > I do think the sparse warning logic needs some work. -Wall logically > should mean "all warnings", but gcc historically uses it to mean "some > reasonable set of warnings". However, since Sparse exists solely to > give warnings, it defaults to "some reasonable set of warnings", and > uses -Wall for "all warnings". Perhaps for gcc compatibility, that > should change. I agree in principle, but I'm yet to hear of a warning that sparse emits and that should not be enabled by -Wall. > I'd love to hear suggestions for how Sparse should > behave. A few options: > > * Leave the current behavior, and tell people annoyed by Sparse > warnings they don't care about that they should either avoid > -Wall or use -Wall -Wno-warning-i-do-not-care-about. That's the best approach until sparse learns to report some ridiculously minor warnings. > * Ignore -Wall since we already give a reasonable default set of > warnings, and possibly introduce a -Weverything or similar that > really does mean "turn on every Sparse warning". gcc has -Wextra (formerly -W) to enable some (but still not all) warnings not enabled by -Wall. The warnings not enabled by -Wall -Wextra include e.g. -Wlong-long and -Winline, which search so some specific conditions (long long used, function cannot be inlined etc). I'm ignoring "-pedantic", "-ansi" and language standards for now, as they have more effect than just the set of warnings. If we want to be (sort of) gcc compatible, we would have following groups: 1) Severe warnings - always enabled. Such warnings should always be avoided by the programmers, as they are likely to break or they indicate some non-trivial assumptions that should be better expressed in the code. Example - implicit cast from pointer to integer. 2) Major warnings - enabled by -Wall. They may be harder to find and trickier to avoid, but they should be avoided too. They may indicate poorly written but correct code. Example - missing prototype. 3) Minor warnings - enabled by -Wextra. Such warnings may be OK in some cases, although pedantic coders would want to avoid them. Example - comparing signed and unsigned integers. 4) Specific warnings - enabled by specific flags. Those are not warnings in a usual sense. They are used to look for some conditions that can affect e.g. performance retardation or portability to some unusual platforms. Examples - use of long long, explicit cast from pointer to integer. Some of the groups can be are empty now. If no warning can be classified as "major", then sparse will ignore -Wall. Given that warnings is sparse's main product, I can imagine that to be the case, i.e. nothing is hard for sparse to find. > * Change -Wall to mean "turn on some additional warnings, but not > all of them". We then have to decide which warnings -Wall > should enable, and again we might want to add a -Weverything. As I said, whether sparse recognizes -Wall depends on classification of the warnings. I'm not very enthusiastic about -Weverything, as it would (eventually) dump together "warnings" that are not actually warnings. (Oh horror, your program uses long long and casts pointers to integers explicitly!) > * Turn off all warnings by default unless you give -Wall. This > seems entirely broken, given the purpose of Sparse. Indeed, that would be wrong, unless we decide that sparse cannot recognize any severe warnings. -- Regards, Pavel Roskin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-07-06 21:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <200707060939.l669drk2019988@imap1.linux-foundation.org>
2007-07-06 17:21 ` [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet Linus Torvalds
2007-07-06 18:40 ` Derek M Jones
2007-07-06 18:48 ` Linus Torvalds
2007-07-06 19:53 ` Josh Triplett
2007-07-06 21:39 ` Pavel Roskin
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).