# [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning 13 messages from 2010-10-04 to 2010-10-04. Participants: Ævar Arnfjörð Bjarmason, Jonathan Nieder, René Scharfe, Matthieu Moy, Junio C Hamano, yj2133011. Thread: https://gitlist.dev/t/25331 ## Ævar Arnfjörð Bjarmason, 2010-10-04 09:21 Subject: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <1286184071-28457-1-git-send-email-avarab@gmail.com> URL: https://gitlist.dev/e/1286184071-28457-1-git-send-email-avarab%40gmail.com ``` 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 Signed-off-by: Ævar Arnfjörð Bjarmason --- 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 ``` ## Jonathan Nieder, 2010-10-04 09:35 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <20101004093519.GN24884@burratino> URL: https://gitlist.dev/e/20101004093519.GN24884%40burratino In-Reply-To: <1286184071-28457-1-git-send-email-avarab@gmail.com> ``` Æ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, 2010-10-04 09:42 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: URL: https://gitlist.dev/e/AANLkTim%2BQH6%3DXJikOsZs%3Dqt9ZFHjP7gWz0zo9n4vz%2Bgk%40mail.gmail.com In-Reply-To: <1286184071-28457-1-git-send-email-avarab@gmail.com> ``` On Mon, Oct 4, 2010 at 09:21, Ævar Arnfjörð Bjarmason 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. ``` ## Ævar Arnfjörð Bjarmason, 2010-10-04 09:47 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: URL: https://gitlist.dev/e/AANLkTimBzC7f7teH8Rriykg7o5kwX6mD2g2q-0yOXSiF%40mail.gmail.com In-Reply-To: <20101004093519.GN24884@burratino> ``` On Mon, Oct 4, 2010 at 09:35, Jonathan Nieder 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. ``` ## Jonathan Nieder, 2010-10-04 10:35 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <20101004103550.GA4738@burratino> URL: https://gitlist.dev/e/20101004103550.GA4738%40burratino In-Reply-To: ``` Æ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 --- 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 ``` ## Jonathan Nieder, 2010-10-04 10:50 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <20101004105008.GB4738@burratino> URL: https://gitlist.dev/e/20101004105008.GB4738%40burratino In-Reply-To: <20101004103550.GA4738@burratino> ``` 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, 2010-10-04 10:53 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <4CA9B217.6050600@lsrfire.ath.cx> URL: https://gitlist.dev/e/4CA9B217.6050600%40lsrfire.ath.cx In-Reply-To: <1286184071-28457-1-git-send-email-avarab@gmail.com> ``` 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 ``` ## Matthieu Moy, 2010-10-04 11:45 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: URL: https://gitlist.dev/e/vpqsk0mp4fc.fsf%40bauges.imag.fr In-Reply-To: <4CA9B217.6050600@lsrfire.ath.cx> ``` René Scharfe 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/ ``` ## yj2133011, 2010-10-04 12:28 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <1286195322208-5598910.post@n2.nabble.com> URL: https://gitlist.dev/e/1286195322208-5598910.post%40n2.nabble.com In-Reply-To: <1286184071-28457-1-git-send-email-avarab@gmail.com> ``` 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. ``` ## Junio C Hamano, 2010-10-04 17:54 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <7v8w2d972o.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8w2d972o.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Ævar Arnfjörð Bjarmason 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. ``` ## Ævar Arnfjörð Bjarmason, 2010-10-04 18:10 Subject: [PATCH v2] cache.h: work around broken NetBSD system headers Message-ID: <1286215813-21224-1-git-send-email-avarab@gmail.com> URL: https://gitlist.dev/e/1286215813-21224-1-git-send-email-avarab%40gmail.com In-Reply-To: <7v8w2d972o.fsf@alter.siamese.dyndns.org> ``` 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 Signed-off-by: Ævar Arnfjörð Bjarmason --- On Mon, Oct 4, 2010 at 17:54, Junio C Hamano 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 ``` ## Junio C Hamano, 2010-10-04 19:23 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <7v4od192zo.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4od192zo.fsf%40alter.siamese.dyndns.org In-Reply-To: <4CA9B217.6050600@lsrfire.ath.cx> ``` Looks much saner; thanks. I think we can steal the log message from Ævar v2 to explain what this change is about. ``` ## René Scharfe, 2010-10-04 21:11 Subject: Re: [PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning Message-ID: <4CAA4311.1070408@lsrfire.ath.cx> URL: https://gitlist.dev/e/4CAA4311.1070408%40lsrfire.ath.cx In-Reply-To: <7v4od192zo.fsf@alter.siamese.dyndns.org> ``` 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 ```