Subject: Re: [PATCH 2/3] parse_pack_header_option(): avoid unaligned memory writes

From: Jeff King <[email-masked]>
Date: Sat, 18 Jan 2025 04:36:12 -0500

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