#2003 mkstemp(3) implementation in msys should move to library

WSL
open
posix (2)
Feature
later
Feature_in_WSL_4.1
True
2013-12-07
2013-07-13
No

giflib fails to build because the mingw library does not supply mkstemp(3). But there is an implementation of mkstemp(3) in msys git.

That implementation and its header declaration should be moved to where mingw-using applications can link it.

1 Attachments

Related

Issues: #2227

Discussion

  • Earnie Boyd

    Earnie Boyd - 2013-07-13
    • status: unread --> pending
    • Type: Bug --> Support
    • Resolution: none --> invalid
    • Category: Unknown --> Waiting_User_Response
     
  • Earnie Boyd

    Earnie Boyd - 2013-07-13

    I've labeled this as invalid because MSYS Git is a modified version of MSYS. However, why is giflib finding any function reference in any version of MSYS when you're not using a compiler for the build environment of *-pc-msys? Perhaps an environment issue?

     
  • Eric S. Raymond

    Eric S. Raymond - 2013-07-13

    giflib is not finding the function reference in msys.

    I got a bug report that giflib was failing to build under mingew because the mingw library doesn't have mkstemp(3). I Googled and found a mingew-friendly implementation of mkstemp(3) in msys git.

    I'd prefer not to see complaints about giflib not building because a normal part of the C environment like mkstemp(3) is inexplicably missing. I don't particularly care where you get the mkstemp(3) implementation to fix this problem; I was trying to be helpful by pointing out where a known-good one already exists.

    This is your bug. Please add mkstemp(3) to your library, however you choose to do it.

     
    Last edit: Eric S. Raymond 2013-07-13
  • Keith Marshall

    Keith Marshall - 2013-07-13

    I'd prefer not to see complaints about giflib not building because a normal part of the C environment like mkstemp(3) is inexplicably missing.

    It is not a normal part of the C environment on MS-Windows; it is missing because Microsoft do not provide it, in MSVCRT.DLL.

    This is your bug.

    I respectfully disagree; it is your omission, in failing to provide an appropriate replacement, (via AC_REPLACE_FUNCS, or whatever mechanism may be appropriate for giflib).

    Please add mkstemp(3) to your library, however you choose to do it.

    You are welcome to provide a properly attributed and documented patch, and we will be pleased to consider it. It is not MinGW's mission, to provide a complete POSIX emulation on MS-Windows; (that is the goal of Cygwin). This is not a MinGW bug; it is a porting issue, which may be addressed as a feature request. I am reclassifying it accordingly.

     
  • Keith Marshall

    Keith Marshall - 2013-07-13
    • Type: Support --> Feature
     
  • Keith Marshall

    Keith Marshall - 2013-07-18
    • assigned_to: Earnie Boyd
    • Patch attached: False --> True
     
  • Keith Marshall

    Keith Marshall - 2013-07-18

    Okay, you piqued my interest, not least because I kludged around this same issue, three or four years ago, for one of my own porting projects. I didn't contribute anything, at the time, because, while that kludge got me past the hurdle, I was never entirely satisfied with it as a generally practical solution; (indeed, reviewing it now, I'm convinced that it was thoroughly unsatisfactory).

    Googling around today, I still wasn't able to find any entirely satisfactory solution. I did find several suggestions, similar to my original kludge, and therefore equally unsatisfactory. One candidate, which I found at:

    http://stackoverflow.com/questions/6036227/mkstemp-implementation-for-win32

    comes close, but not quite close enough, so I've reworked the idea to create the attached patch.

    In reviewing this patch, we should consider:

    1. mkstemp isn't expected to be provided on Microsoft hosts, because MSVCRT.DLL doesn't support it.

    2. It doesn't qualify as a C99 extension to MinGW, because ISO-C99 doesn't specify it.

    3. It may qualify as a POSIX compatibility extension; it was promoted from XOpen Systems Interface extension to POSIX core, as of SUSv5.

    On this basis, the attached patch is offered for consideration. Earnie, if we do decide to adopt it, (and I would be in favour), we may wish to reconsider the exposure restrictions I've set in stdlib.h

     
    Last edit: Keith Marshall 2013-07-18
    • Earnie Boyd

      Earnie Boyd - 2013-07-18

      I'll have to look closer at the change to stdlib.h. Do we really need to filter all that is filtered? Also __CRT_ALIAS can be used instead of creating __MKSTEMP_INLINE.

      Markup envy.

       
      Last edit: Earnie Boyd 2013-07-18
      • Keith Marshall

        Keith Marshall - 2013-07-18

        Do we really need to filter all that is filtered?

        I'm fairly certain that we don't. I took those from the feature checks documented in the Debian mkstemp(3) and mkdtemp(3) manpages, but we'd normally expect the host compiler to specify something suitable by default; that's why I suggested a review. Maybe !__STRICT_ANSI__ would be a sufficient alternative, in our case?

        Also __CRT_ALIAS can be used instead of creating __MKSTEMP_INLINE.

        I thought that might be the case, but maybe not; I wanted to draw attention to the __always_inline__ implementation. That could be a problem: for example, autoconf's AC_CHECK_FUNCS may not be able to find it; an extern __inline__ implementation may be required, to work around that.

         
        • Earnie Boyd

          Earnie Boyd - 2013-07-18

          _CRTALIAS (we should change this to __CRT_ALIAS to be consistent) is defined with the __always_inline__ attribute.

          __STRICT_ANSI__ is one. Maybe a __MINGW_NOPOSIX to remove those extensions without removing all that __STRICT_ANSI__ would remove.

           
          • Keith Marshall

            Keith Marshall - 2013-07-19

            Markup envy.

            Envy? Or ennui? Try back-quotes around the affected expression; all those back-slashes are distracting, when transformed to POT for e-mail.

            _CRTALIAS (we should change this to __CRT_ALIAS to be consistent)

            I agree.

            is defined with the __always_inline__ attribute.

            I know. That wasn't the point. My purpose, which appears to have been served, was to give pause for thought; __always_inline__ breaks autoconf detection:

            AC_INIT
            AC_PROG_CC
            AC_CHECK_FUNCS([mkstemp])
            

            says "checking for mkstemp... no", when running

            ./configure --host=mingw32
            

            after installing the patch. Maybe __CRT_INLINE will fare better, in place of __MKSTEMP_INLINE; I'll need to play with it some more.

            __STRICT_ANSI__ is one. Maybe a __MINGW_NOPOSIX

            Or some such...

            to remove those extensions without removing all that __STRICT_ANSI__ would remove.

            Yeah. I guess __STRICT_ANSI__ should block all the Microsoft specific cruft, while leaving pure ANSI code. We also have __NO_ISOCEXT, which IIUC, should disable our C99 extensions, while leaving the Microsoft cruft, and any ANSI code which Microsoft does support; maybe __NO_POSIXEXT as a consistently named equivalent for our POSIX extensions?

            Arguably, there may be cases where we should support _BSD_SOURCE, (getopt.h uses it to hide the optreset brain damage), and/or _GNU_SOURCE; this probably isn't one of them.

             
            • Earnie Boyd

              Earnie Boyd - 2013-07-19

              Envy as defined by Aristotle. Thanks for the suggestion.

              AC_CHECK_FUNCS([mkstemp])
              says "checking for mkstemp... no", when running

              Is that because AC_CHECK_FUNCS doesn't include the header when checking for the function? Then there is a lot it will miss. We could use a library alias in the .def file to overcome that and only declare it in the headers based on the filters.

              I like __NO_POSIXEXT.

              there may be cases where we should support _BSD_SOURCE

              Maybe.

              and/or _GNU_SOURCE; this probably isn't one of them.

              Probably not.

               
              • Keith Marshall

                Keith Marshall - 2013-07-19

                AC_CHECK_FUNCS([mkstemp])
                says "checking for mkstemp... no", when running

                Is that because AC_CHECK_FUNCS doesn't include the header when checking for the function?

                Exactly so. It checks only for a publicly exposed (linkable) symbol, using a simple prototype, without consideration for the real function signature.

                Then there is a lot it will miss.

                Undoubtedly so. It doesn't stand a chance with __stdcall, for example. In practice, that isn't such a great problem, because __stdcall isn't generally portable, and the principal purpose of AC_CHECK_FUNCS is to detect mostly portable functions, which are known to be missing on some platforms; mkstemp is one such.

                We could use a library alias in the .def file to overcome that ...

                I didn't think we had a .def file for libmingwex.a; notwithstanding, this works:

                #ifndef __MKSTEMP_INLINE
                #define __MKSTEMP_INLINE  _CRTALIAS  /* FIXME: __CRT_ALIAS preferred */
                #endif
                
                int __cdecl __MINGW_NOTHROW __mingw_mkstemp( int, char * );
                
                __MKSTEMP_INLINE int __cdecl __MINGW_NOTHROW mkstemp( char *__name_template )
                 { return __mingw_mkstemp( MKSTEMP_INVOKE, __name_template ); }
                

                when accompanied by a trivial stub, (in mkstemp-stub.c, say):

                #define  _GNU_SOURCE  /* not required if we adjust stblib.h filters appropriately */
                #define __MKSTEMP_INLINE
                #include <stdlib.h>
                

                to expose a linkable API.

                 
                • Earnie Boyd

                  Earnie Boyd - 2013-07-19

                  I didn't think we had a .def file for libmingwex.a

                  That does not mean we cannot create one. We already have libraries created with .def files and object files being added to them later.

                  However, I want to look at the possibility of a stub library for other _CRTALIAS functions.

                   
              • Keith Marshall

                Keith Marshall - 2014-07-23

                I like __NO_POSIXEXT.

                I know I suggested it, but on further reflection, I don't much like this. POSIX itself prescribes _POSIX_C_SOURCE, with a defined value to specify a version of the standard to which conformity is expected; much better by far to adhere to that, than to invent yet another non-standard symbol. (The POSIX.1-2008 standard stipulates that conforming applications must define _POSIX_C_SOURCE to make POSIX function declarations visible; the applicable value, for this particular version of POSIX, which is the first to declare mkdtemp(), is 200809L; correspondingly, the first version to declare mkstemp() is POSIX.1-2001, for which the applicable value is 200112L).

                 
  • Earnie Boyd

    Earnie Boyd - 2013-07-18
    • labels: --> posix
    • Group: OTHER --> WSL
    • Resolution: invalid --> later
    • Category: Waiting_User_Response --> Feature_in_WSL_4.1
     
  • Keith Marshall

    Keith Marshall - 2013-07-19

    I want to look at the possibility of a stub library for other _CRTALIAS functions.

    If we adopt a convention such as I've suggested with __MKSTEMP_INLINE, we can arrange for make to compile the stub functions on the fly, without any need to maintain separate source; (I can provide an example, if required). Whether these are added directly to libmingwex.a, or a separate stubs library is not entirely immaterial; a separate stubs library would need to be linked by default, (so that configure script test compilations find the symbols, without requiring any special configure-time only options), but would become completely redundant in the subsequent make phase, (where inlining should obviate the need for them, entirely). Thus, simply adding them to libmingwex.a is possibly the more elegant choice.

    In either case, I can't see the advantage of maintaining a separate .def file.

     
  • Earnie Boyd

    Earnie Boyd - 2013-09-28

    Reviewing this and the discussion to date again today I'm thinking we want to put any POSIX enhancements to a separate library, I liking libmingwexp for the name, and provide -mnoposix for a compiler switch to control the specs file. The -mnoposix could also define __NO_POSIX__ which would be set not only when -mnoposix is used but also when __STRICT_ANSI__ is defined. And we could add a define for __NO_POSIX__ when __STRICT_ANSI__ is defined within _mingw.h to ensure that we do not need to worry about it.

    I am also considering -mnomingwex which would define a new __NO_MINGWEX__ for the user and drop the library from the linker. This is in conjunction for what I've just added to the 4.1-dev branch.

    If we decide to do this we can release a GCC early with the changes so that we can be confident that more persons have the required changes than those who wane behind in older versions. Using a newer library allows those with older versions to enjoy the functions even if needing to manually add them.

    we can arrange for make to compile the stub functions on the fly, without any need to maintain separate source; (I can provide an example, if required)

    Required or not it doesn't hurt to compare what others would do.

     
  • Earnie Boyd

    Earnie Boyd - 2013-09-28
    • status: pending --> open
     
  • Keith Marshall

    Keith Marshall - 2013-10-10

    we can arrange for make to compile the stub functions on the fly, without any need to maintain separate source; (I can provide an example, if required)

    Required or not it doesn't hurt to compare what others would do.

    Well, conceptually the __MKSTEMP_INLINE form, (with a unique identifier per function, and using mkstemp() as an example), is convenient because it allows us to do:

    <file name="Makefile" fragment="true">
    %-stub.o:
            $(MAKE) --no-print-dir -f Makestub headers='$^' $@
    
    mkstemp-stub.o: stdlib.h
    </file>
    
    <file name="Makestub" fragment="false">
    CC = mingw32-gcc
    
    %-stub.c: $(headers)
            echo '#define _GNU_SOURCE' > $@
            echo '#define __'`echo $* | tr a-z A-Z`'_INLINE' >> $@
            for hdr in $^; do echo '#include <'`basename $$hdr`'>' >> $@; done
    </file>
    

    so that, when the mkstemp-stub.o rule is invoked in the Makefile, the rules in Makestub create a C source stub dynamically, explicitly defining the __MKSTEMP_INLINE to be nothing, then compiles it to create the mkstemp-stub.o object stub defining the mkstemp() public symbol, and finally, deletes the source stub, (so we've no need to keep it in the distribution or repository trees).

    The disadvantage of this is that we have to add clutter, such as:

    #ifndef __MKSTEMP_INLINE
    #define __MKSTEMP_INLINE  __CRT_ALIAS
    #endif
    

    for each and every unique macro in this category of stubified function declarations. However, suppose we could adapt this strategy so that, with some care in the layout, we can just define:

    __CRT_ALIAS int __cdecl __MINGW_NOTHROW mkstemp( char *__name_template )
      { return __mingw_mkstemp( MKSTEMP_INVOKE, __name_template ); }
    

    and have the __CRT_ALIAS filtered out of just this one function definition, when compiling its stub? Provided we take care to keep the __CRT_ALIAS and the name of the inline function on the same physical line in the header file, something along the lines of:

    <file name="Makefile" fragment="true">
    %-stub.o:
            mkdir -p ./tmp/include
            $(MAKE) --no-print-dir -f Makestub headers='$^' $@
            rm -rf tmp
    
    mkstemp-stub.o: stdlib.h
    </file>
    
    <file name="Makestub" fragment="false">
    CC = mingw32-gcc
    CPPFLAGS = -I ./tmp/include
    
    %-stub.c: $(headers)
            echo '#define _GNU_SOURCE' > $@
            for hdr in $^; do echo '#include <'`basename $$hdr`'>' >> $@; \
              sed 's/__CRT_ALIAS \(.*$*\)/\1/' $$hdr > tmp/include/`basename $$hdr`; \
              done
    </file>
    

    would seem to get the job done. (Obviously, it may be necessary to adjust the way in which the header include path is defined, and the compiler is invoked, in an actual Makestub implementation).

     
  • Keith Marshall

    Keith Marshall - 2013-12-07

    conceptually the __MKSTEMP_INLINE form, (with a unique identifier per function, and using mkstemp() as an example), is convenient ...

    However, here's an example of a better technique; if we rewrite the definition of __CRT_ALIAS, with a redundant STUBNAME argument, thus:

    #define __CRT_ALIAS( STUBNAME )    static __FORCE_INLINE
    #define __FORCE_INLINE       __inline__ __attribute__((__always_inline__))
    

    and use it, as in this example stdlib.h implementation of mkstemp(3):

    /*
     * mkstemp(3) function support; added per feature request #2003
     */
    int __cdecl __MINGW_NOTHROW __mingw_mkstemp( int, char * );
    
    /* The following macros are a MinGW specific extension, to facilite
     * use of _O_TEMPORARY, (in addition to the POSIX required attributes),
     * when creating the temporary file.  Note that they require fcntl.h,
     * which stdlib.h should NOT automatically include; we leave the onus
     * on the user to explicitly include it, if using _MKSTEMP_SETMODE.
     */
    #define _MKSTEMP_INVOKE       0
    #define _MKSTEMP_DEFAULT     _O_CREAT | _O_EXCL | _O_RDWR
    #define _MKSTEMP_SETMODE(M) __mingw_mkstemp( _MKSTEMP_DEFAULT | (M), NULL )
    
    __CRT_ALIAS( "mkstemp-stub.o" )
    int __cdecl __MINGW_NOTHROW mkstemp( char *__name_template )
    { return __mingw_mkstemp( _MKSTEMP_INVOKE, __name_template ); }
    

    (noting that we must place the __CRT_ALIAS declaration on its own line, with its associated stub file name argument, above the actual inline function implementation), then we can adapt the following makefile fragment:

    CC = @CC@
    CFLAGS = @CFLAGS@
    CPPFLAGS = @CPPFLAGS@
    
    OBJEXT = @OBJEXT@
    
    STUB_CFLAGS = $(CFLAGS) $(CPPFLAGS) -fomit-frame-pointer
    
    vpath %.h @srcdir@/include
    
    CRT_ALIAS_FILTER = sed '/__CRT_ALIAS( *"$(@F)" *)/d'
    CRT_ALIAS_OBJECTS = $(STDLIB_ALIAS_OBJECTS) ...
    
    STDLIB_ALIAS_OBJECTS = ... mkstemp ...
    
    $(CRT_ALIAS_OBJECTS:%=%-stub.$(OBJEXT)): %-stub.o:
        mkdir -p ./tmp/include
        echo '#define _GNU_SOURCE' > crtalias-stub.c
        for hdr in $^; \
          do stubhdr=`basename $$hdr`; \
             echo '#include <'$$stubhdr'>' >> crtalias-stub.c; \
             $(CRT_ALIAS_FILTER) $$hdr > ./tmp/include/$$stubhdr; \
          done
        $(CC) -I ./tmp/include -c $(STUB_CFLAGS) -o $@ crtalias-stub.c
        rm crtalias-stub.c
        rm -rf tmp
    
    $(STDLIB_ALIAS_OBJECTS:%=%-stub.$(OBJEXT)): stdlib.h
    

    (expressed here in autoconf's Makefile.in template format), to generate any number of external stubs, without requiring any further supplementary files, or use of recursive make invocations.

     
    Last edit: Keith Marshall 2013-12-07

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks