Bug 198518

Summary: REGRESSION(r245586): static assertion failed: Match result and EncodedMatchResult should be the same size
Product: WebKit Reporter: Alberto Garcia <berto>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, commit-queue, erack, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, saam, tzagallo, ysuzuki
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch
none
Patch
none
Patch none

Alberto Garcia
Reported 2019-06-04 00:57:14 PDT
The build fails in most architectures (exceptions: i386, x86_64, aarch64). ------------------------------------------------------- These errors are from mips64: In file included from ../Source/JavaScriptCore/runtime/RegExp.h:25, from ../Source/JavaScriptCore/runtime/RegExpCachedResult.h:30, from ../Source/JavaScriptCore/runtime/RegExpGlobalData.h:28, from ../Source/JavaScriptCore/runtime/JSGlobalObject.h:42, from ../Source/JavaScriptCore/runtime/ExecutableBase.h:33, from ../Source/JavaScriptCore/runtime/ScriptExecutable.h:28, from ../Source/JavaScriptCore/runtime/GlobalExecutable.h:29, from ../Source/JavaScriptCore/runtime/EvalExecutable.h:29, from ../Source/JavaScriptCore/runtime/DirectEvalExecutable.h:28, from ../Source/JavaScriptCore/bytecode/DirectEvalCodeCache.h:31, from ../Source/JavaScriptCore/bytecode/CodeBlock.h:42, from ../Source/JavaScriptCore/bytecode/StructureStubInfo.h:28, from ../Source/JavaScriptCore/bytecode/BytecodeDumper.h:33, from DerivedSources/JavaScriptCore/BytecodeStructs.h:32, from ../Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:31: ../Source/JavaScriptCore/runtime/MatchResult.h: At global scope: ../Source/JavaScriptCore/runtime/MatchResult.h:82:35: error: static assertion failed: Match result and EncodedMatchResult should be the same size static_assert(sizeof(MatchResult) == sizeof(EncodedMatchResult), "Match result and EncodedMatchResult should be the same size"); DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h: In instantiation of ‘ToType WTF::bitwise_cast(FromType) [with ToType = JSC::MatchResult; FromType = long unsigned int]’: ../Source/JavaScriptCore/runtime/MatchResult.h:55:48: required from here DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:136:36: error: static assertion failed: bitwise_cast size of FromType and ToType must be equal! static_assert(sizeof(FromType) == sizeof(ToType), "bitwise_cast size of FromType and ToType must be equal!"); ------------------------------------------------------- This one is from armhf: In file included from DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:32, from DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:25, from ../Source/WebCore/config.h:50, from ../Source/WebCore/rendering/style/StyleGeneratedImage.cpp:24, from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-23.cpp:1: ../Source/WebCore/rendering/style/StyleRareInheritedData.cpp:78:47: error: static assertion failed: StyleRareInheritedData_should_bit_pack COMPILE_ASSERT(sizeof(StyleRareInheritedData) <= sizeof(GreaterThanOrSameSizeAsStyleRareInheritedData), StyleRareInheritedData_should_bit_pack);
Attachments
proposed patch (1013 bytes, patch)
2019-06-24 12:10 PDT, Eike Rathke
no flags
Patch (1.83 KB, patch)
2019-06-24 13:19 PDT, Michael Catanzaro
no flags
Patch (2.10 KB, patch)
2019-06-24 13:24 PDT, Michael Catanzaro
no flags
Michael Catanzaro
Comment 1 2019-06-04 05:24:30 PDT
(In reply to Alberto Garcia from comment #0) > This one is from armhf: > > In file included from DerivedSources/ForwardingHeaders/wtf/StdLibExtras.h:32, > from DerivedSources/ForwardingHeaders/wtf/FastMalloc.h:25, > from ../Source/WebCore/config.h:50, > from > ../Source/WebCore/rendering/style/StyleGeneratedImage.cpp:24, > from > DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-23.cpp:1: > ../Source/WebCore/rendering/style/StyleRareInheritedData.cpp:78:47: error: > static assertion failed: StyleRareInheritedData_should_bit_pack > COMPILE_ASSERT(sizeof(StyleRareInheritedData) <= > sizeof(GreaterThanOrSameSizeAsStyleRareInheritedData), > StyleRareInheritedData_should_bit_pack); This is bug #198274. For GNOME, we are hitting this on aarch64 as well. Let's use this bug for the first issue.
Eike Rathke
Comment 2 2019-06-17 04:56:52 PDT
Dumb question, is this fixed in webkitgtk-2.25.2?
Eike Rathke
Comment 3 2019-06-17 05:00:51 PDT
CI on master indicates it isn't.
Alberto Garcia
Comment 4 2019-06-18 01:12:32 PDT
(In reply to Eike Rathke from comment #2) > Dumb question, is this fixed in webkitgtk-2.25.2? It isn't, in Debian it fails in (at least) ppc64, ppc64el and s390x. https://buildd.debian.org/status/package.php?p=webkit2gtk&suite=experimental
Eike Rathke
Comment 5 2019-06-24 12:10:47 PDT
Created attachment 372782 [details] proposed patch It is unclear to me why in Source/JavaScriptCore/runtime/MatchResult.h the EncodedMatchResult should depend on CPU, but clearly with using EncodedMatchResult = uint64_t; and static_assert(sizeof(MatchResult) == sizeof(EncodedMatchResult)) and the two size_t member variables of MatchResult that can compile only if size_t is 32-bit and the member variables aligned. So the straight forward change would be to do exactly that, but I don't know if there is a reason to have CPU type involved here (because of JIT asm). Attached is a tad ugly patch which on ppc64le at least compiles. Does that suit the purpose? If yes, I can create a ready-to-commit patch.
Michael Catanzaro
Comment 6 2019-06-24 12:24:22 PDT
Comment on attachment 372782 [details] proposed patch You can set r? to request review and cq? to request commit-queue. But the r+ and cq+ flags are reserved for WebKit reviewers and committers.
Eike Rathke
Comment 7 2019-06-24 12:36:51 PDT
Comment on attachment 372782 [details] proposed patch Meh, misclicked, was supposed to be 'r?'
Michael Catanzaro
Comment 8 2019-06-24 12:54:04 PDT
Comment on attachment 372782 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=372782&action=review > Source/JavaScriptCore/runtime/MatchResult.h 2019-06-24 18:36:36.947345256 +0000:43 > #if CPU(ARM64) || CPU(X86_64) > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 1 > using EncodedMatchResult = MatchResult; > -#else > +#elif (defined(SIZE_WIDTH) && (SIZE_WIDTH == 32)) || (SIZE_MAX == 0xffffffff) > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 0 > using EncodedMatchResult = uint64_t; > +#else > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 1 > +using EncodedMatchResult = MatchResult; > #endif Hm, well I see this should fix the problem. Thanks. But it's really messy and honestly not great. Looking at the commit that introduced this build failure, r245586 "Cleanup Yarr regexp code around paren contexts," I see the code is rather complicated and I'm not confident in how to fix this nicely without reverting that change, but that commit was clearly incorrect. It assumes size_t is 32-bit except on x86_64 and aarch64, which is of course not right. The code only works if size_t happens to be the same as uint32_t, so I guess it should use uint32_t if that's what's desired. However, the constructor uses size_t and changing it to use uint32_t would require auditing a large number of callsites, so that should be left to JSC developers. I've just visited bug #198063 to ask Keith to comment here. Let's see what he thinks.
Michael Catanzaro
Comment 9 2019-06-24 12:55:22 PDT
Comment on attachment 372782 [details] proposed patch Thanks again. BTW, the patch should have a ChangeLog entry when setting r? which you can create with: Tools/Scripts/prepare-ChangeLog -b 198518 (If it's just a WIP patch without a changelog, don't set r? yet.)
Mark Lam
Comment 10 2019-06-24 13:03:20 PDT
Comment on attachment 372782 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=372782&action=review Also missing ChangeLog. > Source/JavaScriptCore/runtime/MatchResult.h 2019-06-24 18:36:36.947345256 +0000:43 > #if CPU(ARM64) || CPU(X86_64) > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 1 > using EncodedMatchResult = MatchResult; > -#else > +#elif (defined(SIZE_WIDTH) && (SIZE_WIDTH == 32)) || (SIZE_MAX == 0xffffffff) > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 0 > using EncodedMatchResult = uint64_t; > +#else > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 1 > +using EncodedMatchResult = MatchResult; > #endif This fix is invalid. The issue is that MatchResult contains 2 size_t values. This means, on 32-bit platforms, sizeof(MatchResult) is 64-bit, but on 64-bit platforms, sizeof(MatchResult) is 128-bit. The proper fix is simply: #if CPU(ADDRESS32) using EncodedMatchResult = uint64_t; static_assert(sizeof(size_t) == sizeof(uint32_t), ""); #else using EncodedMatchResult = MatchResult; #endif
Keith Miller
Comment 11 2019-06-24 13:13:37 PDT
(In reply to Mark Lam from comment #10) > Comment on attachment 372782 [details] > proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372782&action=review > > Also missing ChangeLog. > > > Source/JavaScriptCore/runtime/MatchResult.h 2019-06-24 18:36:36.947345256 +0000:43 > > #if CPU(ARM64) || CPU(X86_64) > > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 1 > > using EncodedMatchResult = MatchResult; > > -#else > > +#elif (defined(SIZE_WIDTH) && (SIZE_WIDTH == 32)) || (SIZE_MAX == 0xffffffff) > > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 0 > > using EncodedMatchResult = uint64_t; > > +#else > > +#define ENCODEDMATCHRESULT_IS_MATCHRESULT 1 > > +using EncodedMatchResult = MatchResult; > > #endif > > This fix is invalid. > > The issue is that MatchResult contains 2 size_t values. This means, on > 32-bit platforms, sizeof(MatchResult) is 64-bit, but on 64-bit platforms, > sizeof(MatchResult) is 128-bit. > > The proper fix is simply: > > #if CPU(ADDRESS32) > using EncodedMatchResult = uint64_t; > static_assert(sizeof(size_t) == sizeof(uint32_t), ""); > #else > using EncodedMatchResult = MatchResult; > #endif I think a better assertion would be static_assert(sizeof(EncodedMatchResult) == 2 * sizeof(size_t)), which should be true on all platforms. I like the rest of your change though. The point of the EncodedMatchResult type is to ensure that the C calling convention will return the result in the two return registers rather than on the stack.
Michael Catanzaro
Comment 12 2019-06-24 13:19:41 PDT
Thanks Mark, Keith! I was about to post my own solution using SIZE_MAX, but yours is much better. Eike, Berto, could you please test this patch and make sure it works on the architectures you care about?
Michael Catanzaro
Comment 13 2019-06-24 13:19:56 PDT
Michael Catanzaro
Comment 14 2019-06-24 13:24:41 PDT
Eike Rathke
Comment 15 2019-06-24 13:39:59 PDT
(In reply to Mark Lam from comment #10) > Also missing ChangeLog. Yes, it was a WIP patch. > The issue is that MatchResult contains 2 size_t values. This means, on > 32-bit platforms, sizeof(MatchResult) is 64-bit, but on 64-bit platforms, > sizeof(MatchResult) is 128-bit. > > The proper fix is simply: > > #if CPU(ADDRESS32) > using EncodedMatchResult = uint64_t; > static_assert(sizeof(size_t) == sizeof(uint32_t), ""); > #else > using EncodedMatchResult = MatchResult; > #endif I thought of such as well, but wasn't sure because of these odd Windows 64/32 platforms where at least a long still is only 32-bit. I'll try Michael's patch. (In reply to Keith Miller from comment #11) > The point of the EncodedMatchResult type is to ensure that the C calling > convention will return the result in the two return registers rather than on > the stack. Ah, thanks for enlightenment.
Eike Rathke
Comment 16 2019-06-24 14:00:51 PDT
That part compiled at least on ppc64le, I'll prepare a Fedora rawhide build of 2.25.2 with the patch applied to cover the other platforms as well.
Eike Rathke
Comment 17 2019-06-25 02:57:35 PDT
Ok, that did it. Thanks!
Alberto Garcia
Comment 18 2019-06-25 04:57:50 PDT
(In reply to Michael Catanzaro from comment #14) > Created attachment 372791 [details] > Patch I just built 2.25.2 on aarch64 with this patch.
Michael Catanzaro
Comment 19 2019-06-25 08:04:02 PDT
Comment on attachment 372791 [details] Patch Maybe Mark or Keith could set r+ if satisfied. Thanks again for the help!
WebKit Commit Bot
Comment 20 2019-06-25 09:08:33 PDT
Comment on attachment 372791 [details] Patch Clearing flags on attachment: 372791 Committed r246792: <https://trac.webkit.org/changeset/246792>
WebKit Commit Bot
Comment 21 2019-06-25 09:08:35 PDT
All reviewed patches have been landed. Closing bug.
Eike Rathke
Comment 22 2019-06-26 01:28:34 PDT
CI Jenkins builds are back to normal for ppc64, ppc64le and s390x.
Note You need to log in before you can comment on or make changes to this bug.