Buggy fputs(f,s) vs. fputs(s,f) on Win32 (maybe not only)

[p5pRT]

Migrated from rt.perl.org#72704 (status was 'resolved')

Searchable as RT72704$

[p5pRT]

From @kmx

Hi,

I have hit into a strange "maybe-bug" on Win32 platform.

The problem occurs when a XS modules tries to call function fputs() from
XS/C code.

The standard UNIX declaration of fputs looks AFAIK like this​:
int fputs(const char *s, FILE *stream);

Thus one would expect that also in XS/C code you just use
"fputs(string,filehandle);" However if you do this on Win32 perl you
will see an warning like this (my example comes from Math​::Pari where I
revealed the problem).

Pari.xs​: In function 'XS_Math__Pari_dumpStack'​:
Pari.xs​:3798​: warning​: passing argument 2 of '(*Perl_IStdIO_ptr((struct
PerlInterpreter *)Perl_get_context()))->pPuts' from incompatible pointer
type
Pari.xs​:3798​: note​: expected 'struct FILE *' but argument is of type
'char *'
Pari.xs​:3798​: warning​: passing argument 3 of '(*Perl_IStdIO_ptr((struct
PerlInterpreter *)Perl_get_context()))->pPuts' from incompatible pointer
type
Pari.xs​:3798​: note​: expected 'const char *' but argument is of type
'struct FILE *'

Swapping the arguments fixed the warning, so it seems to me than in
Win32/perl XS code it is expected to use​:
fputs(filehandle,string);

whereas on Linux/perl box it is expected to use​:
fputs(string,filehandle);

I have tracked this issue to iperlsys.h where I lost in a bunch of
macros and typedefs.

Note​: 5.11.4 used for testing is based on release tarball, gcc compiler,
all options left in default.

Thanks for any help.

--
kmx

Perl Info

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.11.4:

Configured by kmx at Wed Jan 27 16:25:21 2010.

Summary of my perl5 (revision 5 version 11 subversion 4) configuration:
   
  Platform:
    osname=MSWin32, osvers=4.0, archname=MSWin32-x86-multi-thread
    uname=''
    config_args='undef'
    hint=recommended, useposix=true, d_sigaction=undef
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags =' -s -O2 -DWIN32 -DHAVE_DES_FCRYPT 
-DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -fno-strict-aliasing
-mms-bitfields -DPERL_MSVCRT_READFIX',
    optimize='-s -O2',
    cppflags='-DWIN32'
    ccversion='', gccversion='4.4.3', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='long
long', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='g++', ldflags ='-s -L"c:\sb\perl\lib\CORE" -L"c:\sb\c\lib"'
    libpth=c:\sb\c\lib c:\sb\c\i686-w64-mingw32\lib
    libs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32
-ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr
-lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    perllibs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool
-lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid
-lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    libc=, so=dll, useshrplib=true, libperl=libperl511.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-mdll -s -L"c:\sb\perl\lib\CORE"
-L"c:\sb\c\lib"'

Locally applied patches:
    


@INC for perl 5.11.4:
    C:/sb/perl/site/lib
    C:/sb/perl/lib
    .


Environment for perl 5.11.4:
    HOME=D:\profile
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
   
PATH=C:\sb\perl\bin;C:\sb\c\bin;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\System32\Wbem
    PERL_BADLANG (unset)
    PERL_MM_USE_DEFAULT=1
    SHELL (unset)

[p5pRT]

From @kmx

Dne st 10.02.2010 12​:44​:10, kmxx napsal(a)​:

Hi,

I have hit into a strange "maybe-bug" on Win32 platform.

The problem occurs when a XS modules tries to call function fputs()
from XS/C code.

The standard UNIX declaration of fputs looks AFAIK like this​:
int fputs(const char *s, FILE *stream);

Thus one would expect that also in XS/C code you just use
"fputs(string,filehandle);" However if you do this on Win32 perl you
will see an warning like this (my example comes from Math​::Pari where
I revealed the problem).

Pari.xs​: In function 'XS_Math__Pari_dumpStack'​:
Pari.xs​:3798​: warning​: passing argument 2 of '(*Perl_IStdIO_ptr
((struct PerlInterpreter *)Perl_get_context()))->pPuts' from
incompatible pointer type
Pari.xs​:3798​: note​: expected 'struct FILE *' but argument is of type
'char *'
Pari.xs​:3798​: warning​: passing argument 3 of
'(*Perl_IStdIO_ptr((struct PerlInterpreter *)Perl_get_context()))
->pPuts' from incompatible pointer type
Pari.xs​:3798​: note​: expected 'const char *' but argument is of type
'struct FILE *'

Swapping the arguments fixed the warning, so it seems to me than in
Win32/perl XS code it is expected to use​:
fputs(filehandle,string);

whereas on Linux/perl box it is expected to use​:
fputs(string,filehandle);

I have tracked this issue to iperlsys.h where I lost in a bunch of
macros and typedefs.

Note​: 5.11.4 used for testing is based on release tarball, gcc
compiler, all options left in default.

Thanks for any help.

Can somebody please comment on this issue. I know that is not new in
perl 5.11.x (perhaps not even in 5.10.x) but from my point of view it
seems to be a clear bug.

Thanks in advance for any suggestions.

--
kmx

[p5pRT]

@kmx - Status changed from 'new' to 'open'

[p5pRT]

From @steve-m-hay

kmx via RT wrote on 2010-03-03​:

Dne st 10.02.2010 12​:44​:10, kmxx napsal(a)​:

Hi,

I have hit into a strange "maybe-bug" on Win32 platform.

The problem occurs when a XS modules tries to call function fputs()
from XS/C code.

The standard UNIX declaration of fputs looks AFAIK like this​:
int fputs(const char *s, FILE *stream);

Thus one would expect that also in XS/C code you just use
"fputs(string,filehandle);" However if you do this on Win32 perl you
will see an warning like this (my example comes from Math​::Pari where
I revealed the problem).

Pari.xs​: In function 'XS_Math__Pari_dumpStack'​:
Pari.xs​:3798​: warning​: passing argument 2 of '(*Perl_IStdIO_ptr
((struct PerlInterpreter *)Perl_get_context()))->pPuts' from
incompatible pointer type
Pari.xs​:3798​: note​: expected 'struct FILE *' but argument is of type
'char *'
Pari.xs​:3798​: warning​: passing argument 3 of
'(*Perl_IStdIO_ptr((struct PerlInterpreter *)Perl_get_context()))
->pPuts' from incompatible pointer type
Pari.xs​:3798​: note​: expected 'const char *' but argument is of type
'struct FILE *'

Swapping the arguments fixed the warning, so it seems to me than in
Win32/perl XS code it is expected to use​:
fputs(filehandle,string);

whereas on Linux/perl box it is expected to use​:
fputs(string,filehandle);

I have tracked this issue to iperlsys.h where I lost in a bunch of
macros and typedefs.

Note​: 5.11.4 used for testing is based on release tarball, gcc
compiler, all options left in default.

Thanks for any help.

Can somebody please comment on this issue. I know that is not new in
perl 5.11.x (perhaps not even in 5.10.x) but from my point of view it
seems to be a clear bug.

Thanks in advance for any suggestions.

Sorry, I meant to reply to this earlier but completely forgot.

I think it is deliberate​: it is explicitly mentioned in perlapio.pod that the arguments have been reversed compared to the C library function (probably so that all the PerlIO functions have the 'f' first). And there's a further comment about it in perlxstut.pod too.

Are you sure the arguments are not reversed on Linux too? There's no mention of the reversal being Win32-specific.

[p5pRT]

From @iabyn

On Thu, Mar 04, 2010 at 04​:40​:41PM -0000, Steve Hay wrote​:

Sorry, I meant to reply to this earlier but completely forgot.

I think it is deliberate​: it is explicitly mentioned in perlapio.pod that the arguments have been reversed compared to the C library function (probably so that all the PerlIO functions have the 'f' first). And there's a further comment about it in perlxstut.pod too.

Are you sure the arguments are not reversed on Linux too? There's no mention of the reversal being Win32-specific.

While the args of the PerlIO functions are explicity reversed, I think
that OP is right, in that something is probably awry with the macro
expansions.

For example in fakesdio.h, you have

  #define fputs(s,f) PerlIO_puts(f,s)

which does the correct arg reversal.

*However*​: out in XS/win32 land, you appear to have​:

XSUB.h​: #define fputs PerlSIO_fputs

iperlsys.h​:

  #if defined(PERL_IMPLICIT_SYS)
  #define PerlSIO_fputs(f,s) (*PL_StdIO->pPuts)(PL_StdIO, (f),(s))
  #else
  #define PerlSIO_fputs(f,s) fputs(s,f)
  #endif

and the type of function expected in the pPuts slot is LPPuts, which is
defined as

  typedef int (*LPPuts)(struct IPerlStdIO*, FILE*, const char*);

So under some circumstances,

  fputs(str,handle)
=> PerlIO_puts(str,handle)
=> (*PL_StdIO->pPuts)(PL_StdIO, (str),(handle))

which doesn't match the LPPuts definition.

But that's a horrible twisty maze of macros that I don't really understand.

--
Any [programming] language that doesn't occasionally surprise the
novice will pay for it by continually surprising the expert.
  -- Larry Wall

[p5pRT]

From @steve-m-hay

On 4 March 2010 20​:17, Dave Mitchell <davem@​iabyn.com> wrote​:

On Thu, Mar 04, 2010 at 04​:40​:41PM -0000, Steve Hay wrote​:

Sorry, I meant to reply to this earlier but completely forgot.

I think it is deliberate​: it is explicitly mentioned in perlapio.pod that the arguments have been reversed compared to the C library function (probably so that all the PerlIO functions have the 'f' first). And there's a further comment about it in perlxstut.pod too.

Are you sure the arguments are not reversed on Linux too? There's no mention of the reversal being Win32-specific.

While the args of the PerlIO functions are explicity reversed, I think
that OP is right, in that something is probably awry with the macro
expansions.

For example in fakesdio.h, you have

   #define fputs(s,f) PerlIO_puts(f,s)

which does the correct arg reversal.

*However*​: out in XS/win32 land, you appear to have​:

XSUB.h​: #define fputs PerlSIO_fputs

iperlsys.h​:

   #if defined(PERL_IMPLICIT_SYS)
       #define PerlSIO_fputs(f,s) (*PL_StdIO->pPuts)(PL_StdIO, (f),(s))
   #else
       #define PerlSIO_fputs(f,s)              fputs(s,f)
   #endif

and the type of function expected in the pPuts slot is LPPuts, which is
defined as

   typedef int         (*LPPuts)(struct IPerlStdIO*, FILE*, const char*);

So under some circumstances,

   fputs(str,handle)
=>  PerlIO_puts(str,handle)
=>  (*PL_StdIO->pPuts)(PL_StdIO, (str),(handle))

which doesn't match the LPPuts definition.

I'm not claiming to fully understand this macro maze myself, but I
also don't follow how you arrived at your three lines above.

The way I see it (which could be entirely wrong...) is this​:

If you're using PerlIO and you want to access an original CRT IO
function then you should use one of the PerlLIO_* or PerlSIO_* names
which are specifically designed for this purpose.

iperlsys.h defines PerlSIO_fputs(f, s) as

(*PL_StdIO->pPuts)(PL_StdIO, (f),(s))

when using PERL_IMPLICIT_SYS (with the pPuts slot typedef'ed as you
show above​: with the FILE* before the const char*), or

fputs(s, f)

otherwise. The latter is the original CRT function, with its args put
back to their original order. The former goes through the host layer
code which allows embedders to redefine functions where necessary. The
default implementation on Win32 is win32/perlhost.h, in which the
pPuts slot is implemented as

int
PerlStdIOPuts(struct IPerlStdIO* piPerl, FILE* pf, const char *s)
{
  return win32_fputs(s, pf);
}

(note that the args get reversed in there!) and in win32/win32.c we have

DllExport int
win32_fputs(const char *s,FILE *pf)
{
  return fputs(s, pf);
}

i.e. the default implementation simply takes us back to the original
CRT function again.

There's also something in win32/win32iop.h which I'm not entirely
clear about​: it additionally redirects fputs to win32_fputs if
WIN32IO_IS_STDIO is not defined, which is only the case when
PERL_IMPLICIT_SYS is not defined (see win32/win32.h). I think maybe
that is so that XS authors can lazily write fputs() instead of the
more correct PerlSIO_fputs(), but it only works if you don't have
PERL_IMPLICIT_SYS defined. (If this is right, I'm not sure why the
shortcut isn't provided for PERL_IMPLICIT_SYS. Possibly it wouldn't
make sense somehow, but we'd need someone who actually understands the
host layer stuff to explain. Jan?)

The OP is using Strawberry Perl, which does build with
PERL_IMPLICIT_SYS defined, and hence doesn't get that lazy shortcut.
So the problem could simply be that one *needs* to use the PerlLIO /
PerlSIO names under PERL_IMPLICIT_SYS and perhaps the OP didn't do so
(no doubt for the reason following below...)?

The other big problem here is that none of the PerlLIO / PerlSIO
functions or their use appears to be documented anywhere. I learned of
them from the (sorely missed) Nick I-S when writing
Win32-SharedFileOpen (which proved more useful as a learning exercise
than as anything else). The Changes file notes that exactly such a
problem was fixed in 3.10, and SharedFileOpen.xs contains a large
comment about it all near the top.

I guess the problem doesn't bite often because only Win32 makes use of
PERL_IMPLICIT_SYS (?).

[p5pRT]

From @jandubois

On Thu, 04 Mar 2010, Dave Mitchell wrote​:

*However*​: out in XS/win32 land, you appear to have​:

XSUB.h​: #define fputs PerlSIO_fputs

That one is correct. PerlSIO_* APIs are supposed to have the same prototype
as their corresponding stdio ones.

iperlsys.h​:

\#if defined\(PERL\_IMPLICIT\_SYS\)
\#define PerlSIO\_fputs\(f\,s\) \(\*PL\_StdIO\->pPuts\)\(PL\_StdIO\, \(f\)\,\(s\)\)
\#else
\#define PerlSIO\_fputs\(f\,s\)        fputs\(s\,f\)
\#endif

This however is busted. The parameters should not be swapped.

The same problem exists for the fputc() function as well. I've attached my
attempt at correcting all the mistakes I found. Core Perl still builds
fine on Windows and OS X; but I haven't tried building any extensions that
use fputs() or fputc().

In case the patch gets mangled by email and/or RT, you can read it on github​:

  http​://github.com/jandubois/perl/commit/8656122df48f238e6526299295e8f96524ac4158

or download the raw diff at​:

  http​://github.com/jandubois/perl/commit/8656122df48f238e6526299295e8f96524ac4158.diff

So please test and report back if this fixes the problem for Math​::Pari on Windows!

Cheers,
-Jan

--- a/iperlsys.h
+++ b/iperlsys.h
@​@​ -78,8 +78,8 @​@​
typedef int (*LPGetCnt)(struct IPerlStdIO*, FILE*);
typedef STDCHAR* (*LPGetPtr)(struct IPerlStdIO*, FILE*);
typedef char* (*LPGets)(struct IPerlStdIO*, FILE*, char*, int);
-typedef int (*LPPutc)(struct IPerlStdIO*, FILE*, int);
-typedef int (*LPPuts)(struct IPerlStdIO*, FILE*, const char*);
+typedef int (*LPPutc)(struct IPerlStdIO*, int, FILE*);
+typedef int (*LPPuts)(struct IPerlStdIO*, const char *, FILE*);
typedef int (*LPFlush)(struct IPerlStdIO*, FILE*);
typedef int (*LPUngetc)(struct IPerlStdIO*, int,FILE*);
typedef int (*LPFileno)(struct IPerlStdIO*, FILE*);
@​@​ -225,10 +225,10 @​@​
  (*PL_StdIO->pGetCnt)(PL_StdIO, (f))
#define PerlSIO_get_ptr(f) \
  (*PL_StdIO->pGetPtr)(PL_StdIO, (f))
-#define PerlSIO_fputc(f,c) \
- (*PL_StdIO->pPutc)(PL_StdIO, (f),(c))
-#define PerlSIO_fputs(f,s) \
- (*PL_StdIO->pPuts)(PL_StdIO, (f),(s))
+#define PerlSIO_fputc(c,f) \
+ (*PL_StdIO->pPutc)(PL_StdIO, (c),(f))
+#define PerlSIO_fputs(s,f) \
+ (*PL_StdIO->pPuts)(PL_StdIO, (s),(f))
#define PerlSIO_fflush(f) \
  (*PL_StdIO->pFlush)(PL_StdIO, (f))
#define PerlSIO_fgets(s, n, fp) \
@​@​ -311,8 +311,8 @​@​
#define PerlSIO_get_cnt(f) 0
#define PerlSIO_get_ptr(f) NULL
#endif
-#define PerlSIO_fputc(f,c) fputc(c,f)
-#define PerlSIO_fputs(f,s) fputs(s,f)
+#define PerlSIO_fputc(c,f) fputc(c,f)
+#define PerlSIO_fputs(s,f) fputs(s,f)
#define PerlSIO_fflush(f) Fflush(f)
#define PerlSIO_fgets(s, n, fp) fgets(s,n,fp)
#if defined(VMS) && defined(__DECC)
--- a/perlsdio.h
+++ b/perlsdio.h
@​@​ -34,8 +34,8 @​@​
#define PerlIO_fdopen PerlSIO_fdopen
#define PerlIO_reopen PerlSIO_freopen
#define PerlIO_close(f) PerlSIO_fclose(f)
-#define PerlIO_puts(f,s) PerlSIO_fputs(f,s)
-#define PerlIO_putc(f,c) PerlSIO_fputc(f,c)
+#define PerlIO_puts(f,s) PerlSIO_fputs(s,f)
+#define PerlIO_putc(f,c) PerlSIO_fputc(c,f)
#if defined(VMS)
# if defined(__DECC)
  /* Unusual definition of ungetc() here to accomodate fast_sv_gets()'
--- a/win32/perlhost.h
+++ b/win32/perlhost.h
@​@​ -669,13 +669,13 @​@​
}

int
-PerlStdIOPutc(struct IPerlStdIO* piPerl, FILE* pf, int c)
+PerlStdIOPutc(struct IPerlStdIO* piPerl, int c, FILE* pf)
{
  return win32_fputc(c, pf);
}

int
-PerlStdIOPuts(struct IPerlStdIO* piPerl, FILE* pf, const char *s)
+PerlStdIOPuts(struct IPerlStdIO* piPerl, const char *s, FILE* pf)
{
  return win32_fputs(s, pf);
}

[p5pRT]

From @jandubois

On Thu, 04 Mar 2010, Jan Dubois wrote​:

On Thu, 04 Mar 2010, Dave Mitchell wrote​:

*However*​: out in XS/win32 land, you appear to have​:

XSUB.h​: #define fputs PerlSIO_fputs

That one is correct. PerlSIO_* APIs are supposed to have the same prototype
as their corresponding stdio ones.

iperlsys.h​:

\#if defined\(PERL\_IMPLICIT\_SYS\)
\#define PerlSIO\_fputs\(f\,s\) \(\*PL\_StdIO\->pPuts\)\(PL\_StdIO\, \(f\)\,\(s\)\)
\#else
\#define PerlSIO\_fputs\(f\,s\)        fputs\(s\,f\)
\#endif

This however is busted. The parameters should not be swapped.

The same problem exists for the fputc() function as well. I've attached my
attempt at correcting all the mistakes I found. Core Perl still builds
fine on Windows and OS X; but I haven't tried building any extensions that
use fputs() or fputc().

Note that is would be _possible_ to make a much smaller patch than the
one I supplied by just swapping the parameters in

  #define PerlSIO_fputs(f,s) (*PL_StdIO->pPuts)(PL_StdIO, (f),(s))

(and also in the corresponding PerlSIO_fputc() #define). It would
however be rather confusing longer term, as that works only
accidentally​: perlsdio.h doesn't swap the parameter order when it
should, and iperlsys.h then compensates for that by swapping when it
shouldn't. So I would much prefer to use the comprehensive patch
provided in my previous message to reduce the amount of brain-pain when
somebody looks at these again in the future.

Cheers,
-Jan

[p5pRT]

From @kmx

Hi,

firts of all thaks for your effort you have invested into this issue.

So please test and report back if this fixes the problem for
Math​::Pari on Windows!

I will do, however my build env is slighly disorganised at the moment so
it will take approx. a week.

But I have prepared a small failing test example - see attached
TestFputs-0.01.tar.gz (just do perl Makefile.Pl + make test). If anybody
is able to test your patch earlier.

Thanks again.

--
kmx

[p5pRT]

From @kmx

TestFputs-0.01.tar.gz

[p5pRT]

From @kmx

I forgot to Cc​: p5p list

Dne čt 04.bře.2010 22​:09​:09, kmxx napsal(a)​:

Hi,

firts of all thaks for your effort you have invested into this issue.

So please test and report back if this fixes the problem for
Math​::Pari on Windows!

I will do, however my build env is slighly disorganised at the
moment so it will take approx. a week.

But I have prepared a small failing test example - see attached
TestFputs-0.01.tar.gz (just do perl Makefile.Pl + make test). If
anybody is able to test your patch earlier.

Thanks again.

--
kmx

attachment link​:
http​://rt.perl.org/rt3/Ticket/Attachment/664560/317278/TestFputs-0.01.tar.gz

[p5pRT]

From @jandubois

On Thu, 04 Mar 2010, Steve Hay wrote​:

The way I see it (which could be entirely wrong...) is this​:

If you're using PerlIO and you want to access an original CRT IO
function then you should use one of the PerlLIO_* or PerlSIO_* names
which are specifically designed for this purpose.

I believe that is wrong. At least the PerlSIO_* names are an implementation
detail and should not be used directly from XS extensions at all.

XS code should not have to care if it is being compiled using PerlIO
or not, and it should not matter if PERL_IMPLICIT_SYS is being used
either. The code should be able to use the standard CRT functions,
and the Perl macros will redefine them to do the right thing for
the actual set of Perl build options in effect.

There's also something in win32/win32iop.h which I'm not entirely
clear about​: it additionally redirects fputs to win32_fputs if
WIN32IO_IS_STDIO is not defined, which is only the case when
PERL_IMPLICIT_SYS is not defined (see win32/win32.h). I think maybe
that is so that XS authors can lazily write fputs() instead of the
more correct PerlSIO_fputs(), but it only works if you don't have
PERL_IMPLICIT_SYS defined. (If this is right, I'm not sure why the
shortcut isn't provided for PERL_IMPLICIT_SYS. Possibly it wouldn't
make sense somehow, but we'd need someone who actually understands the
host layer stuff to explain. Jan?)

XSUB.h already redefines fputs to PerlSIO_fputs for PERL_IMPLICIT_SYS.
Without PERL_IMPLICIT_SYS PerlSIO_fputs would eventually be redefined
back to fputs to call the C RTL version. The win32/win32iop.h
defines make sure it instead ends up at win32_fputs(). This doesn't
matter for fputs(), but for any function that we have "fixed" (e.g.
stat()) we want the XS code to call our implementation.

Note that this is just a theory; I have not verified that it actually
works. I never build Perl on Windows without PERL_IMPLICIT_SYS, and
I doubt many other people do.

The other big problem here is that none of the PerlLIO / PerlSIO
functions or their use appears to be documented anywhere. I learned of
them from the (sorely missed) Nick I-S when writing
Win32-SharedFileOpen (which proved more useful as a learning exercise
than as anything else). The Changes file notes that exactly such a
problem was fixed in 3.10, and SharedFileOpen.xs contains a large
comment about it all near the top.

I don't understand why you need to call PerlSIO_flcose() in that module
given that XSUB.h already redefines fclose() to PerlSIO_fclose under
PERL_IMPLICIT_SYS.

Cheers,
-Jan

[p5pRT]

From @steve-m-hay

Jan Dubois wrote on 2010-03-05​:

On Thu, 04 Mar 2010, Steve Hay wrote​:

The way I see it (which could be entirely wrong...) is this​:

If you're using PerlIO and you want to access an original CRT IO
function then you should use one of the PerlLIO_* or PerlSIO_* names
which are specifically designed for this purpose.
I believe that is wrong. At least the PerlSIO_* names are an
implementation detail and should not be used directly from XS extensions
at all.

Clearly I'm still on a learning exercise now ;-)

XS code should not have to care if it is being compiled using PerlIO
or not, and it should not matter if PERL_IMPLICIT_SYS is being used
either. The code should be able to use the standard CRT functions,
and the Perl macros will redefine them to do the right thing for
the actual set of Perl build options in effect.

Sounds good, but it didn't work at the time. It does now, though, after trying it again with a current perl...

There's also something in win32/win32iop.h which I'm not entirely clear
about​: it additionally redirects fputs to win32_fputs if
WIN32IO_IS_STDIO is not defined, which is only the case when
PERL_IMPLICIT_SYS is not defined (see win32/win32.h). I think maybe
that is so that XS authors can lazily write fputs() instead of the more
correct PerlSIO_fputs(), but it only works if you don't have
PERL_IMPLICIT_SYS defined. (If this is right, I'm not sure why the
shortcut isn't provided for PERL_IMPLICIT_SYS. Possibly it wouldn't
make sense somehow, but we'd need someone who actually understands the
host layer stuff to explain. Jan?)

XSUB.h already redefines fputs to PerlSIO_fputs for PERL_IMPLICIT_SYS.
Without PERL_IMPLICIT_SYS PerlSIO_fputs would eventually be redefined
back to fputs to call the C RTL version. The win32/win32iop.h
defines make sure it instead ends up at win32_fputs(). This doesn't
matter for fputs(), but for any function that we have "fixed" (e.g.
stat()) we want the XS code to call our implementation.

Note that this is just a theory; I have not verified that it actually
works. I never build Perl on Windows without PERL_IMPLICIT_SYS, and
I doubt many other people do.

The other big problem here is that none of the PerlLIO / PerlSIO
functions or their use appears to be documented anywhere. I learned of
them from the (sorely missed) Nick I-S when writing
Win32-SharedFileOpen (which proved more useful as a learning exercise
than as anything else). The Changes file notes that exactly such a
problem was fixed in 3.10, and SharedFileOpen.xs contains a large
comment about it all near the top.
I don't understand why you need to call PerlSIO_flcose() in that module
given that XSUB.h already redefines fclose() to PerlSIO_fclose under
PERL_IMPLICIT_SYS.

I just looked at the module again. The Changes file says fclose() was changed to PerlSIO_fclose() to allow building with perl-5.8.0 with PERL_IMPLICIT_SYS, so I tried that with the PerlSIO_ prefix deleted. It builds and passes all tests, but emits two compiler warnings​:

SharedFileOpen.xs(499) : warning C4047​: 'function' : 'struct _PerlIO ** ' differs in levels of indirection from 'struct _iobuf *'
SharedFileOpen.xs(499) : warning C4024​: 'Perl_PerlIO_close' : different types for formal and actual parameter 2

Using PerlSIO_fclose() instead fixes that.

However, trying with bleadperl I don't get the same result. The problem is that perl-5.8.0's XSUB.h didn't redefine fclose to PerlSIO_fclose​: it redefined it to PerlIO_close instead. It got fixed (by NI-S) on 26 Jan 2003, the very day after I uploaded my "fixed" CPAN module​:

http​://perl5.git.perl.org/perl.git/commit/f9415d2

So it looks like I should undo all my PerlSIO_/PerlLIO_ stuff, and add a local fix for the fclose bug in perl-5.8.0 to maintain compatibility with that version of perl. You live and learn.

[p5pRT]

From @jandubois

Dne čt 04.bře.2010 22​:09​:09, kmxx napsal(a)​:

Hi,

firts of all thaks for your effort you have invested into this
issue.

So please test and report back if this fixes the problem for
Math​::Pari on Windows!

I will do, however my build env is slighly disorganised at the
moment so it will take approx. a week.

But I have prepared a small failing test example - see attached
TestFputs-0.01.tar.gz (just do perl Makefile.Pl + make test). If
anybody is able to test your patch earlier.

Yes, the patch fixes the issue exhibited by your test example.

Given the current code freeze this fix will have to wait until after
the 5.12 release, unless someone wants to convince Jesse that this should
be a blocker bug. I could see it either way, but given that this bug
has been in Perl since at least 5.8.0 it is hard to argue that it should
be *blocking* a Perl release.

So assuming it won't make it into 5.12 I plan to add the attached simple
fix to ActivePerl, and I would suggest Strawberry does the same. It does
not sanitize the macros to be internally consistent, but it fixes the
bug in a backward compatible manner (as long as XS code doesn't call
PerlSIO_fputs() directly).

Cheers,
-Jan

Inline Patch

--- iperlsys.h~ 2010-02-09 12:34:54.000000000 -0800
+++ iperlsys.h  2010-03-05 12:24:17.674828900 -0800
@@ -226,9 +226,9 @@
 #define PerlSIO_get_ptr(f)                                             \
        (*PL_StdIO->pGetPtr)(PL_StdIO, (f))
 #define PerlSIO_fputc(f,c)                                             \
-       (*PL_StdIO->pPutc)(PL_StdIO, (f),(c))
+       (*PL_StdIO->pPutc)(PL_StdIO, (c),(f))
 #define PerlSIO_fputs(f,s)                                             \
-       (*PL_StdIO->pPuts)(PL_StdIO, (f),(s))
+       (*PL_StdIO->pPuts)(PL_StdIO, (s),(f))
 #define PerlSIO_fflush(f)                                              \
        (*PL_StdIO->pFlush)(PL_StdIO, (f))
 #define PerlSIO_fgets(s, n, fp)                                        \

[p5pRT]

From @csjewell

On Fri Mar 05 12​:51​:32 2010, jdb wrote​:

Dne čt 04.bře.2010 22​:09​:09, kmxx napsal(a)​:

Hi,

firts of all thaks for your effort you have invested into this
issue.

So please test and report back if this fixes the problem for
Math​::Pari on Windows!

I will do, however my build env is slighly disorganised at the
moment so it will take approx. a week.

But I have prepared a small failing test example - see attached
TestFputs-0.01.tar.gz (just do perl Makefile.Pl + make test). If
anybody is able to test your patch earlier.

Yes, the patch fixes the issue exhibited by your test example.

Given the current code freeze this fix will have to wait until after
the 5.12 release, unless someone wants to convince Jesse that this should
be a blocker bug. I could see it either way, but given that this bug
has been in Perl since at least 5.8.0 it is hard to argue that it should
be *blocking* a Perl release.

So assuming it won't make it into 5.12 I plan to add the attached simple
fix to ActivePerl, and I would suggest Strawberry does the same. It does
not sanitize the macros to be internally consistent, but it fixes the
bug in a backward compatible manner (as long as XS code doesn't call
PerlSIO_fputs() directly).

I have one question. Would this patch be required for 5.10.x versions of
Perl, as well, or just for 5.11.x/5.12.x?

[The reason I ask is that I plan to build a 5.10.x version of strawberry
until 5.12.1.1 is out.]

--Curtis

[p5pRT]

From @timbunce

Could this be the cause of this NYTProf failure on Windows?
https://rt.cpan.org/Ticket/Display.html?id=55049

Tim.

On Fri, Mar 05, 2010 at 12​:50​:34PM -0800, Jan Dubois wrote​:

Dne čt 04.bře.2010 22​:09​:09, kmxx napsal(a)​:

Hi,

firts of all thaks for your effort you have invested into this
issue.

So please test and report back if this fixes the problem for
Math​::Pari on Windows!

I will do, however my build env is slighly disorganised at the
moment so it will take approx. a week.

But I have prepared a small failing test example - see attached
TestFputs-0.01.tar.gz (just do perl Makefile.Pl + make test). If
anybody is able to test your patch earlier.

Yes, the patch fixes the issue exhibited by your test example.

Given the current code freeze this fix will have to wait until after
the 5.12 release, unless someone wants to convince Jesse that this should
be a blocker bug. I could see it either way, but given that this bug
has been in Perl since at least 5.8.0 it is hard to argue that it should
be *blocking* a Perl release.

So assuming it won't make it into 5.12 I plan to add the attached simple
fix to ActivePerl, and I would suggest Strawberry does the same. It does
not sanitize the macros to be internally consistent, but it fixes the
bug in a backward compatible manner (as long as XS code doesn't call
PerlSIO_fputs() directly).

Cheers,
-Jan

--- iperlsys.h~ 2010-02-09 12​:34​:54.000000000 -0800
+++ iperlsys.h 2010-03-05 12​:24​:17.674828900 -0800
@​@​ -226,9 +226,9 @​@​
#define PerlSIO_get_ptr(f) \
(*PL_StdIO->pGetPtr)(PL_StdIO, (f))
#define PerlSIO_fputc(f,c) \
- (*PL_StdIO->pPutc)(PL_StdIO, (f),(c))
+ (*PL_StdIO->pPutc)(PL_StdIO, (c),(f))
#define PerlSIO_fputs(f,s) \
- (*PL_StdIO->pPuts)(PL_StdIO, (f),(s))
+ (*PL_StdIO->pPuts)(PL_StdIO, (s),(f))
#define PerlSIO_fflush(f) \
(*PL_StdIO->pFlush)(PL_StdIO, (f))
#define PerlSIO_fgets(s, n, fp) \

[p5pRT]

From @kmx

Yes, the patch fixes the issue exhibited by your test example.

Thanks for testing, Jan.

Given the current code freeze this fix will have to wait until after
the 5.12 release, unless someone wants to convince Jesse that this should
be a blocker bug. I could see it either way, but given that this bug
has been in Perl since at least 5.8.0 it is hard to argue that it should
be *blocking* a Perl release.

So assuming it won't make it into 5.12 I plan to add the attached simple
fix to ActivePerl, and I would suggest Strawberry does the same. It does
not sanitize the macros to be internally consistent, but it fixes the
bug in a backward compatible manner (as long as XS code doesn't call
PerlSIO_fputs() directly).

From that point of view it is not a blocker as both ActivePerl +
Strawberry (and other Win32 perls) live with it for years. However it is
definitely long lasting defect that hurts mostly (if not only) Win32
perl users.

If not accepted in 5.12 please do not forget to commit this patch after
code unfreeze.

I have one question. Would this patch be required for 5.10.x versions of
Perl, as well, or just for 5.11.x/5.12.x?

Definitely yes.

--
kmx

[p5pRT]

From @kmx

May I nominate the patch fixings RT proposed by Jan to get into 5.12.1?

Thanks.

--
kmx

[p5pRT]

From @kmx

Dne čt 04.bře.2010 18​:55​:26, jdb napsal(a)​:

...

The same problem exists for the fputc() function as well. I've
attached my attempt at correcting all the mistakes I found. Core
Perl still builds fine on Windows and OS X; but I haven't tried
building any extensions that use fputs() or fputc().

In case the patch gets mangled by email and/or RT, you can read
it on github​:

http​://github.com/jandubois/perl/commit/8656122df48f

Now - after blead unfreeze - can I ask somebody (perhaps Jan, the author
of the above mentioned patch) to commit this fix to blead?

Thanks.

--
kmx

[p5pRT]

From @jandubois

On Wed Apr 14 23​:55​:56 2010, kmxx wrote​:

Now - after blead unfreeze - can I ask somebody (perhaps Jan, the author
of the above mentioned patch) to commit this fix to blead?

Fix has been committed as change 634b482.

I'll ask for supporting votes to integrate into maint-5.12 once rgs has
his new tool up and running.

Same thing for commit 20c8f8f which is similar (fgets() doesn't have
swapped parameters, it is missing a redefinition macro completely).

[p5pRT]

From @kmx

Dne st 21.dub.2010 23​:55​:55, jdb napsal(a)​:

...

Fix has been committed as change 634b482.

I'll ask for supporting votes to integrate into maint-5.12 once rgs has
his new tool up and running.

Same thing for commit 20c8f8f which is similar (fgets() doesn't have
swapped parameters, it is missing a redefinition macro completely).

If my vote counts I would like to support including 634b482 + 20c8f8f
to 5.12.1

--
kmx

[p5pRT]

From @kmx

I see comming deadline for 5.12.1.

Could somebody competent consider including 634b482 + 20c8f8f to 5.12.1

Thanks

--
kmx

[p5pRT]

From @jandubois

On Wed, 05 May 2010, kmx via RT wrote​:

I see comming deadline for 5.12.1.

Could somebody competent consider including 634b482 + 20c8f8f
to 5.12.1

634b482 is already in maint-5.12 as 72c7417​:

  http​://perl5.git.perl.org/perl.git/commitdiff/72c7417

20c8f8f has been requested (by me) and got a second vote from Vincent.
It requires one other person with commit access to vote for it to
be eligible for merging.

Given that this bug was detected with Devel​::NYTProf, maybe Tim is
willing to put his vote in for this one (it is a single line addition
to XSUB.h).

Cheers,
-Jan

commit 20c8f8f
Author​: Jan Dubois <jand@​activestate.com>
Date​: Wed Apr 21 16​:49​:09 2010 -0700

  XSUB.h is supposed to redefine fgets under PERL_IMPLICIT_SYS, but doesn't.
 
  See also http​://rt.cpan.org/Public/Bug/Display.html?id=55049
  with workaround in http​://code.google.com/p/perl-devel-nytprof/source/detail?r=1168

Inline Patch

diff --git a/XSUB.h b/XSUB.h
index f23df37..06cb1c3 100644
--- a/XSUB.h
+++ b/XSUB.h
@@ -507,6 +507,7 @@ Rethrows a previously caught exception.  See L<perlguts/"Exception 
 #    define ferror             PerlSIO_ferror
 #    define clearerr           PerlSIO_clearerr
 #    define getc               PerlSIO_getc
+#    define fgets              PerlSIO_fgets
 #    define fputc              PerlSIO_fputc
 #    define fputs              PerlSIO_fputs
 #    define fflush             PerlSIO_fflush

[p5pRT]

From @timbunce

On Wed, May 05, 2010 at 12​:27​:02PM -0700, Jan Dubois wrote​:

On Wed, 05 May 2010, kmx via RT wrote​:

I see comming deadline for 5.12.1.

Could somebody competent consider including 634b482 + 20c8f8f
to 5.12.1

634b482 is already in maint-5.12 as 72c7417​:

http​://perl5.git.perl.org/perl.git/commitdiff/72c7417

20c8f8f has been requested (by me) and got a second vote from Vincent.
It requires one other person with commit access to vote for it to
be eligible for merging.

Given that this bug was detected with Devel​::NYTProf, maybe Tim is
willing to put his vote in for this one (it is a single line addition
to XSUB.h).

It certainly gets a nod from me. (Strictly speaking I don't have a vote
as I never did sort out commit rights to the new repository.)

Tim.

Cheers,
-Jan

commit 20c8f8f
Author​: Jan Dubois <jand@​activestate.com>
Date​: Wed Apr 21 16​:49​:09 2010 -0700

XSUB\.h is supposed to redefine fgets under PERL\_IMPLICIT\_SYS\, but doesn't\.

See also http​://rt\.cpan\.org/Public/Bug/Display\.html?id=55049
with workaround in http​://code\.google\.com/p/perl\-devel\-nytprof/source/detail?r=1168

diff --git a/XSUB.h b/XSUB.h
index f23df37..06cb1c3 100644
--- a/XSUB.h
+++ b/XSUB.h
@​@​ -507,6 +507,7 @​@​ Rethrows a previously caught exception. See L<perlguts/"Exception
# define ferror PerlSIO_ferror
# define clearerr PerlSIO_clearerr
# define getc PerlSIO_getc
+# define fgets PerlSIO_fgets
# define fputc PerlSIO_fputc
# define fputs PerlSIO_fputs
# define fflush PerlSIO_fflush

[p5pRT]

From @craigberry

On Thu, May 6, 2010 at 6​:05 AM, Tim Bunce <Tim.Bunce@​pobox.com> wrote​:

On Wed, May 05, 2010 at 12​:27​:02PM -0700, Jan Dubois wrote​:

On Wed, 05 May 2010, kmx via RT wrote​:

I see comming deadline for 5.12.1.

Could somebody competent consider including 634b482 + 20c8f8f
to 5.12.1

634b482 is already in maint-5.12 as 72c7417​:

   http​://perl5.git.perl.org/perl.git/commitdiff/72c7417

20c8f8f has been requested (by me) and got a second vote from Vincent.
It requires one other person with commit access to vote for it to
be eligible for merging.

Given that this bug was detected with Devel​::NYTProf, maybe Tim is
willing to put his vote in for this one (it is a single line addition
to XSUB.h).

It certainly gets a nod from me. (Strictly speaking I don't have a vote
as I never did sort out commit rights to the new repository.)

20c8f8f is now in maint-5.12 as​:

<http​://perl5.git.perl.org/perl.git/commitdiff/e560917618a7d70a9f3420f75c62e08872bf97b5>

[p5pRT]

@iabyn - Status changed from 'open' to 'resolved'