Bug 166047 – Macro expansion producing 'defined' has undefined behavior

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

[Taras Tsugrii]

When compiling JSC with master build of clang, I get many errors (warnings which are treated as errors) like:
/tmp/WebKit/Source/JavaScriptCore/API/JSBase.h:141:122: note: expanded from macro 'JSC_OBJC_API_ENABLED'
#define JSC_OBJC_API_ENABLED (defined(__clang__) && defined(__APPLE__) && ((defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && !defined(__i386__)) || (defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE)))
                                                                                                                         ^
In file included from /tmp/WebKit/Source/JavaScriptCore/testRegExp.cpp:26:

/tmp/WebKit/Source/JavaScriptCore/runtime/JSGlobalObject.h:594:5: error: macro expansion producing 'defined' has undefined behavior [-Werror,-Wexpansion-to-defined]

The fix is trivial - instead of checks like #if FOO, #if FOO == 1 has to be used and instead of #define FOO (...), #if (...) #define FOO = 1 #else #define FOO = 0 #endif should be used.

[Taras Tsugrii]

Created attachment 297493 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 297493 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Fix undefined behavior caused by macro expansion producing 'defined'

Another instance of this pattern is in WebKit2/Shared/API/Cocoa/WKFoundation.h

Also, ThirdParty/ANGLE/src/tests/preprocessor_tests appears to test what happens in this scenario.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:302
> -#if JSC_OBJC_API_ENABLED
> +#if JSC_OBJC_API_ENABLED == 1

This is not the normal pattern for how we test for things being enabled, and thus it's error prone. Why was this part of the fix necessary?

[Darin Adler]

Comment on attachment 297493 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGlobalObject.h:302
>> +#if JSC_OBJC_API_ENABLED == 1
> 
> This is not the normal pattern for how we test for things being enabled, and thus it's error prone. Why was this part of the fix necessary?

== 1 is definitely not what we want here

[Taras Tsugrii]

Comment on attachment 297493 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:3
>> +        Fix undefined behavior caused by macro expansion producing 'defined'
> 
> Another instance of this pattern is in WebKit2/Shared/API/Cocoa/WKFoundation.h
> 
> Also, ThirdParty/ANGLE/src/tests/preprocessor_tests appears to test what happens in this scenario.

thanks Alexey! Indeed, I saw that this is tested in ThirdParty/ANGLE/src/tests/preprocessor_tests:ExpandedDefinedParsedInsideIf but it breaks the build by default on a master build of clang, unless the warning is disabled :(

>>> Source/JavaScriptCore/runtime/JSGlobalObject.h:302
>>> +#if JSC_OBJC_API_ENABLED == 1
>> 
>> This is not the normal pattern for how we test for things being enabled, and thus it's error prone. Why was this part of the fix necessary?
> 
> == 1 is definitely not what we want here

oops, sorry, this is indeed not necessary - the original code used to be a warning because of the way JSC_OBJC_API_ENABLED was defined in JSBase.h, but now with the fix above, it's fine to leave this line as it was. I'll update the patch.

[Taras Tsugrii]

Created attachment 297554 [details]
Patch

[Darin Adler]

Comment on attachment 297554 [details]
Patch

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

Looks good to me.

> Source/WebKit2/Shared/API/Cocoa/WKFoundation.h:34
>  #if TARGET_OS_IPHONE
>  #define WK_API_ENABLED 1
>  #else
> -#define WK_API_ENABLED (defined(__clang__) && defined(__APPLE__) && !defined(__i386__))
> +#if (defined(__clang__) && defined(__APPLE__) && !defined(__i386__))
> +#define WK_API_ENABLED 1

This seems a bit inelegant. I would write:

    #if TARGET_OS_IPHONE || (defined(__clang__) && defined(__APPLE__) && !defined(__i386__))
    <...>
    #endif

Rather than #if #else #if #else.

[Taras Tsugrii]

Comment on attachment 297554 [details]
Patch

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

>> Source/WebKit2/Shared/API/Cocoa/WKFoundation.h:34
>> +#define WK_API_ENABLED 1
> 
> This seems a bit inelegant. I would write:
> 
>     #if TARGET_OS_IPHONE || (defined(__clang__) && defined(__APPLE__) && !defined(__i386__))
>     <...>
>     #endif
> 
> Rather than #if #else #if #else.

doh, I was so laser focused on not breaking anything that I totally missed the external context. Thanks for the suggestion, Darin!

[Taras Tsugrii]

Created attachment 297561 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297561 [details]
Patch

Clearing flags on attachment: 297561

Committed r210053: <http://trac.webkit.org/changeset/210053>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.