[webkit-reviews] review denied: [Bug 212734] Port DFGDoesGCCheck to 32 bits : [Attachment 401022] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Paulo Matos <pmatos at igalia.com>'s
request for review:
Bug 212734: Port DFGDoesGCCheck to 32 bits
https://bugs.webkit.org/show_bug.cgi?id=212734

Attachment 401022: Patch

https://bugs.webkit.org/attachment.cgi?id=401022&action=review




--- Comment #12 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 401022
  --> https://bugs.webkit.org/attachment.cgi?id=401022
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401022&action=review

There is some value in this patch, but the 32-bit encoding is just wrong and
won't work.  As I said before, I already have a patch brewing that will solve
the 32-bit encoding problem.  Since the build is no longer breaking, can you
wait for my patch?

> Source/JavaScriptCore/ChangeLog:10
> +	   This change implements a port of the initial patch for 32bits. As it
stands, 32bits ports
> +	   are not compiling.

It's no longer true the 32-bits ports are not compiling.  Can you remove this?

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:50
> +    static size_t encode(bool expectDoesGC, unsigned nodeIndex, unsigned
nodeOp)

size_t is the wrong type to use here.  This can break the build for the
arm64_32 port.	I recommend that you add the following at the top of this
struct:

#if USE(JSVALUE64)
using EncodedWord = uint64_t;
#else
using EncodedWord = uint32_t;
#endif

... and use EncodedWord here instead of size_t.

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:67
> -
> +    

please undo this.

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:114
> +    static constexpr unsigned specialShift = 1;
> +    static constexpr unsigned nodeOpShift = 1;

This is wrong.	Based on the present encoding, the special section will collide
with the nodeOp section, and you will get non-sensical results from the doesGC
error messages.