Bug 193523 – Gigacages should start allocations from a slide

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Gigacages should start allocations from a slide

[Keith Miller]

Created attachment 359341 [details]
Patch

[Keith Miller]

rdar://problem/44958707

[Geoffrey Garen]

Comment on attachment 359341 [details]
Patch

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

r- because red bots, also a suggested improvement

> Source/bmalloc/bmalloc/Heap.cpp:68
> +            ptrdiff_t offset = ((random * vmPageSize()) % (gigacageSize() - Gigacage::minimumCageSizeAfterSlide));

This seems wrong. You're losing low bits of entropy by multiplying.

The best way to do this is probably to compute a random offset, allowing for minimumCageSizeAfterSlide, and then round down to page alignment at the very end. That way, you don't need an ASSERT about page alignment either. You can trade an assert for code that enforces the condition you want.

[Yusuke Suzuki]

Comment on attachment 359341 [details]
Patch

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

> Source/WTF/wtf/Gigacage.h:37
> +const size_t primitiveGigacageMask = gigacageSizeToMask(primitiveGigacageSize);
> +const size_t jsValueGigacageMask = gigacageSizeToMask(jsValueGigacageSize);
> +const size_t gigacageBasePtrsSize = 8 * 1024;

`constexpr size_t` would be nice. And `8 * Sizes::KB` would be more readable.

> Source/bmalloc/bmalloc/Gigacage.cpp:45
> +const size_t gigacageRunway = 32llu * 1024 * 1024 * 1024;

Ditto for `constexpr size_t`. And I think `32 * Sizes::GB` would be more readable.

> Source/bmalloc/bmalloc/Gigacage.h:43
> +const size_t primitiveGigacageSize = 0x80000000;
> +const size_t jsValueGigacageSize = 0x40000000;

It seems that primitiveGigacageSize is 2GB, and jsValueGigacageSize is 1GB. So, if minimumCageSizeAfterSlide is 2GB, it goes to the some funny state. Is my understanding correct?
And I think we can use `2 * bmalloc::Sizes::GB` nad `1 * bmalloc::Sizes::GB` here.

> Source/bmalloc/bmalloc/Gigacage.h:45
> +const size_t minimumCageSizeAfterSlide = 2 * Sizes::GB;

Ditto for `constexpr size_t`.

> Source/bmalloc/bmalloc/Gigacage.h:49
> +const size_t primitiveGigacageSize = 0x800000000;
> +const size_t jsValueGigacageSize = 0x400000000;

This is causing build failure in 32bit arch since size_t is 32bit while the value is larger than 32bit, while gigacage is not used in 32bit anyway.

> Source/bmalloc/bmalloc/Gigacage.h:51
> +const size_t minimumCageSizeAfterSlide = 4 * bmalloc::Sizes::GB;

Ditto for `constexpr size_t`.

> Source/bmalloc/bmalloc/Gigacage.h:68
> +const size_t primitiveGigacageMask = gigacageSizeToMask(primitiveGigacageSize);
> +const size_t jsValueGigacageMask = gigacageSizeToMask(jsValueGigacageSize);

Ditto for `constexpr size_t`.

> Source/bmalloc/bmalloc/Sizes.h:48
>      static const size_t kB = 1024;
>      static const size_t MB = kB * kB;
> +    static const size_t GB = kB * kB * kB;
>  
>      static const size_t alignment = 8;
>      static const size_t alignmentMask = alignment - 1ul;

`static constexpr` would be nice.

[Keith Miller]

Comment on attachment 359341 [details]
Patch

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

>> Source/WTF/wtf/Gigacage.h:37
>> +const size_t gigacageBasePtrsSize = 8 * 1024;
> 
> `constexpr size_t` would be nice. And `8 * Sizes::KB` would be more readable.

Is there an advantage of constexpr over const? I can make the change though.

>> Source/bmalloc/bmalloc/Gigacage.cpp:45
>> +const size_t gigacageRunway = 32llu * 1024 * 1024 * 1024;
> 
> Ditto for `constexpr size_t`. And I think `32 * Sizes::GB` would be more readable.

Sure.

>> Source/bmalloc/bmalloc/Gigacage.h:43
>> +const size_t jsValueGigacageSize = 0x40000000;
> 
> It seems that primitiveGigacageSize is 2GB, and jsValueGigacageSize is 1GB. So, if minimumCageSizeAfterSlide is 2GB, it goes to the some funny state. Is my understanding correct?
> And I think we can use `2 * bmalloc::Sizes::GB` nad `1 * bmalloc::Sizes::GB` here.

Good catch! it appears that I messed up the math. I thought it was 4GB and 2GB... Whoops!

>> Source/bmalloc/bmalloc/Gigacage.h:49
>> +const size_t jsValueGigacageSize = 0x400000000;
> 
> This is causing build failure in 32bit arch since size_t is 32bit while the value is larger than 32bit, while gigacage is not used in 32bit anyway.

Yeah, I'll fix that.

>> Source/bmalloc/bmalloc/Heap.cpp:68
>> +            ptrdiff_t offset = ((random * vmPageSize()) % (gigacageSize() - Gigacage::minimumCageSizeAfterSlide));
> 
> This seems wrong. You're losing low bits of entropy by multiplying.
> 
> The best way to do this is probably to compute a random offset, allowing for minimumCageSizeAfterSlide, and then round down to page alignment at the very end. That way, you don't need an ASSERT about page alignment either. You can trade an assert for code that enforces the condition you want.

Yeah, I was thinking that it would be the same as rounding down. But I think you are right since gigacageSize() - Gigacage::minimumCageSizeAfterSlide may not be a power of 2.

[Yusuke Suzuki]

Comment on attachment 359341 [details]
Patch

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

>>> Source/WTF/wtf/Gigacage.h:37
>>> +const size_t gigacageBasePtrsSize = 8 * 1024;
>> 
>> `constexpr size_t` would be nice. And `8 * Sizes::KB` would be more readable.
> 
> Is there an advantage of constexpr over const? I can make the change though.

In C++14, they are basically the same.
But in C++17, constexpr+static member variable will become `inline` variable. It means that, finally, we can ensure that constant variables are compiled as compile time constant, instead of the reference to the constant variable.
This prevents us from "undefined reference to the variable XXX" error (and sometimes, we used `+variable` hack like https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/b3/B3Common.cpp#L71).

While `inline` will be required for these namespace variables in C++17, I like using `constexpr` for all constant variables basically.

[Keith Miller]

Created attachment 359396 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


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

[Keith Miller]

Created attachment 359409 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


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

[Keith Miller]

Created attachment 359410 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


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

[Keith Miller]

Created attachment 359425 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


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

[Keith Miller]

Created attachment 359430 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


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

[Mark Lam]

Comment on attachment 359430 [details]
Patch

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

> Source/bmalloc/bmalloc/Sizes.h:126
>  }

nit: can you add a // namespace Sizes here?

[Mark Lam]

Comment on attachment 359430 [details]
Patch

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

r=me with suggestions and fixes.

> Source/bmalloc/ChangeLog:8
> +        This patch makes is so that Gigacage Heaps slide the start of the

typo: /makes is so/makes it so/.

> Source/bmalloc/bmalloc/Gigacage.cpp:47
> +constexpr uint64_t gigacageRunway = 32llu * 1024 * 1024 * 1024;

Why not just use a size_t gigacageRunway to begin with?

> Source/bmalloc/bmalloc/Gigacage.cpp:112
> +        return static_cast<size_t>(gigacageRunway);

You can remove this cast since we can define gigacageRunway as a size_t to begin with.

> Source/bmalloc/bmalloc/Heap.cpp:67
> +            RELEASE_BASSERT(gigacageSize() > Gigacage::minimumCageSizeAfterSlide);

Can you just make this a static_assert in Source/bmalloc/bmalloc/Gigacage.h instead?

    static_assert(primitiveGigacageSize > minimumCageSizeAfterSlide);
    static_assert(jsValueGigacageSize > minimumCageSizeAfterSlide);

That way, we can catch this configuration error at compile time instead.

[Guillaume Emont]

Comment on attachment 359430 [details]
Patch

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

We see a lot of crashes on arm/linux (where Gigacage is disabled) with this patch. Doing the changes I suggest below seem to fix them all.

> Source/bmalloc/bmalloc/Gigacage.h:218
> +BINLINE bool isCaged(Kind, const void*) { return false; }

We have a few cases of:
  RELEASE_ASSERT(isCaged(kind, p));
in wtf/Gigacage.cpp as well as similar code in wtf/JSValueMalloc.cpp. I believe these should be modified to something like:
  RELEASE_ASSERT(!wasEnabled() || isCaged(kind, p));

What do you think?

> Source/bmalloc/bmalloc/Gigacage.h:219
> +template<typename T> BINLINE T* caged(Kind, T* ptr) { BCRASH(); return ptr; }

This BCRASH() triggers a lot of crashes on arm/linux, and removing it seems to have no ill effect, so I feel like it should be removed, unless there is a strong reason to keep it. The alternative to removing it would be to modify all the code that calls caged() to have an alternative case if Gigacage is disabled.

[Keith Miller]

Created attachment 359501 [details]
Patch for landing

[Keith Miller]

Comment on attachment 359430 [details]
Patch

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

>> Source/bmalloc/bmalloc/Gigacage.cpp:47
>> +constexpr uint64_t gigacageRunway = 32llu * 1024 * 1024 * 1024;
> 
> Why not just use a size_t gigacageRunway to begin with?

I changed it I think because this originally needed to compile on 32-bit. Since it doesn't anymore I can change it back.

>> Source/bmalloc/bmalloc/Gigacage.cpp:112
>> +        return static_cast<size_t>(gigacageRunway);
> 
> You can remove this cast since we can define gigacageRunway as a size_t to begin with.

Done.

>> Source/bmalloc/bmalloc/Gigacage.h:218
>> +BINLINE bool isCaged(Kind, const void*) { return false; }
> 
> We have a few cases of:
>   RELEASE_ASSERT(isCaged(kind, p));
> in wtf/Gigacage.cpp as well as similar code in wtf/JSValueMalloc.cpp. I believe these should be modified to something like:
>   RELEASE_ASSERT(!wasEnabled() || isCaged(kind, p));
> 
> What do you think?

I'll just have this always return true if caging is disabled.

>> Source/bmalloc/bmalloc/Gigacage.h:219
>> +template<typename T> BINLINE T* caged(Kind, T* ptr) { BCRASH(); return ptr; }
> 
> This BCRASH() triggers a lot of crashes on arm/linux, and removing it seems to have no ill effect, so I feel like it should be removed, unless there is a strong reason to keep it. The alternative to removing it would be to modify all the code that calls caged() to have an alternative case if Gigacage is disabled.

Fair enough, I'll remove the assert.

>> Source/bmalloc/bmalloc/Heap.cpp:67
>> +            RELEASE_BASSERT(gigacageSize() > Gigacage::minimumCageSizeAfterSlide);
> 
> Can you just make this a static_assert in Source/bmalloc/bmalloc/Gigacage.h instead?
> 
>     static_assert(primitiveGigacageSize > minimumCageSizeAfterSlide);
>     static_assert(jsValueGigacageSize > minimumCageSizeAfterSlide);
> 
> That way, we can catch this configuration error at compile time instead.

Sure.

>> Source/bmalloc/bmalloc/Sizes.h:126
>>  }
> 
> nit: can you add a // namespace Sizes here?

Done.

[Mark Lam]

(In reply to Keith Miller from comment #21)
> >> Source/bmalloc/bmalloc/Gigacage.h:218
> >> +BINLINE bool isCaged(Kind, const void*) { return false; }
> > 
> > We have a few cases of:
> >   RELEASE_ASSERT(isCaged(kind, p));
> > in wtf/Gigacage.cpp as well as similar code in wtf/JSValueMalloc.cpp. I believe these should be modified to something like:
> >   RELEASE_ASSERT(!wasEnabled() || isCaged(kind, p));
> > 
> > What do you think?
> 
> I'll just have this always return true if caging is disabled.

I was going to disagree with this because I think that it's misleading for isCaged() to return true all the time.  I was thinking that explicitly adding the !wasEnabled() check in the RELEASE_ASSERTs would be clearer about what is happening.

However, I see that the GIGACAGE_ENABLED version of caged() effectively always returns the ptr if !wasEnabled(), and isCaged() always returns true if caged(kind, ptr) == ptr.  Hence, if !GIGACAGE_ENABLED, this reduces to isCaged() always returning true.

I withdraw my objection.  Sorry for the noise.

[WebKit Commit Bot]

Comment on attachment 359501 [details]
Patch for landing

Clearing flags on attachment: 359501

Committed r240160: <https://trac.webkit.org/changeset/240160>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to WebKit Commit Bot from comment #23)
> Comment on attachment 359501 [details]
> Patch for landing
> 
> Clearing flags on attachment: 359501
> 
> Committed r240160: <https://trac.webkit.org/changeset/240160>

This appears to have broken the internal tvOS build.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #25)
> (In reply to WebKit Commit Bot from comment #23)
> > Comment on attachment 359501 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 359501
> > 
> > Committed r240160: <https://trac.webkit.org/changeset/240160>
> 
> This appears to have broken the internal tvOS build.

I have a fix.

[Matt Lewis]

Reverted r240160 for reason:

This broke multiple internal builds.

Committed r240171: <https://trac.webkit.org/changeset/240171>

[David Kilzer (:ddkilzer)]

Created attachment 359532 [details]
Patch to fix the builds when re-landing

This should have fixed the builds had r240160 not been rolled out.

[Keith Miller]

Committed r240175: <https://trac.webkit.org/changeset/240175>

[David Kilzer (:ddkilzer)]

(In reply to Keith Miller from comment #29)
> Committed r240175: <https://trac.webkit.org/changeset/240175>

Follow-up fix when GIGACAGE_ENABLED is 0:

Committed r240186: <https://trac.webkit.org/changeset/240186>