Bug 192491 - [WTF] Debug build fails due conflicting abort() method
Summary: [WTF] Debug build fails due conflicting abort() method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-07 02:05 PST by Adrian Perez
Modified: 2018-12-08 14:11 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.35 KB, patch)
2018-12-07 02:14 PST, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (1.50 KB, patch)
2018-12-08 11:09 PST, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2018-12-07 02:05:18 PST
When building passing -DCMAKE_BUILD_TYPE=Debug:

 - The XMLHttpRequest class defines an ::abort() method (in Source/WebCore/xml/XMLHttpRequest.h)
 - The CRASH() and CRASH_UNDER_CONSTEXPR_CONTEXT macros (in Source/WTF/wtf/Assertions.h) call
   the abort() function, but inside the code for XMLHttpRequest that would implicitly call
   the method. Ugh.

Probably the best solution is to make the macros in Assertions.h use the
namespace (so they expand to std::abort), to avoid the method being picked
when expandin the macros.

Compilation error follows.

----

In file included from DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:32,
                 from DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:25,
                 from ../Source/WebCore/config.h:51,
                 from CSSValueKeywords.gperf:4,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource1.cpp:1:
../Source/WebCore/xml/XMLHttpRequest.h: In static member function ‘static void* WebCore::XMLHttpRequest::operator new(size_t, NotNullTag, void*)’:
DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:23: error: cannot call member function ‘void WebCore::XMLHttpRequest::abort()’ without object
 #define CRASH() abort()
                       ^
DerivedSources/ForwardingHeaders/wtf/Assertions.h:587:30: note: in expansion of macro ‘CRASH’
 #define CRASH_WITH_INFO(...) CRASH()
                              ^~~~~
DerivedSources/ForwardingHeaders/wtf/Assertions.h:323:9: note: in expansion of macro ‘CRASH_WITH_INFO’
         CRASH_WITH_INFO(__VA_ARGS__); \
         ^~~~~~~~~~~~~~~
DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:288:9: note: in expansion of macro ‘ASSERT’
         ASSERT(location); \
         ^~~~~~
DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:294:5: note: in expansion of macro ‘WTF_MAKE_FAST_ALLOCATED_IMPL’
     WTF_MAKE_FAST_ALLOCATED_IMPL \DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:23: error: cannot call member function ‘void WebCore::XMLHttpRequest::abort()’ without object
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../Source/WebCore/xml/XMLHttpRequest.h:53:5: note: in expansion of macro ‘WTF_MAKE_FAST_ALLOCATED’
     WTF_MAKE_FAST_ALLOCATED;
     ^~~~~~~~~~~~~~~~~~~~~~~
Comment 1 Adrian Perez 2018-12-07 02:14:41 PST
Created attachment 356789 [details]
Patch
Comment 2 Michael Catanzaro 2018-12-07 11:20:49 PST
Comment on attachment 356789 [details]
Patch

I wonder why none of the bots noticed this. Or why it doesn't affect me locally. I've done many debug builds.

This was good thinking, but unfortunately this is one of a very few headers that get included in C (and Objective C) so we can't use namespaces here. E.g. from EWS:

/home/buildbot/ews/WebKit/Source/JavaScriptCore/API/tests/testapi.c: In function ‘createStringWithContentsOfFile’:
DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:21: error: expected expression before ‘:’ token
 #define CRASH() std::abort()
                     ^

I was going to suggest renaming XMLHttpRequest::abort instead, but there are more abort()s in the codebase... NetworkResourceLoader.cpp, testbmalloc.cpp....

We could add a new conditional to check #if __cplusplus? But that would be messier.
Comment 3 Adrian Perez 2018-12-08 10:55:28 PST
(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 356789 [details]
> Patch
> 
> I wonder why none of the bots noticed this. Or why it doesn't affect me
> locally. I've done many debug builds.
> 
> This was good thinking, but unfortunately this is one of a very few headers
> that get included in C (and Objective C) so we can't use namespaces here.
> E.g. from EWS:
> 
> /home/buildbot/ews/WebKit/Source/JavaScriptCore/API/tests/testapi.c: In
> function ‘createStringWithContentsOfFile’:
> DerivedSources/ForwardingHeaders/wtf/Assertions.h:244:21: error: expected
> expression before ‘:’ token
>  #define CRASH() std::abort()
>                      ^
> 
> I was going to suggest renaming XMLHttpRequest::abort instead, but there are
> more abort()s in the codebase... NetworkResourceLoader.cpp,
> testbmalloc.cpp....
> 
> We could add a new conditional to check #if __cplusplus? But that would be
> messier.

I think checking for __cplusplus is reasonable, I'll update the patch.
Comment 4 Adrian Perez 2018-12-08 11:09:02 PST
Created attachment 356878 [details]
Patch
Comment 5 Michael Catanzaro 2018-12-08 13:44:43 PST
Comment on attachment 356878 [details]
Patch

:/
Comment 6 WebKit Commit Bot 2018-12-08 14:10:45 PST
Comment on attachment 356878 [details]
Patch

Clearing flags on attachment: 356878

Committed r239012: <https://trac.webkit.org/changeset/239012>
Comment 7 WebKit Commit Bot 2018-12-08 14:10:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-12-08 14:11:24 PST
<rdar://problem/46576939>