Bug 200243 - Fix CRASH_WITH_INFO() so that it doesn't complain about unused parameters on non Clang / MSVC compilers.
Summary: Fix CRASH_WITH_INFO() so that it doesn't complain about unused parameters on ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-29 14:29 PDT by Mark Lam
Modified: 2019-07-30 09:07 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch. (1.64 KB, patch)
2019-07-29 14:34 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (1.39 KB, patch)
2019-07-29 14:36 PDT, Mark Lam
saam: review+
Details | Formatted Diff | Diff
Patch (2.86 KB, patch)
2019-07-29 17:34 PDT, Michael Catanzaro
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-07-29 14:29:06 PDT
...
Comment 1 Mark Lam 2019-07-29 14:34:41 PDT
Created attachment 375106 [details]
proposed patch.
Comment 2 Mark Lam 2019-07-29 14:36:05 PDT
Created attachment 375107 [details]
proposed patch.
Comment 3 Mark Lam 2019-07-29 15:00:56 PDT
Sign.  This is more annoying to fix than I thought.  It appears that GCC does not support ##__VA_ARGS__ expansion.
Comment 4 Mark Lam 2019-07-29 15:06:58 PDT
(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.
Comment 5 Michael Catanzaro 2019-07-29 15:15:04 PDT
Oh thanks! (And nice timing, because this was coming up next on my TODO. :)
Comment 6 Mark Lam 2019-07-29 15:16:48 PDT
(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.
Comment 7 Michael Catanzaro 2019-07-29 15:19:46 PDT
Yeah I'll try.
Comment 8 Michael Catanzaro 2019-07-29 15:44:49 PDT
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.
Comment 9 Michael Catanzaro 2019-07-29 15:45:57 PDT
(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.)
Comment 10 Michael Catanzaro 2019-07-29 16:09:47 PDT
(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.
Comment 11 Michael Catanzaro 2019-07-29 17:23:18 PDT
OK, solution is just to use an inline function instead of a macro and call it a day.
Comment 12 Michael Catanzaro 2019-07-29 17:34:24 PDT
Created attachment 375131 [details]
Patch
Comment 13 Mark Lam 2019-07-29 17:41:54 PDT
Comment on attachment 375131 [details]
Patch

r=me if the EWS bots are happy.
Comment 14 Michael Catanzaro 2019-07-30 08:23:02 PDT
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.
Comment 15 Michael Catanzaro 2019-07-30 09:04:53 PDT
Committed r248008: <https://trac.webkit.org/changeset/248008>
Comment 16 Radar WebKit Bug Importer 2019-07-30 09:07:02 PDT
<rdar://problem/53712058>