Thread: cast pid_t to int when using *printf
gcc (3.2.3 on Solaris 9) warns about a couple of places where a pid_t is
formatted with %d by a printf-family function. This patch explicitly
casts to int to suppress the warning.
-O
Index: src/backend/postmaster/pgstat.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/postmaster/pgstat.c,v
retrieving revision 1.80
diff -u -c -r1.80 pgstat.c
*** src/backend/postmaster/pgstat.c 29 Aug 2004 05:06:46 -0000 1.80
--- src/backend/postmaster/pgstat.c 24 Sep 2004 06:46:27 -0000
***************
*** 1505,1511 ****
snprintf(pgStat_fname, MAXPGPATH, PGSTAT_STAT_FILENAME, DataDir);
/* tmpfname need only be set correctly in this process */
snprintf(pgStat_tmpfname, MAXPGPATH, PGSTAT_STAT_TMPFILE,
! DataDir, getpid());
/*
* Arrange to write the initial status file right away
--- 1505,1511 ----
snprintf(pgStat_fname, MAXPGPATH, PGSTAT_STAT_FILENAME, DataDir);
/* tmpfname need only be set correctly in this process */
snprintf(pgStat_tmpfname, MAXPGPATH, PGSTAT_STAT_TMPFILE,
! DataDir, (int)getpid());
/*
* Arrange to write the initial status file right away
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/postmaster/postmaster.c,v
retrieving revision 1.425
diff -u -c -r1.425 postmaster.c
*** src/backend/postmaster/postmaster.c 9 Sep 2004 00:59:33 -0000 1.425
--- src/backend/postmaster/postmaster.c 24 Sep 2004 06:46:27 -0000
***************
*** 2835,2841 ****
*/
ereport(DEBUG3,
(errmsg_internal("%s child[%d]: starting with (",
! progname, getpid())));
for (i = 0; i < ac; ++i)
ereport(DEBUG3,
(errmsg_internal("\t%s", av[i])));
--- 2835,2841 ----
*/
ereport(DEBUG3,
(errmsg_internal("%s child[%d]: starting with (",
! progname, (int)getpid())));
for (i = 0; i < ac; ++i)
ereport(DEBUG3,
(errmsg_internal("\t%s", av[i])));
On Fri, 2004-09-24 at 16:51, Oliver Jowett wrote: > gcc (3.2.3 on Solaris 9) warns about a couple of places where a pid_t is > formatted with %d by a printf-family function. For curiosity's sake, what formatting escape does gcc prefer? -Neil
Neil Conway wrote: > On Fri, 2004-09-24 at 16:51, Oliver Jowett wrote: > >>gcc (3.2.3 on Solaris 9) warns about a couple of places where a pid_t is >>formatted with %d by a printf-family function. > > > For curiosity's sake, what formatting escape does gcc prefer? I don't think there is an escape for pid_t, you always have to cast it. -O
Am Freitag, 24. September 2004 09:34 schrieb Oliver Jowett: > Neil Conway wrote: > > On Fri, 2004-09-24 at 16:51, Oliver Jowett wrote: > >>gcc (3.2.3 on Solaris 9) warns about a couple of places where a pid_t is > >>formatted with %d by a printf-family function. > > > > For curiosity's sake, what formatting escape does gcc prefer? > > I don't think there is an escape for pid_t, you always have to cast it. I think what he was asking is this: Since pid_t has to be a signed integer type, but gcc does not accept %d for it, then it could be that pid_t is wider than an int, so casting it to int would potentially lose information. (Btw., the Windows port defines pid_t as unsigned long; that's surely wrong.) -- Peter Eisentraut http://developer.postgresql.org/~petere/
> (Btw., the Windows port defines pid_t as unsigned long; > that's surely wrong.) In what way is that wrong? A PID on Windows is a DWORD, which is an unsigned long. Or am I missing something (probably..)? //Magnus
Am Freitag, 24. September 2004 11:06 schrieb Magnus Hagander: > > (Btw., the Windows port defines pid_t as unsigned long; > > that's surely wrong.) > > In what way is that wrong? A PID on Windows is a DWORD, which is an > unsigned long. Or am I missing something (probably..)? The mingw header files define pid_t as int, so we shouldn't redefine it in the first place. The rest of the POSIX world also assumes that pid_t is signed, so you might break a bunch of interfaces if it's not. Note that this is independent of the fact that the actual process identifiers are all positive, both on Windows and on Unix systems. (Tertiary note: Never #define one type to another, always use typedef.) -- Peter Eisentraut http://developer.postgresql.org/~petere/
Peter Eisentraut wrote: > Am Freitag, 24. September 2004 09:34 schrieb Oliver Jowett: > >>Neil Conway wrote: >> >>>On Fri, 2004-09-24 at 16:51, Oliver Jowett wrote: >>> >>>>gcc (3.2.3 on Solaris 9) warns about a couple of places where a pid_t is >>>>formatted with %d by a printf-family function. >>> >>>For curiosity's sake, what formatting escape does gcc prefer? >> >>I don't think there is an escape for pid_t, you always have to cast it. > > > I think what he was asking is this: Since pid_t has to be a signed integer > type, but gcc does not accept %d for it, then it could be that pid_t is wider > than an int, so casting it to int would potentially lose information. pid_t on the Solaris/sparc system is a long (but both int and long are 32 bits). Some experimentation shows that gcc is happy with a %ld format specifier. But compiling the same code on a Linux/x86 system makes gcc complain when applying %ld to pid_t, so we will need a cast there either way. I notice that elog.c casts MyProcPid to long and uses %ld. Amusingly, MyProcPid is explicitly an int.. -O
On Fri, 2004-09-24 at 20:31, Oliver Jowett wrote: > pid_t on the Solaris/sparc system is a long (but both int and long are > 32 bits). Some experimentation shows that gcc is happy with a %ld format > specifier. But compiling the same code on a Linux/x86 system makes gcc > complain when applying %ld to pid_t, so we will need a cast there either > way. I guess it would be safest to use %ld and cast pid_t to long. Of course, this seems a little paranoid -- is there actually a system with sizeof(pid_t) != 4? -Neil
Neil Conway <neilc@samurai.com> writes:
> I guess it would be safest to use %ld and cast pid_t to long. Of course,
> this seems a little paranoid -- is there actually a system with
> sizeof(pid_t) != 4?
Traditionally PIDs fit in 16 bits, let alone 32. I'd recommend that we
standardize on casting pid_t to int for printing purposes; I think
that's what's being done in more places than not. Also, as you note, we
are using int variables to hold PIDs in many places --- I don't think
it's worth running around and changing those either.
regards, tom lane
Peter Eisentraut wrote:
> Am Freitag, 24. September 2004 11:06 schrieb Magnus Hagander:
> > > (Btw., the Windows port defines pid_t as unsigned long;
> > > that's surely wrong.)
> >
> > In what way is that wrong? A PID on Windows is a DWORD, which is an
> > unsigned long. Or am I missing something (probably..)?
>
> The mingw header files define pid_t as int, so we shouldn't redefine it in the
> first place. The rest of the POSIX world also assumes that pid_t is signed,
> so you might break a bunch of interfaces if it's not. Note that this is
> independent of the fact that the actual process identifiers are all positive,
> both on Windows and on Unix systems.
>
> (Tertiary note: Never #define one type to another, always use typedef.)
I have fixed the Win32 type defines and removed the pid_t because as you
mentioned it was not needed. We only have these left and converted to
typedef:
typedef int uid_t;
typedef int gid_t;
typedef long key_t;
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > Neil Conway <neilc@samurai.com> writes: > > I guess it would be safest to use %ld and cast pid_t to long. Of course, > > this seems a little paranoid -- is there actually a system with > > sizeof(pid_t) != 4? > > Traditionally PIDs fit in 16 bits, let alone 32. I'd recommend that we > standardize on casting pid_t to int for printing purposes; Done. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Oliver Jowett wrote:
> Peter Eisentraut wrote:
> > Am Freitag, 24. September 2004 09:34 schrieb Oliver Jowett:
> >
> >>Neil Conway wrote:
> >>
> >>>On Fri, 2004-09-24 at 16:51, Oliver Jowett wrote:
> >>>
> >>>>gcc (3.2.3 on Solaris 9) warns about a couple of places where a pid_t is
> >>>>formatted with %d by a printf-family function.
> >>>
> >>>For curiosity's sake, what formatting escape does gcc prefer?
> >>
> >>I don't think there is an escape for pid_t, you always have to cast it.
> >
> >
> > I think what he was asking is this: Since pid_t has to be a signed integer
> > type, but gcc does not accept %d for it, then it could be that pid_t is wider
> > than an int, so casting it to int would potentially lose information.
>
> pid_t on the Solaris/sparc system is a long (but both int and long are
> 32 bits). Some experimentation shows that gcc is happy with a %ld format
> specifier. But compiling the same code on a Linux/x86 system makes gcc
> complain when applying %ld to pid_t, so we will need a cast there either
> way.
>
> I notice that elog.c casts MyProcPid to long and uses %ld. Amusingly,
> MyProcPid is explicitly an int..
I see in include/sys/types.h on Solaris 9:
#if defined(_LP64) || defined(_I32LPx)
typedef uint_t nlink_t; /* file link type */
typedef int pid_t; /* process id type */
#else
typedef ulong_t nlink_t; /* (historical version) */
typedef long pid_t; /* (historical version) */
#endif
I am confused why you are seeing long for pid_t? What is your Solaris
system information? If Solaris needs adjustments, there are a lot of
place we print getpid().
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian wrote: > Tom Lane wrote: >>Traditionally PIDs fit in 16 bits, let alone 32. I'd recommend that we >>standardize on casting pid_t to int for printing purposes; > > > Done. Uh, what? Your patch removes the casting of pid_t to int -- Tom was suggesting that we consistently cast pid_t to int. (Also your patch removes casting from uid_t to int in the case of geteuid() -- why?) For instance: http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/command.c.diff?r1=1.126&r2=1.127 -Neil
Bruce Momjian wrote: > I see in include/sys/types.h on Solaris 9: > > #if defined(_LP64) || defined(_I32LPx) > typedef uint_t nlink_t; /* file link type */ > typedef int pid_t; /* process id type */ > #else > typedef ulong_t nlink_t; /* (historical version) */ > typedef long pid_t; /* (historical version) */ > #endif > I am confused why you are seeing long for pid_t? What is your Solaris > system information? If Solaris needs adjustments, there are a lot of > place we print getpid(). This is also what is on the Solaris system I was using. gcc -E showed that the #else branch was being taken. The #if branch is taken when compiling in 64-bit mode (gcc -m64). We're fine so long as everything casts to either int or long. I only saw warnings from a couple of places that did not do a cast at all. -O
Neil Conway wrote: > Bruce Momjian wrote: > > Tom Lane wrote: > >>Traditionally PIDs fit in 16 bits, let alone 32. I'd recommend that we > >>standardize on casting pid_t to int for printing purposes; > > > > > > Done. > > Uh, what? Your patch removes the casting of pid_t to int -- Tom was > suggesting that we consistently cast pid_t to int. (Also your patch > removes casting from uid_t to int in the case of geteuid() -- why?) > > For instance: > > http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/command.c.diff?r1=1.126&r2=1.127 From Tom: > Traditionally PIDs fit in 16 bits, let alone 32. I'd recommend that we > standardize on casting pid_t to int for printing purposes; OK, I read Tom's email saying that we use %d consistently. I didn't realize he was also saying cast getpid(), but that is easy to do. Before we had "%ld" sometimes, (int) cast others, and sometimes neither. It is now consistent and we can make the change all at once. So I assume everyone wants: printf("%d", (int) getpid())? Is this correct? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
OK, 'int' cast added to getpid() calls with %d; patch attached. --------------------------------------------------------------------------- Bruce Momjian wrote: > Neil Conway wrote: > > Bruce Momjian wrote: > > > Tom Lane wrote: > > >>Traditionally PIDs fit in 16 bits, let alone 32. I'd recommend that we > > >>standardize on casting pid_t to int for printing purposes; > > > > > > > > > Done. > > > > Uh, what? Your patch removes the casting of pid_t to int -- Tom was > > suggesting that we consistently cast pid_t to int. (Also your patch > > removes casting from uid_t to int in the case of geteuid() -- why?) > > > > For instance: > > > > http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/command.c.diff?r1=1.126&r2=1.127 > > >From Tom: > > > Traditionally PIDs fit in 16 bits, let alone 32. I'd recommend that we > > standardize on casting pid_t to int for printing purposes; > > OK, I read Tom's email saying that we use %d consistently. I didn't > realize he was also saying cast getpid(), but that is easy to do. > > Before we had "%ld" sometimes, (int) cast others, and sometimes neither. > It is now consistent and we can make the change all at once. > > So I assume everyone wants: > > printf("%d", (int) getpid())? > > Is this correct? > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 359-1001 > + If your life is a hard drive, | 13 Roberts Road > + Christ can be your backup. | Newtown Square, Pennsylvania 19073 > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/backend/access/transam/xlog.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/access/transam/xlog.c,v retrieving revision 1.173 diff -c -c -r1.173 xlog.c *** src/backend/access/transam/xlog.c 12 Oct 2004 21:54:35 -0000 1.173 --- src/backend/access/transam/xlog.c 14 Oct 2004 19:59:10 -0000 *************** *** 1513,1519 **** * up pre-creating an extra log segment. That seems OK, and better * than holding the lock throughout this lengthy process. */ ! snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d", XLogDir, getpid()); unlink(tmppath); --- 1513,1519 ---- * up pre-creating an extra log segment. That seems OK, and better * than holding the lock throughout this lengthy process. */ ! snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d", XLogDir, (int)getpid()); unlink(tmppath); *************** *** 1633,1639 **** /* * Copy into a temp file name. */ ! snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d", XLogDir, getpid()); unlink(tmppath); --- 1633,1639 ---- /* * Copy into a temp file name. */ ! snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d", XLogDir, (int)getpid()); unlink(tmppath); *************** *** 2898,2904 **** /* * Write into a temp file name. */ ! snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d", XLogDir, getpid()); unlink(tmppath); --- 2898,2904 ---- /* * Write into a temp file name. */ ! snprintf(tmppath, MAXPGPATH, "%s/xlogtemp.%d", XLogDir, (int)getpid()); unlink(tmppath); Index: src/backend/postmaster/pgstat.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/postmaster/pgstat.c,v retrieving revision 1.80 diff -c -c -r1.80 pgstat.c *** src/backend/postmaster/pgstat.c 29 Aug 2004 05:06:46 -0000 1.80 --- src/backend/postmaster/pgstat.c 14 Oct 2004 19:59:20 -0000 *************** *** 1505,1511 **** snprintf(pgStat_fname, MAXPGPATH, PGSTAT_STAT_FILENAME, DataDir); /* tmpfname need only be set correctly in this process */ snprintf(pgStat_tmpfname, MAXPGPATH, PGSTAT_STAT_TMPFILE, ! DataDir, getpid()); /* * Arrange to write the initial status file right away --- 1505,1511 ---- snprintf(pgStat_fname, MAXPGPATH, PGSTAT_STAT_FILENAME, DataDir); /* tmpfname need only be set correctly in this process */ snprintf(pgStat_tmpfname, MAXPGPATH, PGSTAT_STAT_TMPFILE, ! DataDir, (int)getpid()); /* * Arrange to write the initial status file right away Index: src/backend/postmaster/postmaster.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v retrieving revision 1.432 diff -c -c -r1.432 postmaster.c *** src/backend/postmaster/postmaster.c 12 Oct 2004 21:54:40 -0000 1.432 --- src/backend/postmaster/postmaster.c 14 Oct 2004 19:59:28 -0000 *************** *** 2760,2766 **** */ ereport(DEBUG3, (errmsg_internal("%s child[%d]: starting with (", ! progname, getpid()))); for (i = 0; i < ac; ++i) ereport(DEBUG3, (errmsg_internal("\t%s", av[i]))); --- 2760,2766 ---- */ ereport(DEBUG3, (errmsg_internal("%s child[%d]: starting with (", ! progname, (int)getpid()))); for (i = 0; i < ac; ++i) ereport(DEBUG3, (errmsg_internal("\t%s", av[i]))); Index: src/bin/psql/command.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v retrieving revision 1.127 diff -c -c -r1.127 command.c *** src/bin/psql/command.c 9 Oct 2004 02:46:41 -0000 1.127 --- src/bin/psql/command.c 14 Oct 2004 19:59:33 -0000 *************** *** 1133,1139 **** const char *tmpdirenv = getenv("TMPDIR"); snprintf(fnametmp, sizeof(fnametmp), "%s/psql.edit.%d.%d", ! tmpdirenv ? tmpdirenv : "/tmp", geteuid(), getpid()); #else GetTempFileName(".", "psql", 0, fnametmp); #endif --- 1133,1139 ---- const char *tmpdirenv = getenv("TMPDIR"); snprintf(fnametmp, sizeof(fnametmp), "%s/psql.edit.%d.%d", ! tmpdirenv ? tmpdirenv : "/tmp", geteuid(), (int)getpid()); #else GetTempFileName(".", "psql", 0, fnametmp); #endif Index: src/interfaces/ecpg/ecpglib/misc.c =================================================================== RCS file: /cvsroot/pgsql/src/interfaces/ecpg/ecpglib/misc.c,v retrieving revision 1.23 diff -c -c -r1.23 misc.c *** src/interfaces/ecpg/ecpglib/misc.c 9 Oct 2004 02:46:42 -0000 1.23 --- src/interfaces/ecpg/ecpglib/misc.c 14 Oct 2004 19:59:35 -0000 *************** *** 253,259 **** return; } ! sprintf(f, "[%d]: %s", getpid(), format); va_start(ap, format); vfprintf(debugstream, f, ap); --- 253,259 ---- return; } ! sprintf(f, "[%d]: %s", (int)getpid(), format); va_start(ap, format); vfprintf(debugstream, f, ap);