#5150 unix: tcl.h does not include sys/stat.h

obsolete: 8.5.13
open
5
2013-01-17
2012-12-24
rofl0r
No

tcl.h typedefs Tcl_StatBuf to struct stat/stat64 but does not include <sys/stat.h>

this leads to compilation error in tclCmdAH.c because the storage size of variables of type Tcl_StatBuf is unknown.

Discussion

1 2 3 > >> (Page 1 of 3)
  • rofl0r
    rofl0r
    2012-12-24

    • assigned_to: nobody --> stwo
     
  • The unix-dependent includes come through tclUnixPort.h, included (through tclPort.h) by tclInt.h.
    So it is not as plain as a failure to include an obviously needed system header, but rather some ifdeffery that fails in this specific case (that I cannot reproduce). Any specific info re the OS and/or configure/-D flags ?

     
  • Please attach the Makefile generated by Tcl's configure in this environment.

     
  • rofl0r
    rofl0r
    2012-12-24

    tcl Makefile as generated on sabotage linux i386

     
    Attachments
  • rofl0r
    rofl0r
    2012-12-24

    here's the relevant header part in musl libc:
    http://git.musl-libc.org/cgit/musl/tree/include/sys/stat.h#n98

    i added -D_GNU_SOURCE to the CFLAGS when launching configure, so this part got visible.
    tcl.h takes the typedef struct stat64 Tcl_StatBuf route. however without seeing the macro in sys/stat.h this cannot succeed, because stat64 is only a macro.

     
  • So what ? Now your sys/stat.h is included, and stat64 gets mapped to stat, Tcl ends up needing a "struct stat" somewhere. I assume it is in <bits/stat.h> as in most unices, isn't it.

    To make this exchange more efficient, please provide the generated Makefile without any tweaks.
    Patching tcl.h or forcing CFLAGS sound brittle ; better fix the root problem in the configure logic.

     
  • Ok, just seen the attachment, sorry ;)

     
  • Still, there might be a side effect to -D_GNU_SOURCE. Please give the vanilla Makefile, the one lacking HAVE_SYS_STAT.

     
  • rofl0r
    rofl0r
    2012-12-24

    i never used any other Makefile
    adding -D_GNU_SOURCE to CFLAGS is something i automatically do always

    so it was not added specifically to circumvent some problems here.

    this is the exact Makefile that causes the build failure.

     
  • rofl0r
    rofl0r
    2012-12-24

    musl's sys/stat.h includes an arch specific bits/stat.h that provides the actual definition of struct stat.
    http://git.musl-libc.org/cgit/musl/tree/arch/i386/bits/stat.h

    however it is not legal to include bits/stat.h directly, this is an internal header!
    if you do this, that would explain the breakage.

     
  • rofl0r
    rofl0r
    2012-12-24

    looking at tclUnixPort.h, it appears to me that sys/stat.h should be included before any stat-related ifdeffery
    there is code that defines things to be stat64, but since sys/stat.h is included later, the stat64 macro does not get expanded.

     
  • > it is not legal to include bits/stat.h directly,
    > this is an internal header! -if you do this,
    > that would explain the breakage

    Ahem. Do you realize that this is the deed of (your/musl's) sys/stat.h ? Tcl just includes <sys/stat.h> just as everybody else.

     
  • >looking at tclUnixPort.h, it appears to me that sys/stat.h should be
    >included before any stat-related ifdeffery
    >there is code that defines things to be stat64, but since sys/stat.h is
    >included later, the stat64 macro does not get expanded.

    Red herring. This code is just a pair of prototypes, passing opaque pointers. Even if I agree that they don't benefit from the macro, there is no harm done beyond a possible warning.

    If you want to proceed on this, please give the error message that you go when trying to compile with the vanilla Makefile generated by configure (not the one tweaked with whatever habits you have).

     
  • rofl0r
    rofl0r
    2012-12-25

    i mean this part here

    #elif defined(HAVE_STRUCT_STAT64)
    # define TclOSstat stat64
    # define TclOSlstat lstat64
    #else
    # define TclOSstat stat
    # define TclOSlstat lstat
    #endif

    the inclusion of stat.h comes later
    so whatever this tries to do, it doesn't see the stat64 macro and thus may fail.
    it is probably not related to the error i'm seeing.

    however it is very likely that the problem we're facing here has the same cause:
    tcl.h gets included before <sys/stat.h>, so the macro doesnt get expanded
    and the compiler fails to find the struct stat64.

     
  • rofl0r
    rofl0r
    2012-12-25

    again i have not tweaked anything.
    this is the Makefile that gets created when running CFLAGS=-D_GNU_SOURCE ./configure.
    i never used anything else.
    it is perfectly legal to define _GNU_SOURCE, the libc is free to do it automatically for example by defining it in features, and so am i.
    since stat64 is completely non-standard, the implementation is free to define it as macro so you better make sure it works, by putting the inclusion of sys/stat.h before your typedefs.

     
  • rofl0r
    rofl0r
    2012-12-25

    here's the bug: tclPort.h

    #include "tcl.h"
    #if !defined(_WIN32)
    # include "tclUnixPort.h"
    #endif

    it should include tclUnixPort.h before tcl.h, problem solved

    and for maximum portability put the include block in tclUnixport before any typedefs and macro definitions

     
  • Transferring to Jan Nijtmans, who did a similar move in a specific case (cygwin).
    Jan, any comment on the side-effects you feared, so that you didn't generalize ?
    (I'm referring to ad7cfb6ee3 where you moved tclWinPort above tcl.h, which looks like a good idea of wider applicability ;)

     
    • assigned_to: stwo --> nijtmans
     
  • Jan Nijtmans
    Jan Nijtmans
    2012-12-25

    > Jan, any comment on the side-effects you feared, so that you didn't
    > generalize ?
    Well, that change was for fixing a compiler warming with mingw, not
    for cygwin, but I agree with your analysis. It would be good to
    include sys/stat.h before tcl.h. The problem with this is
    the compat/*.h files, which currently need tcl.h to be
    included first. On cygwin/linux/solaris those are not used,
    so the move would be safe. But on older systems .....

    Another remark: The inclusion order didn't change
    between Tcl 8.5 and 8.6 (I can see in Makefile.tcl
    that the error occurred in 8.6, but I am assuming
    the same 'bug' is in 8.5 as well).

    The best solution here might be to move the
    sys/stat.h include from tcl*Port.h to tclInt.h.
    See:
    <http://core.tcl.tk/tcl/info/02a51cbb69>
    That has the least probability to break
    on any other platform.

    rofl0r, Alex, would that work for you? Any
    problems you can see with this change?

     
  • Jan Nijtmans
    Jan Nijtmans
    2012-12-25

    • labels: --> 52. Portability Support
    • milestone: --> obsolete: 8.5.13
     
  • Sorry, but digging a platform-specific header out and sticking it on top of the main generic one doesn't sound like a solid basis... Instead, I'd rather understand in detail why some parts of tcl*Port (like the compat/ ones) depend on being after tcl.h, and either stop this dependency, or move them in a separate file, to be included after tcl.h

    Can you please give these details (or just the kind of macros from tcl.h that are involed) ?

     
  • Don Porter
    Don Porter
    2012-12-27

    Only skimming the comments here. One thing to keep in mind
    with any editing of the #include dependency graph is to preserve
    the goals of the 8.5 era "header reform." See 922727.

     
  • Jan Nijtmans
    Jan Nijtmans
    2012-12-27

    Thanks, Don! But I'm fully aware of this header reform, and I
    don't think that my latest suggestions violatest this:
    <http://core.tcl.tk/tcl/info/dcbf22ad99>
    I don't see any danger in this, but please put in your view!

     
  • rofl0r
    rofl0r
    2012-12-27

    thanks njitmans for picking this up. however your fix is only a workaround this specific issue, but it still leaves everything else broken.
    this should be fixed properly by including all system headers before doing anything that depends on the types and other things defined in those headers, as ferrieux correctly noted.

    njitmans: i think it would help if you could answer ferrieux' question:
    "Can you please give these details (or just the kind of macros from tcl.h
    that are involed) ?"

     
1 2 3 > >> (Page 1 of 3)