...due to recent ValueCheck integration of pointer checking. Patch coming.
Created attachment 50268 [details] patch
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.
Comment on attachment 50268 [details] patch r=me
+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.
> +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.
> 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.
> > 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.
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
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.
Created attachment 50369 [details] patch Here's an attempt at fixing Alexey's last review comment.
Reopening for review.
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.
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.
> 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.
(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.
Created attachment 50429 [details] patch Turned out not to be too hard to add the improvements Alexey mentioned.
> 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.
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.
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
Committed revision 55811.