agora inbox for [email protected]  

help / color / mirror / Atom feed

From: Andy Fan <[email protected]>
To: Peter Eisentraut <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Dean Rasheed <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: Re: -Wformat-signedness
Date: Sun, 27 Oct 2024 11:59:51 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CA+hUKGJ7EQm9extQAgrFZNNUKqRT8Vv5t1tKqA-5nEcYn0+wNA@mail.gmail.com>
	<[email protected]>

Peter Eisentraut <[email protected]> writes:

Hi,

> On 2020-10-29 22:37, Thomas Munro wrote:
>> There're probably mostly harmless, being mostly error and debug
>> messages and the like, and considering that eg OID parsing tolerates
>> negative numbers when reading them back in, but for what it's worth:
>> GCC complains about many %d vs %u type mixups if you build with
>> $SUBJECT.
>
> I had looked into this some time ago.  I have dusted off my patch
> again. The attached version fixes all warnings for me.

When Dean pointed me this thread[1], I was thinking we need to add the
"-Wformat-signedness" and fix all the existing warnning. Then after some
research, it is not such easy and seems we need some agreement first if
we want to fix them.  

> The following are the main categories of issues:
>
> 1. enums are unsigned by default in gcc, so all those internal error
> messages "unrecognized blah kind: %d" need to be changed to %u.

IIUC, we have agreed that we should cast enum to int and continue to use
"%d". At least Tom suggested this and Thomas agreed this [1] and Peter
didn't raise any opposition.

> 2. Various trickery at the boundary of internal counters that are
> unsigned and external functions or views using signed types.  These need
> another look.

I also noticed we lack of UNSIGNED INT32/64 SQL type.  Changing the
counter to signed looks not good to me as well. This one looks doesn't
have an agreement yet.  

> 3. Various messages print signed values using %x formats, which need to
> be unsigned.  These might also need another look.
>
> 4. Issues with constants being signed by default.  For example, things
> like elog(ERROR, "foo is %u but should be %u", somevar, 55) warns
> because of the constant.  Should be changed to something like 55U for
> symmetry, or change the %u to %d.  This also reaches into genbki
> territory with all the OID constants being generated.
>
> 5. Some "surprising" but correct C behavior.  For example, unsigned
> short is promoted to int (not unsigned int) in variable arguments, so
> needs a %d format.
>
> 6. Finally, a bunch of uses were just plain wrong and should be corrected.

7. __FILE__ in gcc is 'int', but we elog() it with "%u".  Should we
change it to "%d"? 

> I haven't found anything that is a really serious bug, but I imagine you
> could run into trouble in various ways when you exceed the INT_MAX
> value.  But then again, if you use up INT_MAX WAL timelines, you
> probably have other problems. ;-)

Me too, just that want some clean code:) But FWIW, "-Wformat-signedness"
is not supported by clang so far, so if people is using clang, they
still can't benefit from this changes. My soluation (I use clang
everyday) is adding a "gcc-checker" for my c file, if I make such
mistake, it can remind me directly.  

[0] https://www.postgresql.org/message-id/874j4yl4cj.fsf%40163.com 
[1]
https://www.postgresql.org/message-id/CA%2BhUKGJNUk434tcsVbs5YUGsujZbveo43QcZeWbv0xPzg9us-A%40mail.g...

-- 
Best Regards
Andy Fan

view thread (6+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: -Wformat-signedness
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox