From b670cb7fd745b891cb1e109cd6f389e659ec15f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 26 Jul 2022 17:36:01 +0000 Subject: [PATCH 1/2] Fix verifier choking when inlining a promise in the RIR bytecode compiler `function() invisible(return(5))` would trip the bytecode verifier since we'd emit ret_ in the inlined promise in the middle of the `invisible` call sequence, leaving stuff on the stack... For now, track when inlining promises and emit return_ in those cases (since return_ does a longjump and resets stack). Ideally we would just not emit any code after finding a call to return, but this would be more involved to change. --- rir/src/bc/Compiler.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/rir/src/bc/Compiler.cpp b/rir/src/bc/Compiler.cpp index 2ad133d76..667915acf 100644 --- a/rir/src/bc/Compiler.cpp +++ b/rir/src/bc/Compiler.cpp @@ -76,6 +76,7 @@ class CompilerContext { std::stack loops; CodeContext* parent; std::unordered_map loadsSlotInCache; + bool inliningPromise = false; CodeContext(SEXP ast, FunctionWriter& fun, CodeContext* p) : cs(fun, ast), parent(p) {} @@ -144,6 +145,9 @@ class CompilerContext { bool inLoop() const { return code.top()->inLoop(); } + bool inliningPromise() const { return code.top()->inliningPromise; } + void setInliningPromise(bool val) { code.top()->inliningPromise = val; } + LoopContext& loop() { return code.top()->loops.top(); } bool loopNeedsContext() { @@ -1097,7 +1101,7 @@ bool compileSpecialCall(CompilerContext& ctx, SEXP ast, SEXP fun, SEXP args_, else compileExpr(ctx, args[0]); - if (ctx.inLoop() || ctx.isInPromise()) + if (ctx.inLoop() || ctx.isInPromise() || ctx.inliningPromise()) cs << BC::return_(); else cs << BC::ret(); @@ -1748,7 +1752,9 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, } if (arg_type == ArgType::RAW_VALUE) { + ctx.setInliningPromise(true); compileExpr(ctx, CAR(arg), false); + ctx.setInliningPromise(false); return; } @@ -1776,7 +1782,9 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, if (arg_type == ArgType::EAGER_PROMISE) { // Compile the expression to evaluate it eagerly, and // wrap the return value in a promise without rir code + ctx.setInliningPromise(true); compileExpr(ctx, CAR(arg), false); + ctx.setInliningPromise(false); prom = compilePromiseNoRir(ctx, CAR(arg)); } else if (arg_type == ArgType::EAGER_PROMISE_FROM_TOS) { // The value we want to wrap in the argument's promise is From 32ba6b5c4251151c08ab074ee64305b2483f7950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 26 Jul 2022 17:42:17 +0000 Subject: [PATCH 2/2] cleanups --- rir/src/bc/CodeVerifier.cpp | 3 +-- rir/src/bc/Compiler.cpp | 10 ++++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/rir/src/bc/CodeVerifier.cpp b/rir/src/bc/CodeVerifier.cpp index 102068d82..66c40acdb 100644 --- a/rir/src/bc/CodeVerifier.cpp +++ b/rir/src/bc/CodeVerifier.cpp @@ -123,7 +123,7 @@ void CodeVerifier::calculateAndVerifyStack(Code* code) { Opcode* cptr = code->code(); q.push(State(cptr)); - while (not q.empty()) { + while (!q.empty()) { State i = q.top(); q.pop(); if (state.find(i.pc) != state.end()) { @@ -259,7 +259,6 @@ void CodeVerifier::verifyFunctionLayout(SEXP sexp) { Rf_error("RIR Verifier: cached clear_binding_cache_ with " "invalid index"); } - if (*cptr == Opcode::mk_promise_ || *cptr == Opcode::mk_eager_promise_) { unsigned* promidx = reinterpret_cast(cptr + 1); diff --git a/rir/src/bc/Compiler.cpp b/rir/src/bc/Compiler.cpp index 667915acf..fac838afc 100644 --- a/rir/src/bc/Compiler.cpp +++ b/rir/src/bc/Compiler.cpp @@ -116,7 +116,6 @@ class CompilerContext { }; class PromiseContext : public CodeContext { - public: PromiseContext(SEXP ast, FunctionWriter& fun, CodeContext* p) : CodeContext(ast, fun, p) {} @@ -1759,7 +1758,7 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, } // Constant arguments do not need to be promise wrapped - if (arg_type != ArgType::EAGER_PROMISE_FROM_TOS) + if (arg_type != ArgType::EAGER_PROMISE_FROM_TOS) { switch (TYPEOF(CAR(arg))) { case LANGSXP: case SYMSXP: @@ -1777,6 +1776,7 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, cs << BC::push(eager); return; } + } Code* prom; if (arg_type == ArgType::EAGER_PROMISE) { @@ -1788,12 +1788,14 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, prom = compilePromiseNoRir(ctx, CAR(arg)); } else if (arg_type == ArgType::EAGER_PROMISE_FROM_TOS) { // The value we want to wrap in the argument's promise is - // already on TOS, no nead to compile the expression. + // already on TOS, no need to compile the expression. // Wrap it in a promise without rir code. prom = compilePromiseNoRir(ctx, CAR(arg)); - } else { // ArgType::PROMISE + } else if (arg_type == ArgType::PROMISE) { // Compile the expression as a promise. prom = compilePromise(ctx, CAR(arg)); + } else { + assert(false); } size_t idx = cs.addPromise(prom);