Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lua throw: set panic function as main priority #6

Open
wants to merge 1 commit into
base: ee-v5.4.4
Choose a base branch
from

Conversation

israpps
Copy link

@israpps israpps commented Dec 26, 2023

Makes panic function trigger first (if programmer provided one) instead of testing first on longjmp() Wich has issues on PS2SDK lately.

Impact on behaviour of already existing apps: Only if they already use a panic func.

Example of panic function (adapted to enceladus and tweaked to avoid the longjmp crash)

#define TPRINTF(arg, x...) \
    printf(arg, ##x); \
    scr_printf(arg, ##x)


int test_error(lua_State * L) {
    scr_clear();
    init_scr();
    scr_clear();
    scr_clear();
    scr_setCursor(0);
    int n = lua_gettop(L);
    int i;
    scr_printf("\t");
    TPRINTF("LUA ERR.\n");

    if (n == 0) {
        scr_printf("\t");
        TPRINTF("Stack is empty!!!!\n");
    }

    for (i = 1; i <= n; i++) {
        scr_printf("\t");
        TPRINTF("%i: ", i);
        switch(lua_type(L, i)) {
        case LUA_TNONE:
            TPRINTF("Invalid");
            break;
        case LUA_TNIL:
            TPRINTF("(Nil)");
            break;
        case LUA_TNUMBER:
            TPRINTF("(Number) %f", lua_tonumber(L, i));
            break;
        case LUA_TBOOLEAN:
            TPRINTF("(Bool)   %s", (lua_toboolean(L, i) ? "true" : "false"));
            break;
        case LUA_TSTRING:
            TPRINTF("%s", lua_tostring(L, i));
            break;
        case LUA_TTABLE:
            TPRINTF("(Table)");
            break;
        case LUA_TFUNCTION:
            TPRINTF("(Function)");
            break;
        default:
            TPRINTF("Unknown");
        }
    TPRINTF("\n");
    }
    SleepThread(); //else, crash still ocurs
    return 0;
}

Makes panic function trigger first (if programmer provided one) instead of testing first on `longjmp()` Wich has issues on PS2SDK lately
israpps added a commit to israpps/Enceladus that referenced this pull request Dec 26, 2023
depends on ps2dev/lua#6 to be able to fix the issues with `longjmp()`
@fjtrujy
Copy link
Member

fjtrujy commented Dec 28, 2023

@israpps please take a look to check if this is still failing, if not close the PR

@israpps
Copy link
Author

israpps commented Dec 28, 2023

@israpps please take a look to check if this is still failing, if not close the PR

made some tests and it seems to be fixed.

@israpps israpps closed this Dec 28, 2023
@israpps israpps reopened this Dec 28, 2023
@israpps
Copy link
Author

israpps commented Dec 28, 2023

thinking about this a bit, this feature is actually helpful, since the lua stack has more information when the panic function is called compared to the information obtained by checking it after the lua_pcall finished

and since the panic feature has to be explicitly coded by the developer, we dont have to worry about it changing existing projects behavior

@fjtrujy
Copy link
Member

fjtrujy commented Dec 28, 2023

thinking about this a bit, this feature is actually helpful, since the lua stack has more information when the panic function is called compared to the information obtained by checking it after the lua_pcall finished

and since the panic feature has to be explicitly coded by the developer, we dont have to worry about it changing existing projects behavior

But this is a “common behavior to all platforms ” if you think it still makes sense my recommendation is to create PR to original LUA repo

@israpps
Copy link
Author

israpps commented Dec 28, 2023

thinking about this a bit, this feature is actually helpful, since the lua stack has more information when the panic function is called compared to the information obtained by checking it after the lua_pcall finished

and since the panic feature has to be explicitly coded by the developer, we dont have to worry about it changing existing projects behavior

But this is a “common behavior to all platforms ” if you think it still makes sense my recommendation is to create PR to original LUA repo

The original purpose of the panic function (if I understood correctly) is to provide a "if everything else fails" error handler. Not sure they'll like it.

@fjtrujy
Copy link
Member

fjtrujy commented Dec 28, 2023

So then I would be against apply this change, the more “mainstream” we are, the better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants