socketry / io-event

MIT License
66 stars 15 forks source link

-std=c99 is disabling struct timespec #88

Closed eregon closed 8 months ago

eregon commented 8 months ago

We've noticed this error of this gem on TruffleRuby: https://github.com/sinatra/sinatra/actions/runs/7157298277/job/19487986409#step:4:198

``` current directory: /home/runner/work/sinatra/sinatra/vendor/bundle/truffleruby/3.2.2.8/gems/io-event-1.3.3/ext make DESTDIR\= sitearchdir\=./.gem.20231210-2929-jwpll6 sitelibdir\=./.gem.20231210-2929-jwpll6 compiling ./io/event/event.c compiling ./io/event/selector/selector.c compiling ./io/event/selector/epoll.c compiling ./io/event/interrupt.c In file included from /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/intern.h:60, from /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/ruby.h:197, from /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby.h:38, from ./io/event/selector/selector.h:23, from ./io/event/interrupt.c:26: /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/internal/intern/time.h:167:31: error: return type is an incomplete type 167 | static inline struct timespec rb_time_timespec(VALUE time) { | ^~~~~~~~~~~~~~~~ /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/internal/intern/time.h: In function ‘rb_time_timespec’: /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/internal/intern/time.h:168:21: error: storage size of ‘result’ isn’t known 168 | struct timespec result; | ^~~~~~ /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/internal/intern/time.h:170:12: warning: ‘return’ with a value, in function returning void [-Wreturn-type] 170 | return result; | ^~~~~~ /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/internal/intern/time.h:167:31: note: declared here 167 | static inline struct timespec rb_time_timespec(VALUE time) { | ^~~~~~~~~~~~~~~~ /home/runner/.rubies/truffleruby-head/lib/cext/include/ruby/internal/intern/time.h:168:21: warning: unused variable ‘result’ [-Wunused-variable] 168 | struct timespec result; | ^~~~~~ make: *** [Makefile:566: interrupt.o] Error 1 ```

I investigated locally and I reduced it to a pure C example:

#include <time.h>

int main(int argc, char const *argv[])
{
    struct timespec ts;
    return sizeof(ts);
}

This compiles fine with no flags, but it fails if passed -std=c99, which this gem does in https://github.com/socketry/io-event/blob/75f4fb06e813a83a2d43165e1f2d28f051886fb0/ext/extconf.rb#L17:

$ gcc test.c        
$ gcc -std=c99 test.c
test.c: In function ‘main’:
test.c:5:21: error: storage size of ‘ts’ isn’t known
    5 |     struct timespec ts;
      |                     ^~
zsh: exit 1     gcc -std=c99 test.c
eregon commented 8 months ago

My /usr/include/time.h on Fedora 37 has:

#if defined __USE_POSIX199309 || defined __USE_ISOC11
# include <bits/types/struct_timespec.h>
#endif

So I guess -std=c99 doesn't set POSIX 93 (and nor do CRuby headers) and it isn't considered C11.

I think the best is to remove -std=c99 in this gem, as it causes this bug. However I would be interested if anyone knows of other solutions. Setting -std= in C feels like a trap, I have only seen it cause problems (at least on Linux).

It seems by chance the issue does not happen on CRuby because struct timespec is not actually used in headers, unlike on TruffleRuby, https://github.com/oracle/truffleruby/blob/7c5bef1358b704a5cfa67ff17c4c763d567ff9e0/lib/cext/include/ruby/internal/intern/time.h#L168, where it needs to be. And https://github.com/oracle/truffleruby/blob/7c5bef1358b704a5cfa67ff17c4c763d567ff9e0/lib/cext/include/ruby/internal/intern/time.h#L35 probably "saves it" on CRuby but it certainly feels hacky to define it like that vs expecting the header to properly declare it.

The man page of gcc says:

       -std=
           Determine the language standard.   This option is currently only supported when compiling C or C++.

           The compiler can accept several base standards, such as c90 or c++98, and GNU dialects of those standards, such
           as gnu90 or gnu++98.  When a base standard is specified, the compiler accepts all programs following that
           standard plus those using GNU extensions that do not contradict it.  For example, -std=c90 turns off certain
           features of GCC that are incompatible with ISO C90, such as the "asm" and "typeof" keywords, but not other GNU
           extensions that do not have a meaning in ISO C90, such as omitting the middle term of a "?:" expression. On the
           other hand, when a GNU dialect of a standard is specified, all features supported by the compiler are enabled,
           even when those features change the meaning of the base standard.  As a result, some strict-conforming programs
           may be rejected.  The particular standard is used by -Wpedantic to identify which features are GNU extensions
           given that version of the standard. For example -std=gnu90 -Wpedantic warns about C++ style // comments, while
           -std=gnu99 -Wpedantic does not.

           A value for this option must be provided; possible values are

           c90
           c89
           iso9899:1990
               Support all ISO C90 programs (certain GNU extensions that conflict with ISO C90 are disabled). Same as
               -ansi for C code.

           iso9899:199409
               ISO C90 as modified in amendment 1.

           c99
           c9x
           iso9899:1999
           iso9899:199x
               ISO C99.  This standard is substantially completely supported, modulo bugs and floating-point issues
               (mainly but not entirely relating to optional C99 features from Annexes F and G).  See
               <https://gcc.gnu.org/c99status.html> for more information.  The names c9x and iso9899:199x are deprecated.

And indeed -std=gnu99 works fine.

FWIW clang also fails with -std=c99:

$ clang -std=c99 test.c  
test.c:5:21: error: variable has incomplete type 'struct timespec'
    struct timespec ts;
                    ^
test.c:5:12: note: forward declaration of 'struct timespec'
    struct timespec ts;
           ^
1 error generated.
eregon commented 8 months ago

And given that /usr/include/bits/types/struct_timespec.h is not trivial it also seems a bad idea to e.g. use the timespec definition of the man page:

           struct timespec {
               time_t   tv_sec;        /* seconds */
               long     tv_nsec;       /* nanoseconds */
           };

because the actual definition is a lot more complex:

$ cat /usr/include/bits/types/struct_timespec.h
/* NB: Include guard matches what <linux/time.h> uses.  */
#ifndef _STRUCT_TIMESPEC
#define _STRUCT_TIMESPEC 1

#include <bits/types.h>
#include <bits/endian.h>
#include <bits/types/time_t.h>

/* POSIX.1b structure for a time value.  This is like a `struct timeval' but
   has nanoseconds instead of microseconds.  */
struct timespec
{
#ifdef __USE_TIME_BITS64
  __time64_t tv_sec;        /* Seconds.  */
#else
  __time_t tv_sec;      /* Seconds.  */
#endif
#if __WORDSIZE == 64 \
  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
  __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
#else
# if __BYTE_ORDER == __BIG_ENDIAN
  int: 32;           /* Padding.  */
  long int tv_nsec;  /* Nanoseconds.  */
# else
  long int tv_nsec;  /* Nanoseconds.  */
  int: 32;           /* Padding.  */
# endif
#endif
};

#endif
eregon commented 8 months ago

Digging a bit more, in /usr/include/features.h:

#ifndef _FEATURES_H
#define _FEATURES_H 1

/* These are defined by the user (or the compiler)
   to specify the desired environment:

   __STRICT_ANSI__  ISO Standard C.
   _ISOC99_SOURCE   Extensions to ISO C89 from ISO C99.
   _ISOC11_SOURCE   Extensions to ISO C99 from ISO C11.
   _ISOC2X_SOURCE   Extensions to ISO C99 from ISO C2X.
   __STDC_WANT_LIB_EXT2__
            Extensions to ISO C99 from TR 27431-2:2010.
   __STDC_WANT_IEC_60559_BFP_EXT__
            Extensions to ISO C11 from TS 18661-1:2014.
   __STDC_WANT_IEC_60559_FUNCS_EXT__
            Extensions to ISO C11 from TS 18661-4:2015.
   __STDC_WANT_IEC_60559_TYPES_EXT__
            Extensions to ISO C11 from TS 18661-3:2015.
   __STDC_WANT_IEC_60559_EXT__
            ISO C2X interfaces defined only in Annex F.

   _POSIX_SOURCE    IEEE Std 1003.1.
   _POSIX_C_SOURCE  If ==1, like _POSIX_SOURCE; if >=2 add IEEE Std 1003.2;
            if >=199309L, add IEEE Std 1003.1b-1993;
            if >=199506L, add IEEE Std 1003.1c-1995;
            if >=200112L, all of IEEE 1003.1-2004
            if >=200809L, all of IEEE 1003.1-2008
   _XOPEN_SOURCE    Includes POSIX and XPG things.  Set to 500 if
            Single Unix conformance is wanted, to 600 for the
            sixth revision, to 700 for the seventh revision.
   _XOPEN_SOURCE_EXTENDED XPG things and X/Open Unix extensions.
   _LARGEFILE_SOURCE    Some more functions for correct standard I/O.
   _LARGEFILE64_SOURCE  Additional functionality from LFS for large files.
   _FILE_OFFSET_BITS=N  Select default filesystem interface.
   _ATFILE_SOURCE   Additional *at interfaces.
   _DYNAMIC_STACK_SIZE_SOURCE Select correct (but non compile-time constant)
            MINSIGSTKSZ, SIGSTKSZ and PTHREAD_STACK_MIN.
   _GNU_SOURCE      All of the above, plus GNU extensions.
   _DEFAULT_SOURCE  The default set of features (taking precedence over
            __STRICT_ANSI__).

   _FORTIFY_SOURCE  Add security hardening to many library functions.
            Set to 1 or 2; 2 performs stricter checks than 1.

   _REENTRANT, _THREAD_SAFE
            Obsolete; equivalent to _POSIX_C_SOURCE=199506L.

   The `-ansi' switch to the GNU C compiler, and standards conformance
   options such as `-std=c99', define __STRICT_ANSI__.  If none of
   these are defined, or if _DEFAULT_SOURCE is defined, the default is
   to have _POSIX_SOURCE set to one and _POSIX_C_SOURCE set to
   200809L, as well as enabling miscellaneous functions from BSD and
   SVID.  If more than one of these are defined, they accumulate.  For
   example __STRICT_ANSI__, _POSIX_SOURCE and _POSIX_C_SOURCE together
   give you ISO C, 1003.1, and 1003.2, but nothing else.

   These are defined by this file and are used by the
   header files to decide what to declare or define:

   __GLIBC_USE (F)  Define things from feature set F.  This is defined
            to 1 or 0; the subsequent macros are either defined
            or undefined, and those tests should be moved to
            __GLIBC_USE.
   __USE_ISOC11     Define ISO C11 things.
   __USE_ISOC99     Define ISO C99 things.
   __USE_ISOC95     Define ISO C90 AMD1 (C95) things.
   __USE_ISOCXX11   Define ISO C++11 things.
   __USE_POSIX      Define IEEE Std 1003.1 things.
   __USE_POSIX2     Define IEEE Std 1003.2 things.
   __USE_POSIX199309    Define IEEE Std 1003.1, and .1b things.
   __USE_POSIX199506    Define IEEE Std 1003.1, .1b, .1c and .1i things.
   __USE_XOPEN      Define XPG things.
   __USE_XOPEN_EXTENDED Define X/Open Unix things.
   __USE_UNIX98     Define Single Unix V2 things.
   __USE_XOPEN2K        Define XPG6 things.
   __USE_XOPEN2KXSI     Define XPG6 XSI things.
   __USE_XOPEN2K8       Define XPG7 things.
   __USE_XOPEN2K8XSI    Define XPG7 XSI things.
   __USE_LARGEFILE  Define correct standard I/O things.
   __USE_LARGEFILE64    Define LFS things with separate names.
   __USE_FILE_OFFSET64  Define 64bit interface as default.
   __USE_MISC       Define things from 4.3BSD or System V Unix.
   __USE_ATFILE     Define *at interfaces and AT_* constants for them.
   __USE_DYNAMIC_STACK_SIZE Define correct (but non compile-time constant)
            MINSIGSTKSZ, SIGSTKSZ and PTHREAD_STACK_MIN.
   __USE_GNU        Define GNU extensions.
   __USE_FORTIFY_LEVEL  Additional security measures used, according to level.

   The macros `__GNU_LIBRARY__', `__GLIBC__', and `__GLIBC_MINOR__' are
   defined by this file unconditionally.  `__GNU_LIBRARY__' is provided
   only for compatibility.  All new code should use the other symbols
   to test for features.

   All macros listed above as possibly being defined by this file are
   explicitly undefined if they are not explicitly defined.
   Feature-test macros that are not defined by the user or compiler
   but are implied by the other feature-test macros defined (or by the
   lack of any definitions) are defined by the file.

   ISO C feature test macros depend on the definition of the macro
   when an affected header is included, not when the first system
   header is included, and so they are handled in
   <bits/libc-header-start.h>, which does not have a multiple include
   guard.  Feature test macros that can be handled from the first
   system header included are handled here.  */

...

#if defined _POSIX_C_SOURCE && (_POSIX_C_SOURCE - 0) >= 199309L
# define __USE_POSIX199309  1
#endif

What's odd is on all truffleruby platforms #define _GNU_SOURCE 1 is set, from ruby/config.h. And that seems included by lib/cext/include/ruby/internal/config.h and then lib/cext/include/ruby/internal/intern/time.h.

Maybe the issue is _GNU_SOURCE does not take precedence over __STRICT_ANSI__ and __STRICT_ANSI__ is set by -std=c99 (standards conformance options such as '-std=c99', define __STRICT_ANSI__ in the comments above).

eregon commented 8 months ago
#define _GNU_SOURCE 1
#include <time.h>

int main(int argc, char const *argv[])
{
    struct timespec ts;
    return sizeof(ts);
}

does compile fine with gcc -std=c99 test.c, so something is odd in the real case. My guess is it's io-event including time.h without such a macro before requiring ruby, then struct timespec will never be defined because the header was already included.

eregon commented 8 months ago

My guess is it's io-event including time.h without such a macro before requiring ruby, then struct timespec will never be defined because the header was already included.

Indeed, it's https://github.com/socketry/io-event/blob/75f4fb06e813a83a2d43165e1f2d28f051886fb0/ext/io/event/interrupt.c#L24 Adding #include <ruby.h> before that line fixes it.

So the usual recommendation here is "always #include <ruby.h> before any other header, for every file in your native extension", because it does the proper defines and otherwise we get an issue like this one.

eregon commented 8 months ago

Fix in https://github.com/socketry/io-event/pull/89. As a note it affects truffleruby-head but not 23.1.0 because that uses a different header (the change is necessary to execute C extensions natively: https://github.com/oracle/truffleruby/issues/3118).

ioquatix commented 8 months ago

Thanks, gosh, this is kind of crazy problem. Thanks for the deep investigation and all the details. I'll make a release with the proposed fix.

ioquatix commented 8 months ago

Released in v1.4.1.

eregon commented 8 months ago

Thanks!