Bug 194912 – Make JSScript:cacheBytecodeWithError update the cache when the script changes

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

gonna look at this now.

[Saam Barati]

Created attachment 362778 [details]
WIP

[Saam Barati]

Created attachment 362785 [details]
patch

[Tadeu Zagallo]

Comment on attachment 362785 [details]
patch

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

Looks good to me. I just have a couple questions that are not directly related to the patch, and the windows build is broken.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:221
> +        , m_baseAddress(bitwise_cast<const uint8_t*>(baseAddress))

This should probably be `static_cast`, no?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:338
> +        return bitwise_cast<const uint8_t*>(this) + m_offset;

why do you need bitwise_cast here instead of reinterpret_cast?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:2164
> +    if (m_cacheVersion != JSC_BYTECODE_CACHE_VERSION)
> +        return false;
> +    if (m_bootSessionUUID.decode(decoder) != bootSessionUUIDString())
> +        return false;

Do you think it's worth moving it into a helper method, since it's done here and in decode?

[Saam Barati]

Comment on attachment 362785 [details]
patch

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

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:221
>> +        , m_baseAddress(bitwise_cast<const uint8_t*>(baseAddress))
> 
> This should probably be `static_cast`, no?

yeah it should be

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:338
>> +        return bitwise_cast<const uint8_t*>(this) + m_offset;
> 
> why do you need bitwise_cast here instead of reinterpret_cast?

I like bitwise_cast because of the static assert it does that the from type should be the same size as the to type.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:2164
>> +        return false;
> 
> Do you think it's worth moving it into a helper method, since it's done here and in decode?

Yeah I do.

[Mark Lam]

Comment on attachment 362785 [details]
patch

LGTM

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/242239/webkit

[Radar WebKit Bug Importer]

<rdar://problem/48492608>

[Saam Barati]

Attempting Windows build fix in:
https://trac.webkit.org/changeset/242247/webkit

[Dominik Inführ]

Hi, I think that patch broke debug builds with gcc: In Source/JavaScriptCore/runtime/CachedTypes.cpp ASSERT_NOT_REACHED (a non-constexpr function) is invoked from the constexpr-function tagFromSourceCodeType.

See https://trac.webkit.org/browser/webkit/trunk/Source/JavaScriptCore/runtime/CachedTypes.cpp#L1827

[Dominik Inführ]

Additional note: With "broken debug build" I meant that compilation is failing on (probably) all architectures (I tested x64 & ARM).

[Saam Barati]

(In reply to Dominik Inführ from comment #11)
> Additional note: With "broken debug build" I meant that compilation is
> failing on (probably) all architectures (I tested x64 & ARM).

Can you land a fix to remove constexpr from it? FWIW, clang debug builds I think are fine

[Dominik Inführ]

Should we use ASSERT_UNDER_CONSTEXPR_CONTEXT instead?

[Dominik Inführ]

Nevermind this isn't building as well...

[Dominik Inführ]

Here is the patch for the build issue: https://bugs.webkit.org/show_bug.cgi?id=195205.

[Dominik Inführ]

BTW thanks for responding so fast!