Bug 35899 – Switching malloc implementations requires a world rebuild

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

[Geoffrey Garen]

...due to recent ValueCheck integration of pointer checking.

Patch coming.

[Geoffrey Garen]

Created attachment 50268 [details]
patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:378:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 4 files


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

[Maciej Stachowiak]

Comment on attachment 50268 [details]
patch

r=me

[Alexey Proskuryakov]

+void fastCheckConsistency(const void* p)

I'd give this function a name related to what it does. Something like pointerIsAllocated, perhaps?

Also, the prefix "fast" makes it look like it works with fastMalloc, while it's the opposite that is true.

[Geoffrey Garen]

> +void fastCheckConsistency(const void* p)
> 
> I'd give this function a name related to what it does. Something like
> pointerIsAllocated, perhaps?

Right now, the function does the consistency checking, assertions included.

Are you suggesting moving the assertions to the caller?

> Also, the prefix "fast" makes it look like it works with fastMalloc, while it's
> the opposite that is true.

It is the fastMalloc library that does the consistency checking. Whether fastMalloc happens to be using the system malloc or tcmalloc is its own implementation detail.

[Alexey Proskuryakov]

> Are you suggesting moving the assertions to the caller?

Perhaps. Or maybe the function could be called somehow else - "consistency" doesn't mean much for a void*.

> It is the fastMalloc library that does the consistency checking.

I didn't know there was such a thing as "fastMalloc library" encompassing all WebKit allocator behaviors. I always thought of it as a particular allocator, and I'm sure some others would be confused, too.

[Geoffrey Garen]

> > Are you suggesting moving the assertions to the caller?
> 
> Perhaps. Or maybe the function could be called somehow else - "consistency"
> doesn't mean much for a void*.

OK. Can you suggest something specific?

> > It is the fastMalloc library that does the consistency checking.
> 
> I didn't know there was such a thing as "fastMalloc library" encompassing all
> WebKit allocator behaviors. I always thought of it as a particular allocator,
> and I'm sure some others would be confused, too.

I didn't mean to say that FastMalloc services all WebKit allocations. Indeed, it doesn't. And, therefore, if you use ValueCheck<void*> with a pointer allocated by an allocator that behaves differently than calling fastMalloc/fastFree would, you'll get undefined behavior. But that was true before this patch, too.

[Geoffrey Garen]

I'm going to close this bug so it doesn't show up in the commit queue, but we can continue discussing what to do next with it.

http://trac.webkit.org/changeset/55706

[Alexey Proskuryakov]

As mentioned above, "isPointerAllocated" or "allocationSize" would be a great thing for a FastMalloc.h to expose. We might even be able implement it for fastMalloc.

[Geoffrey Garen]

Created attachment 50369 [details]
patch

Here's an attempt at fixing Alexey's last review comment.

[Geoffrey Garen]

Reopening for review.

[Alexey Proskuryakov]

Comment on attachment 50369 [details]
patch

It seems that tcmalloc version of fastMallocSize could still use some work. To match OS-provided ones, it should return 0 for pointers to unallocated memory, but there are at least two cases when it doesn't:

1) It will crash if span is null.
2) It disregards low bits of a pointer, so it can mistakenly return a non-zero result.

r=me though, this is already a good improvement. Please add a FIXME if you decide to land as is.

[Darin Adler]

Comment on attachment 50369 [details]
patch

> +    const PageID p = reinterpret_cast<uintptr_t>(ptr) >> kPageShift;

Should not use const. Should be named "p", not "page". Should check that the low bits are all zero, as Alexey suggested, and return 0 if they are not.

> +    size_t cl = pageheap->GetSizeClassIfCached(p);
> +    if (cl)
> +        return ByteSizeForClass(cl);

Should be named "sizeClass", not "cl". Should be defined inside the if statement.

> +    Span* span = pageheap->GetDescriptor(p);
> +    ASSERT(span);

Should return 0 if span is 0 rather than asserting, as Alexey suggested. Unless the GetDescriptor function is guaranteed to never return 0, in which case it is appropriate to assert.

> +    cl = span->sizeclass;
> +    if (cl) {

Should be named "sizeClass", not "cl". Should be defined inside the if statement.

[Alexey Proskuryakov]

> Should check that the low bits are all zero, as Alexey suggested, and return 0 if they are not.

Is this a correct thing to do? I was somewhat vague in my comment, but I think we'd be returning 0 for all blocks in a page but the first one if we simply checked the bits for being zero.

Right now, the function is guaranteed to return a correct result for allocated pointers, and has undefined behavior otherwise - which is not exactly what malloc_size does, but not too bad either.

[Darin Adler]

(In reply to comment #14)
> Is this a correct thing to do? I was somewhat vague in my comment, but I think
> we'd be returning 0 for all blocks in a page but the first one if we simply
> checked the bits for being zero.

Oh, OK. Seems like the correct check should not be too difficult. I didn't read the rest of the FastMalloc code to figure out what it would be.

[Geoffrey Garen]

Created attachment 50429 [details]
patch

Turned out not to be too hard to add the improvements Alexey mentioned.

[Geoffrey Garen]

> Should not use const. Should be named "p", not "page".
> Should be named "sizeClass", not "cl".

I made the 'if' changes Darin mentioned, but I'd like to keep these names, if that's OK. I agree that they're not optimal, and not in the WebKit coding style, but they match the naming conventions of the rest of the file, which makes text search a lot easier.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:4140:  tc_length is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/FastMalloc.cpp:4158:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/wtf/FastMalloc.cpp:4158:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 3 in 5 files


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

[Sam Weinig]

Comment on attachment 50429 [details]
patch

> +    for (void* free = span->objects; free != NULL; free = *((void**) free))
> +        if (ptr == free)
> +            return 0;

Braces please.

r=me

[Geoffrey Garen]

Committed revision 55811.