Bug 200243 – Fix CRASH_WITH_INFO() so that it doesn't complain about unused parameters on non Clang / MSVC compilers.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

...

[Mark Lam]

Created attachment 375106 [details]
proposed patch.

[Mark Lam]

Created attachment 375107 [details]
proposed patch.

[Mark Lam]

Sign.  This is more annoying to fix than I thought.  It appears that GCC does not support ##__VA_ARGS__ expansion.

[Mark Lam]

(In reply to Mark Lam from comment #3)
> Sign.  This is more annoying to fix than I thought.  It appears that GCC
> does not support ##__VA_ARGS__ expansion.

Not true.  I misread the build failure logs.  Let me try again.

[Michael Catanzaro]

Oh thanks! (And nice timing, because this was coming up next on my TODO. :)

[Mark Lam]

(In reply to Michael Catanzaro from comment #5)
> Oh thanks! (And nice timing, because this was coming up next on my TODO. :)

Can you take the bug?  I don't have gcc and I can't reproduce the issue.

[Michael Catanzaro]

Yeah I'll try.

[Michael Catanzaro]

After reading https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html I was confused why your patch didn't work, but eventually I noticed:

"""
CPP retains the comma when conforming to a specific C standard. Otherwise the comma is dropped as an extension to the standard.
"""

Indeed, your patch works if I try this:


diff --git a/Source/cmake/OptionsCommon.cmake b/Source/cmake/OptionsCommon.cmake
index a574ef8c222..75f60ea9384 100644
--- a/Source/cmake/OptionsCommon.cmake
+++ b/Source/cmake/OptionsCommon.cmake
@@ -1,6 +1,6 @@
 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
-set(CMAKE_CXX_EXTENSIONS OFF)
+set(CMAKE_CXX_EXTENSIONS ON)
 
 add_definitions(-DBUILDING_WITH_CMAKE=1)
 add_definitions(-DHAVE_CONFIG_H=1)


Apple ports are compiled with GNU language extensions enabled (which clang implements for compatibility), but CMake ports are not. We could turn them on to make this work if Apple is OK with starting to use GNU extensions throughout the codebase, but it's probably better for WebKit to stick to language standards. The standard solution, according to the documentation, is to replace ##__VA_ARGS__ with format __VA_OPT__(,) __VA_ARGS__, but this requires C++20, and well, please no.

Let me see if I can make it work by using the va_list in the function. It will probably mess up the stack and defeat the entire point of using the CRASH_WITH_INFO() macro, but this is just for the fallback path anyway.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> We could turn them
> on to make this work if Apple is OK with starting to use GNU extensions
> throughout the codebase

(Currently they only work in Apple-specific code. That might be ironic.)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8) 
> Let me see if I can make it work by using the va_list in the function.

Well that doesn't work because of course there's no way to get the vargs into the function without using __VA_ARGS__, oops.

[Michael Catanzaro]

OK, solution is just to use an inline function instead of a macro and call it a day.

[Michael Catanzaro]

Created attachment 375131 [details]
Patch

[Mark Lam]

Comment on attachment 375131 [details]
Patch

r=me if the EWS bots are happy.

[Michael Catanzaro]

Comment on attachment 375131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375131&action=review

> Source/WTF/wtf/Assertions.h:606
> +// FIXME: When we enable C++20, we should replace ##__VA_ARGS with format __VA_OPT__(,) __VA_ARGS__

Typo here, should be ##__VA_ARGS__. Will land manually.

[Michael Catanzaro]

Committed r248008: <https://trac.webkit.org/changeset/248008>

[Radar WebKit Bug Importer]

<rdar://problem/53712058>