Bug 178108 – bmalloc should support strictly type-segregated isolated heaps

• Component: bmalloc
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 323267 [details]
it's a start

[Filip Pizlo]

Created attachment 323546 [details]
a bit more

[Filip Pizlo]

Created attachment 323571 [details]
I think I know what it will look like now

[Filip Pizlo]

Created attachment 323597 [details]
a bit more

[Filip Pizlo]

Created attachment 323630 [details]
more!

[Filip Pizlo]

Created attachment 323728 [details]
it's looking pretty serious

[Filip Pizlo]

The basic architecture:

- Each heap is type-specialized for a size and instantiated on a per-type basis.
- Use type-specialization to fold as much size-oriented math as possible.
- Super dedicated first-fit discipline, so that we can maximize the amount of pages scavenged as well as their contiguity.
- Bump'n'pop allocator in each page. Bump'n'pop increases our ability to do first-fit, which increases our ability to free pages.
- Riptide-style bitvector tracking of pages.
- Bmalloc-style logging of deallocated objects. This amortizes lock acquisition.
- Single lock around each isoheap, which is good enough, since we acquire locks only on slow paths.
- Small isoheaps will only need to allocate one page (meta-data will be 100s of bytes stored in a bmalloc'd chunk).
- No limit on isoheap size. We do a unode-style expansion of the heap to be a linked list of "page directories" that allow for arbitrary sized heaps. The directories are bmalloced.

[Filip Pizlo]

Created attachment 323745 [details]
it's almost done

[Filip Pizlo]

Created attachment 323789 [details]
even more

I figured out an opportunity to make things a lot better in IsoTLS.  I'll do that, and then I think I'll be done.

[Filip Pizlo]

Created attachment 323817 [details]
a bit more

[Filip Pizlo]

Created attachment 323829 [details]
I think it's written

[Filip Pizlo]

Created attachment 323849 [details]
fixed up scavenger integration

[Filip Pizlo]

Created attachment 323913 [details]
wrote a test

Still haven't compiled it yet though.  I figured it would be pointless unless something included the *Inlines.h files.  So, I starting working on a test suite.

[Filip Pizlo]

Created attachment 323931 [details]
starting to fix compile errors

[Filip Pizlo]

Created attachment 323940 [details]
closer to building

[Filip Pizlo]

Created attachment 323945 [details]
it compiles!

[Filip Pizlo]

Created attachment 323950 [details]
it passes a test!

[Filip Pizlo]

Created attachment 323983 [details]
scavenging seems to work!

[Filip Pizlo]

Created attachment 325470 [details]
passes easy tests, now expanding to renderobject

[Filip Pizlo]

Created attachment 325481 [details]
more

[Filip Pizlo]

Created attachment 325486 [details]
more!

[Filip Pizlo]

Created attachment 325540 [details]
it is written

[Filip Pizlo]

A really easy way of making this more efficient is to support the idea of an IsoHeap allocating objects in an IsoPage of some other IsoHeap. When this happens, we would somehow make freeing that object only free it back to the IsoHeap that allocated it. This would mean that an IsoPage would have to be able to enter a multi-heap state, where it separately manages bits for each heap that owns it, and knows its separate state in each heap. We can only free it when it's free in all heaps.

Seems doable but super hard.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #23)
> A really easy way of making this more efficient is to support the idea of an
> IsoHeap allocating objects in an IsoPage of some other IsoHeap. When this
> happens, we would somehow make freeing that object only free it back to the
> IsoHeap that allocated it. This would mean that an IsoPage would have to be
> able to enter a multi-heap state, where it separately manages bits for each
> heap that owns it, and knows its separate state in each heap. We can only
> free it when it's free in all heaps.
> 
> Seems doable but super hard.

Probably, at the time of allocation, we would flag the allocated object as being from another heap.  Then the IsoPage would continue to "belong" to one heap but would allocate some rare data for maintaining separate states for other heaps.

We'd have to appropriately throttle use of this feature.  For example, it's probably only practical for heaps that don't yet have their own pages.  Maybe it's also only practical to steal in this way from IsoHeaps that only have one page.

Or, maybe IsoHeaps would use special shared pages until those shared pages fill up.

This seems hard in two ways - the actual implementation and the heuristics it requires.

[Filip Pizlo]

CC'd some people who know more about rendering.

IsoHeaps are a security feature that cause each ISO_ALLOCATED type to have its own isolated heap at the virtual memory level (IsoHeaps can trade physical pages, but once a virtual memory address is associated with an object of a type, only other objects of exactly that type will appear at that address in the future).  In order to mitigate the perf overhead of managing multiple heaps, IsoHeap is type-specialized so that all math having to do with the size and alignment of the types it allocates is constant-folded.  Also, this ensures that we can always rapidly find the right IsoHeap allocator for any type.

Please take a look at the WebCore part of this change.  The flipside of those optimizations is that each ISO_ALLOCATED type needs two annotations:

- Inside the class:
    WTF_MAKE_ISO_ALLOCATED(ClassType);

- In the .cpp file for that class:
    WTF_MAKE_ISO_ALLOCATED_IMPL(ClassType);

Alternatively, if you're willing to #include <wtf/IsoMallocInlines.h>, then you can just do this inside the class:
    WTF_MAKE_ISO_ALLOCATED_INLINE(ClassType)

I recommend the latter for classes declared in .cpp files.  I don't recommend #including <wtf/IsoMallocInlines.h> in .h files.

You have to do this dance for each type in a type hierarchy, because there is no way (that I thought of) to create new overrides in the base class that automagically instantiate a separate IsoHeap per type.  I hope that's only annoying for me - the person refactoring code to use IsoHeap - and not everyone after.  Hopefully, it's not hard to remember to add those ISO_ALLOCATED annotations to new classes.  If you don't do it, then you will RELEASE_ASSERT on first allocation, unless the subclass happens to have the same size, in which case it'll just work.

[Simon Fraser (smfr)]

(In reply to Filip Pizlo from comment #25)

> IsoHeaps are a security feature that cause each ISO_ALLOCATED type to have
> its own isolated heap at the virtual memory level (IsoHeaps can trade
> physical pages, but once a virtual memory address is associated with an
> object of a type, only other objects of exactly that type will appear at
> that address in the future).

This sounds like it will make it more likely that objects of the same type will tend to re-use addresses more often than before, which could lead to more predictable use-after-free behavior. Is that a concern?

[Filip Pizlo]

(In reply to Simon Fraser (smfr) from comment #26)
> (In reply to Filip Pizlo from comment #25)
> 
> > IsoHeaps are a security feature that cause each ISO_ALLOCATED type to have
> > its own isolated heap at the virtual memory level (IsoHeaps can trade
> > physical pages, but once a virtual memory address is associated with an
> > object of a type, only other objects of exactly that type will appear at
> > that address in the future).
> 
> This sounds like it will make it more likely that objects of the same type
> will tend to re-use addresses more often than before, which could lead to
> more predictable use-after-free behavior. Is that a concern?

No for two reasons.

First, the worst use-after-free's are the ones that lead to type confusion.  If we can only alias objects of the same type with UAF then it doesn't give you a path to RCE.  It could still be used for UXSS.

Second, attackers are good at making bmalloc or any other malloc allocate objects predictably.  IsoHeaps don't make that fundamentally easier (it was feasible before and is still feasible now).

[Simon Fraser (smfr)]

(In reply to Filip Pizlo from comment #27)

> No for two reasons.
> 
> First, the worst use-after-free's are the ones that lead to type confusion. 
> If we can only alias objects of the same type with UAF then it doesn't give
> you a path to RCE.  It could still be used for UXSS.
> 
> Second, attackers are good at making bmalloc or any other malloc allocate
> objects predictably.  IsoHeaps don't make that fundamentally easier (it was
> feasible before and is still feasible now).

Cool.

Your macros sound fine. Shame we can't detect lack of the macros on a subclass at compile time, though.

[Filip Pizlo]

Created attachment 325747 [details]
it compiles!

[Filip Pizlo]

(In reply to Filip Pizlo from comment #29)
> Created attachment 325747 [details]
> it compiles!

And it seems to work.  Safari started just fine.

Now to test it more...

[Geoffrey Garen]

> - Super dedicated first-fit discipline, so that we can maximize the amount
> of pages scavenged as well as their contiguity.
> - Bump'n'pop allocator in each page. Bump'n'pop increases our ability to do
> first-fit, which increases our ability to free pages.

Tbis is a bummer, since it undoes the optimization provided by bmalloc. I'd really like to avoid doing this unless we meet a high burden of proof that it's required.

[Filip Pizlo]

(In reply to Geoffrey Garen from comment #31)
> > - Super dedicated first-fit discipline, so that we can maximize the amount
> > of pages scavenged as well as their contiguity.
> > - Bump'n'pop allocator in each page. Bump'n'pop increases our ability to do
> > first-fit, which increases our ability to free pages.
> 
> Tbis is a bummer, since it undoes the optimization provided by bmalloc. I'd
> really like to avoid doing this unless we meet a high burden of proof that
> it's required.

It wouldn't matter for the code.  It's not possible to literally reuse SmallPage etc because of other constraints.  So, if I used immix-style allocation, then I'd have to implement it a second time.  Since I'm more familiar with the riptide-style, I just went with that.

I think that the immix-style is probably better for perf.  I was going to test that in a follow-up.  It's not in this patch only because I wanted to get something to work quickly.

[Filip Pizlo]

Created attachment 325807 [details]
fixed a bug

I can now browse twitter.

[Filip Pizlo]

Created attachment 325823 [details]
it sorta works

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:102:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:131:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:146:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:56:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


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

[Filip Pizlo]

Created attachment 325844 [details]
fix some builds

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:56:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


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

[Filip Pizlo]

Created attachment 325897 [details]
more build fixing

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:56:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


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

[Filip Pizlo]

Created attachment 325960 [details]
more fixes

This seems to make a lot of tests pass.

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:60:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 284 files


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

[Build Bot]

Comment on attachment 325960 [details]
more fixes

Attachment 325960 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5095397

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 325968 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Created attachment 326003 [details]
the patch

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:60:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 28 in 285 files


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

[Filip Pizlo]

Created attachment 326007 [details]
the patch

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


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

[Filip Pizlo]

Created attachment 326008 [details]
the patch

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:47:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:50:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:64:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


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

[Filip Pizlo]

Created attachment 326010 [details]
the patch

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:48:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:51:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:65:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:230:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:283:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


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

[Filip Pizlo]

Created attachment 326012 [details]
the patch

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:48:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:51:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:65:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:231:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:284:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


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

[Ryosuke Niwa]

WebCore changes look fine to me. I just wish we didn't have to add _IMPL macro to cpp files but I guess including IsoMallocInlines.h would regress the build time?

[Filip Pizlo]

(In reply to Ryosuke Niwa from comment #54)
> WebCore changes look fine to me. I just wish we didn't have to add _IMPL
> macro to cpp files but I guess including IsoMallocInlines.h would regress
> the build time?

For now the #include situation also makes it very easy to hack on the isoheap implementation.  It's very fast to recompile webcore since only a small number of unified sources end up including IsoMallocInlines.

[Filip Pizlo]

PLT, motionmark, and membuster are all neutral.

[Filip Pizlo]

Created attachment 326049 [details]
the patch

Just updating changelogs to include new perf results.

It's neutral on everything I tried (PLT, membuster, motionmark).

[Build Bot]

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


ERROR: Source/bmalloc/test/testbmalloc.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/test/testbmalloc.cpp:104:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:133:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/test/testbmalloc.cpp:148:  Consider using CHECK_GE instead of CHECK(a >= b)  [readability/check] [2]
ERROR: Source/bmalloc/bmalloc/IsoAllocatorInlines.h:78:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/DeferredDecommit.h:35:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/FreeList.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/AllIsoHeaps.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLS.cpp:61:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/rendering/svg/RenderSVGResourcePattern.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:48:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:51:  The parameter name "directory" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoPage.h:65:  The parameter name "freeList" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Bits.h:231:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/Bits.h:284:  Missing spaces around |  [whitespace/operators] [3]
ERROR: Source/WebCore/rendering/RenderTableSection.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntryInlines.h:59:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:53:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/bmalloc.h:87:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapInlines.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoHeapImpl.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.h:38:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLS.h:77:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/IsoTLSLayout.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/bmalloc/bmalloc/IsoTLSEntry.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderImageResource.h:39:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 33 in 286 files


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

[Saam Barati]

Comment on attachment 326049 [details]
the patch

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

r=me
I have a few comments and a couple questions.

> Source/bmalloc/ChangeLog:18
> +        pages. Pages are corrected into directories. Directories track pages using bitvectors, so

corrected => collected

> Source/bmalloc/bmalloc/Bits.h:1
> +/*

It's a bit unfortunate that this is probably almost identical to WTF's implementation.

> Source/bmalloc/bmalloc/Bits.h:496
> +        return BitReference(&this->m_words.word(index >> 5), 1 << (index & 31));

WHy 5?

> Source/bmalloc/bmalloc/DeferredDecommit.h:45
> +
> +
> +    

unneeded whitespace

> Source/bmalloc/bmalloc/IsoDirectory.h:37
> +class IsoDirectoryBaseBase {

BaseBase :(

> Source/bmalloc/bmalloc/IsoDirectory.h:42
> +    virtual void didDecommit(unsigned index) = 0;

Do we really need this to be virtual? There is only one implementation.

> Source/bmalloc/bmalloc/IsoDirectory.h:69
> +    void didBecome(IsoPage<Config>*, IsoPageTrigger) override;

This only has one implementation. Do we need it to be virtual?

> Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49
> +    unsigned pageIndex = (m_eligible | ~m_committed).findBit(m_firstEligible, true);;

style: extra semicolon at the end

> Source/bmalloc/bmalloc/IsoHeapImpl.h:67
> +    void didBecomeEligible(IsoDirectory<Config, numPagesInInlineDirectory>*); // This one probably needs to set a bit or something.
> +    void didBecomeEligible(IsoDirectory<Config, IsoDirectoryPage<Config>::numPages>*); // This one needs to do things.

these comments are not helpful

> Source/bmalloc/bmalloc/IsoHeapImpl.h:96
> +    // The basic operations that we need to support are:

Part of this text seems to have been written before you implemented things. Maybe you can clean some of it up now that you've made implementation decisions?

> Source/bmalloc/bmalloc/IsoHeapImpl.h:121
> +    // - Deallocation should probably use a deallocation log, just like the rest of bmalloc.
> +    //   -> This avoids having to use any kinds of atomics on the freeing fast path. The slow path can
> +    //      just grab a lock around the whole isoheap or something.

This makes things seem up in the air, but are they?

> Source/bmalloc/bmalloc/IsoHeapImpl.h:122
> +    // - When we create an isoheap, we want to make sure that TLCs find out.

TLC?

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:46
> +        if (result.kind == EligibilityKind::Full)

Do we want to assert it's not OOM kind?

> Source/bmalloc/bmalloc/IsoHeapImplInlines.h:62
> +        if (result.kind != EligibilityKind::Full)

ditto w.r.t OOM?

> Source/bmalloc/bmalloc/IsoPage.h:40
> +    static constexpr size_t pageSize = 16384;

Do we want this to be 4096 on iOS? Doesn't bmalloc already have a notion of this somewhere?

> Source/bmalloc/bmalloc/IsoPage.h:46
> +    static constexpr unsigned numObjects = pageSize / Config::objectSize;

Do we want to add static_assert(numObjects > 0)?

> Source/bmalloc/bmalloc/IsoPage.h:55
> +    // This must have a trivial destructor.

Nit: Maybe it's worth putting this comment by the field declarations?

> Source/bmalloc/bmalloc/IsoPageInlines.h:104
> +        unsigned beginWord = begin >> 5;
> +        unsigned endWord = end >> 5;

Maybe we can give this 5 a name? And also static cast that (sizeof(unsigned) * 8 == 1<<5)?

> Source/bmalloc/bmalloc/IsoPageInlines.h:124
> +            unsigned endBeginSlop = (begin + 31) & ~31;
> +            unsigned beginEndSlop = end & ~31;

seems like this may be related to the 5? as in 2^5 - 1?
Also, bmalloc already has roundUpToMultipleOf, can we use that here w/ 32 instead of it handwritten?

This math confuses me :(

> Source/bmalloc/bmalloc/IsoPageInlines.h:180
> +        unsigned wordIndex = index >> 5;

why not / 32 here? llvm should do the right thing and it's easier to read

> Source/bmalloc/bmalloc/IsoPageInlines.h:184
> +        unsigned bitMask = 1 << (index & 31);
> +        if (word & bitMask)
> +            continue;

This confuses me. What is the purpose of this?

> Source/bmalloc/bmalloc/IsoPageInlines.h:186
> +        if (!word)
> +            m_numNonEmptyWords++;

Shouldn't this condition be flipped?

> Source/bmalloc/bmalloc/IsoPageInlines.h:231
> +        for (unsigned bitIndex = 0; bitIndex < 32; ++bitIndex) {

nit: maybe it's worth writing this as while(word) { ... }? Or is it worth having this be unrolled perhaps?

> Source/bmalloc/bmalloc/IsoTLSInlines.h:109
> +    if (!handle.isInitialized()) {
> +        std::lock_guard<StaticMutex> locker(handle.m_initializationLock);

Can't two threads race to enter here at the same time? Shouldn't we check isInitialized again?

[Saam Barati]

Comment on attachment 326049 [details]
the patch

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

> Source/bmalloc/bmalloc/IsoTLSInlines.h:108
> +    if (!handle.isInitialized()) {

Maybe UNLIKELY?

[Saam Barati]

Comment on attachment 326049 [details]
the patch

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

> Source/bmalloc/bmalloc/IsoPage.h:91
> +    // 000 - Deallocated. It has no objects and its memory is not paged in.
> +    // 111 - Empty.
> +    // 101 - Eligible for allocation, meaning that there is at least one free object in the page.
> +    // 001 - Full.
> +    // 001 - Currently being used for allocation.

Which 3 bits are these digits in reference to?

[Filip Pizlo]

(In reply to Saam Barati from comment #59)
> Comment on attachment 326049 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326049&action=review
> 
> r=me
> I have a few comments and a couple questions.
> 
> > Source/bmalloc/ChangeLog:18
> > +        pages. Pages are corrected into directories. Directories track pages using bitvectors, so
> 
> corrected => collected

Fixed.

> 
> > Source/bmalloc/bmalloc/Bits.h:1
> > +/*
> 
> It's a bit unfortunate that this is probably almost identical to WTF's
> implementation.

Yeah. :-(

It's subtly different in that it allocates the bits inline.  But in a world where WTF and bmalloc were one, this would just be a different specialization of the various FastBitVector templates/base classes.

> 
> > Source/bmalloc/bmalloc/Bits.h:496
> > +        return BitReference(&this->m_words.word(index >> 5), 1 << (index & 31));
> 
> WHy 5?

Shift by 5 === multiply/divide by 32.

> 
> > Source/bmalloc/bmalloc/DeferredDecommit.h:45
> > +
> > +
> > +    
> 
> unneeded whitespace

Fixed.

> 
> > Source/bmalloc/bmalloc/IsoDirectory.h:37
> > +class IsoDirectoryBaseBase {
> 
> BaseBase :(
> 
> > Source/bmalloc/bmalloc/IsoDirectory.h:42
> > +    virtual void didDecommit(unsigned index) = 0;
> 
> Do we really need this to be virtual? There is only one implementation.

Yes.  There are infinity implementations, since the implementation is in a template class.

I use this idiom in a bunch of places in this code.  Say you have Foo<T> for many T's.  And say that Foo<T> has a method that does not depend on T.  And say that you want to call this method for a raw Foo*, without having to know T.  You can do this by creating a FooBase that has a pure version method and Foo<T> overrides it, and then the callers use FooBase* as the T-less pointer to Foo<T>.

> 
> > Source/bmalloc/bmalloc/IsoDirectory.h:69
> > +    void didBecome(IsoPage<Config>*, IsoPageTrigger) override;
> 
> This only has one implementation. Do we need it to be virtual?

See above.

> 
> > Source/bmalloc/bmalloc/IsoDirectoryInlines.h:49
> > +    unsigned pageIndex = (m_eligible | ~m_committed).findBit(m_firstEligible, true);;
> 
> style: extra semicolon at the end

Fixed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:67
> > +    void didBecomeEligible(IsoDirectory<Config, numPagesInInlineDirectory>*); // This one probably needs to set a bit or something.
> > +    void didBecomeEligible(IsoDirectory<Config, IsoDirectoryPage<Config>::numPages>*); // This one needs to do things.
> 
> these comments are not helpful

Removed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:96
> > +    // The basic operations that we need to support are:
> 
> Part of this text seems to have been written before you implemented things.
> Maybe you can clean some of it up now that you've made implementation
> decisions?

Removed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:121
> > +    // - Deallocation should probably use a deallocation log, just like the rest of bmalloc.
> > +    //   -> This avoids having to use any kinds of atomics on the freeing fast path. The slow path can
> > +    //      just grab a lock around the whole isoheap or something.
> 
> This makes things seem up in the air, but are they?

Removed.

> 
> > Source/bmalloc/bmalloc/IsoHeapImpl.h:122
> > +    // - When we create an isoheap, we want to make sure that TLCs find out.
> 
> TLC?

TLC = thread local cache.  Also sometimes known as a TLAB = thread local allocation buffer.

TLC and TLAB are common terms in GC literature.  I have a tendency to use them interchangeably.

> 
> > Source/bmalloc/bmalloc/IsoHeapImplInlines.h:46
> > +        if (result.kind == EligibilityKind::Full)
> 
> Do we want to assert it's not OOM kind?

No.  It's OK to have OOM.  This code will return the OOM in this case.

> 
> > Source/bmalloc/bmalloc/IsoHeapImplInlines.h:62
> > +        if (result.kind != EligibilityKind::Full)
> 
> ditto w.r.t OOM?

If we ran out of memory, we should tell the user about it.  This code is right.

> 
> > Source/bmalloc/bmalloc/IsoPage.h:40
> > +    static constexpr size_t pageSize = 16384;
> 
> Do we want this to be 4096 on iOS? Doesn't bmalloc already have a notion of
> this somewhere?

Other way around.  iOS wants 16384.  I don't think it's beneficial to have the size be 4096 on macOS.

> 
> > Source/bmalloc/bmalloc/IsoPage.h:46
> > +    static constexpr unsigned numObjects = pageSize / Config::objectSize;
> 
> Do we want to add static_assert(numObjects > 0)?

Added.

> 
> > Source/bmalloc/bmalloc/IsoPage.h:55
> > +    // This must have a trivial destructor.
> 
> Nit: Maybe it's worth putting this comment by the field declarations?

Fixed.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:104
> > +        unsigned beginWord = begin >> 5;
> > +        unsigned endWord = end >> 5;
> 
> Maybe we can give this 5 a name? And also static cast that (sizeof(unsigned)
> * 8 == 1<<5)?

I think that assert would be super tautological.  The other bitvector codes in WK don't do that.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:124
> > +            unsigned endBeginSlop = (begin + 31) & ~31;
> > +            unsigned beginEndSlop = end & ~31;
> 
> seems like this may be related to the 5? as in 2^5 - 1?
> Also, bmalloc already has roundUpToMultipleOf, can we use that here w/ 32
> instead of it handwritten?
> 
> This math confuses me :(

Sorry, I know the raw math better than any helpers.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:180
> > +        unsigned wordIndex = index >> 5;
> 
> why not / 32 here? llvm should do the right thing and it's easier to read

Yeah.  I've just trained myself to think in terms of shifts.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:184
> > +        unsigned bitMask = 1 << (index & 31);
> > +        if (word & bitMask)
> > +            continue;
> 
> This confuses me. What is the purpose of this?

If the object is in use, don't put it on the free list.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:186
> > +        if (!word)
> > +            m_numNonEmptyWords++;
> 
> Shouldn't this condition be flipped?

No.  We're setting bits.  Some bits area already set.  We want to detect if the word before we set the bit was zero.  If it was, then us setting the bit increases the number of words that are not zero.  If the word was already not zero, then we aren't changing the number of non-zero words by setting a bit.

> 
> > Source/bmalloc/bmalloc/IsoPageInlines.h:231
> > +        for (unsigned bitIndex = 0; bitIndex < 32; ++bitIndex) {
> 
> nit: maybe it's worth writing this as while(word) { ... }? Or is it worth
> having this be unrolled perhaps?

Oh yeah!  I would fix it, except I already tested everything, so I'm paranoid.

Also, forEachLiveObject and the other iterators are purely for white-box testing and debugging.  They don't have to be efficient.  They just have to be right.

> 
> > Source/bmalloc/bmalloc/IsoTLSInlines.h:109
> > +    if (!handle.isInitialized()) {
> > +        std::lock_guard<StaticMutex> locker(handle.m_initializationLock);
> 
> Can't two threads race to enter here at the same time? Shouldn't we check
> isInitialized again?

You're totally right.  Fixed.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/224537/webkit

[Saam Barati]

Comment on attachment 326049 [details]
the patch

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

> Source/bmalloc/bmalloc/IsoHeap.h:41
> +// It's not valid to create an IsoHeap except in static storage.

I just realized at least partly why this is. You use the below scheme for 2s complement wraparound for zero-initialized data to allow TLS to notice when it doesn't have that up to that offset initialized.
1. Do we care we're using somewhat UB in C++?
2. Maybe it's worth a comment we rely on this wraparound.

[Filip Pizlo]

(In reply to Saam Barati from comment #64)
> Comment on attachment 326049 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326049&action=review
> 
> > Source/bmalloc/bmalloc/IsoHeap.h:41
> > +// It's not valid to create an IsoHeap except in static storage.
> 
> I just realized at least partly why this is. You use the below scheme for 2s
> complement wraparound for zero-initialized data to allow TLS to notice when
> it doesn't have that up to that offset initialized.
> 1. Do we care we're using somewhat UB in C++?

Wraparound for unsigned is well defined.  It's only signed wraparound that is undef.

> 2. Maybe it's worth a comment we rely on this wraparound.

[Geoffrey Garen]

> I think that the immix-style is probably better for perf.  I was going to
> test that in a follow-up.

Sounds good.

In addition to finding the best solution for performance, I think it's important to avoid the bad old days where we had N allocators, each with a different set of strategies and tradeoffs for throughput, high-water-mark memory use, long-term memory use, and free list integrity, reflecting the N personalities that implemented them.

Ideally, the only tradeoff you should have to consider when adopting an isolated heap is the fact that you won't share memory with other objects anymore.

[Radar WebKit Bug Importer]

<rdar://problem/35568759>

[Michael Catanzaro]

Is there a guideline for when to use WTF_MAKE_ISO_ALLOCATED instead of WTF_MAKE_FAST_ALLOCATED or the system allocator?