Summary: | ANGLE should build with -Wshorten-64-to-32 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
Component: | ANGLE | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dino, eric.carlson, feature-media-reviews, kbr, kling, mvujovic, sam, webkit-ews, webkit.review.bot | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 106274 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Attachment 182578 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ChangeLog', u'Sour..." exit_code: 1
Source/ThirdParty/ANGLE/src/compiler/ShaderLang.cpp:65: More than one command on the same line in if [whitespace/parens] [4]
Source/ThirdParty/ANGLE/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Source/ThirdParty/ANGLE/src/compiler/osinclude.h:52: OS_TLSIndex is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 3 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 182578 [details] Patch Attachment 182578 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15873495 Comment on attachment 182578 [details] Patch Attachment 182578 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15873496 (In reply to comment #2) > (From update of attachment 182578 [details]) > Attachment 182578 [details] did not pass qt-ews (qt): > Output: http://queues.webkit.org/results/15873495 Qt's gcc doesn't know about -Wshorten-64-to-32?! Filed upstream as ANGLE Issue 396: <http://code.google.com/p/angleproject/issues/detail?id=396> Created attachment 182587 [details]
Patch v2
Most of the fixes were simply adding static_cast<int>() operators in C++ and one C-style (int) cast after enabling the warning flag. However, this did find one issue with the OS_TLSIndex type on 64-bit systems like Mac OS X which could cause duplicate OS_TLSIndex keys to be used when the 64-bit values were truncated to 32-bit values. OS_TLSIndex is defined as an unsigned int, but values of type pthread_key_t are assigned to it, and pthread_key_t is a 64-bit value on x86_64 but a 32-bit value on i386. The obvious fix is to define OS_TLSIndex in terms of pthread_key_t: -typedef unsigned int OS_TLSIndex; -#define OS_INVALID_TLS_INDEX 0xFFFFFFFF +typedef pthread_key_t OS_TLSIndex; +#define OS_INVALID_TLS_INDEX (static_cast<OS_TLSIndex>(-1)) Attachment 182587 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ChangeLog', u'Sour..." exit_code: 1
Source/ThirdParty/ANGLE/src/compiler/ShaderLang.cpp:65: More than one command on the same line in if [whitespace/parens] [4]
Source/ThirdParty/ANGLE/src/compiler/osinclude.h:52: OS_TLSIndex is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Just a heads up- I'm awaiting review on updating ANGLE to r1641 in bug 106274. I'll update my patch if this lands first. (In reply to comment #10) > Just a heads up- I'm awaiting review on updating ANGLE to r1641 in bug 106274. I'll update my patch if this lands first. Thanks! I'm fine with Bug 106274 landing first as well. (In reply to comment #11) > (In reply to comment #10) > > Just a heads up- I'm awaiting review on updating ANGLE to r1641 in bug 106274. I'll update my patch if this lands first. > > Thanks! I'm fine with Bug 106274 landing first as well. Cool- thanks! I think it'll be a little easier to land the ANGLE update first if that's all right. All of WebKit's ANGLE changes are currently upstreamed to ANGLE. After the update lands, we can start tracking the next set of changes (like this one) to be upstreamed in the ANGLE ChangeLog. I'll ping Dean for a review of the ANGLE update. Created attachment 182706 [details]
Patch v3
Attachment 182706 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ChangeLog', u'Sour..." exit_code: 1
Source/ThirdParty/ANGLE/src/compiler/osinclude.h:47: OS_TLSIndex is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 182706 [details]
Patch v3
Looks good to me. I'll help land your patch on ANGLE bug 396.
(In reply to comment #15) > (From update of attachment 182706 [details]) > Looks good to me. I'll help land your patch on ANGLE bug 396. Thanks Kenneth! I know upstream ANGLE uses gyp, so the setting for GCC_WARN_64_TO_32_BIT_CONVERSION=YES will be in a different place. Comment on attachment 182706 [details] Patch v3 Clearing flags on attachment: 182706 Committed r139758: <http://trac.webkit.org/changeset/139758> All reviewed patches have been landed. Closing bug. |
Created attachment 182578 [details] Patch <http://webkit.org/b/000000> Reviewed by NOBODY (OOPS!). * Configurations/Base.xcconfig: Enable -Wshorten-64-to-32 by setting GCC_WARN_64_TO_32_BIT_CONVERSION to YES. * src/compiler/MapLongVariableNames.cpp: (LongNameMap::Size): Add static_cast<int>(). * src/compiler/ShaderLang.cpp: Ditto. (getVariableInfo): (ShGetInfo): * src/compiler/ValidateLimitations.cpp: (ValidateLimitations::validateFunctionCall): Change ParamIndex from int to size_t. Add static_cast<int>() when passing as argument to getParam(). * src/compiler/glslang.l: Disable -Wshorten-64-to-32 by pragma. * src/compiler/glslang_lex.cpp: Ditto. * src/compiler/osinclude.h: Change type of OS_TLSIndex to pthread_key_t. Define OS_INVALID_TLS_INDEX by using static_cast<OS_TLSIndex>(-1). * src/compiler/preprocessor/new/Input.cpp: (pp::Input::Input): Add static_cast<int>(). * src/compiler/preprocessor/new/Tokenizer.cpp: Disable -Wshorten-64-to-32 by pragma. * src/compiler/preprocessor/new/Tokenizer.l: Ditto. * src/compiler/preprocessor/scanner.c: (yylex_CPP): Add C-style (int) cast. --- 12 files changed, 46 insertions(+), 10 deletions(-)