[PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Ævar Arnfjörð Bjarmason]

Wrap "S_IFREG | 0644" in parentheses to avoid a "suggest parentheses
around arithmetic in operand of |" warning from GCC 4.1.3 on NetBSD
5.0.2.

I spotted and fixed this independently on NetBSD, but later found that
there was a NetBSD Problem Report that included this fix.

NetBSD-PR: http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168
Reported-by: Dan McMahill <dmcmahill@NetBSD.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index a43d326..e8b7e09 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -93,7 +93,7 @@ static int builtin_diff_blobs(struct rev_info *revs,
 			      int argc, const char **argv,
 			      struct blobinfo *blob)
 {
-	unsigned mode = canon_mode(S_IFREG | 0644);
+	unsigned mode = canon_mode((S_IFREG | 0644));
 
 	if (argc > 1)
 		usage(builtin_diff_usage);
-- 
1.7.3.159.g610493

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> -	unsigned mode = canon_mode(S_IFREG | 0644);
> +	unsigned mode = canon_mode((S_IFREG | 0644));

Just curious:

#define canon_mode(mode) \
        (S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
        S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)

#define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)

Since S_ISREG et al are macros, typically they would put their
argument in parentheses in the definition.  How are they defined
in NetBSD sys/stat.h?  What is canon_mode(S_IFREG | 0644) being
misinterpreted to mean?

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 4, 2010 at 09:35, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> -     unsigned mode = canon_mode(S_IFREG | 0644);
>> +     unsigned mode = canon_mode((S_IFREG | 0644));
>
> Just curious:
>
> #define canon_mode(mode) \
>        (S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
>        S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)
>
> #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)
>
> Since S_ISREG et al are macros, typically they would put their
> argument in parentheses in the definition.  How are they defined
> in NetBSD sys/stat.h?  What is canon_mode(S_IFREG | 0644) being
> misinterpreted to mean?

Oh it's a bug in NetBSD, sorry for not being explicit about that:

    $ grep S_ISREG /usr/include/sys/stat.h
    #define S_ISREG(m)      ((m & _S_IFMT) == _S_IFREG)     /* regular file */

    $ grep S_ISREG /usr/include/linux/stat.h
    #define S_ISREG(m)      (((m) & S_IFMT) == S_IFREG)

I.e. GCC sees `S_IFREG | 0644 & _S_IFMT' on NetBSD but `(S_IFREG |
0644) & _S_IFMT' on Linux.

Since bitwise AND (&) has precedence over bitwise OR it's probably a
logic error on NetBSD too, not just an annoying warning.

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Oh it's a bug in NetBSD, sorry for not being explicit about that:
>
>     $ grep S_ISREG /usr/include/sys/stat.h
>     #define S_ISREG(m)      ((m & _S_IFMT) == _S_IFREG)     /* regular file */
>
>     $ grep S_ISREG /usr/include/linux/stat.h
>     #define S_ISREG(m)      (((m) & S_IFMT) == S_IFREG)
>
> I.e. GCC sees `S_IFREG | 0644 & _S_IFMT' on NetBSD but `(S_IFREG |
> 0644) & _S_IFMT' on Linux.
>
> Since bitwise AND (&) has precedence over bitwise OR it's probably a
> logic error on NetBSD too, not just an annoying warning.

In that case, I'd prefer to work this around at the definition of
canon_mode(), like

    /*
     * extra ()-pair around S_ISREG() and friends to work around platform
     * header with buggy definitions like
     * #define S_ISREG(x) ((x & _S_IFMT) == _S_IFREG)
     */
    #define canon_mode(mode) \
    	(S_ISREG((mode)) ? (S_IFREG | ce_permissions(mode)) : \
        ...

instead of contaminating the calling sites.  Otherwise new calling sites
we will add in the future need to be aware of the same bug for no good
reason.

[PATCH v2] cache.h: work around broken NetBSD system headers

[Ævar Arnfjörð Bjarmason]

Wrap the `mode' argument to the `canon_mode' macro in extra
parentheses to avoid a bug with the S_* macros in sys/stat.h on
NetBSD.

This issue was originally spotted in NetBSD Problem Report #42168 and
worked around by der Mouse, but later filed as a bug with NetBSD
itself in NetBSD Problem Report #43937 by me.

The issue is that NetBSD doesn't take care to wrap its macro arguments
in parentheses, so on Linux and other sane systems we have S_ISREG(m)
defined as something like:

    (((m) & S_IFMT) == S_IFREG)

But on NetBSD:

    ((m & _S_IFMT) == _S_IFREG)

Since a caller in builtin/diff.c called our macro as `S_IFREG | 0644'
this bug introduced a logic error on NetBSD, since the precedence of
bit-wise & is higher than | in C.

NetBSD-PR: http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=43937
Originally-reported-by: der Mouse <mouse@Rodents-Montreal.ORG>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, Oct 4, 2010 at 17:54, Junio C Hamano <gitster@pobox.com> wrote:
> instead of contaminating the calling sites.  Otherwise new calling sites
> we will add in the future need to be aware of the same bug for no good
> reason.

Agreed. Here's a v2 that does that. With an updated commit message to
mention the bug I filed with NetBSD.

 cache.h |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 2ef2fa3..891d5d0 100644
--- a/cache.h
+++ b/cache.h
@@ -277,9 +277,15 @@ static inline int ce_to_dtype(const struct cache_entry *ce)
 	else
 		return DT_UNKNOWN;
 }
+
+/*
+ * We use extra parentheses around mode to work around a NetBSD issue
+ * described in NetBSD Problem Report #43937. See
+ * http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=43937
+ */
 #define canon_mode(mode) \
-	(S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
-	S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)
+	(S_ISREG((mode)) ? (S_IFREG | ce_permissions(mode)) : \
+	S_ISLNK((mode)) ? S_IFLNK : S_ISDIR((mode)) ? S_IFDIR : S_IFGITLINK)
 
 #define flexible_size(STRUCT,len) ((offsetof(struct STRUCT,name) + (len) + 8) & ~7)
 #define cache_entry_size(len) flexible_size(cache_entry,len)
-- 
1.7.3.159.g610493

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 4, 2010 at 09:21, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Wrap "S_IFREG | 0644" in parentheses to avoid a "suggest parentheses
> around arithmetic in operand of |" warning from GCC 4.1.3 on NetBSD
> 5.0.2.
>
> I spotted and fixed this independently on NetBSD, but later found that
> there was a NetBSD Problem Report that included this fix.

With this and Jonathan's xdiff patch git compiles without warnings on
NetBSD, aside from this:

    imap-send.c: In function 'ssl_socket_connect':
    imap-send.c:310: warning: assignment discards qualifiers from
pointer target type
    imap-send.c:312: warning: assignment discards qualifiers from
pointer target type

I don't see a sane way around that[1], since it appears the NetBSD
people have patched openssl's function definitions without bumping the
OpenSSL version number. Either that or OpenSSL itself changed from
const char* to char* to const char* again, I didn't investigate that.

But tests on NetBSD with /bin/sh still fail since we use cd -P, but we
have unapplied patches for that so I didn't pursue it:

    http://article.gmane.org/gmane.comp.version-control.git/136561/match=
    http://article.gmane.org/gmane.comp.version-control.git/136562/match=

1. We could check for __NetBSD__ and the NetBSD version, but it's not
   worthwhile for a single warning.

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> But tests on NetBSD with /bin/sh still fail since we use cd -P, but we
> have unapplied patches for that so I didn't pursue it:
> 
>     http://article.gmane.org/gmane.comp.version-control.git/136561/match=
>     http://article.gmane.org/gmane.comp.version-control.git/136562/match=

Aren't these v1.7.0-rc0~76^2 and v1.7.0-rc0~76^2^?

Here's a patch for the more important of the remaining problems.  I'm
just guessing here; untested, of course.

-- 8< --
Subject: tests: use pwd -P to simulate cd -P for portability

NetBSD supports pwd -P but not cd -P.  POSIX has required both for
a while, so this should not be an issue for most Unix-like platforms.

The test harness uses cd -P to ensure $PWD and $(pwd) agree;
cd $(pwd -P) should do that, too.

If pwd -P fails on some platform, with this patch, the test harness
will die with 'FATAL: Unexpected exit with code 1'.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 830e5e7..184bf84 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -916,9 +916,10 @@ rm -fr "$test" || {
 }
 
 test_create_repo "$test"
-# Use -P to resolve symlinks in our working directory so that the cwd
-# in subprocesses like git equals our $PWD (for pathname comparisons).
-cd -P "$test" || exit 1
+cd "$test" || exit 1
+# Resolve symlinks in our working directory so that the cwd in
+# subprocesses like git equals our $PWD (for pathname comparisons).
+dir=$(pwd -P) && cd "$dir" || exit 1
 
 HOME=$(pwd)
 export HOME
-- 
1.7.2.3

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Jonathan Nieder]

Jonathan Nieder wrote:

> The test harness uses cd -P to ensure $PWD and $(pwd) agree;

As Ævar noticed, this sentence as it stands doesn't make much sense.
The idea is rather to make $(pwd -L) and $(pwd -P) agree --- the
former is accessible through $PWD, the latter through getwd() and
/bin/pwd.

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[René Scharfe]

Am 04.10.2010 11:21, schrieb Ævar Arnfjörð Bjarmason:
> -	unsigned mode = canon_mode(S_IFREG | 0644);
> +	unsigned mode = canon_mode((S_IFREG | 0644));

That doesn't look pretty.

How about something like the following instead?  It untangles the
?-:-chain in canon_mode and allows passing of an argument with side
effects.  All the S_ISxxx macros get a single variable as parameter.
Does it fix the issue on NetBSD?
---
 cache.h |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 2ef2fa3..3d5ed51 100644
--- a/cache.h
+++ b/cache.h
@@ -277,9 +277,16 @@ static inline int ce_to_dtype(const struct cache_entry *ce)
 	else
 		return DT_UNKNOWN;
 }
-#define canon_mode(mode) \
-	(S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
-	S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)
+static inline unsigned int canon_mode(unsigned int mode)
+{
+	if (S_ISREG(mode))
+		return S_IFREG | ce_permissions(mode);
+	if (S_ISLNK(mode))
+		return S_IFLNK;
+	if (S_ISDIR(mode))
+		return S_IFDIR;
+	return S_IFGITLINK;
+}
 
 #define flexible_size(STRUCT,len) ((offsetof(struct STRUCT,name) + (len) + 8) & ~7)
 #define cache_entry_size(len) flexible_size(cache_entry,len)
-- 
1.7.3

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Matthieu Moy]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> How about something like the following instead?

> diff --git a/cache.h b/cache.h
> index 2ef2fa3..3d5ed51 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -277,9 +277,16 @@ static inline int ce_to_dtype(const struct cache_entry *ce)
>  	else
>  		return DT_UNKNOWN;
>  }
> -#define canon_mode(mode) \
> -	(S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
> -	S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)
> +static inline unsigned int canon_mode(unsigned int mode)
> +{
> +	if (S_ISREG(mode))
> +		return S_IFREG | ce_permissions(mode);
> +	if (S_ISLNK(mode))
> +		return S_IFLNK;
> +	if (S_ISDIR(mode))
> +		return S_IFDIR;
> +	return S_IFGITLINK;
> +}

That sounds much better to me. I don't know whether it fixes the issue
on NetBSD though.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[Junio C Hamano]

Looks much saner; thanks.  I think we can steal the log message from Ævar
v2 to explain what this change is about.

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[René Scharfe]

Am 04.10.2010 21:23, schrieb Junio C Hamano:
> Looks much saner; thanks.  I think we can steal the log message from Ævar
> v2 to explain what this change is about.

Good idea. :)

I forgot this:

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

[yj2133011]

Write good cheer, host.


--------------------------------------------------------
This 
http://www.tomtop.com/black-remote-controller-charger2x-2800mah-battery-packs-for-wii_p11124.html?aid=z
Wii Charger  looks good n stylish. It lights up all blue which made me think
this is good. Get a 
http://www.tomtop.com/mini-bluetooth-keyboard-for-ps3-mac-os-android-pc-pda.html?aid=z
Wireless Keyboard  and mouse combo for your computer workstation and reduce
desktop clutter.

-----
The voice input and output is very good in this 
http://www.tomtop.com/black-ps3-wireless-bluetooth-headset-for-playstation-3.html?aid=z
Wireless PS3 Headset . It is compatible with all PS3 games.Buy from Reliable 
http://www.tomtop.com/google-android-7-notebook-3g-tablet-pc-umpc-wifi-mid-pda.html?aid=z
Google Android PC  apad Wholesalers.
-- 
View this message in context: http://git.661346.n2.nabble.com/PATCH-diff-S-IFREG-0644-to-S-IFREG-0644-to-avoid-warning-tp5598387p5598910.html
Sent from the git mailing list archive at Nabble.com.