Ævar Arnfjörð Bjarmason [off-list ref] 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.
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 [off-list ref]
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
On Mon, Oct 4, 2010 at 17:54, Junio C Hamano [off-list ref] 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