From 71a66a999b0bc17a7cbd3a6b7ffc17e0fc221fce Mon Sep 17 00:00:00 2001 From: Jan Oravec Date: Wed, 18 Mar 2020 21:22:37 -0700 Subject: [PATCH] Generate properly synced catch block for IterInit Summary: IterInit bytecode discards a stack value containing the iterable and then emits an IterInit IR opcode that consumes it. The problem is that there was no exception stack boundary between these two operations, so we emitted a catch block with an incorrectly synced stack offsets. This forced us to preserve the iterable across the opcode, so that the catch block can sync it to the stack. This did not cause any real issues until now because of super hacky fixup logic that synced the SP properly, so the unwindier did not attempt to decref this extra value. However, oulgen's unwinder improvements experience this issue and free the iterable twice. Fix this by syncing the boundary and removing the hacky fixup logic. I will try to improve the enforcement of this situation in the next diff, which is likely to catch more instances of this bug. Reviewed By: ricklavoie, oulgen Differential Revision: D20524184 fbshipit-source-id: 7d571115fd1f6aad293aa6c2b6edabc173dc6cd8 --- hphp/runtime/vm/jit/extra-data.h | 26 ++++---------------------- hphp/runtime/vm/jit/irgen-iter.cpp | 7 +++++-- hphp/runtime/vm/jit/irlower-internal.cpp | 3 --- hphp/runtime/vm/jit/irlower-iter.cpp | 12 +----------- hphp/runtime/vm/jit/irlower.h | 1 - 5 files changed, 10 insertions(+), 39 deletions(-) diff --git a/hphp/runtime/vm/jit/extra-data.h b/hphp/runtime/vm/jit/extra-data.h index 22e584939b0..b28ed3087ac 100644 --- a/hphp/runtime/vm/jit/extra-data.h +++ b/hphp/runtime/vm/jit/extra-data.h @@ -465,24 +465,6 @@ struct IterData : IRExtraData { IterArgs args; }; -/* - * When initializing an iterator, we need one more piece of info - whether the - * base came from the stack or from a local - to make the right native call. - */ -struct IterInitData : public IterData { - enum class Source { Stack, Local }; - - IterInitData(IterArgs args, Source source) - : IterData(args), source(source) {} - - std::string show() const { - auto const prefix = source == Source::Stack ? "Stack " : "Local "; - return folly::to(prefix, IterData(*this).show()); - } - - Source source; -}; - struct IterTypeData : IRExtraData { IterTypeData(uint32_t iterId, IterSpecialization type) : iterId{iterId} @@ -1706,12 +1688,12 @@ X(LdIterPos, IterId); X(LdIterEnd, IterId); X(KillIter, IterId); X(IterFree, IterId); -X(IterInit, IterInitData); -X(IterInitK, IterInitData); +X(IterInit, IterData); +X(IterInitK, IterData); X(IterNext, IterData); X(IterNextK, IterData); -X(LIterInit, IterInitData); -X(LIterInitK, IterInitData); +X(LIterInit, IterData); +X(LIterInitK, IterData); X(LIterNext, IterData); X(LIterNextK, IterData); X(ConstructInstance, ClassData); diff --git a/hphp/runtime/vm/jit/irgen-iter.cpp b/hphp/runtime/vm/jit/irgen-iter.cpp index bbff87dfff7..908b2b98377 100644 --- a/hphp/runtime/vm/jit/irgen-iter.cpp +++ b/hphp/runtime/vm/jit/irgen-iter.cpp @@ -91,8 +91,11 @@ void emitIterInit(IRGS& env, IterArgs ita, Offset doneOffset) { specializeIterInit(env, doneOffset, ita, kInvalidId); discard(env, 1); + updateMarker(env); + env.irb->exceptionStackBoundary(); + auto const op = ita.hasKey() ? IterInitK : IterInit; - auto const data = IterInitData(ita, IterInitData::Source::Stack); + auto const data = IterData(ita); auto const result = gen(env, op, data, base, fp(env)); implIterInitJmp(env, doneOffset, result); } @@ -117,7 +120,7 @@ void emitLIterInit(IRGS& env, IterArgs ita, auto const op = base->isA(TArrLike) ? (ita.hasKey() ? LIterInitK : LIterInit) : (ita.hasKey() ? IterInitK : IterInit); - auto const data = IterInitData(ita, IterInitData::Source::Local); + auto const data = IterData(ita); auto const result = gen(env, op, data, base, fp(env)); implIterInitJmp(env, doneOffset, result); } diff --git a/hphp/runtime/vm/jit/irlower-internal.cpp b/hphp/runtime/vm/jit/irlower-internal.cpp index 90d5d7344ac..092cc099954 100644 --- a/hphp/runtime/vm/jit/irlower-internal.cpp +++ b/hphp/runtime/vm/jit/irlower-internal.cpp @@ -120,9 +120,6 @@ Fixup makeFixup(const BCMarker& marker, SyncOptions sync) { auto const stackOff = [&] { switch (sync) { - case SyncOptions::SyncAdjustOne: - return marker.spOff() -= 1; - case SyncOptions::None: // We can get here if we are memory profiling, since we override the // normal sync settings and sync anyway. diff --git a/hphp/runtime/vm/jit/irlower-iter.cpp b/hphp/runtime/vm/jit/irlower-iter.cpp index 8f72628f927..8fd7ba51d5e 100644 --- a/hphp/runtime/vm/jit/irlower-iter.cpp +++ b/hphp/runtime/vm/jit/irlower-iter.cpp @@ -126,18 +126,8 @@ void implIterInit(IRLS& env, const IRInstruction* inst) { args.imm(0); } - // new_iter_object decrefs the base object if it propagates an exception out, - // so if the base came from the stack, we adjust the stack pointer by 1 on - // an unwind, skipping over the base. - auto const sync = [&]{ - switch (inst->extra()->source) { - case IterInitData::Source::Stack: return SyncOptions::SyncAdjustOne; - case IterInitData::Source::Local: return SyncOptions::Sync; - } - always_assert(false); - }(); auto const target = CallSpec::direct(new_iter_object); - cgCallHelper(v, env, target, callDest(env, inst), sync, args); + cgCallHelper(v, env, target, callDest(env, inst), SyncOptions::Sync, args); } void implIterNext(IRLS& env, const IRInstruction* inst) { diff --git a/hphp/runtime/vm/jit/irlower.h b/hphp/runtime/vm/jit/irlower.h index 3778796e572..f9408672bdc 100644 --- a/hphp/runtime/vm/jit/irlower.h +++ b/hphp/runtime/vm/jit/irlower.h @@ -39,7 +39,6 @@ namespace irlower { enum class SyncOptions { None, Sync, - SyncAdjustOne, SyncStublogue, }; -- 2.11.4.GIT