Re: OP_HALT is really useful

Oh wait, one more (not sure if these are still applicable, but maybe worth a look):

This won't be directly applicable because it has LUA_TRIGGERHOOK code in it (related to a sampling profiler hook we added but never shared), but the principle of the bug may still apply and the description + patch should make it reasonably easy to confirm and apply a corresponding fix.

commit 024bcdb6f71d01534b72fd7ea19ebf26b5a95914

Author: Dan Tull <dtull@adobe.com>

Date: Thu Sep 27 10:48:48 2012 -0800

(@852507)

Fix a very rare but nasty bug in breakpoints in the Lua VM code:

1. Set a single breakpoint (it helps to pick a common entry point from native code).

2. Pause the app so that you stop on the same opcode via the trigger hook.

3. While paused via trigger, clear the breakpoint.

4. Resume the debuggee.

Result: SEGFAULT

Solution: don't fetch the instruction until after the hook executes so you don't end up with a dangling halt.

Normally, I'd delimit the patch based so the original version is preserved

It would be overly messy for no behavioral difference in this case, though.

We never saw this because pause didn't used to be as fast as it is now, so there was only a tiny probability of stopping due to a pause request on a line that also had a breakpoint (and clearing that breakpoint).

[git-p4: depot-paths = "//lua/main/third_party/lua_tp/lua_5_1/": change = 852508]

diff --git a/src/lvm.c b/src/lvm.c

index e970cd9..c7f02b7 100644

--- a/src/lvm.c

+++ b/src/lvm.c

@@ -417,8 +417,6 @@ void luaV_execute (lua_State *L, int nexeccalls) {

k = cl->p->k;

/* main loop of interpreter */

for (;;) {

- Instruction i = *pc++;

- StkId ra;

#ifdef LUA_TRIGGERHOOK

if ((L->hookmask & (LUA_MASKLINE | LUA_MASKCOUNT | LUA_MASKTRIGGER)) &&

(--L->hookcount == 0 || L->hookmask & LUA_MASKLINE || G(L)->triggers != 0)) {

@@ -426,13 +424,15 @@ void luaV_execute (lua_State *L, int nexeccalls) {

if ((L->hookmask & (LUA_MASKLINE | LUA_MASKCOUNT)) &&

(--L->hookcount == 0 || L->hookmask & LUA_MASKLINE)) {

#endif

- traceexec(L, pc);

+ traceexec(L, pc + 1);

if (L->status == LUA_YIELD) { /* did hook yield? */

- L->savedpc = pc - 1;

+ L->savedpc = pc;

return;

}

base = L->base;

}

+ Instruction i = *pc++;

+ StkId ra;

/* warning!! several calls may realloc the stack and invalidate `ra' */

#ifdef LUA_HALT_OP

resume:

From: lua-l-bounces@lists.lua.org <lua-l-bounces@lists.lua.org> on behalf of Dan Tull <dtull@adobe.com>
Sent: Thursday, May 24, 2018 6:47 PM
To: Lua mailing list
Subject: Re: OP_HALT is really useful

> ... implementing OP_HALT changes the overhead from running with a debugger from 'severe' to 'unnoticeable'.

Sweet! That's exactly why I wrote it. Glad it was helpful (if rather ancient at this point). The difference between slowing down every opcode to check if there's a breakpoint and breakpoints that only have overhead if they get hit is like night and day.

> I would submit to Roberto et al. that adding something OP_HALT-ish to the official Lua sources might not be a bad idea...

One other fun experiment I did with OP_HALT is to use it to pepper a whole codebase with self-clearing breakpoints for nearly zero overhead instruction level code coverage (code coverage with the line hook is punishingly slow in some of our apps whereas I accidentally left my experimental breakpoint based code coverage on for a couple of weeks because I didn't notice the speed penalty). It can also be used for dtrace-like instrumentation points.

At one point, I did apply the OP_HALT patches to 5.2 and 5.3 internally, but we have never really found a compelling reason to actually move from our 5.1.x derivative, so I never published those patches (it was fairly straightforward but not automatic to port the patch from 5.1 as I recall) since they never got enough use to be all that confident in their correctness.

DT

There were a couple of other bugs found since that patch was posted as well (mostly quite obscure, but no sense keeping them to myself):

commit e861f4f9cd3440d2710f8f1d66c3db541aae52b5

Author: Dan Tull <dtull@adobe.com>

Date: Thu Jul 18 18:01:07 2013 -0500

Fix obscure bug in breakpoint offset adjustment.

These two opcode tests should be done on the same opcode. Otherwise, it could result in the breakpoint landing on the previous opcode and thus the previous line. This would only have a user visible effect in the rare case of a SETLIST opcode with a non-zero C argument as the last argument on its line followed by an opcode whose C argument was 0 on the subsequent line. In the even rarer case where the opcode prior to the SETLIST opcode was also one that should not be patched, it could even cause corruption of the bytecode's validity that could lead to undefined behavior (including crashing).

diff --git a/src/ldebug.c b/src/ldebug.c

index 5a20b7e..c096f62 100644

--- a/src/ldebug.c

+++ b/src/ldebug.c

@@ -991,7 +991,7 @@ static int avoidpseudo(lua_State *L, Proto *p, int offset) {

default:

return offset; // bare JMP, which is fine.

}

- } else if (GET_OPCODE(pr) == OP_SETLIST && GETARG_C(i) == 0) {

+ } else if (GET_OPCODE(pr) == OP_SETLIST && GETARG_C(pr) == 0) {

// a SETLIST with a C of 0 treats the next opcode as a raw int

// to do very large batch settable operations, which means that

// "opcode" should not be patched, obviously.

commit febf510f9aecc96871bcef411df258bbc297cbad

Author: Dan Tull <dtull@adobe.com>

Date: Thu Mar 28 14:04:55 2013 -0500

Fixes to keep OP_HALT from confusing debug.getinfo.

Some of the routines in ldebug.c scan the bytecode to decide how to describe registers and stack frames.

If there are OP_HALT opcodes (breakpoints) in just the wrong places, it can confuse this logic and cause backtraces to have less name information than expected or incorrect names/types in error messages.

Merged from //lua/tools/halide CL 892871 and 893037.

diff --git a/src/ldebug.c b/src/ldebug.c

index 1dfb1b7..5a20b7e 100644

--- a/src/ldebug.c

+++ b/src/ldebug.c

@@ -314,6 +314,13 @@ LUA_API int lua_getinfo (lua_State *L, const char *what, lua_Debug *ar) {

#define checkreg(pt,reg) check((reg) < (pt)->maxstacksize)

+// note: duplicated in lvm.c and ldo.c due to dependency tangle (below requires lopcodes.h and lobject.h)

+#ifdef LUA_HALT_OP

+#define GET_REAL_INSTR(i,p) (GET_OPCODE(i) == OP_HALT ? (p->halts[GETARG_Bx(i)].orig) : (i))

+#else

+#define GET_REAL_INSTR(i,p) (i)

+#endif

static int precheck (const Proto *pt) {

@@ -323,7 +330,7 @@ static int precheck (const Proto *pt) {

(pt->is_vararg & VARARG_HASARG));

check(pt->sizeupvalues <= pt->nups);

check(pt->sizelineinfo == pt->sizecode || pt->sizelineinfo == 0);

- check(pt->sizecode > 0 && GET_OPCODE(pt->code[pt->sizecode-1]) == OP_RETURN);

+ check(pt->sizecode > 0 && GET_OPCODE(GET_REAL_INSTR(pt->code[pt->sizecode-1], pt)) == OP_RETURN);

return 1;

}

@@ -370,7 +377,7 @@ static Instruction symbexec (const Proto *pt, int lastpc, int reg) {

last = pt->sizecode-1; /* points to final return (a `neutral' instruction) */

check(precheck(pt));

for (pc = 0; pc < lastpc; pc++) {

- Instruction i = pt->code[pc];

+ Instruction i = GET_REAL_INSTR(pt->code[pc], pt);

OpCode op = GET_OPCODE(i);

int a = GETARG_A(i);

int b = 0;

@@ -403,7 +410,7 @@ static Instruction symbexec (const Proto *pt, int lastpc, int reg) {

go all the way back to the first of them (if any) */

for (j = 0; j < dest; j++) {

Instruction d = pt->code[dest-1-j];

- if (!(GET_OPCODE(d) == OP_SETLIST && GETARG_C(d) == 0)) break;

+ if (!(GET_OPCODE(GET_REAL_INSTR(d, pt)) == OP_SETLIST && GETARG_C(GET_REAL_INSTR(d, pt)) == 0)) break;

}

/* if 'j' is even, previous value is not a setlist (even if

it looks like one) */

@@ -418,13 +425,13 @@ static Instruction symbexec (const Proto *pt, int lastpc, int reg) {

}

if (testTMode(op)) {

check(pc+2 < pt->sizecode); /* check skip */

- check(GET_OPCODE(pt->code[pc+1]) == OP_JMP);

+ check(GET_OPCODE(GET_REAL_INSTR(pt->code[pc+1], pt)) == OP_JMP);

}

switch (op) {

case OP_LOADBOOL: {

if (c == 1) { /* does it jump? */

check(pc+2 < pt->sizecode); /* check its jump */

- check(GET_OPCODE(pt->code[pc+1]) != OP_SETLIST ||

+ check(GET_OPCODE(GET_REAL_INSTR(pt->code[pc+1], pt)) != OP_SETLIST ||

GETARG_C(pt->code[pc+1]) != 0);

}

break;

@@ -503,7 +510,7 @@ static Instruction symbexec (const Proto *pt, int lastpc, int reg) {

nup = pt->p[b]->nups;

check(pc + nup < pt->sizecode);

for (j = 1; j <= nup; j++) {

- OpCode op1 = GET_OPCODE(pt->code[pc + j]);

+ OpCode op1 = GET_OPCODE(GET_REAL_INSTR(pt->code[pc + j], pt));

check(op1 == OP_GETUPVAL || op1 == OP_MOVE);

}

if (reg != NO_REG) /* tracing? */

@@ -521,7 +528,7 @@ static Instruction symbexec (const Proto *pt, int lastpc, int reg) {

default: break;

}

- return pt->code[last];

+ return GET_REAL_INSTR(pt->code[last], pt);

}

#undef check

@@ -596,7 +603,7 @@ static const char *getfuncname (lua_State *L, CallInfo *ci, const char **name) {

if ((isLua(ci) && ci->tailcalls > 0) || !isLua(ci - 1))

return NULL; /* calling function is not Lua (or is unknown) */

ci--; /* calling function */

- i = ci_func(ci)->l.p->code[currentpc(L, ci)];

+ i = GET_REAL_INSTR(ci_func(ci)->l.p->code[currentpc(L, ci)], ci_func(ci)->l.p);

if (GET_OPCODE(i) == OP_CALL || GET_OPCODE(i) == OP_TAILCALL ||

GET_OPCODE(i) == OP_TFORLOOP)

return getobjname(L, ci, GETARG_A(i), name);

commit 761cd7022def90b2953ab71dcc6109235af91eea

Author: Dan Tull <dtull@adobe.com>

Date: Thu Jul 10 18:00:35 2014 -0500

Fix for instruction operand truncation issue.

The Bx operand can only hold values in the range [0, 2^18), so at most 2^18 breakpoints can be set.

Previously, an attempt to set breakpoints beyond that point resulted in application of incorrect original opcodes which could lead to bad behavior.

However, aside from experimentation with programmatically setting many breakpoints this limit is highly unlikely to be reached.

diff --git a/src/ldebug.c b/src/ldebug.c

index c096f62..c6a39ed 100644

--- a/src/ldebug.c

+++ b/src/ldebug.c

@@ -1031,6 +1031,10 @@ static int sethalt(lua_State *L, Proto *p, int offset, lua_Hook hook) {

existing = findhalt(L, p, offset);

if (existing < 0) {

+ if (p->sizehalts > MAXARG_Bx) {

+ return -1;

+ }

luaM_reallocvector(L, p->halts, p->sizehalts, p->sizehalts + 1, Halt);

existing = p->sizehalts;

p->sizehalts = p->sizehalts + 1;

From: lua-l-bounces@lists.lua.org <lua-l-bounces@lists.lua.org> on behalf of Sven Olsen <sven2718@gmail.com>
Sent: Sunday, May 13, 2018 7:09 PM
To: Lua mailing list
Subject: OP_HALT is really useful

I've just spent some time playing with VS Code, which has a couple very nice community written Lua debuggers. However, as Seungjae Lee notes in his README.md, the performance hit that one sees from using one of these debuggers is reduced dramatically if one registers breakpoints using Dan Tull's OP_HALT patch in place of debug.sethook().

I don't fully understand the reasons /why/ OP_HALT is so useful, but, I can confirm that, in my own use case at least, implementing OP_HALT changes the overhead from running with a debugger from 'severe' to 'unnoticeable'.

Unfortunately, Dan's original patch is now 7+ years old, and Seungjae Lee's 5.1.5 version is written against a version of the codebase that's only slightly newer. I have managed to apply Seungjae Lee's version of OP_HALT to my own modded copy of Lua 5.2, and it appears to work well. So, if there's a need for a 5.2 version, interested parties should let me know, and perhaps we can create an official-ish entry for OP_HALT on the power patches page. (This is not an entirely altruistic suggestion -- I suspect there may be some some quirks with the interaction of OP_HALT and 5.2-era features like GOTOs that could use a bit more investigation.)

Beyond that, I would submit to Roberto et al. that adding something OP_HALT-ish to the official Lua sources might not be a bad idea -- I was shocked by what a difference it makes.

-Sven