Bug 198518 – REGRESSION(r245586): static assertion failed: Match result and EncodedMatchResult should be the same size

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alberto Garcia]

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

[Michael Catanzaro]

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

Dumb question, is this fixed in webkitgtk-2.25.2?

[Eike Rathke]

CI on master indicates it isn't.

[Alberto Garcia]

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

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 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 on attachment 372782 [details]
proposed patch

Meh, misclicked, was supposed to be 'r?'

[Michael Catanzaro]

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

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

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]

Created attachment 372790 [details]
Patch

[Michael Catanzaro]

Created attachment 372791 [details]
Patch

[Eike Rathke]

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

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]

Ok, that did it. Thanks!

[Alberto Garcia]

(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 on attachment 372791 [details]
Patch

Maybe Mark or Keith could set r+ if satisfied. Thanks again for the help!

[WebKit Commit Bot]

Comment on attachment 372791 [details]
Patch

Clearing flags on attachment: 372791

Committed r246792: <https://trac.webkit.org/changeset/246792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eike Rathke]

CI Jenkins builds are back to normal for ppc64, ppc64le and s390x.