| Message ID | 20250117125530.GB2893666@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4f02f4d68d8eefe728008974640839ef6e1b2182 |
| Headers | show |
| Series | [1/3] packfile: factor out --pack_header argument parsing | expand |
Jeff King <peff@peff.net> writes:
> + put_be32(hdr, PACK_SIGNATURE);
Tonight's comedy. PACK_SIGNATURE is defined as 0x5041434b (in pack.h)
In <compat/bswap.h> we want to take advantage of the fact that
assigning any unsigned integer to *(unsigned char *) would assign
the integer's least significant 8 bits.
static inline void put_be32(void *ptr, uint32_t value)
{
unsigned char *p = ptr;
p[0] = value >> 24;
p[1] = value >> 16;
p[2] = value >> 8;
p[3] = value >> 0;
}
But sparse seems not to like that.
compat/bswap.h:175:22: error: cast truncates bits from constant value (5041 becomes 41)
compat/bswap.h:176:22: error: cast truncates bits from constant value (504143 becomes 43)
compat/bswap.h:177:22: error: cast truncates bits from constant value (5041434b becomes 4b)
Of course we could do the mask, but should we have to?
I think the real compiler would be clever ehough to produce the
identical binary with the following patch that is only needed to
squelch this error, but I feel dirty after writing this.
By the way, a "git grep" finds
put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
in the fsmonitor.c file, which does not get flagged only because the
CPP macro expands to a small integer (2). That is doubly insulting.
compat/bswap.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git c/compat/bswap.h w/compat/bswap.h
index 512f6f4b99..b34054f2bd 100644
--- c/compat/bswap.h
+++ w/compat/bswap.h
@@ -171,23 +171,23 @@ static inline uint64_t get_be64(const void *ptr)
static inline void put_be32(void *ptr, uint32_t value)
{
unsigned char *p = ptr;
- p[0] = value >> 24;
- p[1] = value >> 16;
- p[2] = value >> 8;
- p[3] = value >> 0;
+ p[0] = (value >> 24) & 0xff;
+ p[1] = (value >> 16) & 0xff;
+ p[2] = (value >> 8) & 0xff;
+ p[3] = (value >> 0) & 0xff;
}
static inline void put_be64(void *ptr, uint64_t value)
{
unsigned char *p = ptr;
- p[0] = value >> 56;
- p[1] = value >> 48;
- p[2] = value >> 40;
- p[3] = value >> 32;
- p[4] = value >> 24;
- p[5] = value >> 16;
- p[6] = value >> 8;
- p[7] = value >> 0;
+ p[0] = (value >> 56) & 0xff;
+ p[1] = (value >> 48) & 0xff;
+ p[2] = (value >> 40) & 0xff;
+ p[3] = (value >> 32) & 0xff;
+ p[4] = (value >> 24) & 0xff;
+ p[5] = (value >> 16) & 0xff;
+ p[6] = (value >> 8) & 0xff;
+ p[7] = (value >> 0) & 0xff;
}
#endif /* COMPAT_BSWAP_H */
On Fri, Jan 17, 2025 at 05:27:21PM -0800, Junio C Hamano wrote: > static inline void put_be32(void *ptr, uint32_t value) > { > unsigned char *p = ptr; > p[0] = value >> 24; > p[1] = value >> 16; > p[2] = value >> 8; > p[3] = value >> 0; > } > > But sparse seems not to like that. > > compat/bswap.h:175:22: error: cast truncates bits from constant value (5041 becomes 41) > compat/bswap.h:176:22: error: cast truncates bits from constant value (504143 becomes 43) > compat/bswap.h:177:22: error: cast truncates bits from constant value (5041434b becomes 4b) > > Of course we could do the mask, but should we have to? Cute. I think the above is well defined in terms of the C standard. But I could see how a linter might want to remind you that you're truncating a constant. It is kind of lame that it only flags the call with a constant. If you want to warn people that they are accidentally truncating, surely it's obvious in the code above that truncation is _possible_ depending on the value. It seems like it's either worth flagging as a dangerous construct, or not; but doing it only for a constant is not super helpful. > I think the real compiler would be clever ehough to produce the > identical binary with the following patch that is only needed to > squelch this error, but I feel dirty after writing this. I checked with "gcc -s" and it produces the same asm before and after your patch, with both -O0 and -O2. So I don't think there's a practical downside. As far as feeling dirty, I dunno. It is basically telling any linter "yes, I know we are truncating here". Since it is contained within put_be32() and won't spread across the code base, I'm not too offended by it. I guess the other option is to pass -Wno-cast-truncate to sparse. > By the way, a "git grep" finds > > put_be32(&hdr_version, INDEX_EXTENSION_VERSION2); > > in the fsmonitor.c file, which does not get flagged only because the > CPP macro expands to a small integer (2). That is doubly insulting. Heh. Yeah, that goes back to my "kind of lame" comment above. ;) -Peff
diff --git a/packfile.c b/packfile.c index 2bf9e57330..2d80d80cb3 100644 --- a/packfile.c +++ b/packfile.c @@ -2318,17 +2318,20 @@ int is_promisor_object(struct repository *r, const struct object_id *oid) int parse_pack_header_option(const char *in, unsigned char *out, unsigned int *len) { - struct pack_header *hdr; + unsigned char *hdr; char *c; - hdr = (struct pack_header *)out; - hdr->hdr_signature = htonl(PACK_SIGNATURE); - hdr->hdr_version = htonl(strtoul(in, &c, 10)); + hdr = out; + put_be32(hdr, PACK_SIGNATURE); + hdr += 4; + put_be32(hdr, strtoul(in, &c, 10)); + hdr += 4; if (*c != ',') return -1; - hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10)); + put_be32(hdr, strtoul(c + 1, &c, 10)); + hdr += 4; if (*c) return -1; - *len = sizeof(*hdr); + *len = hdr - out; return 0; }
In order to recreate a pack header in our in-memory buffer, we cast the buffer to a "struct pack_header" and assign the individual fields. This is reported to cause SIGBUS on sparc64 due to alignment issues. We can work around this by using put_be32() which will write individual bytes into the buffer. Reported-by: Koakuma <koachan@protonmail.com> Signed-off-by: Jeff King <peff@peff.net> --- Fingers crossed that this is sufficient, and we don't have more alignment problems. packfile.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)