Bug 180724 – Fill out some Poisoned APIs, fix some bugs, and add some tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Also rename Int32Poisoned to ConstExprPoisoned.  The key it takes is actually a uint32_t.  So, Int32 is really a misnomer.  In addition, the key needs to be a constexpr.  So, ConstExprPoisoned is a better name for it.

[Radar WebKit Bug Importer]

<rdar://problem/36006884>

[Mark Lam]

Created attachment 329169 [details]
proposed patch.

[JF Bastien]

Comment on attachment 329169 [details]
proposed patch.

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

r=me

> Source/WTF/wtf/Poisoned.h:55
> +struct isValidReference : std::integral_constant<bool, !isFunctionPointer<T>::value && !isVoidPointer<T>::value> { };

What's a "valid reference"? What makes it "valid"?

> Source/WTF/wtf/Poisoned.h:194
> +constexpr uintptr_t makePoison(uint32_t key)

inline?

[Mark Lam]

Comment on attachment 329169 [details]
proposed patch.

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

>> Source/WTF/wtf/Poisoned.h:55
>> +struct isValidReference : std::integral_constant<bool, !isFunctionPointer<T>::value && !isVoidPointer<T>::value> { };
> 
> What's a "valid reference"? What makes it "valid"?

I'll rename this to isConvertibleToReference, which is what I really meant.

>> Source/WTF/wtf/Poisoned.h:194
>> +constexpr uintptr_t makePoison(uint32_t key)
> 
> inline?

Thanks.  Fixed.

[Mark Lam]

Created attachment 329172 [details]
patch for landing + speculative build fixes.

[Mark Lam]

Created attachment 329182 [details]
patch for landing + speculative build fixes 2.

The build failures on the EWS bots are weird: it fails to specialize the PoisonedImpl template correctly for the Poisoned variant, but does it correctly for the ConstExprPoisoned variant.  I cannot reproduce this build failure locally, but the build failure error messages seem to imply that the EWS toolchain is confused with what the type of the poison key should be used in its inferrence.  I'm now uploading a patch that takes the linkage of the key out of the file where it's used to see if can kick the EWS toolchain into not inferring the wrong type.  Let's see if this makes a difference.

[Mark Lam]

Created attachment 329185 [details]
test to see if EWS toolchains support C++14.

[Mark Lam]

Created attachment 329187 [details]
test for EWS.

[Mark Lam]

Created attachment 329188 [details]
test for EWS.

[Mark Lam]

Created attachment 329189 [details]
test for EWS.

[Mark Lam]

Created attachment 329195 [details]
test for EWS.

[Mark Lam]

Created attachment 329197 [details]
test for EWS.

[Mark Lam]

I've managed to reproduce this build error on godbolt.org.  This is what godbolt.org says:

gcc 6.3 - Build fails
gcc 7.1 - Build fails
gcc 7.2 - Build passes

clang 3.9.1 - Build fails
clang 4.0.0 - Build passes

In all cases, I was using -std=c++14.

Hence, the issue is with the compilers and not my code.

That said, let me see if there's a way I can work around this compiler bug.

[JF Bastien]

Do you have a small repro from godbolt?

[Mark Lam]

Created attachment 329205 [details]
patch for landing.

I'm going to go with disabling the use of the Poisoned template methods that take a PoisonedImpl<K2, k, T2> argument in the tests that uses these methods with the uintptr_t& key type.  The compiler bug does not manifest when the methods are used with a constexpr uintptr_t key type.  In practice, we're likely to only assigned values which are poisoned with different keys when using the ConstExprPoisoned variant.  Hence, this compiler bug will not be an issue.

To make sure we don't break any builds, we only need to #if out the use of these methods in the API tests.  Since we don't use these methods for the Poisoned variant in real code, there's nothing more to do there.

I'll land this patch if the EWS bots are happy with it.

[Mark Lam]

Surprise surprise.  It looks like the old buggy compilers don't like assignment of Poisoned<keyA, DerivedClassPtr> to Poisoned<keyA, BaseClassPtr> either.  The only think it seems to support is assignment of Poisoned<keyA, SameClassPtr> to Poisoned<keyA, SameClassPtr>.  Again, this does not affect the ConstExprPoisoned variant.

I'll exclude these additional cases from the tests as well.

[Mark Lam]

Created attachment 329206 [details]
patch for landing.

[Mark Lam]

Created attachment 329207 [details]
patch for landing.

[Mark Lam]

Created attachment 329212 [details]
patch for landing.

Looks like the EWS thinks it's using at least clang 4, but is still failing to build.  Let's try filtering ENABLE_MIXED_POISON on clang 5 instead.

[Mark Lam]

Created attachment 329213 [details]
patch for landing.

Looks like Apple's versioning of clang is different than the open source one.  Here's a patch based with a speculative fix based on that versioning difference.

[Mark Lam]

Created attachment 329229 [details]
patch for landing.

[EWS Watchlist]

Attachment 329229 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Poisoned.h:48:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

The bots are green.  I'm going to land this.

[Mark Lam]

Thanks for the review.  Landed in r225857: <http://trac.webkit.org/r225857>.