Bug 198518 - REGRESSION(r245586): static assertion failed: Match result and EncodedMatchResult should be the same size
Summary: REGRESSION(r245586): static assertion failed: Match result and EncodedMatchRe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-04 00:57 PDT by Alberto Garcia
Modified: 2019-06-26 01:28 PDT (History)
12 users (show)

See Also:


Attachments
proposed patch (1013 bytes, patch)
2019-06-24 12:10 PDT, Eike Rathke
no flags Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2019-06-24 13:19 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.10 KB, patch)
2019-06-24 13:24 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alberto Garcia 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);
Comment 1 Michael Catanzaro 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.
Comment 2 Eike Rathke 2019-06-17 04:56:52 PDT
Dumb question, is this fixed in webkitgtk-2.25.2?
Comment 3 Eike Rathke 2019-06-17 05:00:51 PDT
CI on master indicates it isn't.
Comment 4 Alberto Garcia 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
Comment 5 Eike Rathke 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.
Comment 6 Michael Catanzaro 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.
Comment 7 Eike Rathke 2019-06-24 12:36:51 PDT
Comment on attachment 372782 [details]
proposed patch

Meh, misclicked, was supposed to be 'r?'
Comment 8 Michael Catanzaro 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.
Comment 9 Michael Catanzaro 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.)
Comment 10 Mark Lam 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
Comment 11 Keith Miller 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.
Comment 12 Michael Catanzaro 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?
Comment 13 Michael Catanzaro 2019-06-24 13:19:56 PDT
Created attachment 372790 [details]
Patch
Comment 14 Michael Catanzaro 2019-06-24 13:24:41 PDT
Created attachment 372791 [details]
Patch
Comment 15 Eike Rathke 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.
Comment 16 Eike Rathke 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.
Comment 17 Eike Rathke 2019-06-25 02:57:35 PDT
Ok, that did it. Thanks!
Comment 18 Alberto Garcia 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.
Comment 19 Michael Catanzaro 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!
Comment 20 WebKit Commit Bot 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>
Comment 21 WebKit Commit Bot 2019-06-25 09:08:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Eike Rathke 2019-06-26 01:28:34 PDT
CI Jenkins builds are back to normal for ppc64, ppc64le and s390x.