Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail
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 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests
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 - 1.20 --- src/port/snprintf.c 16 Mar 2005 14:59:00 - *** *** 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; } ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression
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 - 1.141
--- src/bin/psql/command.c 16 Mar 2005 21:17:50 -
***
*** 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 - 1.72
--- src/include/port.h 16 Mar 2005 21:17:50 -
***
*** 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 printfpg_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 - 1.22
--- src/port/snprintf.c 16 Mar 2005 21:17:51 -
***
*** 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 */
#undefvsnprintf
#undefsnprintf
#undeffprintf
#undefprintf
--- 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 */
#undefvsnprintf
#undefsnprintf
+ #undefsprintf
#undeffprintf
#undefprintf
***
*** 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
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests
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 [EMAIL PROTECTED] 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 - 1.19
--- src/port/snprintf.c 16 Mar 2005 05:46:33 -
***
*** 151,170
#define FMTSTR 1
#define FMTNUM 2
! #define FMTFLOAT3
! #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_U3
! #define FMTFLOAT4
! #define FMTCHAR 5
! #define FMTWIDTH6
! #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;
charfunc;
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 '-':
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail
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 [EMAIL PROTECTED] 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
*** ./src/port/snprintf.c.orig Sat Mar 12 09:13:43 2005
--- ./src/port/snprintf.c Sat Mar 12 10:01:44 2005
***
*** 195,200
--- 195,202
int pointflag;
char func;
int realpos;
+ int longflag;
+ int longlongflag;
} *fmtpar, **fmtparptr;
/* Create enough structures to hold all arguments */
***
*** 264,275
--- 266,279
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;
goto nextch;
***
*** 301,316
#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;
--- 305,312
#endif
case 'u':
case 'U':
! fmtpar[fmtpos].longflag = longflag;
! fmtpar[fmtpos].longlongflag = longlongflag;
fmtpar[fmtpos].fmtbegin = fmtbegin;
fmtpar[fmtpos].fmtend = format;
fmtpar[fmtpos].numvalue = value;
***
*** 325,340
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;
--- 321,328
break;
case 'o':
case 'O':
! fmtpar[fmtpos].longflag = longflag;
! fmtpar[fmtpos].longlongflag = longlongflag;
fmtpar[fmtpos].fmtbegin = fmtbegin;
fmtpar[fmtpos].fmtend = format;
fmtpar[fmtpos].numvalue = value;
***
*** 349,365
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;
--- 337,344
break;
case 'd':
case 'D':
! fmtpar[fmtpos].longflag = longflag;
! fmtpar[fmtpos].longlongflag = longlongflag;
fmtpar[fmtpos].fmtbegin = fmtbegin;
fmtpar[fmtpos].fmtend = format;
fmtpar[fmtpos].numvalue = value;
***
*** 373,387
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;
--- 352,359
fmtpos++;
break;
case 'x':
! fmtpar[fmtpos].longflag = longflag;
! fmtpar[fmtpos].longlongflag = longlongflag;
fmtpar[fmtpos].fmtbegin = fmtbegin;
fmtpar[fmtpos].fmtend = format;
fmtpar[fmtpos].numvalue = value;
***
*** 395,409
fmtpos++;
break;
case 'X':
! if (longflag)
! {
! if (longlongflag)
! value = va_arg(args, uint64);
! else
! value = va_arg(args, unsigned long);
!
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests
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. 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. OK, here is a patch that changes snprintf() to pg_snprintf() for platforms that need our snprintf. We do the same thing with pgrename/pgunlink. It manages to preserve printf-like argument checking in gcc. I considered using a macro just in libpq but realized it was going to be too ugly and too easy to break without us detecting it. -- 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.432 diff -c -c -r1.432 configure *** configure 2 Mar 2005 15:42:35 - 1.432 --- configure 10 Mar 2005 21:05:27 - *** *** 14973,14978 --- 14973,14983 # Now we have checked all the reasons to replace snprintf if test $pgac_need_repl_snprintf = yes; then + + cat confdefs.h \_ACEOF + #define USE_SNPRINTF 1 + _ACEOF + LIBOBJS=$LIBOBJS snprintf.$ac_objext fi Index: configure.in === RCS file: /cvsroot/pgsql/configure.in,v retrieving revision 1.405 diff -c -c -r1.405 configure.in *** configure.in2 Mar 2005 15:42:35 - 1.405 --- configure.in10 Mar 2005 21:05:28 - *** *** 1143,1148 --- 1143,1149 # Now we have checked all the reasons to replace snprintf if test $pgac_need_repl_snprintf = yes; then + AC_DEFINE(USE_SNPRINTF, 1, [Use replacement snprintf() functions.]) AC_LIBOBJ(snprintf) fi Index: src/bin/pg_ctl/pg_ctl.c === RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v retrieving revision 1.54 diff -c -c -r1.54 pg_ctl.c *** src/bin/pg_ctl/pg_ctl.c 22 Feb 2005 04:39:22 - 1.54 --- src/bin/pg_ctl/pg_ctl.c 10 Mar 2005 21:05:30 - *** *** 337,355 if (log_file != NULL) #ifndef WIN32 /* Cygwin doesn't have START */ snprintf(cmd, MAXPGPATH, %s\%s\ %s%s \%s\ \%s\ 21 %s, #else snprintf(cmd, MAXPGPATH, %sSTART /B \\ \%s\ %s%s \%s\ \%s\ 21%s, - #endif SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts, DEVNULL, log_file, SYSTEMQUOTE); else #ifndef WIN32 /* Cygwin doesn't have START */ snprintf(cmd, MAXPGPATH, %s\%s\ %s%s \%s\ 21 %s, #else snprintf(cmd, MAXPGPATH, %sSTART /B \\ \%s\ %s%s \%s\ 21%s, - #endif SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts, DEVNULL, SYSTEMQUOTE); return system(cmd); } --- 337,359 if (log_file != NULL) #ifndef WIN32 /* Cygwin doesn't have START */ snprintf(cmd, MAXPGPATH, %s\%s\ %s%s \%s\ \%s\ 21 %s, +SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts, +DEVNULL, log_file, SYSTEMQUOTE); #else snprintf(cmd, MAXPGPATH, %sSTART /B \\ \%s\ %s%s \%s\ \%s\ 21%s, SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts, DEVNULL, log_file, SYSTEMQUOTE); + #endif else #ifndef WIN32 /* Cygwin doesn't have START */ snprintf(cmd, MAXPGPATH, %s\%s\ %s%s \%s\ 21 %s, +SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts, +DEVNULL, SYSTEMQUOTE); #else snprintf(cmd, MAXPGPATH, %sSTART /B \\ \%s\ %s%s \%s\ 21%s, SYSTEMQUOTE, postgres_path, pgdata_opt, post_opts, DEVNULL, SYSTEMQUOTE); + #endif return system(cmd); }
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail
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. 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? Third thought: Windows' linker seems to be broken enough that it may create problems of this ilk that exist on no other platform. If you're takling the combination of libpq and Windows, we are definitly safe for dynamic linking, which is what most ppl will use. Because the DLL will only export the entrypoitns that we explicitly define in the DEF files, and those are also the only ones that are present in the import library. //Magnus ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests
Magnus Hagander wrote: 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. 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? Third thought: Windows' linker seems to be broken enough that it may create problems of this ilk that exist on no other platform. If you're takling the combination of libpq and Windows, we are definitly safe for dynamic linking, which is what most ppl will use. Because the DLL will only export the entrypoitns that we explicitly define in the DEF files, and those are also the only ones that are present in the import library. Right. It is Unix that has the problem. It seems we are supplying a special snprintf() only so gettext() in libintl will use ours instead of the operating system's. Isn't there a way to target just that library for our replacement snprintf()? Our code itself doesn't need the positional parameters. Could we read the snprintf translation string and process positional parameters _before_ we sent it to gettext()? -- 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 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests
Am Mittwoch, 2. März 2005 16:50 schrieb Bruce Momjian: Right. It is Unix that has the problem. It seems we are supplying a special snprintf() only so gettext() in libintl will use ours instead of the operating system's. Isn't there a way to target just that library for our replacement snprintf()? Our code itself doesn't need the positional parameters. No, it's exactly our code that needs the snprintf(). libintl does not need it. Could we read the snprintf translation string and process positional parameters _before_ we sent it to gettext()? That would defeat the entire point of this exercise. Then translators would have to translate each possible substitution separately and we wouldn't need positional parameters at all. -- Peter Eisentraut http://developer.postgresql.org/~petere/ ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression
Peter Eisentraut wrote: Am Mittwoch, 2. M?rz 2005 16:50 schrieb Bruce Momjian: Right. It is Unix that has the problem. It seems we are supplying a special snprintf() only so gettext() in libintl will use ours instead of the operating system's. Isn't there a way to target just that library for our replacement snprintf()? Our code itself doesn't need the positional parameters. No, it's exactly our code that needs the snprintf(). libintl does not need it. OK, I figured that out later. gettext() gets the string, but we are the ones to match the string to the args. Could we read the snprintf translation string and process positional parameters _before_ we sent it to gettext()? That would defeat the entire point of this exercise. Then translators would have to translate each possible substitution separately and we wouldn't need positional parameters at all. Right. Should we consider using a macro for snprintf/vsnprintf/printf in our code and map those to pg_* names so we don't let those symbols out of our code? The big problem is that client apps like psql need the port/snprintf functions we have. We actually have libpgport on the link line for clients so it is really only libpq exporting it that is a problem. If we could prevent that we would be fine. I think we got away with replacing system functions in the past because we were replacing either broken or missing functions. In this case, snprintf has a variety of format flags, some OS-specific, and the functionality we are adding is not critical in most 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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests
Bruce Momjian wrote:
Bruce Momjian wrote:
Tom Lane wrote:
Bruce Momjian pgman@candle.pha.pa.us writes:
Tom Lane wrote:
Does it help if you flip the order of the snprintf and vsnprintf
functions in snprintf.c?
Yes, it fixes the problem and I have applied the reordering with a
comment.
Fascinating.
I will start working on fixing the large fmtpar allocations now.
Quite honestly, this code is not worth micro-optimizing because it
is fundamentally broken. See my other comments in this thread.
I am working on something that just counts the '%' characters in the
format string and allocates an array that size.
Can we find a BSD-license implementation that follows the spec?
I would think NetBSD would be our best bet. I will download it and take
a look.
Oops, I remember now that NetBSD doesn't support it. I think FreeBSD does.
OK, I have the structure exceess patched at least with this applied
patch.
Have we considered what is going to happen to applications when they use
our snprintf instead of the one from the operating system? I don't
think it is going to always match and might cause confusion. Look at
the confusion is caused us.
Can we force only gettext() to call our special snprintf verions but
leave the application symbol space unchanged?
I just looked at my BSD/OS libpq based on CVS and see [v]snprintf
exported:
$ nm /pg/interfaces/libpq/libpq.so|grep snprintf
00052434 T snprintf
000523e0 T vsnprintf
--
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.11
diff -c -c -r1.11 snprintf.c
*** src/port/snprintf.c 2 Mar 2005 03:21:52 - 1.11
--- src/port/snprintf.c 2 Mar 2005 05:17:01 -
***
*** 32,49
* SUCH DAMAGE.
*/
! /* might be in either frontend or backend */
#include postgres_fe.h
#ifndef WIN32
#include sys/ioctl.h
#endif
#include sys/param.h
- #ifndef NL_ARGMAX
- #define NL_ARGMAX 4096
- #endif
-
/*
**SNPRINTF, VSNPRINT -- counted versions of printf
**
--- 32,48
* SUCH DAMAGE.
*/
! #ifndef FRONTEND
! #include postgres.h
! #else
#include postgres_fe.h
+ #endif
#ifndef WIN32
#include sys/ioctl.h
#endif
#include sys/param.h
/*
**SNPRINTF, VSNPRINT -- counted versions of printf
**
***
*** 157,167
int realpos = 0;
int position;
char*output;
! /* In thread mode this structure is too large. */
! #ifndef ENABLE_THREAD_SAFETY
! static
! #endif
! struct{
const char* fmtbegin;
const char* fmtend;
void* value;
--- 156,164
int realpos = 0;
int position;
char*output;
! int percents = 1;
! const char *p;
! struct fmtpar {
const char* fmtbegin;
const char* fmtend;
void* value;
***
*** 179,188
int pointflag;
charfunc;
int realpos;
! } fmtpar[NL_ARGMAX+1], *fmtparptr[NL_ARGMAX+1];
!
format_save = format;
output = buffer;
while ((ch = *format++))
{
--- 176,205
int pointflag;
charfunc;
int realpos;
! } *fmtpar, **fmtparptr;
+ /* Create enough structures to hold all arguments */
+ for (p = format; *p != '\0'; p++)
+ if (*p == '%') /* counts %% as two, so overcounts */
+ percents++;
+ #ifndef FRONTEND
+ fmtpar = pgport_palloc(sizeof(struct fmtpar) * percents);
+ fmtparptr = pgport_palloc(sizeof(struct fmtpar *) * percents);
+ #else
+ if ((fmtpar = malloc(sizeof(struct fmtpar) * percents)) == NULL)
+ {
+ fprintf(stderr, _(out of memory\n));
+ exit(1);
+ }
+ if ((fmtparptr = malloc(sizeof(struct fmtpar *) * percents)) == NULL)
+ {
+ fprintf(stderr, _(out of memory\n));
+ exit(1);
+ }
+ #endif
+
format_save = format;
+
output = buffer;
while ((ch = *format++))
{
***
*** 418,426
performpr:
/* shuffle pointers */
for(i = 1; i fmtpos; i++)
- {
fmtparptr[i] = fmtpar[fmtpar[i].realpos];
- }
output = buffer;
format = format_save;
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail
Tom lane wrote: With CVS-tip snprintf I get result = '3 42' result = '3 3505' I get similar results: result = '3 42' result = '9e-313 1413754129' Now I agree with you, it is fundamentally broken. We need to replace this implementation. Bruce Momjian wrote: I can confirm that using %I64d for the printf format allows the regression tests to pass for int8. But snprintf.c code does not support %I64d construct. It must be picking OS's vsnprintf() Bruce Momjian wrote: I think FreeBSD does. I started with FreeBSD's vsnprintf() at first but was set back by it's complexity and decided to modify the port/snprintf.c code. Now would you like me to incorporate FreeBSD's one into the code. Give me a week and I will come with the patch. Best regards, Nicolai Tufar ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] [pgsql-hackers-win32] [HACKERS] snprintf causes regression tests to fail
Nicolai Tufar [EMAIL PROTECTED] writes: I started with FreeBSD's vsnprintf() at first but was set back by it's complexity and decided to modify the port/snprintf.c code. Now would you like me to incorporate FreeBSD's one into the code. Give me a week and I will come with the patch. It's all yours ... regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
