Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Improve Script CPP API diagnostics
  • Loading branch information
kwolekr committed Aug 6, 2015
1 parent 3183d5a commit bcf47bc
Show file tree
Hide file tree
Showing 16 changed files with 129 additions and 107 deletions.
62 changes: 51 additions & 11 deletions src/script/common/c_internal.cpp
Expand Up @@ -69,11 +69,51 @@ int script_exception_wrapper(lua_State *L, lua_CFunction f)
return lua_error(L); // Rethrow as a Lua error.
}

void script_error(lua_State *L)
/*
* Note that we can't get tracebacks for LUA_ERRMEM or LUA_ERRERR (without
* hacking Lua internals). For LUA_ERRMEM, this is because memory errors will
* not execute the the error handler, and by the time lua_pcall returns the
* execution stack will have already been unwound. For LUA_ERRERR, there was
* another error while trying to generate a backtrace from a LUA_ERRRUN. It is
* presumed there is an error with the internal Lua state and thus not possible
* to gather a coherent backtrace. Realistically, the best we can do here is
* print which C function performed the failing pcall.
*/
void script_error(lua_State *L, int pcall_result, const char *fxn)
{
const char *s = lua_tostring(L, -1);
std::string str(s ? s : "");
throw LuaError(str);
if (pcall_result == 0)
return;

const char *err_type;
switch (pcall_result) {
case LUA_ERRRUN:
err_type = "Runtime";
break;
case LUA_ERRMEM:
err_type = "OOM";

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja Aug 24, 2015

Member

Perhaps spelling this out would be helpfull for those that aren't familiar with the acronym.

This comment has been minimized.

Copy link
@est31

est31 Aug 24, 2015

Contributor

It won't help much either if spelling out, because often there is enough memory available, but due to some bug it still gives "OOM" as a reason. So its better to have an opaque set of letters than even more non-experienced users think its hw fault.

break;
case LUA_ERRERR:
err_type = "Double fault";
break;
default:
err_type = "Unknown";
}

const char *err_descr = lua_tostring(L, -1);
if (!err_descr)
err_descr = "<no description>";

std::string err_msg(err_type);
if (fxn) {
err_msg += " error in ";
err_msg += fxn;
err_msg += "(): ";
} else {
err_msg += " error: ";
}
err_msg += err_descr;

throw LuaError(err_msg);
}

// Push the list of callbacks (a lua table).
Expand All @@ -82,7 +122,8 @@ void script_error(lua_State *L)
// - runs the callbacks
// - replaces the table and arguments with the return value,
// computed depending on mode
void script_run_callbacks(lua_State *L, int nargs, RunCallbacksMode mode)
void script_run_callbacks_f(lua_State *L, int nargs,
RunCallbacksMode mode, const char *fxn)
{
FATAL_ERROR_IF(lua_gettop(L) < nargs + 1, "Not enough arguments");

Expand All @@ -104,14 +145,12 @@ void script_run_callbacks(lua_State *L, int nargs, RunCallbacksMode mode)
// Stack now looks like this:
// ... <error handler> <run_callbacks> <table> <mode> <arg#1> <arg#2> ... <arg#n>

if (lua_pcall(L, nargs + 2, 1, errorhandler)) {
script_error(L);
}
script_error(L, lua_pcall(L, nargs + 2, 1, errorhandler), fxn);

lua_remove(L, -2); // Remove error handler
}

void log_deprecated(lua_State *L, std::string message)
void log_deprecated(lua_State *L, const std::string &message)
{
static bool configured = false;
static bool dolog = false;
Expand All @@ -131,9 +170,10 @@ void log_deprecated(lua_State *L, std::string message)

if (doerror) {
if (L != NULL) {
script_error(L);
script_error(L, LUA_ERRRUN, NULL);
} else {
FATAL_ERROR("Can't do a scripterror for this deprecated message, so exit completely!");
FATAL_ERROR("Can't do a scripterror for this deprecated message, "
"so exit completely!");
}
}

Expand Down
17 changes: 14 additions & 3 deletions src/script/common/c_internal.h
Expand Up @@ -34,6 +34,16 @@ extern "C" {

#include "common/c_types.h"

#define PCALL_RESL(L, RES) do { \
int result_ = (RES); \
if (result_ != 0) { \
script_error((L), result_, __FUNCTION__); \
} \
} while (0)

#define script_run_callbacks(L, nargs, mode) \
script_run_callbacks_f((L), (nargs), (mode), __FUNCTION__)

// What script_run_callbacks does with the return values of callbacks.
// Regardless of the mode, if only one callback is defined,
// its return value is the total return value.
Expand Down Expand Up @@ -67,8 +77,9 @@ enum RunCallbacksMode
std::string script_get_backtrace(lua_State *L);
int script_error_handler(lua_State *L);
int script_exception_wrapper(lua_State *L, lua_CFunction f);
void script_error(lua_State *L);
void script_run_callbacks(lua_State *L, int nargs, RunCallbacksMode mode);
void log_deprecated(lua_State *L, std::string message);
void script_error(lua_State *L, int pcall_result, const char *fxn);
void script_run_callbacks_f(lua_State *L, int nargs,
RunCallbacksMode mode, const char *fxn);
void log_deprecated(lua_State *L, const std::string &message);

#endif /* C_INTERNAL_H_ */
9 changes: 4 additions & 5 deletions src/script/cpp_api/s_async.cpp
Expand Up @@ -164,9 +164,7 @@ void AsyncEngine::step(lua_State *L, int errorhandler)
lua_pushlstring(L, jobDone.serializedResult.data(),
jobDone.serializedResult.size());

if (lua_pcall(L, 2, 0, errorhandler)) {
script_error(L);
}
PCALL_RESL(L, lua_pcall(L, 2, 0, errorhandler));
}
resultQueueMutex.Unlock();
lua_pop(L, 1); // Pop core
Expand Down Expand Up @@ -293,8 +291,9 @@ void* AsyncWorkerThread::Thread()
toProcess.serializedParams.data(),
toProcess.serializedParams.size());

if (lua_pcall(L, 2, 1, m_errorhandler)) {
scriptError();
int result = lua_pcall(L, 2, 1, m_errorhandler);
if (result) {
PCALL_RES(result);
toProcess.serializedResult = "";
} else {
// Fetch result
Expand Down
4 changes: 2 additions & 2 deletions src/script/cpp_api/s_base.cpp
Expand Up @@ -165,9 +165,9 @@ void ScriptApiBase::realityCheck()
}
}

void ScriptApiBase::scriptError()
void ScriptApiBase::scriptError(int result, const char *fxn)
{
throw LuaError(lua_tostring(m_luastack, -1));
script_error(getStack(), result, fxn);
}

void ScriptApiBase::stackDump(std::ostream &o)
Expand Down
8 changes: 7 additions & 1 deletion src/script/cpp_api/s_base.h
Expand Up @@ -40,6 +40,12 @@ extern "C" {
// use that name to bypass security!
#define BUILTIN_MOD_NAME "*builtin*"

#define PCALL_RES(RES) do { \
int result_ = (RES); \
if (result_ != 0) { \
scriptError(result_, __FUNCTION__); \
} \
} while (0)

class Server;
class Environment;
Expand Down Expand Up @@ -74,7 +80,7 @@ class ScriptApiBase {
{ return m_luastack; }

void realityCheck();
void scriptError();
void scriptError(int result, const char *fxn);
void stackDump(std::ostream &o);

void setServer(Server* server) { m_server = server; }
Expand Down
15 changes: 5 additions & 10 deletions src/script/cpp_api/s_entity.cpp
Expand Up @@ -92,8 +92,7 @@ void ScriptApiEntity::luaentity_Activate(u16 id,
lua_pushlstring(L, staticdata.c_str(), staticdata.size());
lua_pushinteger(L, dtime_s);
// Call with 3 arguments, 0 results
if (lua_pcall(L, 3, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 3, 0, m_errorhandler));
} else {
lua_pop(L, 1);
}
Expand Down Expand Up @@ -140,8 +139,7 @@ std::string ScriptApiEntity::luaentity_GetStaticdata(u16 id)
luaL_checktype(L, -1, LUA_TFUNCTION);
lua_pushvalue(L, object); // self
// Call with 1 arguments, 1 results
if (lua_pcall(L, 1, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 1, 1, m_errorhandler));
lua_remove(L, object); // Remove object

size_t len = 0;
Expand Down Expand Up @@ -210,8 +208,7 @@ void ScriptApiEntity::luaentity_Step(u16 id, float dtime)
lua_pushvalue(L, object); // self
lua_pushnumber(L, dtime); // dtime
// Call with 2 arguments, 0 results
if (lua_pcall(L, 2, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 2, 0, m_errorhandler));
lua_pop(L, 1); // Pop object
}

Expand Down Expand Up @@ -242,8 +239,7 @@ void ScriptApiEntity::luaentity_Punch(u16 id,
push_tool_capabilities(L, *toolcap);
push_v3f(L, dir);
// Call with 5 arguments, 0 results
if (lua_pcall(L, 5, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 5, 0, m_errorhandler));
lua_pop(L, 1); // Pop object
}

Expand All @@ -269,8 +265,7 @@ void ScriptApiEntity::luaentity_Rightclick(u16 id,
lua_pushvalue(L, object); // self
objectrefGetOrCreate(L, clicker); // Clicker reference
// Call with 2 arguments, 0 results
if (lua_pcall(L, 2, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 2, 0, m_errorhandler));
lua_pop(L, 1); // Pop object
}

18 changes: 6 additions & 12 deletions src/script/cpp_api/s_inventory.cpp
Expand Up @@ -48,8 +48,7 @@ int ScriptApiDetached::detached_inventory_AllowMove(
lua_pushinteger(L, to_index + 1); // to_index
lua_pushinteger(L, count); // count
objectrefGetOrCreate(L, player); // player
if (lua_pcall(L, 7, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 7, 1, m_errorhandler));
if(!lua_isnumber(L, -1))
throw LuaError("allow_move should return a number. name=" + name);
int ret = luaL_checkinteger(L, -1);
Expand Down Expand Up @@ -77,8 +76,7 @@ int ScriptApiDetached::detached_inventory_AllowPut(
lua_pushinteger(L, index + 1); // index
LuaItemStack::create(L, stack); // stack
objectrefGetOrCreate(L, player); // player
if (lua_pcall(L, 5, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 5, 1, m_errorhandler));
if (!lua_isnumber(L, -1))
throw LuaError("allow_put should return a number. name=" + name);
int ret = luaL_checkinteger(L, -1);
Expand Down Expand Up @@ -106,8 +104,7 @@ int ScriptApiDetached::detached_inventory_AllowTake(
lua_pushinteger(L, index + 1); // index
LuaItemStack::create(L, stack); // stack
objectrefGetOrCreate(L, player); // player
if (lua_pcall(L, 5, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 5, 1, m_errorhandler));
if (!lua_isnumber(L, -1))
throw LuaError("allow_take should return a number. name=" + name);
int ret = luaL_checkinteger(L, -1);
Expand Down Expand Up @@ -139,8 +136,7 @@ void ScriptApiDetached::detached_inventory_OnMove(
lua_pushinteger(L, to_index + 1); // to_index
lua_pushinteger(L, count); // count
objectrefGetOrCreate(L, player); // player
if (lua_pcall(L, 7, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 7, 0, m_errorhandler));
}

// Report put items
Expand All @@ -164,8 +160,7 @@ void ScriptApiDetached::detached_inventory_OnPut(
lua_pushinteger(L, index + 1); // index
LuaItemStack::create(L, stack); // stack
objectrefGetOrCreate(L, player); // player
if (lua_pcall(L, 5, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 5, 0, m_errorhandler));
}

// Report taken items
Expand All @@ -189,8 +184,7 @@ void ScriptApiDetached::detached_inventory_OnTake(
lua_pushinteger(L, index + 1); // index
LuaItemStack::create(L, stack); // stack
objectrefGetOrCreate(L, player); // player
if (lua_pcall(L, 5, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 5, 0, m_errorhandler));
}

// Retrieves core.detached_inventories[name][callbackname]
Expand Down
17 changes: 6 additions & 11 deletions src/script/cpp_api/s_item.cpp
Expand Up @@ -42,8 +42,7 @@ bool ScriptApiItem::item_OnDrop(ItemStack &item,
LuaItemStack::create(L, item);
objectrefGetOrCreate(L, dropper);
pushFloatPos(L, pos);
if (lua_pcall(L, 3, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 3, 1, m_errorhandler));
if (!lua_isnil(L, -1)) {
try {
item = read_item(L,-1, getServer());
Expand All @@ -68,8 +67,7 @@ bool ScriptApiItem::item_OnPlace(ItemStack &item,
LuaItemStack::create(L, item);
objectrefGetOrCreate(L, placer);
pushPointedThing(pointed);
if (lua_pcall(L, 3, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 3, 1, m_errorhandler));
if (!lua_isnil(L, -1)) {
try {
item = read_item(L,-1, getServer());
Expand All @@ -94,8 +92,7 @@ bool ScriptApiItem::item_OnUse(ItemStack &item,
LuaItemStack::create(L, item);
objectrefGetOrCreate(L, user);
pushPointedThing(pointed);
if (lua_pcall(L, 3, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 3, 1, m_errorhandler));
if(!lua_isnil(L, -1)) {
try {
item = read_item(L,-1, getServer());
Expand All @@ -116,7 +113,7 @@ bool ScriptApiItem::item_OnCraft(ItemStack &item, ServerActiveObject *user,
lua_getfield(L, -1, "on_craft");
LuaItemStack::create(L, item);
objectrefGetOrCreate(L, user);

// Push inventory list
std::vector<ItemStack> items;
for (u32 i = 0; i < old_craft_grid->getSize(); i++) {
Expand All @@ -125,8 +122,7 @@ bool ScriptApiItem::item_OnCraft(ItemStack &item, ServerActiveObject *user,
push_items(L, items);

InvRef::create(L, craft_inv);
if (lua_pcall(L, 4, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 4, 1, m_errorhandler));
if (!lua_isnil(L, -1)) {
try {
item = read_item(L,-1, getServer());
Expand Down Expand Up @@ -156,8 +152,7 @@ bool ScriptApiItem::item_CraftPredict(ItemStack &item, ServerActiveObject *user,
push_items(L, items);

InvRef::create(L, craft_inv);
if (lua_pcall(L, 4, 1, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 4, 1, m_errorhandler));
if (!lua_isnil(L, -1)) {
try {
item = read_item(L,-1, getServer());
Expand Down
6 changes: 2 additions & 4 deletions src/script/cpp_api/s_mainmenu.cpp
Expand Up @@ -55,8 +55,7 @@ void ScriptApiMainMenu::handleMainMenuEvent(std::string text)

// Call it
lua_pushstring(L, text.c_str());
if (lua_pcall(L, 1, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 1, 0, m_errorhandler));
}

void ScriptApiMainMenu::handleMainMenuButtons(const StringMap &fields)
Expand Down Expand Up @@ -85,7 +84,6 @@ void ScriptApiMainMenu::handleMainMenuButtons(const StringMap &fields)
}

// Call it
if (lua_pcall(L, 1, 0, m_errorhandler))
scriptError();
PCALL_RES(lua_pcall(L, 1, 0, m_errorhandler));
}

0 comments on commit bcf47bc

Please sign in to comment.