[Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

[Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

[Fernando]

From: Fernando Gouveia Lima <fernandolimabusiness@gmail.com>

Two comparisions between int and size_t, and int and unsigned long int
cause warning sign compare to fire.

Avoid this by changing the type of variable "i" in add_ref_decoration() to unsigned long int and
casting the variable "filename->len" to int in fmt_output_subject().

Signed-off-by: Fernando Gouveia Lima <fernandolimabusiness@gmail.com>
---
This is my first contribution and i got this idea on the Microproject
page. I hope this is the first of many contributions to the git
community :).

 log-tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 1d05dc1c70..e0848fcccc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -151,7 +151,7 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,
 			      int flags UNUSED,
 			      void *cb_data)
 {
-	int i;
+	long unsigned int i;
 	struct object *obj;
 	enum object_type objtype;
 	enum decoration_type deco_type = DECORATION_NONE;
@@ -458,7 +458,7 @@ void fmt_output_subject(struct strbuf *filename,
 	}
 	strbuf_addf(filename, "%04d-%s", nr, subject);
 
-	if (max_len < filename->len)
+	if (max_len < (int) filename->len)
 		strbuf_setlen(filename, max_len);
 	strbuf_addstr(filename, suffix);
 }
-- 
2.34.1

Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

[Junio C Hamano]

Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:

> @@ -151,7 +151,7 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,
>  			      int flags UNUSED,
>  			      void *cb_data)
>  {
> -	int i;
> +	long unsigned int i;
>  	struct object *obj;
>  	enum object_type objtype;
>  	enum decoration_type deco_type = DECORATION_NONE;

The complaint is about this line of code:

	for (i = 0; i < ARRAY_SIZE(ref_namespace); i++) {

where ref_namespace array is of NAMESPACE__COUNT size, which is the
last enum in "enum ref_namespace".  Anybody can tell it is a small
integer (like 9) that would fit comfotably well within the platform
natural "int" on anybody that can run Git.

ARRAY_SIZE() is defined in turns of sizeof(), so its type is "size_t";
it cannot be narrower than "int", but that is immaterial.  Comparing
'i' that counts up from 0 will never cause a problem.

And the compiler ought to know all of the above, or should stay
silent.

The compiler is wrong in this case.  As I already have said on this
list a few times, -Wsign-compare is often garbage and we should not
bend over backwards to butcher perfectly good code only to silence
its false positives.

> @@ -458,7 +458,7 @@ void fmt_output_subject(struct strbuf *filename,
>  	}
>  	strbuf_addf(filename, "%04d-%s", nr, subject);
>  
> -	if (max_len < filename->len)
> +	if (max_len < (int) filename->len)
>  		strbuf_setlen(filename, max_len);

This conversion is wrong, even if your compiler is made silent with
this change.  The function begins like this:

        void fmt_output_subject(struct strbuf *filename,
                                const char *subject,
                                struct rev_info *info)
        {
                const char *suffix = info->patch_suffix;
                int nr = info->nr;
                int start_len = filename->len;
                int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);

The filename variable is a pointer to a strbuf, whose .len member is
of type size_t, which can be wider than a platform natural "int".
Assigning it to "int start_len" risks overflowing and wrapping
around, and "int max_len" is also wrong.

So casting a large filename->len that would not fit within platform
natural "int" may make your compiler silent when you compare it with
max_len that is also platform natural "int".  But at that point,
what are you comparing with what?  Imagine on a platform with 32-bit
int and when the actual value of filename->len is very very long and
longer than say 3 billion bytes.

In this function, I think a more reasonable thing to do is to
express various length invoved in size_t, and make sure computations
among them does not over/underflow.

	Side note: Those who are maintaining GSoC microproject ideas
	page.  This is the second time in a few days that I had to
	look at a patch that makes the code worse by blindly trying
	to squelch the -Wsign-compare warnings.  Can you please take
	that out of the list of suggested tasks?  Thanks.

Fernando, please do not get discouraged by _our_ code being sloppy
and GSoC microproject ideas page being under-specified.  Neither is
your fault.

And welcome to Git development community.


[Footnote]

Quite honestly, -Wsign-compare is mostly garbage [*] and I wish we
did not add it to the developer settings.  A more effective way to
squelch them is not by sprinkling the casts like this, but to remove
it from config.mak.dev ;-)

https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

[Patrick Steinhardt]

On Wed, May 21, 2025 at 02:08:40PM -0700, Junio C Hamano wrote:
> Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:
> Quite honestly, -Wsign-compare is mostly garbage [*] and I wish we
> did not add it to the developer settings.  A more effective way to
> squelch them is not by sprinkling the casts like this, but to remove
> it from config.mak.dev ;-)
> 
> https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

I'm still not of the opinion that it is garbage. We have tons of
locations where we mismatch integer types only because we never got a
warning from the compiler, and these have caused multiple stack
overflows in the past. The signal-to-noise ratio is high, that much is
certainly true. But if it helps us to avoid security issues in the
future I think that is acceptable.

I do agree though this not a good project for newcomers, as fixing those
bugs is quite intricate overall. So we should definitely remove this
project from the microprojects page.

And in case I'm the only one who thinks that the warning has merit I'm
also happy to be overruled and have it be removed from our developer
settings.

Patrick

Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

[Taylor Blau]

On Fri, May 23, 2025 at 05:12:26PM +0200, Patrick Steinhardt wrote:
> On Wed, May 21, 2025 at 02:08:40PM -0700, Junio C Hamano wrote:
> > Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:
> > Quite honestly, -Wsign-compare is mostly garbage [*] and I wish we
> > did not add it to the developer settings.  A more effective way to
> > squelch them is not by sprinkling the casts like this, but to remove
> > it from config.mak.dev ;-)
> >
> > https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/
>
> I'm still not of the opinion that it is garbage. We have tons of
> locations where we mismatch integer types only because we never got a
> warning from the compiler, and these have caused multiple stack
> overflows in the past. The signal-to-noise ratio is high, that much is
> certainly true. But if it helps us to avoid security issues in the
> future I think that is acceptable.
>
> I do agree though this not a good project for newcomers, as fixing those
> bugs is quite intricate overall. So we should definitely remove this
> project from the microprojects page.

Agreed with the latter point.

> And in case I'm the only one who thinks that the warning has merit I'm
> also happy to be overruled and have it be removed from our developer
> settings.

Between the two of you, I tend to agree more with Junio's assessment
that the output of -Wsign-compare is generally not useful.

I don't think it's *entirely* useless, and your patches squelching its
various warnings has shown that to be the case. But having merged in
those patches to GitHub's private fork (and thus dealt with all of the
-Wsign-compare warnings that are unique to GitHub's fork), I generally
found that the warnings were not legitimate issues or downright wrong.

And indeed, merging those patches in required a fair amount of work
prior to the merge in order to get rid of those warnings. Having done
that work along with Elijah (whose perspective on this I'd be curious to
hear), I am not convinced that it made the code any better or more
secure.

I don't agree with everything in Dan Carpenter's post above there, but
I share their general sentiment. I don't feel so strongly about it
currently as to suggest we rip it out of our config.mak.dev, but I do
think it would be worth at least discussing what benefits and drawbacks
it has.

If there are a reasonable number of genuine overflow bugs or similar
that -Wsign-compare helped us find, then that's good. But if the vast
majority of the warnings are false positives, then -Wsign-compare is at
best an impediment to work around, and at worst may cause us to make a
signed-ness mistake. In that case I would feel more strongly about
removing it.

Fernando -- like Junio mentioned above, none of this is your fault
whatsoever ;-). Welcome indeed.

Thanks,
Taylor

Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I'm still not of the opinion that it is garbage. We have tons of
> locations where we mismatch integer types only because we never got a
> warning from the compiler, and these have caused multiple stack
> overflows in the past.

I know we spotted many possible overflows and wraparound in the
past, but -Wsign-compare being not about sizes but signedness, I'd
consider them more as happy accidents, rather than intended outcome.

If the code had 'a < (int)b' comparison where 'a' is 'int' and 'b'
is 'size_t' [*], the code would still be wrong, but the compiler
would not have said anything.

	[*] which is what a typical "I've suppressed the compiler
	warning that was annoying me" patch would do if the original
	were written 'a < b'.

'a -= b' can be equally bad depending on the value range of 'b', but
it is not about -Wsign-compare and would go unreported, right?

So I think noise from -Wsign-compare are certainly not "false
positives" (in the sense that the comparisons are between signed and
unsigned---the warning option is reporting what it was asked to
report), but are not-false-but-useless positives; what they try to
catch is somehow different from what they could catch to help us.
And that is why I have been skeptical.

> I do agree though this not a good project for newcomers, as fixing those
> bugs is quite intricate overall. So we should definitely remove this
> project from the microprojects page.

Yeah, that is something I am quite certain about.

Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

[Patrick Steinhardt]

On Fri, May 23, 2025 at 09:54:14AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > I do agree though this not a good project for newcomers, as fixing those
> > bugs is quite intricate overall. So we should definitely remove this
> > project from the microprojects page.
> 
> Yeah, that is something I am quite certain about.

I have created https://github.com/git/git.github.io/pull/779 to get rid
of this microproject now.

Patrick