ANGLE should build with -Wshorten-64-to-32

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Apply attached patch (build with -Wshorten-64-to-32).

What is the expected output?

ANGLE should build successfully.

What do you see instead?

ANGLE fails to build with -Werror due to warnings from -Wshorten-64-to-32.

What version of the product are you using? On what operating system?

WebKit r139570 / ANGLE r1170 (merged in WebKit r122870)

See WebKit Bug 106798:  <https://bugs.webkit.org/show_bug.cgi?id=106798>

Original issue reported on code.google.com by ddkilzer@gmail.com on 14 Jan 2013 at 4:18

[GoogleCodeExporter]

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

Original comment by ddkilzer@gmail.com on 14 Jan 2013 at 5:01

Attachments:

• 0001-ANGLE-should-build-with-Wshorten-64-to-32.patch

[GoogleCodeExporter]

Original comment by kbr@chromium.org on 14 Jan 2013 at 7:40

[GoogleCodeExporter]

Original comment by kbr@chromium.org on 14 Jan 2013 at 7:41

[GoogleCodeExporter]

Chris, Justin: could you take a look at the attached patch and indicate whether 
it looks like the right approach from a security perspective?

Original comment by kbr@chromium.org on 14 Jan 2013 at 7:43

[GoogleCodeExporter]

I was looking at this a few weeks ago, and ended up disabling the warnings for 
Win64, so we have a signal to circle back later. So, I'd rather we use that 
approach until we can fix the types, because I couldn't convince myself that we 
could never get a string longer than 2gb into the parser.

Original comment by jschuh@chromium.org on 14 Jan 2013 at 9:04

[GoogleCodeExporter]

Sorry, to be more succinct, I think it's a bad idea to have the explicit size_t 
to int typecasts. Please just add the warning suppressions instead, so we can 
track that to know we need to go back and audit/fix them. The only place we've 
used explicit casts is where we've fully audited the code paths and know 
they're safe.

Original comment by jschuh@chromium.org on 14 Jan 2013 at 9:17

[GoogleCodeExporter]

Thanks for the clarification.  The static_cast<int>() operators could also act 
as documentation, but I'll use the pragma instead.  I may also try "pulling the 
thread" on some of the changes to see how far they go.

Original comment by ddkilzer@gmail.com on 14 Jan 2013 at 11:10

[GoogleCodeExporter]

If you're willing to "pull the thread" I'd certainly appreciate it, but I 
wouldn't consider it a blocking requirement either. One thing we're doing as a 
stopgap in Chrome is to use a combination of allocator hooks and OS limits to 
prevent 2gb contiguous allocations and cap overall memory usage. So, we have a 
safety net until we work our way through this very long list of implicit 
conversions.

Original comment by jschuh@chromium.org on 14 Jan 2013 at 11:17

[GoogleCodeExporter]

Attached the latest patch from Bug 106798 (which is named "Patch v3" there).

This adds #pragma statements to work around the warnings until they can be 
fixed upstream properly, with the exception of the fix for the definition of 
OS_TLSIndex.

Original comment by ddkilzer@gmail.com on 15 Jan 2013 at 6:55

Attachments:

• 0001-ANGLE-should-build-with-Wshorten-64-to-32-v3.patch

[GoogleCodeExporter]

Thanks for the patch. Incorporation into ANGLE is out for review under 
https://codereview.appspot.com/7119043/ .

Original comment by kbr@chromium.org on 15 Jan 2013 at 7:09

[GoogleCodeExporter]

Filed ANGLE Bug 403 for Intermediate.cpp warnings with -Wshorten-64-to-32.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 1:04

[GoogleCodeExporter]

Filed ANGLE Bug 404 for MapLongVariableNames.cpp warnings with 
-Wshorten-64-to-32.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 2:55

[GoogleCodeExporter]

Filed ANGLE Bug 405 for ValidateLimitations.cpp warnings with 
-Wshorten-64-to-32.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 3:29

[GoogleCodeExporter]

Filed ANGLE Bug 406 for Input.cpp warnings with -Wshorten-64-to-32.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 3:36

[GoogleCodeExporter]

Filed ANGLE Bug 407 for ShaderLang.cpp warnings with -Wshorten-64-to-32.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 5:26

[GoogleCodeExporter]

Filed ANGLE Bug 408 for Tokenizer.cpp warnings with -Wshorten-64-to-32.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 10:17

[GoogleCodeExporter]

Filed ANGLE Bug 409 for glslang_lex.cpp warnings with -Wshorten-64-to-32.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 10:36

[GoogleCodeExporter]

That's all of the known issues as of ANGLE r1641.

Original comment by ddkilzer@gmail.com on 27 Jan 2013 at 10:37

[GoogleCodeExporter]

This has been fixed as of 
https://code.google.com/p/angleproject/source/detail?r=1826 .

Original comment by kbr@chromium.org on 12 Feb 2013 at 3:16

• Changed state: Fixed