From: Jeff King <peff@peff•net>
To: Junio C Hamano <gitster@pobox•com>
Cc: Koakuma <koachan@protonmail•com>,
"git@vger•kernel.org" <git@vger•kernel.org>
Subject: Re: [PATCH 2/3] parse_pack_header_option(): avoid unaligned memory writes
Date: Sat, 18 Jan 2025 04:36:12 -0500 [thread overview]
Message-ID: <20250118093612.GC3474411@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq7c6s52ti.fsf@gitster.g>
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
next prev parent reply other threads:[~2025-01-18 9:36 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-17 3:30 [BUG] git crashes with a SIGBUS on sparc64 during pull Koakuma
2025-01-17 12:11 ` Jeff King
2025-01-17 12:52 ` Jeff King
2025-01-17 12:54 ` [PATCH 1/3] packfile: factor out --pack_header argument parsing Jeff King
2025-01-17 22:45 ` Junio C Hamano
2025-01-18 9:23 ` Jeff King
2025-01-18 16:57 ` Koakuma
2025-01-17 12:55 ` [PATCH 2/3] parse_pack_header_option(): avoid unaligned memory writes Jeff King
2025-01-18 1:27 ` Junio C Hamano
2025-01-18 9:36 ` Jeff King [this message]
2025-01-17 12:56 ` [PATCH 3/3] index-pack, unpack-objects: use skip_prefix to avoid magic number Jeff King
2025-01-17 15:55 ` [BUG] git crashes with a SIGBUS on sparc64 during pull Koakuma
2025-01-18 9:20 ` Jeff King
2025-01-18 16:50 ` Koakuma
2025-01-19 13:12 ` [PATCH v2 0/5] " Jeff King
2025-01-19 13:23 ` [PATCH v2 1/5] bswap.h: squelch potential sparse -Wcast-truncate warnings Jeff King
2025-01-19 13:23 ` [PATCH v2 2/5] packfile: factor out --pack_header argument parsing Jeff King
2025-01-19 13:23 ` [PATCH v2 3/5] parse_pack_header_option(): avoid unaligned memory writes Jeff King
2025-01-19 13:25 ` [PATCH v2 4/5] index-pack, unpack-objects: use get_be32() for reading pack header Jeff King
2025-01-19 13:25 ` [PATCH v2 5/5] index-pack, unpack-objects: use skip_prefix to avoid magic number Jeff King
2025-01-20 15:20 ` [PATCH v2 0/5] git crashes with a SIGBUS on sparc64 during pull Koakuma
2025-01-21 16:37 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250118093612.GC3474411@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=koachan@protonmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox