[PATCH] [4.8-rc7, regression] fault_in_multipages_readable() throws set-but-unused error

[PATCH] [4.8-rc7, regression] fault_in_multipages_readable() throws set-but-unused error

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When building XFS with -Werror, it now fails with:

./include/linux/pagemap.h: In function ¿fault_in_multipages_readable¿:
./include/linux/pagemap.h:602:16: error: variable ¿c¿ set but not used [-Werror=unused-but-set-variable]
  volatile char c;
                ^

This is a regression caused by commit e23d415 ("fix
fault_in_multipages_...() on architectures with no-op access_ok()").
Fix it by re-adding the "(void)c" trick taht was previously used to
make the compiler think the variable is used.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 include/linux/pagemap.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 7e3d537..01e8443 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -620,6 +620,7 @@ static inline int fault_in_multipages_readable(const char __user *uaddr,
 		return __get_user(c, end);
 	}
 
+	(void)c;
 	return 0;
 }
 
-- 
2.8.0.rc3

Re: [PATCH] [4.8-rc7, regression] fault_in_multipages_readable() throws set-but-unused error

[Linus Torvalds]

Thanks, applied.

I did happen to notice:

On Sun, Sep 25, 2016 at 4:57 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> ./include/linux/pagemap.h: In function ¿fault_in_multipages_readable¿:
> ./include/linux/pagemap.h:602:16: error: variable ¿c¿ set but not used [-Werror=unused-but-set-variable]

You have some nasty unicode corruption. The email is marked as being

  Content-Type: text/plain; charset=UTF-8
  Content-Transfer-Encoding: 8bit

but those are not the right unicode characters. I think gcc actually
uses back-tick and tick (which is ugly as hell in many fonts since
they aren't generally necessarily symmetric, but oh well). So some
cut-and-paste path of yours corrupted the utf8.

Obviously not a big deal, but you might want to look at your character
set setting in your mailer or other environment. Perhaps some odd
non-utf8 editor environment or something?

I also noticed:

> This is a regression caused by commit e23d415 ("fix
> fault_in_multipages_...() on architectures with no-op access_ok()").

I'd suggest doing

     git config --global core.abbrev 12

because the default git commit shortening value of 7 is practically
too short for the kernel these days. What can I say? I was naïve, and
based my original abbreviation decision on the BitKeeper numbers,
where we were just about to hit 64k commits.  So seven hex digits
seemed plenty. And now, ten years later, I just look stupid.

Anyway - while git will always pick a shortname that is unique (ie it
will add characters until it is unique), it's unique only at that
moment in time. And practically speaking seven hex digits can easily
have collisions in the hear future (ie it already happens with many
commits).

So telling git "give me a minimum of 12 hex digits in hash
abbreviations" should be fairly safe for a while.

I guess I should ping Junio and see if we can just change the default,
but for smaller projects the default abbreviation is likely still
reasonable. So I've been just mentioning that simple "set core.abbrev
to 12" to kernel people..

              Linus

Re: [PATCH] [4.8-rc7, regression] fault_in_multipages_readable() throws set-but-unused error

[Dave Chinner]

On Sun, Sep 25, 2016 at 06:21:05PM -0700, Linus Torvalds wrote:
> Thanks, applied.
> 
> I did happen to notice:
> 
> On Sun, Sep 25, 2016 at 4:57 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > ./include/linux/pagemap.h: In function ¿fault_in_multipages_readable¿:
> > ./include/linux/pagemap.h:602:16: error: variable ¿c¿ set but not used [-Werror=unused-but-set-variable]
> 
> You have some nasty unicode corruption. The email is marked as being
> 
>   Content-Type: text/plain; charset=UTF-8
>   Content-Transfer-Encoding: 8bit

It's whatever git-send-email spat out. I was under the impression it
encodes like that whenever it sees a utf-8 character in a commit...

> but those are not the right unicode characters. I think gcc actually
> uses back-tick and tick (which is ugly as hell in many fonts since
> they aren't generally necessarily symmetric, but oh well). So some
> cut-and-paste path of yours corrupted the utf8.

Yup, normally I remember to fix that up when composing the patch -
ever since GNU went to those fucked up "grammatically correct"
non-ascii quotes it's caused problems with pasting error messages
into ascii-only contexts.

> Obviously not a big deal, but you might want to look at your character
> set setting in your mailer or other environment. Perhaps some odd
> non-utf8 editor environment or something?

I turned off utf-8 support in vim on the machine I write all my code
on - I got sick of stupid stray marks in my code, digraphs being
composed when I just want to replace a single character, git sending
patches in utf-8 encoding because I copied a SoB with a utf-8
character in the name, etc....

> I also noticed:
> 
> > This is a regression caused by commit e23d415 ("fix
> > fault_in_multipages_...() on architectures with no-op access_ok()").
> 
> I'd suggest doing
> 
>      git config --global core.abbrev 12

Ok.

> because the default git commit shortening value of 7 is practically
> too short for the kernel these days.

7 characters, 12 characters, whatever. Neither make any sense in
commit messages by themselves without the short description that
goes along with the hash.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] [4.8-rc7, regression] fault_in_multipages_readable() throws set-but-unused error

[Linus Torvalds]

On Sun, Sep 25, 2016 at 7:25 PM, Dave Chinner <david@fromorbit.com> wrote:
>> You have some nasty unicode corruption. The email is marked as being
>>
>>   Content-Type: text/plain; charset=UTF-8
>>   Content-Transfer-Encoding: 8bit
>
> It's whatever git-send-email spat out. I was under the impression it
> encodes like that whenever it sees a utf-8 character in a commit...

Yes. But it assumes that the commit text was in UTF-8 too. So I
suspect it happened as you were writing the commit message:

> I turned off utf-8 support in vim on the machine I write all my code
> on

I guess that's probably it. You probably had vim try to convert it to
latin1 or something, and the odd utf-8 tick/back-tick thing doesn't do
so cleanly, so..

>- I got sick of stupid stray marks in my code, digraphs being
> composed when I just want to replace a single character, git sending
> patches in utf-8 encoding because I copied a SoB with a utf-8
> character in the name, etc....

Hmm. Generally, the only place we should have non-us-ascii tensd to be
exactly those names. But a sane editor should "just work" and not do
odd crazy digraph crap or things like that. But I don't use vim, so I
don't know what the magic incantation for sanity there is.

> 7 characters, 12 characters, whatever. Neither make any sense in
> commit messages by themselves without the short description that
> goes along with the hash.....

No, I agree, the right format for describing a commit tends to be
along the lines of

  .. commit %h ("%s") ..

exactly like you did.  It's just that with 12 characters in the hex
format, people will still be able to cut-and-paste the hash into git
to get the full commit details even a year from now. With just 7 hex
digits, you may well end up in the situation where you do

    git show e23d415

and git says

    error: short SHA1 e23d415 is ambiguous.
    fatal: ambiguous argument 'e23d415': unknown revision or path not
in the working tree.

(it's not ambiguous today, but in a year or two it quite possibly will be).

It's not impossible to figure out the different possible ambiguous
commits, but it's just inconvenient.

Right now, of the roughly 618,000 commits in the mainline tree, about
98% can be uniquely represented with 7 hex digits. But about 2% o
commits need eight or more hex digits to be unique (the git ID space
includes all the tree and blob objects too, so the uniqueness is not
just "unique within commits", but any kernel git object).

                Linus

Re: [PATCH] [4.8-rc7, regression] fault_in_multipages_readable() throws set-but-unused error

[Stephen Rothwell]

Hi all,

On Sun, 25 Sep 2016 20:06:19 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> exactly like you did.  It's just that with 12 characters in the hex
> format, people will still be able to cut-and-paste the hash into git
> to get the full commit details even a year from now. With just 7 hex
> digits, you may well end up in the situation where you do
> 
>     git show e23d415
> 
> and git says
> 
>     error: short SHA1 e23d415 is ambiguous.
>     fatal: ambiguous argument 'e23d415': unknown revision or path not
> in the working tree.
> 
> (it's not ambiguous today, but in a year or two it quite possibly will be).

I have already had over the years a few instances of 7 digit commit
hashes that are unique in Linus' tree but not in mine (which is a bit
of a pain if you are trying to look for context).

-- 
Cheers,
Stephen Rothwell