Commit e84e9b92 authored by Bruce Dawson's avatar Bruce Dawson Committed by Commit Bot

Revert "Speculative fix to crashes from a CPU bug"

This reverts commit 10360127.

Reason for revert: This fix only had moderate impact and the
underlying CPU bug has now been addressed.

Original change's description:
> Speculative fix to crashes from a CPU bug
> 
> For the last few months Chrome has been seeing many "impossible" crashes
> on Intel Gemini Lake, family 6 model 122 stepping 1 CPUs. These crashes
> only happen with 64-bit Chrome and only happen in the prologue of two
> functions. The crashes come and go across different Chrome versions.
> Analysis of most of the crashes shows that the address of the crashing
> instruction follows some patterns:
> 
> When crashing in GetFieldIndex() the last byte of the address is always
> 1c, 5c, 9c, or dc.
> 
> When crashing in UpdateCaches (fewer unique samples) the last byte of
> the address is always 5d or 9d.
> 
> The address of the function is 0xc or 0xd bytes earlier so the crashing
> functions always start with an address that ends in 10, 50, 90, or d0.
> 
> Those addresses are for the crashes on a load of the __security_cookie.
> The crashes also occasionally happen on the two instructions that follow
> the __security_cookie load in which case the crashing instruction's
> address has been seen to end with 23 or a3. This corresponds to a
> function start address of 10 or 90.
> 
> Since the crash involves reading incorrect instruction bytes when
> crossing a 16-byte boundary and since the crash appears to only happen
> with particular 16-byte alignments it seems reasonable to force the
> function's alignments to a multiple of 32 to see if this reliably
> avoids the crashes. This change uses the gcc/clang __attribute__
> directive to force 32-byte alignment. I have tested this change enough to
> verify that it triggers the desired alignment (with up to 31 "int 3"
> instructions added for padding) but since I have never reproduced this
> crash I have no way of testing its efficacy.
> 
> Bug: chromium:968683, chromium:964273
> Change-Id: Ia6e1c6d1e044b84d274817374b25523303e78b51
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803775
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#63804}

TBR=brucedawson@chromium.org,verwaest@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:968683, chromium:964273
Change-Id: I150ecfebeff95e8f63dbba74d78491867dc17736
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2134728
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Auto-Submit: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: 's avatarToon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66977}
parent 3363adbe
......@@ -4,7 +4,6 @@
#include "src/ic/ic.h"
#include "include/v8config.h"
#include "src/api/api-arguments-inl.h"
#include "src/api/api.h"
#include "src/ast/ast.h"
......@@ -695,10 +694,6 @@ void IC::SetCache(Handle<Name> name, const MaybeObjectHandle& handler) {
}
}
#if defined(__clang__) && defined(V8_OS_WIN)
// Force function alignment to work around CPU bug: https://crbug.com/968683
__attribute__((__aligned__(32)))
#endif
void LoadIC::UpdateCaches(LookupIterator* lookup) {
Handle<Object> code;
if (lookup->state() == LookupIterator::ACCESS_CHECK) {
......
......@@ -4,7 +4,6 @@
#include "src/objects/lookup.h"
#include "include/v8config.h"
#include "src/deoptimizer/deoptimizer.h"
#include "src/execution/isolate-inl.h"
#include "src/execution/protectors-inl.h"
......@@ -943,12 +942,7 @@ Handle<Map> LookupIterator::GetFieldOwnerMap() const {
isolate_);
}
#if defined(__clang__) && defined(V8_OS_WIN)
// Force function alignment to work around CPU bug: https://crbug.com/968683
__attribute__((__aligned__(32)))
#endif
FieldIndex
LookupIterator::GetFieldIndex() const {
FieldIndex LookupIterator::GetFieldIndex() const {
DCHECK(has_property_);
DCHECK(holder_->HasFastProperties(isolate_));
DCHECK_EQ(kField, property_details_.location());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment