Bug 106798 – ANGLE should build with -Wshorten-64-to-32

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[David Kilzer (:ddkilzer)]

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(-)

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 182578 [details]
Patch

Attachment 182578 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15873495

[Early Warning System Bot]

Comment on attachment 182578 [details]
Patch

Attachment 182578 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15873496

[David Kilzer (:ddkilzer)]

(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?!

[David Kilzer (:ddkilzer)]

<rdar://problem/12551571>

[David Kilzer (:ddkilzer)]

Filed upstream as ANGLE Issue 396:
<http://code.google.com/p/angleproject/issues/detail?id=396>

[David Kilzer (:ddkilzer)]

Created attachment 182587 [details]
Patch v2

[David Kilzer (:ddkilzer)]

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))

[WebKit Review Bot]

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.

[Max Vujovic]

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.

[David Kilzer (:ddkilzer)]

(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.

[Max Vujovic]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 182706 [details]
Patch v3

[WebKit Review Bot]

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.

[Kenneth Russell]

Comment on attachment 182706 [details]
Patch v3

Looks good to me. I'll help land your patch on ANGLE bug 396.

[David Kilzer (:ddkilzer)]

(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.

[WebKit Review Bot]

Comment on attachment 182706 [details]
Patch v3

Clearing flags on attachment: 182706

Committed r139758: <http://trac.webkit.org/changeset/139758>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.