jart / blink

tiniest x86-64-linux emulator
ISC License
6.89k stars 217 forks source link

blink/checked.h: fix syntax error when building with GCC 9.4 #150

Closed tkchia closed 1 year ago

tkchia commented 1 year ago
blink/checked.h:10:47: error: missing binary operator before token "("
   10 |      (defined(__has_builtin) && (__has_builtin(__builtin_add_overflow) && \
      |                                               ^
tkchia commented 1 year ago

Unfortunately it seems we simply cannot try to test for __has_builtin and try to use it as well, within a single #if directive. (And a compiler that can grok the __has_builtin(...) syntax probably already supports it anyway.)

So it is necessary to split the relevant part into two directives, like so:

#if ... defined(__has_builtin)
#if __has_builtin(__builtin_add_overflow) ...
...
#endif
#endif

I am guessing that __has_include is susceptible to the same problem — so I have also modified the use of __has_include accordingly.

Thank you!

tkchia commented 1 year ago

Hello @ghaerr,

I am not sure, but I think it is generally a good idea for code to test directly for features they want, whenever possible, in contrast to testing version numbers. :slightly_smiling_face:

Incidentally, the relevant C23 proposal (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2669.pdf) does mention that Microsoft Visual C has an <intsafe.h> header with a somewhat different API (https://docs.microsoft.com/en-us/windows/win32/api/intsafe). I suppose some compiler vendors might want to offer up an interface for safe integer arithmetic that is distinct from the GCC __builtin_... one. :slightly_smiling_face: :slightly_smiling_face:

Thank you!

ghaerr commented 1 year ago

Hello @tkchia,

I think it is generally a good idea for code to test directly for features they want, whenever possible, in contrast to testing version numbers. 🙂

I only ask that because, in the lines immediately before the __has_builtin checks, we check for compiler version numbers!! Is that version check for gcc necessary, as we're essentially duplicating the definitions of chk_add et al below.

Thanks for the information on the safe integer proposal in C23.

Thank you!

tkchia commented 1 year ago

Hello @ghaerr,

My suspicion is that the test for __GNUC__ >= 5 is to cater to super-old versions of GCC that had the builtin functions but did not support __has_builtin yet.

At the same time I guess there might be (who knows?) compilers that somehow manage to support __has_builtin __builtin_add_overflow, but for some reason do not claim compatibility with some version of GCC. (OK, perhaps this combination is not particularly likely. But I see that chibicc does implement some other intrinsic functions, such as __builtin_types_compatible_p.)

Thank you!