Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet

Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet

[Linus Torvalds]

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) {
> _
> 

Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet

[Derek M Jones]

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

Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet

[Linus Torvalds]

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

Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet

[Josh Triplett]

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

Re: [patch 5/9] x86_64: arch_pick_mmap_layout() fixlet

[Pavel Roskin]

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