Re: [PATCH 2/3] parse_pack_header_option(): avoid unaligned memory writes
- From
Jeff King <peff@peff.net>
- Date
- Jan 18, 2025, 09:36 UTC
- Message-ID
- <20250118093612.GC3474411@coredump.intra.peff.net>
- In-Reply-To
- <xmqq7c6s52ti.fsf@gitster.g>
On Fri, Jan 17, 2025 at 05:27:21PM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> 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.
Show 6 quoted lines
> 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