Let's extend.
Created attachment 393567 [details] Patch
To collect more cell info in ASan bots.
Comment on attachment 393567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393567&action=review > Source/JavaScriptCore/runtime/JSCell.cpp:387 > + variousState |= static_cast<uint64_t>(!isFreeListed) << 1; Is this correct? Does !!isFreeListed mean isAllocated?
(In reply to Mark Lam from comment #3) > Comment on attachment 393567 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393567&action=review > > > Source/JavaScriptCore/runtime/JSCell.cpp:387 > > + variousState |= static_cast<uint64_t>(!isFreeListed) << 1; > > Is this correct? Does !!isFreeListed mean isAllocated? typo: I meant !isFreeListed
Comment on attachment 393567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393567&action=review r=me >>> Source/JavaScriptCore/runtime/JSCell.cpp:387 >>> + variousState |= static_cast<uint64_t>(!isFreeListed) << 1; >> >> Is this correct? Does !!isFreeListed mean isAllocated? > > typo: I meant !isFreeListed Nevermind, I've read thru the PreciseAllocation code and this patch some more. This is correct because foundPreciseAllocation is only true if this cell is found in the lower tier of this IsoSubspace, and it is only allocated if it's not on the free list. > Source/JavaScriptCore/runtime/JSCell.cpp:390 > + variousState |= static_cast<uint64_t>(foundPreciseAllocation->isEmpty()) << 2; > + variousState |= static_cast<uint64_t>(foundPreciseAllocation->isNewlyAllocated()) << 4; The isEmpty and isNewlyAllocated flags previously meant that the cell pointer was found in a MarkedBlock of that state. For PreciseAllocations, I think knowing whether it isFreeListed or isAllocated is sufficient. I'm not quite sure what isEmpty and isNewlyAllocated would mean in this case. I suggest just leaving these unset. Instead, can you also set the cellIsProperlyAligned flag ( << 5) in variousState based on whether (foundPreciseAllocation->cell() == cell)? If the cell pointer is found in a PreciseAllocation but does not equal it's expected cell() pointer, then we have a corrupted pointer or someone unscrupulous is making bad pointers. Can you also set the needsDestruction flag in variousState based on whether the SubSpace CellAttributes says it NeedsDestruction or not?
Comment on attachment 393567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393567&action=review >> Source/JavaScriptCore/runtime/JSCell.cpp:390 >> + variousState |= static_cast<uint64_t>(foundPreciseAllocation->isNewlyAllocated()) << 4; > > The isEmpty and isNewlyAllocated flags previously meant that the cell pointer was found in a MarkedBlock of that state. For PreciseAllocations, I think knowing whether it isFreeListed or isAllocated is sufficient. I'm not quite sure what isEmpty and isNewlyAllocated would mean in this case. I suggest just leaving these unset. > > Instead, can you also set the cellIsProperlyAligned flag ( << 5) in variousState based on whether (foundPreciseAllocation->cell() == cell)? If the cell pointer is found in a PreciseAllocation but does not equal it's expected cell() pointer, then we have a corrupted pointer or someone unscrupulous is making bad pointers. > > Can you also set the needsDestruction flag in variousState based on whether the SubSpace CellAttributes says it NeedsDestruction or not? I think isNewlyAllocated (In Eden or not) is nice to have. And isEmpty is also nice too (WeakSet is claered etc.). I'm querying this only when `!isFreeListed` since (1) isNewlyAllocated is meaningless if it is not in free list and (2) if this cell is not in freelist, we are not guaranteeing that WeakSet (isEmpty is accessing this) is not broken. Adding cellIsProperlyAligned sounds nice. Fixed. Not sure whether NeedsDestruction is important since this can be super easily detected based on IsoSubspace, but maybe it would be ok. Added.
Committed r258479: <https://trac.webkit.org/changeset/258479>
<rdar://problem/60466867>