Summary: | Macro expansion producing 'defined' has undefined behavior | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Taras Tsugrii <ttsugrii> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Minor | CC: | andersca, ap, commit-queue, darin, keith_miller, mark.lam, msaboff, saam | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Mac | ||||||||||
OS: | macOS 10.12 | ||||||||||
Attachments: |
|
Description
Taras Tsugrii
2016-12-19 16:21:56 PST
Created attachment 297493 [details]
Patch
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? 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 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. Created attachment 297554 [details]
Patch
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. 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! Created attachment 297561 [details]
Patch
Comment on attachment 297561 [details] Patch Clearing flags on attachment: 297561 Committed r210053: <http://trac.webkit.org/changeset/210053> All reviewed patches have been landed. Closing bug. |