Re: [HACKERS] compile warning in CVS HEAD

I get the following warning compiling CVS HEAD:

[neilc:/Users/neilc/pgsql]% make -C src/backend/utils/error all
[ ... ]
gcc -no-cpp-precomp -O0 -Winline -fno-strict-aliasing -g -Wall
-Wmissing-prototypes -Wmissing-declarations -I../../../../src/include
-I/sw/include -c -o elog.o elog.c -MMD
elog.c: In function `log_line_prefix':
elog.c:1123: warning: passing arg 1 of `localtime' from incompatible
pointer type

This is on Mac OSX 10.3 w/ gcc 3.3

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> I get the following warning compiling CVS HEAD:
> [neilc:/Users/neilc/pgsql]% make -C src/backend/utils/error all
> [ ... ]
> gcc -no-cpp-precomp -O0 -Winline -fno-strict-aliasing -g -Wall
> -Wmissing-prototypes -Wmissing-declarations -I../../../../src/include
> -I/sw/include -c -o elog.o elog.c -MMD
> elog.c: In function `log_line_prefix':
> elog.c:1123: warning: passing arg 1 of `localtime' from incompatible
> pointer type

Hm, looks like this code incorrectly assumes that the tv_sec field of
struct timeval is necessarily the same datatype as time_t. I'd suggest
assigning session_start into a local time_t variable.

regards, tom lane

Tom Lane wrote:

>Neil Conway <neilc(at)samurai(dot)com> writes:
>
>
>>I get the following warning compiling CVS HEAD:
>>[neilc:/Users/neilc/pgsql]% make -C src/backend/utils/error all
>>[ ... ]
>>gcc -no-cpp-precomp -O0 -Winline -fno-strict-aliasing -g -Wall
>>-Wmissing-prototypes -Wmissing-declarations -I../../../../src/include
>>-I/sw/include -c -o elog.o elog.c -MMD
>>elog.c: In function `log_line_prefix':
>>elog.c:1123: warning: passing arg 1 of `localtime' from incompatible
>>pointer type
>>
>>
>
>Hm, looks like this code incorrectly assumes that the tv_sec field of
>struct timeval is necessarily the same datatype as time_t. I'd suggest
>assigning session_start into a local time_t variable.
>

*sigh*

my local (linux) man for gettimeofday says this:

struct timeval {
time_t tv_sec; /* seconds */
suseconds_t tv_usec; /* microseconds */
};

We could do what you say, or could we just cast it?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> *sigh*

> my local (linux) man for gettimeofday says this:

> struct timeval {
> time_t tv_sec; /* seconds */
> suseconds_t tv_usec; /* microseconds */
> };

Yeah, but mine (HPUX) says that tv_sec is "unsigned long". I suspect
that on Darwin the types disagree as to signedness.

> We could do what you say, or could we just cast it?

If they really were different types (as in different widths) then
casting the pointer would be a highly Wrong Thing. I think copying
to a local is safer, even if it does waste a cycle or two.

regards, tom lane

Patch attached. Also adds a malloc() check that Neil wanted.

cheers

andrew

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>*sigh*
>>
>>
>
>
>
>>my local (linux) man for gettimeofday says this:
>>
>>
>
>
>
>> struct timeval {
>> time_t tv_sec; /* seconds */
>> suseconds_t tv_usec; /* microseconds */
>> };
>>
>>
>
>Yeah, but mine (HPUX) says that tv_sec is "unsigned long". I suspect
>that on Darwin the types disagree as to signedness.
>
>
>
>>We could do what you say, or could we just cast it?
>>
>>
>
>If they really were different types (as in different widths) then
>casting the pointer would be a highly Wrong Thing. I think copying
>to a local is safer, even if it does waste a cycle or two.
>
> regards, tom lane
>
>---------------------------(end of broadcast)---------------------------
>TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>
>
>

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Patch attached. Also adds a malloc() check that Neil wanted.

Er, what on God's green earth is elog.c doing malloc'ing stuff at all?
This should be a palloc in the ErrorContext; that's what it's for.
As is, this code is guaranteed to fail under out-of-memory conditions.

regards, tom lane

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>Patch attached. Also adds a malloc() check that Neil wanted.
>>
>>
>
>Er, what on God's green earth is elog.c doing malloc'ing stuff at all?
>This should be a palloc in the ErrorContext; that's what it's for.
>As is, this code is guaranteed to fail under out-of-memory conditions.
>
>
>

Well, it's a one-off allocation into a static variable, if that makes
any difference. If not, I accept the rebuke :-)

I first published the ancestor of this code many months ago, and nobody
has objected until now. But there is always room for improvement I
guess, including in my own knowledge. I'll be happy to change it if need be.

cheers

andrew

Where are we on this patch? There was concern about the malloc() but
that can be fixed.

---------------------------------------------------------------------------

Andrew Dunstan wrote:
>
> Patch attached. Also adds a malloc() check that Neil wanted.
>
> cheers
>
> andrew
>
> Tom Lane wrote:
>
> >Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> >
> >
> >>*sigh*
> >>
> >>
> >
> >
> >
> >>my local (linux) man for gettimeofday says this:
> >>
> >>
> >
> >
> >
> >> struct timeval {
> >> time_t tv_sec; /* seconds */
> >> suseconds_t tv_usec; /* microseconds */
> >> };
> >>
> >>
> >
> >Yeah, but mine (HPUX) says that tv_sec is "unsigned long". I suspect
> >that on Darwin the types disagree as to signedness.
> >
> >
> >
> >>We could do what you say, or could we just cast it?
> >>
> >>
> >
> >If they really were different types (as in different widths) then
> >casting the pointer would be a highly Wrong Thing. I think copying
> >to a local is safer, even if it does waste a cycle or two.
> >
> > regards, tom lane
> >
> >---------------------------(end of broadcast)---------------------------
> >TIP 6: Have you searched our list archives?
> >
> > http://archives.postgresql.org
> >
> >
> >

> Index: src/backend/utils/error/elog.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
> retrieving revision 1.128
> diff -c -r1.128 elog.c
> *** src/backend/utils/error/elog.c 15 Mar 2004 15:56:23 -0000 1.128
> --- src/backend/utils/error/elog.c 18 Mar 2004 22:55:44 -0000
> ***************
> *** 1035,1041 ****
> --- 1035,1045 ----
> int result_len = 2*NAMEDATALEN + format_len +120 ;
>
> if (result == NULL)
> + {
> result = malloc(result_len);
> + if (result == NULL)
> + return "";
> + }
> result[0] = '\0';
>
> if (format_len > 0)
> ***************
> *** 1119,1126 ****
> localtime(&stamp_time));
> break;
> case 's':
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&(MyProcPort->session_start.tv_sec)));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",
> --- 1123,1131 ----
> localtime(&stamp_time));
> break;
> case 's':
> + stamp_time = (time_t)(MyProcPort->session_start.tv_sec);
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&stamp_time));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

OK, I talked to Andrew via IM and he says it is fixed in CVS.

---------------------------------------------------------------------------

Andrew Dunstan wrote:
>
> Patch attached. Also adds a malloc() check that Neil wanted.
>
> cheers
>
> andrew
>
> Tom Lane wrote:
>
> >Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> >
> >
> >>*sigh*
> >>
> >>
> >
> >
> >
> >>my local (linux) man for gettimeofday says this:
> >>
> >>
> >
> >
> >
> >> struct timeval {
> >> time_t tv_sec; /* seconds */
> >> suseconds_t tv_usec; /* microseconds */
> >> };
> >>
> >>
> >
> >Yeah, but mine (HPUX) says that tv_sec is "unsigned long". I suspect
> >that on Darwin the types disagree as to signedness.
> >
> >
> >
> >>We could do what you say, or could we just cast it?
> >>
> >>
> >
> >If they really were different types (as in different widths) then
> >casting the pointer would be a highly Wrong Thing. I think copying
> >to a local is safer, even if it does waste a cycle or two.
> >
> > regards, tom lane
> >
> >---------------------------(end of broadcast)---------------------------
> >TIP 6: Have you searched our list archives?
> >
> > http://archives.postgresql.org
> >
> >
> >

> Index: src/backend/utils/error/elog.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/error/elog.c,v
> retrieving revision 1.128
> diff -c -r1.128 elog.c
> *** src/backend/utils/error/elog.c 15 Mar 2004 15:56:23 -0000 1.128
> --- src/backend/utils/error/elog.c 18 Mar 2004 22:55:44 -0000
> ***************
> *** 1035,1041 ****
> --- 1035,1045 ----
> int result_len = 2*NAMEDATALEN + format_len +120 ;
>
> if (result == NULL)
> + {
> result = malloc(result_len);
> + if (result == NULL)
> + return "";
> + }
> result[0] = '\0';
>
> if (format_len > 0)
> ***************
> *** 1119,1126 ****
> localtime(&stamp_time));
> break;
> case 's':
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&(MyProcPort->session_start.tv_sec)));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",
> --- 1123,1131 ----
> localtime(&stamp_time));
> break;
> case 's':
> + stamp_time = (time_t)(MyProcPort->session_start.tv_sec);
> j += strftime(result+j, result_len-j, "%Y-%m-%d %H:%M:%S",
> ! localtime(&stamp_time));
> break;
> case 'i':
> j += snprintf(result+j,result_len-j,"%s",

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073