Bug 136603 – Make updates suggested by new version of Xcode

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Darin Adler]

Make updates suggested by new version of Xcode

[Darin Adler]

Created attachment 237744 [details]
Patch

[WebKit Commit Bot]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Mark Rowe (bdash)]

Comment on attachment 237744 [details]
Patch

I'm not sure whether setting COMBINE_HIDPI_IMAGES = YES is compatible with our production builds. Can we leave that set to NO for projects that it wasn't already set in until we're able to determine for sure?

[Darin Adler]

Created attachment 237746 [details]
Patch

[Darin Adler]

Committed r173364: <http://trac.webkit.org/changeset/173364>

[Alexey Proskuryakov]

EWS didn't approve of this patch, and it did break the build:

/Volumes/Data/EWS/WebKit/Source/WebKit/mac/Misc/WebNSDataExtras.m:120:21: error: comparison of integers of different signs: 'typeof (length)' (aka 'unsigned long') and 'typeof (1024)' (aka 'int') [-Werror,-Wsign-compare]
    int remaining = MIN(length, WEB_GUESS_MIME_TYPE_PEEK_LENGTH) - (CHANNEL_TAG_LENGTH - 1);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Looks like a fix would not be mechanical.

[Timothy Hatcher]

CLANG_WARN_UNREACHABLE_CODE is causing issues in Release builds in JSC.

/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/bytecode/CodeBlock.cpp:282:14: error: code will never be executed [-Werror,-Wunreachable-code]
/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:126:24: error: code will never be executed [-Werror,-Wunreachable-code]
/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/dfg/DFGPlan.cpp:184:24: error: code will never be executed [-Werror,-Wunreachable-code]
/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:383:26: error: code will never be executed [-Werror,-Wunreachable-code]
/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:370:26: error: code will never be executed [-Werror,-Wunreachable-code]
/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:334:22: error: code will never be executed [-Werror,-Wunreachable-code]
/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/runtime/RegExp.cpp:276:19: error: code will never be executed [-Werror,-Wunreachable-code]
/Users/Timothy/Work/Safari-TOT.git/OpenSource/Source/JavaScriptCore/runtime/RegExp.cpp:399:19: error: code will never be executed [-Werror,-Wunreachable-code]

Seems to be mostly about RELEASE_ASSERT_NOT_REACHED().

[Darin Adler]

(In reply to comment #6)
> Looks like a fix would not be mechanical.

We can just turn that warning off for that project.

[Darin Adler]

(In reply to comment #7)
> CLANG_WARN_UNREACHABLE_CODE is causing issues in Release builds in JSC.

Should turn that warning off for that project.

[Darin Adler]

(In reply to comment #6)
> EWS didn't approve of this patch, and it did break the build:

Maybe only on some versions of the compiler. Did not break the build on my system. Despite not seeing the problem locally, I am pretty sure that adding a U after 1024 would fix the bug without turning off any warnings:

#define WEB_GUESS_MIME_TYPE_PEEK_LENGTH 1024U

(In reply to comment #9)
> (In reply to comment #7)
> > CLANG_WARN_UNREACHABLE_CODE is causing issues in Release builds in JSC.
> 
> Should turn that warning off for that project.

There’s also unreachable code in 32-bit builds of WTF and any other project that includes BitVector.h.

[Alexey Proskuryakov]

> adding a U after 1024 would fix the bug without turning off any warnings

That should make MIN compile, but we'd still have an unchecked cast from the result of MIN to a signed int. While I'm not sure whether that would break the build anywhere, it's clearly wrong.

[Darin Adler]

(In reply to comment #12)
> > adding a U after 1024 would fix the bug without turning off any warnings
> 
> That should make MIN compile, but we'd still have an unchecked cast from the result of MIN to a signed int. While I'm not sure whether that would break the build anywhere, it's clearly wrong.

I don’t know what you mean when you say it’s wrong. It’s not wrong to cast something in the range 0-1024 to an int. I can’t think of any checking that would be needed. Maybe there’s better coding style, but there’s no correctness issue.

[Darin Adler]

(In reply to comment #11)
> There’s also unreachable code in 32-bit builds of WTF and any other project that includes BitVector.h.

To me it seems fine to turn it off until we resolve those issues.

[Darin Adler]

(In reply to comment #13)
> (In reply to comment #12)
> > > adding a U after 1024 would fix the bug without turning off any warnings
> > 
> > That should make MIN compile, but we'd still have an unchecked cast from the result of MIN to a signed int. While I'm not sure whether that would break the build anywhere, it's clearly wrong.
> 
> I don’t know what you mean when you say it’s wrong. It’s not wrong to cast something in the range 0-1024 to an int. I can’t think of any checking that would be needed. Maybe there’s better coding style, but there’s no correctness issue.

Oh, I see, it’s not in the range 0-1024, but I still don’t see a correctness problem. Here’s the code:

    int remaining = MIN(length, WEB_GUESS_MIME_TYPE_PEEK_LENGTH) - (CHANNEL_TAG_LENGTH - 1);

The MIN expression will be of type NSUInteger, CHANNEL_TAG_LENGTH is an int, as is 1, so we’re taking an NSUInteger in the range 0-1024, subtracting an int, and assigning to an int. I believe that is guaranteed to work. As I said, there may be a stylistic issue, but there is not a correctness issue.

[Maciej Stachowiak]

I'm fixing the unreachable code cases on my (local, 64-bit) build. It might still be good to remove the warning in the interim.

I'm introducing COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE) since it's non-clang that is arguably wrong (by warning about problems that can happen on unreachable code paths). I will file a separate bug with that change.

[Maciej Stachowiak]

See https://bugs.webkit.org/show_bug.cgi?id=136616

[Darin Adler]

(In reply to comment #16)
> I'm fixing the unreachable code cases on my (local, 64-bit) build. It might still be good to remove the warning in the interim.

Great!

> I'm introducing COMPILER_QUIRK(CONSIDERS_UNREACHABLE_CODE) since it's non-clang that is arguably wrong (by warning about problems that can happen on unreachable code paths). I will file a separate bug with that change.

I like that. I should not have used !COMPILER(CLANG).