chakra-core / ChakraCore

ChakraCore is an open source Javascript engine with a C API.
MIT License
9.06k stars 1.19k forks source link

Build failure with Clang 16, 17 -- call to undeclared function 'va_arg' #6984

Closed EmployedRussian closed 2 months ago

EmployedRussian commented 2 months ago
./build.sh --debug
...
[  4%] Building C object pal/src/CMakeFiles/Chakra.Pal.dir/safecrt/safecrt_output_s.c.o
In file included from /home/ChakraCore/pal/src/safecrt/safecrt_output_s.c:45:
/home/ChakraCore/pal/src/safecrt/output.inl:854:28: error: call to undeclared function 'va_arg'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
  854 |                 fldwidth = get_int_arg(&argptr);
      |                            ^
/home/ChakraCore/pal/src/safecrt/output.inl:555:41: note: expanded from macro 'get_int_arg'
  555 |     #define get_int_arg(list)           va_arg(*list, int)
      |                                         ^
/home/ChakraCore/pal/src/safecrt/output.inl:854:28: error: expected expression
/home/ChakraCore/pal/src/safecrt/output.inl:555:55: note: expanded from macro 'get_int_arg'
  555 |     #define get_int_arg(list)           va_arg(*list, int)
...

It appears that __GNUC_VA_LIST is defined when it probably shouldn't be.

EmployedRussian commented 2 months ago

The __GNUC_VA_LIST is defined by both clang-15 and clang-17 (in <stdarg.h>).

Something else must be going wrong.

rhuanjl commented 2 months ago

Thanks for raising this - the PAL code is so messy...

One guess is this from palinternal.h line 328:

#ifdef _VAC_
#undef CHAR_BIT
#undef va_arg
#endif

I don't know what _VAC_ is but at a glance it looks to me like undef-ing va_arg can only be a mistake - we don't have any code that would redefine it again, wondering if there used to be but it got deleted and was unneeded as perhaps this was an always false condition that's changed?

EmployedRussian commented 2 months ago

The root cause appears to be: output.inl does not #include <stdarg.h>.

Builds fine with this patch:

diff --git a/pal/src/safecrt/output.inl b/pal/src/safecrt/output.inl
index c63f36df5..63a23e147 100644
--- a/pal/src/safecrt/output.inl
+++ b/pal/src/safecrt/output.inl
@@ -51,7 +51,7 @@ Buffer size required to be passed to _gcvt, fcvt and other fp conversion routine
 //#include <stddef.h>
 //#include <crtdefs.h>
 //#include <stdio.h>
-//#include <stdarg.h>
+#include <stdarg.h>
 //#include <cvt.h>
 //#include <conio.h>
 //#include <internal.h>
ppenzin commented 2 months ago

Seems pretty straight-forward, compiler got stricter about these types of issues over time. PAL is a fruit that keeps on giving. The most mind-boggling part is that it wasn't just not included, it was commented out. There was a time where dotnet's PAL tried to include stdargs in files that use output.inl, but that file has since been removed anyway.

BTW, I started a trunk Clang build on Ubuntu, but ran out of memory, will resume that once more of it arrives. I know there are probably nightly packages, but I'd likely need that for another project anyway. I have a FreeBSD build, but Chakra doesn't yet build there (PAL being probably the largest obstacle for a port, it is a lot of work to fix this fork of it on a new platform).

ppenzin commented 2 months ago

Added a new label for PAL issues.

rhuanjl commented 2 months ago

Closing as presumably fixed by #6985