Thread: Re: [pgsql-hackers-win32] snprintf causes regression tests
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Have we considered what is going to happen to applications when they use > > our snprintf instead of the one from the operating system? > > Hmm ... > > First line of thought: we surely must not insert a snprintf into > libpq.so unless it is 100% up to spec *and* has no performance issues > ... neither of which can be claimed of the CVS-tip version. Agreed, and we have to support all the 64-bit specifications a port might support like %qd and %I64d as well as %lld. I have added that to our current CVS version. > Second line of thought: libpq already feels free to insert allegedly > up-to-spec versions of a number of things, and no one has complained. > Maybe the linker prevents problems by not linking these versions to > any calls from outside libpq? I just tested on BSD/OS and a program with a single printf() call does call our printf if libpq is used on the link line. The program makes no libpq calls at all. > Third thought: Windows' linker seems to be broken enough that it may > create problems of this ilk that exist on no other platform. Yes, strangly the Window's linker is fine because libpqdll.def defines what symbols are exported. I don't think Unix has that capability. Is there any way we can have just gettext() call our snprintf under a special name? -- 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
> > Yes, strangly the Window's linker is fine because libpqdll.def defines > what symbols are exported. I don't think Unix has that capability. A non-static "public" function in a Windows DLL is not available for dynamic linking unless explicitly declared as dll export. This behavior is completely different than UNIX shared libraries. Windows static libraries operate completely differently than Windows DLLs, they work like their UNIX equivilents. So, if you create an snprintf function in code that will be in both a static and dynamic library, the static library may have conflicts where as the dynamic one will not. Don't you love Windows?
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> First line of thought: we surely must not insert a snprintf into
>> libpq.so unless it is 100% up to spec *and* has no performance issues
>> ... neither of which can be claimed of the CVS-tip version.
> Agreed, and we have to support all the 64-bit specifications a port
> might support like %qd and %I64d as well as %lld. I have added that to
> our current CVS version.
I really dislike that idea and request that you revert it.
> Is there any way we can have just gettext() call our snprintf under a
> special name?
The issue only comes up in libpq --- in the backend there is no reason
that snprintf can't be our snprintf, and likewise in self-contained
programs like psql. It might be worth the pain-in-the-neck quality to
have libpq refer to the functions as pq_snprintf etc. Perhaps we could
do this via macros
#define snprintf pq_snprintf
and not have to uglify the source code.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: >> Tom Lane wrote: >>> First line of thought: we surely must not insert a snprintf into >>> libpq.so unless it is 100% up to spec *and* has no performance issues >>> ... neither of which can be claimed of the CVS-tip version. > >> Agreed, and we have to support all the 64-bit specifications a port >> might support like %qd and %I64d as well as %lld. I have added that to >> our current CVS version. > > I really dislike that idea and request that you revert it. > >> Is there any way we can have just gettext() call our snprintf under a >> special name? > > The issue only comes up in libpq --- in the backend there is no reason > that snprintf can't be our snprintf, and likewise in self-contained > programs like psql. It might be worth the pain-in-the-neck quality to > have libpq refer to the functions as pq_snprintf etc. Perhaps we could > do this via macros > > #define snprintf pq_snprintf Didn't I suggest that earlier? :) Also, since it is vsnprintf that seems to be a bigger issue: #define vsnprintf pq_vsnprintf
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> First line of thought: we surely must not insert a snprintf into > >> libpq.so unless it is 100% up to spec *and* has no performance issues > >> ... neither of which can be claimed of the CVS-tip version. > > > Agreed, and we have to support all the 64-bit specifications a port > > might support like %qd and %I64d as well as %lld. I have added that to > > our current CVS version. > > I really dislike that idea and request that you revert it. Done. > > Is there any way we can have just gettext() call our snprintf under a > > special name? > > The issue only comes up in libpq --- in the backend there is no reason > that snprintf can't be our snprintf, and likewise in self-contained > programs like psql. It might be worth the pain-in-the-neck quality to > have libpq refer to the functions as pq_snprintf etc. Perhaps we could > do this via macros > > #define snprintf pq_snprintf > > and not have to uglify the source code. Yes, this is what I was thinking of too. I think it would need a macro in libpq to map the libc names to the pq_* names, and a separate /port C file that maps the normal libc names to the pg_* names. For client applications and the backend, this new C file would catch all the snprintf calls, while for libpq the pg_* calls would be used directly and the new C file with the libc symbols would not be pulled in. Does that sound like a plan? The reason we can't just use the macro everwhere is that we don't want applications using libpq to all be using pg_* functions, only psql and our own. The only other solution I can think of is to make sure all client apps use FRONTEND as a define and trigger the macros from libc names to pg_* names on that. -- 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
Dear all, After struggling for one week to to integrate FreeBSD's vfprintf.c into PostgreSQL I finally gave up. It is too dependent on underlying FreeBSD system functions. To incorporate it into PostgreSQL we need to move vfprintf.c file itself, two dozen files form gdtoa and a half a dozen __XXtoa.c files scattered in apparently random fashion all around FreeBSD source tree. Instead I researched some other implementations of snprintf on the web released under a license compatible with PostgreSQL's. The most suitable one I have come upon is Trio [http://daniel.haxx.se/projects/trio/]. It is distributed under a MIT-like license which, I think will be compatible with us. What do you think about it? Shall I abandon FreeBSD and go ahead ıncorporatıng Trıo? And by the way, what ıs the conclusıon of snprıntf() vs. pg_snprintf() and UNIX libraries discussion a week ago? Which one shall I implement? Regards, Nicolai Tufar
From what I recall from the conversation, I would say rename the vsprintf and the sprintf functions in postgres to pq_vsnprintf and pq_snprintf. Define a couple macros: (in some common header, pqprintf.h?) #define snprintf pq_snprintf #define vsnprintf pq_snprintf Then just maintain the postgres forms of printf which have seemed to be OK except that on Win32 vnsprintf, although in the same object file was not being used. > Dear all, > After struggling for one week to to integrate FreeBSD's vfprintf.c into > PostgreSQL I finally gave up. It is too dependent on underlying > FreeBSD system functions. To incorporate it into PostgreSQL we need > to move vfprintf.c file itself, two dozen files form gdtoa and a half > a dozen __XXtoa.c files scattered in apparently random fashion all > around FreeBSD source tree. > > Instead I researched some other implementations of snprintf on > the web released under a license compatible with PostgreSQL's. > The most suitable one I have come upon is Trio > [http://daniel.haxx.se/projects/trio/]. > It is distributed under a MIT-like license which, I think will be > compatible with us. > > What do you think about it? Shall I abandon FreeBSD and go ahead > ıncorporatıng Trıo? > > And by the way, what ıs the conclusıon of snprıntf() vs. pg_snprintf() > and UNIX libraries discussion a week ago? Which one shall > I implement? > > Regards, > Nicolai Tufar > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org >
Nicolai Tufar wrote: > Dear all, > After struggling for one week to to integrate FreeBSD's vfprintf.c into > PostgreSQL I finally gave up. It is too dependent on underlying > FreeBSD system functions. To incorporate it into PostgreSQL we need > to move vfprintf.c file itself, two dozen files form gdtoa and a half > a dozen __XXtoa.c files scattered in apparently random fashion all > around FreeBSD source tree. > > Instead I researched some other implementations of snprintf on > the web released under a license compatible with PostgreSQL's. > The most suitable one I have come upon is Trio > [http://daniel.haxx.se/projects/trio/]. > It is distributed under a MIT-like license which, I think will be > compatible with us. > > What do you think about it? Shall I abandon FreeBSD and go ahead > ?ncorporat?ng Tr?o? Yes, maybe just add the proper %$ handling from Trio to what we have now. > And by the way, what ?s the conclus?on of snpr?ntf() vs. pg_snprintf() > and UNIX libraries discussion a week ago? Which one shall > I implement? I think the proper direction is not to export snprintf() from libpq so that user applications will use the native snprintf, but our code can use our custom version. I will work on a patch now. -- 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
On Wed, 9 Mar 2005 22:51:27 -0500 (EST), Bruce Momjian <pgman@candle.pha.pa.us> wrote: > > What do you think about it? Shall I abandon FreeBSD and go ahead > > Incorporating Trio? > > Yes, maybe just add the proper %$ handling from Trio to what we have > now. Adding proper %$ from Trio will require too much effort. I would rather not do it. Not because I am lazy but because: 1) Trio team seem to be very serious about standards, update the library as soon as new standards come out: <quote> Trio fully implements the C99 (ISO/IEC 9899:1999) and UNIX98 (the Single Unix Specification, Version 2) standards, as well as many features from other implementations, e.g. the GNU libc and BSD4. </quote> 2) If we integrate the whole library in source code we will not have to maintain it and will rely on Trio team for bug fixes and updates. Integrating it will be very easy since all of the functions begin with "trio_". I used it instead of the src/port/snrpintf.c one and it passes regression tests under Win32 just fine. The downside is that Trio library is rather big. It is 3 .c and 6 .h files totalling 11556 lines. Compiled it is 71224 bytes not stripped and 56204 bytes stripped on Solaris 10 for x86, 32-bit. Even for a shared library it will probably be too much. Trio has a lot of string handling functions which are probably not necessary. Would you like me to try to remove everything unnecessary from it or we will settle with the full version? Regards, Nicolai Tufar
Nicolai Tufar wrote: > On Wed, 9 Mar 2005 22:51:27 -0500 (EST), Bruce Momjian > <pgman@candle.pha.pa.us> wrote: > > > What do you think about it? Shall I abandon FreeBSD and go ahead > > > Incorporating Trio? > > > > Yes, maybe just add the proper %$ handling from Trio to what we have > > now. > > Adding proper %$ from Trio will require too much effort. I would > rather not do it. Not because I am lazy but because: > > 1) Trio team seem to be very serious about standards, update > the library as soon as new standards come out: > <quote> > Trio fully implements the C99 (ISO/IEC 9899:1999) and UNIX98 (the > Single Unix Specification, Version 2) standards, as well as many > features from other implementations, e.g. the GNU libc and BSD4. > </quote> > > 2) If we integrate the whole library in source code we will > not have to maintain it and will rely on Trio team for bug fixes > and updates. Integrating it will be very easy since all of the > functions begin with "trio_". I used it instead of the src/port/snrpintf.c > one and it passes regression tests under Win32 just fine. > > The downside is that Trio library is rather big. It is 3 .c and 6 .h > files totalling 11556 lines. Compiled it is 71224 bytes not stripped > and 56204 bytes stripped on Solaris 10 for x86, 32-bit. Even for > a shared library it will probably be too much. Trio has a lot > of string handling functions which are probably not necessary. > Would you like me to try to remove everything unnecessary from > it or we will settle with the full version? Please see my posting about using a macro for snprintf. If the current implementation of snprintf is enough for our existing translation users we probably don't need to add anything more to it because snprintf will not be exported to client applications. -- 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
On Thu, 10 Mar 2005 16:26:47 -0500 (EST), Bruce Momjian <pgman@candle.pha.pa.us> wrote: > Please see my posting about using a macro for snprintf. If the current > implementation of snprintf is enough for our existing translation users > we probably don't need to add anything more to it because snprintf will > not be exported to client applications. Oh, Bruce. It will be the best solution. I was worried about the problems with my modifications to snprintf.c Tom Lane pointed out. But if we really separate snprintf() used by messages and snprintf() used by the like of src/backend/utils/adt/int8.c then we are safe. We can claim current release safe and I will modify src/port/snprintf.c at my leisure later. I will try out your modifications tomorrow. It is late here and I have a PostgreSQL class to to teach tomorrow ;) I still think that it is more convenient to rip off current implementation of snprintf.c and replace it with a very much stripped down of Trio's one. I will work on it and try to get a patch in one week's time. Thank you all for your patience. Best regards, Nicolai Tufar > > -- > 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 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) >
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Please see my posting about using a macro for snprintf. If the current
> implementation of snprintf is enough for our existing translation users
> we probably don't need to add anything more to it because snprintf will
> not be exported to client applications.
The CVS-tip implementation is fundamentally broken and won't work even
for our internal uses. I've not wasted time complaining about it
because I thought we were going to replace it. If we can't find a
usable replacement then we're going to have to put a lot of effort
into fixing what's there. On the whole I think the effort would be
better spent importing someone else's solution.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Please see my posting about using a macro for snprintf. If the current > > implementation of snprintf is enough for our existing translation users > > we probably don't need to add anything more to it because snprintf will > > not be exported to client applications. > > The CVS-tip implementation is fundamentally broken and won't work even > for our internal uses. I've not wasted time complaining about it > because I thought we were going to replace it. If we can't find a > usable replacement then we're going to have to put a lot of effort > into fixing what's there. On the whole I think the effort would be > better spent importing someone else's solution. Oh, so our existing implementation doesn't even meet our needs. OK. -- 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: >> Bruce Momjian <pgman@candle.pha.pa.us> writes: >> > Please see my posting about using a macro for snprintf. If the >> current >> > implementation of snprintf is enough for our existing translation >> users >> > we probably don't need to add anything more to it because snprintf >> will >> > not be exported to client applications. >> >> The CVS-tip implementation is fundamentally broken and won't work even >> for our internal uses. I've not wasted time complaining about it >> because I thought we were going to replace it. If we can't find a >> usable replacement then we're going to have to put a lot of effort >> into fixing what's there. On the whole I think the effort would be >> better spent importing someone else's solution. > > Oh, so our existing implementation doesn't even meet our needs. OK. Wasn't the issue about odd behavior of the Win32 linker choosing the wrong vnsprintf?
pgsql@mohawksoft.com writes:
>>> Please see my posting about using a macro for snprintf.
> Wasn't the issue about odd behavior of the Win32 linker choosing the wrong
> vnsprintf?
You're right, the point about the macro was to avoid linker weirdness on
Windows. We need to do that part in any case. I think Bruce confused
that issue with the one about whether our version supported %n$
adequately ... which it doesn't just yet ...
regards, tom lane
pgsql@mohawksoft.com wrote: > > Tom Lane wrote: > >> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> > Please see my posting about using a macro for snprintf. If the > >> current > >> > implementation of snprintf is enough for our existing translation > >> users > >> > we probably don't need to add anything more to it because snprintf > >> will > >> > not be exported to client applications. > >> > >> The CVS-tip implementation is fundamentally broken and won't work even > >> for our internal uses. I've not wasted time complaining about it > >> because I thought we were going to replace it. If we can't find a > >> usable replacement then we're going to have to put a lot of effort > >> into fixing what's there. On the whole I think the effort would be > >> better spent importing someone else's solution. > > > > Oh, so our existing implementation doesn't even meet our needs. OK. > > Wasn't the issue about odd behavior of the Win32 linker choosing the wrong > vnsprintf? Ah, but with my new patch to be applied tomorrow to use macros and rename to pg_snprintf there no longer is any conflict with the system versions of these functions. -- 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: > The CVS-tip implementation is fundamentally broken and won't work even > for our internal uses. I've not wasted time complaining about it > because I thought we were going to replace it. If we can't find a > usable replacement then we're going to have to put a lot of effort > into fixing what's there. On the whole I think the effort would be > better spent importing someone else's solution. Bruce Momjian wrote: > Oh, so our existing implementation doesn't even meet our needs. OK. Very well, I too, tend to think that importing some else's solution is the way to go. Tom, have you looked at Trio? If it is fine with you too, I will strip it to the bare minimum needed for snprintf(), vsnprintf() and printf() by Saturday. Regards, Nicolai Tufar
Nicolai Tufar <ntufar@gmail.com> writes:
> Very well, I too, tend to think that importing some else's solution
> is the way to go. Tom, have you looked at Trio?
I have not seen it ... but if it looks reasonable to you, have a go
at it.
regards, tom lane
On Fri, 11 Mar 2005 01:14:31 -0500, Tom Lane wrote: > Nicolai Tufar <ntufar@gmail.com> writes: > > Very well, I too, tend to think that importing some else's solution > > is the way to go. Tom, have you looked at Trio? > > I have not seen it ... but if it looks reasonable to you, have a go > at it. It looks reasonable to me. It claims to: fully implement C99 (ISO/IEC 9899:1999) and UNIX98 (the Single Unix Specification, Version 2) standards, as well as many features from other implementations, e.g. the GNU libc and BSD4. I compiled and run regression tests with it used instead of our current implementation and it worked fine under win32 and Solaris x86. The only problem is that it is 11000 lines as oposed to our curent implementation's 600. I will remove everything unnecessary and submit a patch for consideration. Regards, Nicolai Tufar
Tom Lane wrote:
> pgsql@mohawksoft.com writes:
> >>> Please see my posting about using a macro for snprintf.
>
> > Wasn't the issue about odd behavior of the Win32 linker choosing the wrong
> > vnsprintf?
>
> You're right, the point about the macro was to avoid linker weirdness on
> Windows. We need to do that part in any case. I think Bruce confused
> that issue with the one about whether our version supported %n$
> adequately ... which it doesn't just yet ...
Perhaps I am reading old email in this reply but I thought I should
clarify:
Once we do:
#define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
#define snprintf(...) pg_snprintf(__VA_ARGS__)
#define printf(...) pg_printf(__VA_ARGS__)
we also rename the functions in snprintf.c to pg_* names so there is no
longer a conflict with the system libc versions.
The macro is to prevent our snprintf from leaking out of libraries like
libpq, not to fix the win32 linker problem, which we already had fixed
by reordering the entries in the C file.
Perhaps the macro idea originally came as a fix for Win32 but it is much
larger that that now.
--
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
Nicolai Tufar wrote: > On Thu, 10 Mar 2005 16:26:47 -0500 (EST), Bruce Momjian > <pgman@candle.pha.pa.us> wrote: > > Please see my posting about using a macro for snprintf. If the current > > implementation of snprintf is enough for our existing translation users > > we probably don't need to add anything more to it because snprintf will > > not be exported to client applications. > > Oh, Bruce. It will be the best solution. I was worried about > the problems with my modifications to snprintf.c Tom Lane > pointed out. But if we really separate snprintf() used by > messages and snprintf() used by the like of > src/backend/utils/adt/int8.c then we are safe. We can claim > current release safe and I will modify src/port/snprintf.c at my > leisure later. I will try out your modifications tomorrow. It > is late here and I have a PostgreSQL class to to teach > tomorrow ;) > > I still think that it is more convenient to rip off current > implementation of snprintf.c and replace it with a very much > stripped down of Trio's one. I will work on it and try to get > a patch in one week's time. Thank you all for your patience. I am not heading in the direction of using a different snprintf for messages and for int8.c. I am just renaming the calls via macros so we don't leak snprintf from libpq. One new idea I had was to have pg_snprintf() look over the format string and adjust the arguments to match what the format string is requesting, remove %$ from the format string, and then pass it to the native libc snprintf(). That might be the easiest solution for the many platforms with a good snprintf but not %$ support. Is that possible/easier? -- 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: >> pgsql@mohawksoft.com writes: >> >>> Please see my posting about using a macro for snprintf. >> >> > Wasn't the issue about odd behavior of the Win32 linker choosing the >> wrong >> > vnsprintf? >> >> You're right, the point about the macro was to avoid linker weirdness on >> Windows. We need to do that part in any case. I think Bruce confused >> that issue with the one about whether our version supported %n$ >> adequately ... which it doesn't just yet ... > > Perhaps I am reading old email in this reply but I thought I should > clarify: > > Once we do: > > #define vsnprintf(...) pg_vsnprintf(__VA_ARGS__) > #define snprintf(...) pg_snprintf(__VA_ARGS__) > #define printf(...) pg_printf(__VA_ARGS__) I'm not sure that macros can have variable number of arguments on all supported platforms. I've been burnt by this before.
pgsql@mohawksoft.com wrote:
> > Tom Lane wrote:
> >> pgsql@mohawksoft.com writes:
> >> >>> Please see my posting about using a macro for snprintf.
> >>
> >> > Wasn't the issue about odd behavior of the Win32 linker choosing the
> >> wrong
> >> > vnsprintf?
> >>
> >> You're right, the point about the macro was to avoid linker weirdness on
> >> Windows. We need to do that part in any case. I think Bruce confused
> >> that issue with the one about whether our version supported %n$
> >> adequately ... which it doesn't just yet ...
> >
> > Perhaps I am reading old email in this reply but I thought I should
> > clarify:
> >
> > Once we do:
> >
> > #define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
> > #define snprintf(...) pg_snprintf(__VA_ARGS__)
> > #define printf(...) pg_printf(__VA_ARGS__)
>
>
> I'm not sure that macros can have variable number of arguments on all
> supported platforms. I've been burnt by this before.
The actual patch is:
+ #ifdef __GNUC__
+ #define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
+ #define snprintf(...) pg_snprintf(__VA_ARGS__)
+ #define printf(...) pg_printf(__VA_ARGS__)
+ #else
+ #define vsnprintf pg_vsnprintf
+ #define snprintf pg_snprintf
+ #define printf pg_printf
+ #endif
--
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 <pgman@candle.pha.pa.us> writes:
>> I'm not sure that macros can have variable number of arguments on all
>> supported platforms. I've been burnt by this before.
> The actual patch is:
> + #ifdef __GNUC__
> + #define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
> + #define snprintf(...) pg_snprintf(__VA_ARGS__)
> + #define printf(...) pg_printf(__VA_ARGS__)
> + #else
> + #define vsnprintf pg_vsnprintf
> + #define snprintf pg_snprintf
> + #define printf pg_printf
> + #endif
Uh, why bother with the different approach for gcc?
Also, what happened to fprintf? We're going to need that too for
localization of the client programs.
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> I'm not sure that macros can have variable number of arguments on all
> >> supported platforms. I've been burnt by this before.
>
> > The actual patch is:
>
> > + #ifdef __GNUC__
> > + #define vsnprintf(...) pg_vsnprintf(__VA_ARGS__)
> > + #define snprintf(...) pg_snprintf(__VA_ARGS__)
> > + #define printf(...) pg_printf(__VA_ARGS__)
> > + #else
> > + #define vsnprintf pg_vsnprintf
> > + #define snprintf pg_snprintf
> > + #define printf pg_printf
> > + #endif
>
> Uh, why bother with the different approach for gcc?
Because if we don't do that then the code above fails:
extern int pg_snprintf(char *str, size_t count, const char *fmt,...)
/* This extension allows gcc to check the format string */
__attribute__((format(printf, 3, 4)));
The issue is that the "printf" here is interpreted specially by the
compiler to mean "check arguments as printf". If the preprocessor
changes that, we get a failure. The good news is that only gcc supports
arg checking using __attribute__ and it also supports the __VA_ARGS__
macros. What I think we do lose is argument checking for non-gcc, but
this seems as close as we can get.
> Also, what happened to fprintf? We're going to need that too for
> localization of the client programs.
It was never there. I will add it now.
--
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: > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > >> I'm not sure that macros can have variable number of arguments on all > > >> supported platforms. I've been burnt by this before. > > > > > The actual patch is: > > > > > + #ifdef __GNUC__ > > > + #define vsnprintf(...) pg_vsnprintf(__VA_ARGS__) > > > + #define snprintf(...) pg_snprintf(__VA_ARGS__) > > > + #define printf(...) pg_printf(__VA_ARGS__) > > > + #else > > > + #define vsnprintf pg_vsnprintf > > > + #define snprintf pg_snprintf > > > + #define printf pg_printf > > > + #endif > > > > Uh, why bother with the different approach for gcc? > > Because if we don't do that then the code above fails: > > extern int pg_snprintf(char *str, size_t count, const char *fmt,...) > /* This extension allows gcc to check the format string */ > __attribute__((format(printf, 3, 4))); > > The issue is that the "printf" here is interpreted specially by the > compiler to mean "check arguments as printf". If the preprocessor > changes that, we get a failure. The good news is that only gcc supports > arg checking using __attribute__ and it also supports the __VA_ARGS__ > macros. What I think we do lose is argument checking for non-gcc, but > this seems as close as we can get. I am adding a comment explaining why those macros are used. -- 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
On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian <pgman@candle.pha.pa.us> wrote: > > The CVS-tip implementation is fundamentally broken and won't work even > > for our internal uses. I've not wasted time complaining about it > > because I thought we were going to replace it. If we can't find a > > usable replacement then we're going to have to put a lot of effort > > into fixing what's there. On the whole I think the effort would be > > better spent importing someone else's solution. > > Oh, so our existing implementation doesn't even meet our needs. OK. Which made me wander why did I not aggree with Tom Lane's suggestion to make do three passes instead of two. Tom was right, as usual. It happened to be much easier than I expected. The patch is attached. Please apply. Tom, what do you think? Will it be fine with you? Best regards, Nicolai
Attachment
Resubmission of yesterday's patch so that it would cont conflict with Bruce's cvs commit. Pleas apply. Best regards, Nicolai. On Sat, 12 Mar 2005 01:58:15 +0200, Nicolai Tufar <ntufar@gmail.com> wrote: > On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian > <pgman@candle.pha.pa.us> wrote: > > > The CVS-tip implementation is fundamentally broken and won't work even > > > for our internal uses. I've not wasted time complaining about it > > > because I thought we were going to replace it. If we can't find a > > > usable replacement then we're going to have to put a lot of effort > > > into fixing what's there. On the whole I think the effort would be > > > better spent importing someone else's solution. > > > > Oh, so our existing implementation doesn't even meet our needs. OK. > > Which made me wander why did I not aggree with > Tom Lane's suggestion to make do three passes > instead of two. Tom was right, as usual. It happened to > be much easier than I expected. The patch is attached. > Please apply. > > Tom, what do you think? Will it be fine with you? > > Best regards, > Nicolai > > >
Attachment
Nicolai Tufar wrote: > On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian > <pgman@candle.pha.pa.us> wrote: > > > The CVS-tip implementation is fundamentally broken and won't work even > > > for our internal uses. I've not wasted time complaining about it > > > because I thought we were going to replace it. If we can't find a > > > usable replacement then we're going to have to put a lot of effort > > > into fixing what's there. On the whole I think the effort would be > > > better spent importing someone else's solution. > > > > Oh, so our existing implementation doesn't even meet our needs. OK. (Your new patch is in the queue.) I have been thinking about our current snprintf() implementation. As I remember, we use snprintf mostly for an snprintf that doesn't support long long, and now those that don't support %$. I am wondering if we should just process long long args and %$ args, and pass everything else to the native snprintf. In fact, one trick would be to substitute long long and %$ in the printf format string, and then pass that to the native libc printf, with adjustments for the printf format arguments. That might be simpler than emulating all of snprintf. FYI, now that we are using pg_snprintf macros the native snprintf is available to us. Anyway, I am sure there are some platforms that don't have vsnprint or snprintf, but could we just say we don't support them, or emulate one of we only have the other? -- 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 <pgman@candle.pha.pa.us> writes:
> I am wondering if we should just process long long args and %$ args,
> and pass everything else to the native snprintf.
AFAICS this is a non-starter --- how will you construct the call to
snprintf? Or even vsnprintf? C doesn't provide the tools you need
to make it happen.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I am wondering if we should just process long long args and %$ args, > > and pass everything else to the native snprintf. > > AFAICS this is a non-starter --- how will you construct the call to > snprintf? Or even vsnprintf? C doesn't provide the tools you need > to make it happen. Couldn't you spin through the varargs and reconstruct a new one? Is there no way to create a va_arg va_list structure in C? -- 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 <pgman@candle.pha.pa.us> writes:
> Is there no way to create a va_arg va_list structure in C?
Exactly. The standard lets you *read out* from such a structure,
but there's no provision for creating one on-the-fly.
regards, tom lane
I have applied a modified version of your patch, attached. initdb will not work without %*s support, so I had to add that. Please look over my work. I don't think i handle %*1$ but I am not even sure what that means or if our translators would ever use such a thing. You can probably test that better than I can. Your patch didn't handle signed vs. unsigned va_arg values properly.. There were also maxwidth tests around 's' that I had to remove to support %*. I think those tests are down in fmtstr too but please check. initdb and regression pass. --------------------------------------------------------------------------- Nicolai Tufar wrote: > Resubmission of yesterday's patch so that it would > cont conflict with Bruce's cvs commit. Pleas apply. > > Best regards, > Nicolai. > > > On Sat, 12 Mar 2005 01:58:15 +0200, Nicolai Tufar <ntufar@gmail.com> wrote: > > On Thu, 10 Mar 2005 19:21:41 -0500 (EST), Bruce Momjian > > <pgman@candle.pha.pa.us> wrote: > > > > The CVS-tip implementation is fundamentally broken and won't work even > > > > for our internal uses. I've not wasted time complaining about it > > > > because I thought we were going to replace it. If we can't find a > > > > usable replacement then we're going to have to put a lot of effort > > > > into fixing what's there. On the whole I think the effort would be > > > > better spent importing someone else's solution. > > > > > > Oh, so our existing implementation doesn't even meet our needs. OK. > > > > Which made me wander why did I not aggree with > > Tom Lane's suggestion to make do three passes > > instead of two. Tom was right, as usual. It happened to > > be much easier than I expected. The patch is attached. > > Please apply. > > > > Tom, what do you think? Will it be fine with you? > > > > Best regards, > > Nicolai > > > > > > [ Attachment, skipping... ] -- 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/port/snprintf.c =================================================================== RCS file: /cvsroot/pgsql/src/port/snprintf.c,v retrieving revision 1.19 diff -c -c -r1.19 snprintf.c *** src/port/snprintf.c 12 Mar 2005 04:00:56 -0000 1.19 --- src/port/snprintf.c 16 Mar 2005 05:46:33 -0000 *************** *** 151,170 **** #define FMTSTR 1 #define FMTNUM 2 ! #define FMTFLOAT 3 ! #define FMTCHAR 4 static void dopr(char *buffer, const char *format, va_list args, char *end) { int ch; - int64 value; - double fvalue; int longlongflag = 0; int longflag = 0; int pointflag = 0; int maxwidth = 0; - char *strvalue; int ljust; int len; int zpad; --- 151,170 ---- #define FMTSTR 1 #define FMTNUM 2 ! #define FMTNUM_U 3 ! #define FMTFLOAT 4 ! #define FMTCHAR 5 ! #define FMTWIDTH 6 ! #define FMTLEN 7 static void dopr(char *buffer, const char *format, va_list args, char *end) { int ch; int longlongflag = 0; int longflag = 0; int pointflag = 0; int maxwidth = 0; int ljust; int len; int zpad; *************** *** 173,178 **** --- 173,179 ---- const char* fmtbegin; int fmtpos = 1; int realpos = 0; + int precision; int position; char *output; int percents = 1; *************** *** 195,200 **** --- 196,203 ---- int pointflag; char func; int realpos; + int longflag; + int longlongflag; } *fmtpar, **fmtparptr; /* Create enough structures to hold all arguments */ *************** *** 229,240 **** longflag = longlongflag = pointflag = 0; fmtbegin = format - 1; realpos = 0; ! position = 0; nextch: ch = *format++; switch (ch) { ! case 0: goto performpr; case '-': ljust = 1; --- 232,243 ---- longflag = longlongflag = pointflag = 0; fmtbegin = format - 1; realpos = 0; ! position = precision = 0; nextch: ch = *format++; switch (ch) { ! case '\0': goto performpr; case '-': ljust = 1; *************** *** 251,274 **** case '7': case '8': case '9': ! if (pointflag) ! /* could also be precision */ ! maxwidth = maxwidth * 10 + ch - '0'; ! else { len = len * 10 + ch - '0'; position = position * 10 + ch - '0'; } goto nextch; case '$': realpos = position; len = 0; goto nextch; case '*': ! if (pointflag) ! maxwidth = va_arg(args, int); else ! len = va_arg(args, int); goto nextch; case '.': pointflag = 1; --- 254,282 ---- case '7': case '8': case '9': ! if (!pointflag) { len = len * 10 + ch - '0'; position = position * 10 + ch - '0'; } + else + { + maxwidth = maxwidth * 10 + ch - '0'; + precision = precision * 10 + ch - '0'; + } goto nextch; case '$': realpos = position; len = 0; goto nextch; case '*': ! MemSet(&fmtpar[fmtpos], 0, sizeof(fmtpar[fmtpos])); ! if (!pointflag) ! fmtpar[fmtpos].func = FMTLEN; else ! fmtpar[fmtpos].func = FMTWIDTH; ! fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; ! fmtpos++; goto nextch; case '.': pointflag = 1; *************** *** 301,368 **** #endif case 'u': case 'U': ! /* fmtnum(value,base,dosign,ljust,len,zpad,&output) */ ! if (longflag) ! { ! if (longlongflag) ! value = va_arg(args, uint64); ! else ! value = va_arg(args, unsigned long); ! } ! else ! value = va_arg(args, unsigned int); fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; - fmtpar[fmtpos].numvalue = value; fmtpar[fmtpos].base = 10; fmtpar[fmtpos].dosign = 0; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 'o': case 'O': ! /* fmtnum(value,base,dosign,ljust,len,zpad,&output) */ ! if (longflag) ! { ! if (longlongflag) ! value = va_arg(args, uint64); ! else ! value = va_arg(args, unsigned long); ! } ! else ! value = va_arg(args, unsigned int); fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; - fmtpar[fmtpos].numvalue = value; fmtpar[fmtpos].base = 8; fmtpar[fmtpos].dosign = 0; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 'd': case 'D': ! if (longflag) ! { ! if (longlongflag) ! { ! value = va_arg(args, int64); ! } ! else ! value = va_arg(args, long); ! } ! else ! value = va_arg(args, int); fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; - fmtpar[fmtpos].numvalue = value; fmtpar[fmtpos].base = 10; fmtpar[fmtpos].dosign = 1; fmtpar[fmtpos].ljust = ljust; --- 309,348 ---- #endif case 'u': case 'U': ! fmtpar[fmtpos].longflag = longflag; ! fmtpar[fmtpos].longlongflag = longlongflag; fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 10; fmtpar[fmtpos].dosign = 0; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM_U; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 'o': case 'O': ! fmtpar[fmtpos].longflag = longflag; ! fmtpar[fmtpos].longlongflag = longlongflag; fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 8; fmtpar[fmtpos].dosign = 0; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM_U; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 'd': case 'D': ! fmtpar[fmtpos].longflag = longflag; ! fmtpar[fmtpos].longlongflag = longlongflag; fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 10; fmtpar[fmtpos].dosign = 1; fmtpar[fmtpos].ljust = ljust; *************** *** 373,444 **** fmtpos++; break; case 'x': ! if (longflag) ! { ! if (longlongflag) ! value = va_arg(args, uint64); ! else ! value = va_arg(args, unsigned long); ! } ! else ! value = va_arg(args, unsigned int); fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; - fmtpar[fmtpos].numvalue = value; fmtpar[fmtpos].base = 16; fmtpar[fmtpos].dosign = 0; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 'X': ! if (longflag) ! { ! if (longlongflag) ! value = va_arg(args, uint64); ! else ! value = va_arg(args, unsigned long); ! } ! else ! value = va_arg(args, unsigned int); fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; - fmtpar[fmtpos].numvalue = value; fmtpar[fmtpos].base = -16; fmtpar[fmtpos].dosign = 1; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 's': ! strvalue = va_arg(args, char *); ! if (maxwidth > 0 || !pointflag) ! { ! if (pointflag && len > maxwidth) ! len = maxwidth; /* Adjust padding */ ! fmtpar[fmtpos].fmtbegin = fmtbegin; ! fmtpar[fmtpos].fmtend = format; ! fmtpar[fmtpos].value = strvalue; ! fmtpar[fmtpos].ljust = ljust; ! fmtpar[fmtpos].len = len; ! fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].maxwidth = maxwidth; ! fmtpar[fmtpos].func = FMTSTR; ! fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; ! fmtpos++; ! } break; case 'c': - ch = va_arg(args, int); fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; - fmtpar[fmtpos].charvalue = ch; fmtpar[fmtpos].func = FMTCHAR; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; --- 353,399 ---- fmtpos++; break; case 'x': ! fmtpar[fmtpos].longflag = longflag; ! fmtpar[fmtpos].longlongflag = longlongflag; fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 16; fmtpar[fmtpos].dosign = 0; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM_U; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 'X': ! fmtpar[fmtpos].longflag = longflag; ! fmtpar[fmtpos].longlongflag = longlongflag; fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = -16; fmtpar[fmtpos].dosign = 1; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].func = FMTNUM_U; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; break; case 's': ! fmtpar[fmtpos].fmtbegin = fmtbegin; ! fmtpar[fmtpos].fmtend = format; ! fmtpar[fmtpos].ljust = ljust; ! fmtpar[fmtpos].len = len; ! fmtpar[fmtpos].zpad = zpad; ! fmtpar[fmtpos].maxwidth = maxwidth; ! fmtpar[fmtpos].func = FMTSTR; ! fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; ! fmtpos++; break; case 'c': fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].func = FMTCHAR; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; fmtpos++; *************** *** 448,462 **** case 'f': case 'g': case 'G': - fvalue = va_arg(args, double); fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; - fmtpar[fmtpos].fvalue = fvalue; fmtpar[fmtpos].type = ch; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].maxwidth = maxwidth; ! fmtpar[fmtpos].precision = position; fmtpar[fmtpos].pointflag = pointflag; fmtpar[fmtpos].func = FMTFLOAT; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; --- 403,415 ---- case 'f': case 'g': case 'G': fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].type = ch; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].maxwidth = maxwidth; ! fmtpar[fmtpos].precision = precision; fmtpar[fmtpos].pointflag = pointflag; fmtpar[fmtpos].func = FMTFLOAT; fmtpar[fmtpos].realpos = realpos?realpos:fmtpos; *************** *** 473,494 **** break; } } performpr: ! /* shuffle pointers */ for(i = 1; i < fmtpos; i++) fmtparptr[i] = &fmtpar[fmtpar[i].realpos]; output = buffer; format = format_save; while ((ch = *format++)) { for(i = 1; i < fmtpos; i++) { ! if(ch == '%' && *format == '%') { format++; continue; } ! if(fmtpar[i].fmtbegin == format - 1) { switch(fmtparptr[i]->func){ case FMTSTR: --- 426,499 ---- break; } } + performpr: ! /* reorder pointers */ for(i = 1; i < fmtpos; i++) fmtparptr[i] = &fmtpar[fmtpar[i].realpos]; + + /* assign values */ + for(i = 1; i < fmtpos; i++){ + switch(fmtparptr[i]->func){ + case FMTSTR: + fmtparptr[i]->value = va_arg(args, char *); + break; + case FMTNUM: + if (fmtparptr[i]->longflag) + { + if (fmtparptr[i]->longlongflag) + fmtparptr[i]->numvalue = va_arg(args, int64); + else + fmtparptr[i]->numvalue = va_arg(args, long); + } + else + fmtparptr[i]->numvalue = va_arg(args, int); + break; + case FMTNUM_U: + if (fmtparptr[i]->longflag) + { + if (fmtparptr[i]->longlongflag) + fmtparptr[i]->numvalue = va_arg(args, uint64); + else + fmtparptr[i]->numvalue = va_arg(args, unsigned long); + } + else + fmtparptr[i]->numvalue = va_arg(args, unsigned int); + break; + case FMTFLOAT: + fmtparptr[i]->fvalue = va_arg(args, double); + break; + case FMTCHAR: + fmtparptr[i]->charvalue = va_arg(args, int); + break; + case FMTLEN: + if (i + 1 < fmtpos && fmtpar[i + 1].func != FMTWIDTH) + fmtpar[i + 1].len = va_arg(args, int); + /* For "%*.*f", use the second arg */ + if (i + 2 < fmtpos && fmtpar[i + 1].func == FMTWIDTH) + fmtpar[i + 2].len = va_arg(args, int); + break; + case FMTWIDTH: + if (i + 1 < fmtpos) + fmtpar[i + 1].maxwidth = fmtpar[i + 1].precision = + va_arg(args, int); + break; + } + } + + /* do the output */ output = buffer; format = format_save; while ((ch = *format++)) { for(i = 1; i < fmtpos; i++) { ! if (ch == '%' && *format == '%') { format++; continue; } ! if (fmtpar[i].fmtbegin == format - 1) { switch(fmtparptr[i]->func){ case FMTSTR: *************** *** 497,502 **** --- 502,508 ---- fmtparptr[i]->maxwidth, end, &output); break; case FMTNUM: + case FMTNUM_U: fmtnum(fmtparptr[i]->numvalue, fmtparptr[i]->base, fmtparptr[i]->dosign, fmtparptr[i]->ljust, fmtparptr[i]->len, fmtparptr[i]->zpad, end, &output);
On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian <pgman@candle.pha.pa.us> wrote: > > I have applied a modified version of your patch, attached. I am so sorry, I sent untested patch again. Thank you very much for patience in fixing it. The patch looks perfectly fine and works under Solaris. Under win32 I am still struggling with build environment. In many directories link fails with "undefined reference to `pg_snprintf'" in other it fails with "undefined reference to `_imp__libintl_sprintf'". In yet another directory it fails with both, like in src/interfaces/ecpg/pgtypeslib: dlltool --export-all --output-def pgtypes.def numeric.o datetime.o common.o dt_common.o timestamp.o interval.o pgstrcasecmp.o dllwrap -o libpgtypes.dll --dllname libpgtypes.dll --def pgtypes.def numeric.o datetime.o common.o dt_common.o timestamp.o interval.o pgstrcasecmp.o -L../../../../src/port -lm numeric.o(.text+0x19ea):numeric.c: undefined reference to `_imp__libintl_sprintf' datetime.o(.text+0x476):datetime.c: undefined reference to `pg_snprintf' common.o(.text+0x1cd):common.c: undefined reference to `pg_snprintf' common.o(.text+0x251):common.c: undefined reference to `pg_snprintf' dt_common.o(.text+0x538):dt_common.c: undefined reference to `_imp__libintl_sprintf' dt_common.o(.text+0x553):dt_common.c: undefined reference to `_imp__libintl_sprintf' dt_common.o(.text+0x597):dt_common.c: undefined reference to `_imp__libintl_sprintf' dt_common.o(.text+0x5d5):dt_common.c: undefined reference to `_imp__libintl_sprintf' dt_common.o(.text+0x628):dt_common.c: undefined reference to `_imp__libintl_sprintf' dt_common.o(.text+0x7e8):dt_common.c: more undefined references to `_imp__libintl_sprintf' follow c:\MinGW\bin\dllwrap.exe: c:\MinGW\bin\gcc exited with status 1 make: *** [libpgtypes.a] Error 1 Could someone with a better grasp of configure and win32 environment check it? Aparently no one regularily compiles source code under win32 during development cycle these days. Best regards, Nicolai
Nicolai Tufar wrote: > On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian > <pgman@candle.pha.pa.us> wrote: > > > > I have applied a modified version of your patch, attached. > Here is a patch that fixes the %*$ case. FYI, I am going to pgindent snprintf.c to make it consistent so please us CVS for your next patch. I will work on your Win32 compile problem next. -- 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/port/snprintf.c =================================================================== RCS file: /cvsroot/pgsql/src/port/snprintf.c,v retrieving revision 1.20 diff -c -c -r1.20 snprintf.c *** src/port/snprintf.c 16 Mar 2005 06:00:58 -0000 1.20 --- src/port/snprintf.c 16 Mar 2005 14:59:00 -0000 *************** *** 467,481 **** fmtparptr[i]->charvalue = va_arg(args, int); break; case FMTLEN: ! if (i + 1 < fmtpos && fmtpar[i + 1].func != FMTWIDTH) ! fmtpar[i + 1].len = va_arg(args, int); /* For "%*.*f", use the second arg */ ! if (i + 2 < fmtpos && fmtpar[i + 1].func == FMTWIDTH) ! fmtpar[i + 2].len = va_arg(args, int); break; case FMTWIDTH: if (i + 1 < fmtpos) ! fmtpar[i + 1].maxwidth = fmtpar[i + 1].precision = va_arg(args, int); break; } --- 467,481 ---- fmtparptr[i]->charvalue = va_arg(args, int); break; case FMTLEN: ! if (i + 1 < fmtpos && fmtparptr[i + 1]->func != FMTWIDTH) ! fmtparptr[i + 1]->len = va_arg(args, int); /* For "%*.*f", use the second arg */ ! if (i + 2 < fmtpos && fmtparptr[i + 1]->func == FMTWIDTH) ! fmtparptr[i + 2]->len = va_arg(args, int); break; case FMTWIDTH: if (i + 1 < fmtpos) ! fmtparptr[i + 1]->maxwidth = fmtparptr[i + 1]->precision = va_arg(args, int); break; }
Nicolai Tufar wrote: > On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian > <pgman@candle.pha.pa.us> wrote: > > > > I have applied a modified version of your patch, attached. > > I am so sorry, I sent untested patch again. Thank you very > much for patience in fixing it. The patch looks perfectly > fine and works under Solaris. > Here is another patch that adds sprintf() support, and support for '+', 'h', and fixes '%*s' support. Applied. -- 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/bin/psql/command.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v retrieving revision 1.141 diff -c -c -r1.141 command.c *** src/bin/psql/command.c 11 Mar 2005 17:20:34 -0000 1.141 --- src/bin/psql/command.c 16 Mar 2005 21:17:50 -0000 *************** *** 1574,1584 **** shellName = DEFAULT_SHELL; sys = pg_malloc(strlen(shellName) + 16); sprintf(sys, /* See EDITOR handling comment for an explaination */ - #ifndef WIN32 "exec %s", shellName); #else "%s\"%s\"%s", SYSTEMQUOTE, shellName, SYSTEMQUOTE); #endif result = system(sys); --- 1574,1586 ---- shellName = DEFAULT_SHELL; sys = pg_malloc(strlen(shellName) + 16); + #ifndef WIN32 sprintf(sys, /* See EDITOR handling comment for an explaination */ "exec %s", shellName); #else + sprintf(sys, + /* See EDITOR handling comment for an explaination */ "%s\"%s\"%s", SYSTEMQUOTE, shellName, SYSTEMQUOTE); #endif result = system(sys); Index: src/include/port.h =================================================================== RCS file: /cvsroot/pgsql/src/include/port.h,v retrieving revision 1.72 diff -c -c -r1.72 port.h *** src/include/port.h 11 Mar 2005 19:13:42 -0000 1.72 --- src/include/port.h 16 Mar 2005 21:17:50 -0000 *************** *** 112,117 **** --- 112,120 ---- extern int pg_snprintf(char *str, size_t count, const char *fmt,...) /* This extension allows gcc to check the format string */ __attribute__((format(printf, 3, 4))); + extern int pg_sprintf(char *str, const char *fmt,...) + /* This extension allows gcc to check the format string */ + __attribute__((format(printf, 2, 3))); extern int pg_fprintf(FILE *stream, const char *fmt,...) /* This extension allows gcc to check the format string */ __attribute__((format(printf, 2, 3))); *************** *** 127,137 **** --- 130,142 ---- #ifdef __GNUC__ #define vsnprintf(...) pg_vsnprintf(__VA_ARGS__) #define snprintf(...) pg_snprintf(__VA_ARGS__) + #define sprintf(...) pg_sprintf(__VA_ARGS__) #define fprintf(...) pg_fprintf(__VA_ARGS__) #define printf(...) pg_printf(__VA_ARGS__) #else #define vsnprintf pg_vsnprintf #define snprintf pg_snprintf + #define sprintf pg_sprintf #define fprintf pg_fprintf #define printf pg_printf #endif Index: src/port/snprintf.c =================================================================== RCS file: /cvsroot/pgsql/src/port/snprintf.c,v retrieving revision 1.22 diff -c -c -r1.22 snprintf.c *** src/port/snprintf.c 16 Mar 2005 15:12:18 -0000 1.22 --- src/port/snprintf.c 16 Mar 2005 21:17:51 -0000 *************** *** 67,80 **** /*static char _id[] = "$PostgreSQL: pgsql/src/port/snprintf.c,v 1.22 2005/03/16 15:12:18 momjian Exp $";*/ - int pg_snprintf(char *str, size_t count, const char *fmt,...); - int pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args); - int pg_printf(const char *format,...); static void dopr(char *buffer, const char *format, va_list args, char *end); /* Prevent recursion */ #undef vsnprintf #undef snprintf #undef fprintf #undef printf --- 67,78 ---- /*static char _id[] = "$PostgreSQL: pgsql/src/port/snprintf.c,v 1.22 2005/03/16 15:12:18 momjian Exp $";*/ static void dopr(char *buffer, const char *format, va_list args, char *end); /* Prevent recursion */ #undef vsnprintf #undef snprintf + #undef sprintf #undef fprintf #undef printf *************** *** 104,121 **** } int pg_fprintf(FILE *stream, const char *fmt,...) { int len; va_list args; ! char *buffer[4096]; char *p; va_start(args, fmt); ! len = pg_vsnprintf((char *) buffer, (size_t) 4096, fmt, args); va_end(args); ! p = (char *) buffer; ! for (; *p; p++) putc(*p, stream); return len; } --- 102,133 ---- } int + pg_sprintf(char *str, const char *fmt,...) + { + int len; + va_list args; + char buffer[4096]; + + va_start(args, fmt); + len = pg_vsnprintf(buffer, (size_t) 4096, fmt, args); + va_end(args); + /* limit output to string */ + StrNCpy(str, buffer, (len + 1 < 4096) ? len + 1 : 4096); + return len; + } + + int pg_fprintf(FILE *stream, const char *fmt,...) { int len; va_list args; ! char buffer[4096]; char *p; va_start(args, fmt); ! len = pg_vsnprintf(buffer, (size_t) 4096, fmt, args); va_end(args); ! for (p = buffer; *p; p++) putc(*p, stream); return len; } *************** *** 125,138 **** { int len; va_list args; ! char *buffer[4096]; char *p; va_start(args, fmt); ! len = pg_vsnprintf((char *) buffer, (size_t) 4096, fmt, args); va_end(args); ! p = (char *) buffer; ! for (; *p; p++) putchar(*p); return len; } --- 137,150 ---- { int len; va_list args; ! char buffer[4096]; char *p; va_start(args, fmt); ! len = pg_vsnprintf(buffer, (size_t) 4096, fmt, args); va_end(args); ! ! for (p = buffer; *p; p++) putchar(*p); return len; } *************** *** 141,152 **** * dopr(): poor man's version of doprintf */ ! static void fmtstr(char *value, int ljust, int len, int zpad, int maxwidth, char *end, char **output); ! static void fmtnum(int64 value, int base, int dosign, int ljust, int len, ! int zpad, char *end, char **output); ! static void fmtfloat(double value, char type, int ljust, int len, ! int precision, int pointflag, char *end, char **output); static void dostr(char *str, int cut, char *end, char **output); static void dopr_outch(int c, char *end, char **output); --- 153,165 ---- * dopr(): poor man's version of doprintf */ ! static void fmtstr(char *value, int ljust, int len, int maxwidth, char *end, char **output); ! static void fmtnum(int64 value, int base, int dosign, int forcesign, ! int ljust, int len, int zpad, char *end, char **output); ! static void fmtfloat(double value, char type, int forcesign, ! int ljust, int len, int zpad, int precision, int pointflag, char *end, ! char **output); static void dostr(char *str, int cut, char *end, char **output); static void dopr_outch(int c, char *end, char **output); *************** *** 162,174 **** dopr(char *buffer, const char *format, va_list args, char *end) { int ch; ! int longlongflag = 0; ! int longflag = 0; ! int pointflag = 0; ! int maxwidth = 0; int ljust; int len; int zpad; int i; const char *format_save; const char *fmtbegin; --- 175,188 ---- dopr(char *buffer, const char *format, va_list args, char *end) { int ch; ! int longlongflag; ! int longflag; ! int pointflag; ! int maxwidth; int ljust; int len; int zpad; + int forcesign; int i; const char *format_save; const char *fmtbegin; *************** *** 193,198 **** --- 207,213 ---- int maxwidth; int base; int dosign; + int forcesign; char type; int precision; int pointflag; *************** *** 230,236 **** switch (ch) { case '%': ! ljust = len = zpad = maxwidth = 0; longflag = longlongflag = pointflag = 0; fmtbegin = format - 1; realpos = 0; --- 245,251 ---- switch (ch) { case '%': ! ljust = len = zpad = forcesign = maxwidth = 0; longflag = longlongflag = pointflag = 0; fmtbegin = format - 1; realpos = 0; *************** *** 244,249 **** --- 259,267 ---- case '-': ljust = 1; goto nextch; + case '+': + forcesign = 1; + goto nextch; case '0': /* set zero padding if len not set */ if (len == 0 && !pointflag) zpad = '0'; *************** *** 289,294 **** --- 307,315 ---- else longflag = 1; goto nextch; + case 'h': + /* ignore */ + goto nextch; #ifdef NOT_USED /* *************** *** 318,323 **** --- 339,345 ---- fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 10; fmtpar[fmtpos].dosign = 0; + fmtpar[fmtpos].forcesign = forcesign; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; *************** *** 333,338 **** --- 355,361 ---- fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 8; fmtpar[fmtpos].dosign = 0; + fmtpar[fmtpos].forcesign = forcesign; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; *************** *** 348,353 **** --- 371,377 ---- fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 10; fmtpar[fmtpos].dosign = 1; + fmtpar[fmtpos].forcesign = forcesign; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; *************** *** 362,367 **** --- 386,392 ---- fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = 16; fmtpar[fmtpos].dosign = 0; + fmtpar[fmtpos].forcesign = forcesign; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; *************** *** 376,381 **** --- 401,407 ---- fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].base = -16; fmtpar[fmtpos].dosign = 1; + fmtpar[fmtpos].forcesign = forcesign; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; fmtpar[fmtpos].zpad = zpad; *************** *** 409,417 **** fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].type = ch; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; ! fmtpar[fmtpos].maxwidth = maxwidth; fmtpar[fmtpos].precision = precision; fmtpar[fmtpos].pointflag = pointflag; fmtpar[fmtpos].func = FMTFLOAT; --- 435,444 ---- fmtpar[fmtpos].fmtbegin = fmtbegin; fmtpar[fmtpos].fmtend = format; fmtpar[fmtpos].type = ch; + fmtpar[fmtpos].forcesign = forcesign; fmtpar[fmtpos].ljust = ljust; fmtpar[fmtpos].len = len; ! fmtpar[fmtpos].zpad = zpad; fmtpar[fmtpos].precision = precision; fmtpar[fmtpos].pointflag = pointflag; fmtpar[fmtpos].func = FMTFLOAT; *************** *** 504,523 **** { case FMTSTR: fmtstr(fmtparptr[i]->value, fmtparptr[i]->ljust, ! fmtparptr[i]->len, fmtparptr[i]->zpad, ! fmtparptr[i]->maxwidth, end, &output); break; case FMTNUM: case FMTNUM_U: fmtnum(fmtparptr[i]->numvalue, fmtparptr[i]->base, ! fmtparptr[i]->dosign, fmtparptr[i]->ljust, ! fmtparptr[i]->len, fmtparptr[i]->zpad, end, &output); break; case FMTFLOAT: fmtfloat(fmtparptr[i]->fvalue, fmtparptr[i]->type, ! fmtparptr[i]->ljust, fmtparptr[i]->len, ! fmtparptr[i]->precision, fmtparptr[i]->pointflag, ! end, &output); break; case FMTCHAR: dopr_outch(fmtparptr[i]->charvalue, end, &output); --- 531,552 ---- { case FMTSTR: fmtstr(fmtparptr[i]->value, fmtparptr[i]->ljust, ! fmtparptr[i]->len, fmtparptr[i]->maxwidth, ! end, &output); break; case FMTNUM: case FMTNUM_U: fmtnum(fmtparptr[i]->numvalue, fmtparptr[i]->base, ! fmtparptr[i]->dosign, fmtparptr[i]->forcesign, ! fmtparptr[i]->ljust, fmtparptr[i]->len, ! fmtparptr[i]->zpad, end, &output); break; case FMTFLOAT: fmtfloat(fmtparptr[i]->fvalue, fmtparptr[i]->type, ! fmtparptr[i]->forcesign, fmtparptr[i]->ljust, ! fmtparptr[i]->len, fmtparptr[i]->zpad, ! fmtparptr[i]->precision, fmtparptr[i]->pointflag, ! end, &output); break; case FMTCHAR: dopr_outch(fmtparptr[i]->charvalue, end, &output); *************** *** 545,562 **** } static void ! fmtstr(char *value, int ljust, int len, int zpad, int maxwidth, char *end, char **output) { int padlen, ! strlen; /* amount to pad */ ! if (value == 0) value = "<NULL>"; ! for (strlen = 0; value[strlen]; ++strlen); /* strlen */ ! if (strlen > maxwidth && maxwidth) ! strlen = maxwidth; ! padlen = len - strlen; if (padlen < 0) padlen = 0; if (ljust) --- 574,597 ---- } static void ! fmtstr(char *value, int ljust, int len, int maxwidth, char *end, char **output) { int padlen, ! vallen; /* amount to pad */ ! if (value == NULL) value = "<NULL>"; ! vallen = strlen(value); ! if (vallen > maxwidth && maxwidth) ! vallen = maxwidth; ! if (len < 0) ! { ! /* this could happen with a "*" width spec */ ! ljust = 1; ! len = -len; ! } ! padlen = len - vallen; if (padlen < 0) padlen = 0; if (ljust) *************** *** 575,582 **** } static void ! fmtnum(int64 value, int base, int dosign, int ljust, int len, int zpad, ! char *end, char **output) { int signvalue = 0; uint64 uvalue; --- 610,617 ---- } static void ! fmtnum(int64 value, int base, int dosign, int forcesign, int ljust, ! int len, int zpad, char *end, char **output) { int signvalue = 0; uint64 uvalue; *************** *** 597,602 **** --- 632,639 ---- signvalue = '-'; uvalue = -value; } + else if (forcesign) + signvalue = '+'; } if (base < 0) { *************** *** 658,676 **** } static void ! fmtfloat(double value, char type, int ljust, int len, int precision, ! int pointflag, char *end, char **output) { char fmt[32]; char convert[512]; int padlen = 0; /* amount to pad */ /* we rely on regular C library's sprintf to do the basic conversion */ if (pointflag) sprintf(fmt, "%%.%d%c", precision, type); else sprintf(fmt, "%%%c", type); ! sprintf(convert, fmt, value); if (len < 0) { --- 695,726 ---- } static void ! fmtfloat(double value, char type, int forcesign, int ljust, ! int len, int zpad, int precision, int pointflag, char *end, ! char **output) { + int signvalue = 0; + double uvalue; char fmt[32]; char convert[512]; int padlen = 0; /* amount to pad */ + uvalue = value; /* we rely on regular C library's sprintf to do the basic conversion */ if (pointflag) sprintf(fmt, "%%.%d%c", precision, type); else sprintf(fmt, "%%%c", type); ! ! if (value < 0) ! { ! signvalue = '-'; ! uvalue = -value; ! } ! else if (forcesign) ! signvalue = '+'; ! ! sprintf(convert, fmt, uvalue); if (len < 0) { *************** *** 684,694 **** --- 734,760 ---- if (ljust) padlen = -padlen; + if (zpad && padlen > 0) + { + if (signvalue) + { + dopr_outch(signvalue, end, output); + --padlen; + signvalue = 0; + } + while (padlen > 0) + { + dopr_outch(zpad, end, output); + --padlen; + } + } while (padlen > 0) { dopr_outch(' ', end, output); --padlen; } + if (signvalue) + dopr_outch(signvalue, end, output); dostr(convert, 0, end, output); while (padlen < 0) { *************** *** 701,715 **** dostr(char *str, int cut, char *end, char **output) { if (cut) - { while (*str && cut-- > 0) dopr_outch(*str++, end, output); - } else - { while (*str) dopr_outch(*str++, end, output); - } } static void --- 767,777 ----
Thanks to Andrew Dunstan, I found the cause of these link errors.
Andrew found this in libintl:
#undef snprintf
#define snprintf libintl_snprintf
extern int snprintf (char *, size_t, const char *, ...);
What is happening is that we do:
#define snprintf pg_snprintf
and then libintl.h (?) does:
#define snprintf libintl_snprintf
so the effect is:
#define pg_snprintf libintl_snprintf
In fact, in this example, the system complains about a missing X3 symbol:
#define X1 X2
#define X2 X3
int
main(int argc, char *argv[])
{
X1;
}
so the effet of the defines is:
#define X1 X3
Anyway, the reason ecpg is failing is that it is the only client-side
program that doesn't use libintl for internationalization. It is on our
TODO list to do that, but it hasn't been done yet.
However, only Win32 is seeing this failure, and only when configure
--enable-nls. I think this is because only Win32 does the redefine of
snprint and friends.
Comments?
---------------------------------------------------------------------------
Nicolai Tufar wrote:
> On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
> <pgman@candle.pha.pa.us> wrote:
> >
> > I have applied a modified version of your patch, attached.
>
> I am so sorry, I sent untested patch again. Thank you very
> much for patience in fixing it. The patch looks perfectly
> fine and works under Solaris.
>
> Under win32 I am still struggling with build environment.
> In many directories link fails with "undefined reference to
> `pg_snprintf'" in other it fails with "undefined reference to
> `_imp__libintl_sprintf'". In yet another directory it fails with
> both, like in src/interfaces/ecpg/pgtypeslib:
>
> dlltool --export-all --output-def pgtypes.def numeric.o datetime.o
> common.o dt_common.o timestamp.o interval.o pgstrcasecmp.o
> dllwrap -o libpgtypes.dll --dllname libpgtypes.dll --def pgtypes.def
> numeric.o datetime.o common.o dt_common.o timestamp.o interval.o
> pgstrcasecmp.o -L../../../../src/port -lm
> numeric.o(.text+0x19ea):numeric.c: undefined reference to
> `_imp__libintl_sprintf'
> datetime.o(.text+0x476):datetime.c: undefined reference to `pg_snprintf'
> common.o(.text+0x1cd):common.c: undefined reference to `pg_snprintf'
> common.o(.text+0x251):common.c: undefined reference to `pg_snprintf'
> dt_common.o(.text+0x538):dt_common.c: undefined reference to
> `_imp__libintl_sprintf'
> dt_common.o(.text+0x553):dt_common.c: undefined reference to
> `_imp__libintl_sprintf'
> dt_common.o(.text+0x597):dt_common.c: undefined reference to
> `_imp__libintl_sprintf'
> dt_common.o(.text+0x5d5):dt_common.c: undefined reference to
> `_imp__libintl_sprintf'
> dt_common.o(.text+0x628):dt_common.c: undefined reference to
> `_imp__libintl_sprintf'
> dt_common.o(.text+0x7e8):dt_common.c: more undefined references to
> `_imp__libintl_sprintf' follow
> c:\MinGW\bin\dllwrap.exe: c:\MinGW\bin\gcc exited with status 1
> make: *** [libpgtypes.a] Error 1
>
> Could someone with a better grasp of configure and
> win32 environment check it? Aparently no one regularily
> compiles source code under win32 during development cycle
> these days.
>
>
> Best regards,
> Nicolai
>
> ---------------------------(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
Bruce Momjian <pgman@candle.pha.pa.us> writes: > so the effect is: > > #define pg_snprintf libintl_snprintf That's not how CPP works. > In fact, in this example, the system complains about a missing X3 symbol: > > #define X1 X2 > #define X2 X3 In this case any occurrence of X1 replaced by X2 but then the result is rescanned for macros and X2 is turned into X3. -- greg
After some further digging, I think we have 3 problems.
1. On Windows gettext wants to hijack printf and friends, as below. This
strikes me as rather unfriendly behaviour by a library header file.
Anyway, mercifully libintl.h is included in our source in exactly one
spot, so I think the thing to do for this problem is a) undo that
hijacking and b) make sure any hijacking we want to do occurs after the
point where that file in included (in c.h). This causes most of the
noise, but is probably harmless, since our hijacking does in fact win
out. We need to fix the arnings, though.
2. We have multiple #defines for snprintf and vsnprintf (in port.h and
win32.h).
3. ecpg wants to use our pg*printf routines (because USE_SNPRINTF is
defined) but doesn't know where to find them.
what a mess :-(
cheers
andrew
Bruce Momjian wrote:
>Thanks to Andrew Dunstan, I found the cause of these link errors.
>Andrew found this in libintl:
>
> #undef snprintf
> #define snprintf libintl_snprintf
> extern int snprintf (char *, size_t, const char *, ...);
>
>What is happening is that we do:
>
> #define snprintf pg_snprintf
>
>and then libintl.h (?) does:
>
> #define snprintf libintl_snprintf
>
>so the effect is:
>
> #define pg_snprintf libintl_snprintf
>
>In fact, in this example, the system complains about a missing X3 symbol:
>
> #define X1 X2
> #define X2 X3
>
> int
> main(int argc, char *argv[])
> {
> X1;
> }
>
>so the effet of the defines is:
>
> #define X1 X3
>
>Anyway, the reason ecpg is failing is that it is the only client-side
>program that doesn't use libintl for internationalization. It is on our
>TODO list to do that, but it hasn't been done yet.
>
>However, only Win32 is seeing this failure, and only when configure
>--enable-nls. I think this is because only Win32 does the redefine of
>snprint and friends.
>
>Comments?
>
>---------------------------------------------------------------------------
>
>Nicolai Tufar wrote:
>
>
>>On Wed, 16 Mar 2005 01:00:21 -0500 (EST), Bruce Momjian
>><pgman@candle.pha.pa.us> wrote:
>>
>>
>>>I have applied a modified version of your patch, attached.
>>>
>>>
>>I am so sorry, I sent untested patch again. Thank you very
>>much for patience in fixing it. The patch looks perfectly
>>fine and works under Solaris.
>>
>>Under win32 I am still struggling with build environment.
>>In many directories link fails with "undefined reference to
>>`pg_snprintf'" in other it fails with "undefined reference to
>>`_imp__libintl_sprintf'". In yet another directory it fails with
>>both, like in src/interfaces/ecpg/pgtypeslib:
>>
>>dlltool --export-all --output-def pgtypes.def numeric.o datetime.o
>>common.o dt_common.o timestamp.o interval.o pgstrcasecmp.o
>>dllwrap -o libpgtypes.dll --dllname libpgtypes.dll --def pgtypes.def
>>numeric.o datetime.o common.o dt_common.o timestamp.o interval.o
>>pgstrcasecmp.o -L../../../../src/port -lm
>>numeric.o(.text+0x19ea):numeric.c: undefined reference to
>>`_imp__libintl_sprintf'
>>datetime.o(.text+0x476):datetime.c: undefined reference to `pg_snprintf'
>>common.o(.text+0x1cd):common.c: undefined reference to `pg_snprintf'
>>common.o(.text+0x251):common.c: undefined reference to `pg_snprintf'
>>dt_common.o(.text+0x538):dt_common.c: undefined reference to
>>`_imp__libintl_sprintf'
>>dt_common.o(.text+0x553):dt_common.c: undefined reference to
>>`_imp__libintl_sprintf'
>>dt_common.o(.text+0x597):dt_common.c: undefined reference to
>>`_imp__libintl_sprintf'
>>dt_common.o(.text+0x5d5):dt_common.c: undefined reference to
>>`_imp__libintl_sprintf'
>>dt_common.o(.text+0x628):dt_common.c: undefined reference to
>>`_imp__libintl_sprintf'
>>dt_common.o(.text+0x7e8):dt_common.c: more undefined references to
>>`_imp__libintl_sprintf' follow
>>c:\MinGW\bin\dllwrap.exe: c:\MinGW\bin\gcc exited with status 1
>>make: *** [libpgtypes.a] Error 1
>>
>>Could someone with a better grasp of configure and
>>win32 environment check it? Aparently no one regularily
>>compiles source code under win32 during development cycle
>>these days.
>>
>>
>>Best regards,
>>Nicolai
>>
>>---------------------------(end of broadcast)---------------------------
>>TIP 4: Don't 'kill -9' the postmaster
>>
>>
>>
>
>
>
Andrew Dunstan wrote: > > After some further digging, I think we have 3 problems. > > 1. On Windows gettext wants to hijack printf and friends, as below. This > strikes me as rather unfriendly behaviour by a library header file. > Anyway, mercifully libintl.h is included in our source in exactly one > spot, so I think the thing to do for this problem is a) undo that > hijacking and b) make sure any hijacking we want to do occurs after the > point where that file in included (in c.h). This causes most of the > noise, but is probably harmless, since our hijacking does in fact win > out. We need to fix the arnings, though. > > 2. We have multiple #defines for snprintf and vsnprintf (in port.h and > win32.h). > > 3. ecpg wants to use our pg*printf routines (because USE_SNPRINTF is > defined) but doesn't know where to find them. > > what a mess :-( Based on the "mess" analysis, I decided it is better to allow libintl to use its own snprintf() for Win32 when NLS is enabled, rather than try to override that with our own snprintf. I have added code to configure.in so that we don't check for arg control in native snprintf on Win32 because when we are using NLS, we are going to get their snprintf anyway and not the native one. Patch attached and applied. -- 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: configure =================================================================== RCS file: /cvsroot/pgsql/configure,v retrieving revision 1.435 diff -c -c -r1.435 configure *** configure 5 May 2005 11:50:17 -0000 1.435 --- configure 5 May 2005 19:13:35 -0000 *************** *** 14706,14712 **** # Force use of our snprintf if system's doesn't do arg control # This feature is used by NLS ! if test "$enable_nls" = yes && test $pgac_need_repl_snprintf = no; then echo "$as_me:$LINENO: checking whether printf supports argument control" >&5 echo $ECHO_N "checking whether printf supports argument control... $ECHO_C" >&6 if test "${pgac_cv_printf_arg_control+set}" = set; then --- 14706,14718 ---- # Force use of our snprintf if system's doesn't do arg control # This feature is used by NLS ! if test "$enable_nls" = yes && ! test $pgac_need_repl_snprintf = no && ! # On Win32, libintl replaces snprintf() with its own version that ! # understands arg control, so we don't need our own. In fact, it ! # also uses macros that conflict with ours, so we _can't_ use ! # our own. ! test "$PORTNAME" != "win32"; then echo "$as_me:$LINENO: checking whether printf supports argument control" >&5 echo $ECHO_N "checking whether printf supports argument control... $ECHO_C" >&6 if test "${pgac_cv_printf_arg_control+set}" = set; then Index: configure.in =================================================================== RCS file: /cvsroot/pgsql/configure.in,v retrieving revision 1.408 diff -c -c -r1.408 configure.in *** configure.in 5 May 2005 11:50:18 -0000 1.408 --- configure.in 5 May 2005 19:13:36 -0000 *************** *** 1095,1101 **** # Force use of our snprintf if system's doesn't do arg control # This feature is used by NLS ! if test "$enable_nls" = yes && test $pgac_need_repl_snprintf = no; then PGAC_FUNC_PRINTF_ARG_CONTROL if test $pgac_cv_printf_arg_control != yes ; then pgac_need_repl_snprintf=yes --- 1095,1107 ---- # Force use of our snprintf if system's doesn't do arg control # This feature is used by NLS ! if test "$enable_nls" = yes && ! test $pgac_need_repl_snprintf = no && ! # On Win32, libintl replaces snprintf() with its own version that ! # understands arg control, so we don't need our own. In fact, it ! # also uses macros that conflict with ours, so we _can't_ use ! # our own. ! test "$PORTNAME" != "win32"; then PGAC_FUNC_PRINTF_ARG_CONTROL if test $pgac_cv_printf_arg_control != yes ; then pgac_need_repl_snprintf=yes