threads / patch / 25331

patchdiff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

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

## tl;dr

13 messages between Oct 4, 2010 and Oct 4, 2010. Diffs are folded; open one to read it.

replies: 12people: 6as markdown or json

Ævar Arnfjörð Bjarmason· Oct 4, 2010, 09:21 UTC · lore

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(-)
Show changes to builtin/diff.c +1 −1
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
Jonathan Nieder· Oct 4, 2010, 09:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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?

Ævar Arnfjörð Bjarmason· Oct 4, 2010, 09:47 UTC · re: Jonathan Nieder · lore

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

On Mon, Oct 4, 2010 at 09:35, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 17 quoted lines
> Æ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.

Junio C Hamano· Oct 4, 2010, 17:54 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 13 quoted lines
> 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.

Ævar Arnfjörð Bjarmason· Oct 4, 2010, 18:10 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to cache.h +8 −2
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
Ævar Arnfjörð Bjarmason· Oct 4, 2010, 09:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Mon, Oct 4, 2010 at 09:21, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 6 quoted lines
> 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.
Jonathan Nieder· Oct 4, 2010, 10:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> 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(-)
Show changes to t/test-lib.sh +4 −3
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
Jonathan Nieder· Oct 4, 2010, 10:50 UTC · re: Jonathan Nieder · lore

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

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.

René Scharfe· Oct 4, 2010, 10:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to cache.h +10 −3
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
Matthieu Moy· Oct 4, 2010, 11:45 UTC · re: René Scharfe · lore

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

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> How about something like the following instead?
Show 21 quoted lines
> 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/
René Scharfe· Oct 4, 2010, 21:11 UTC · re: Junio C Hamano · lore

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

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>
yj2133011· Oct 4, 2010, 12:28 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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.

← back to recent threads