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

os.exec misbehaves with programs trapping SIGINT #4889

Closed
dkaszews opened this issue Mar 27, 2024 · 9 comments
Closed

os.exec misbehaves with programs trapping SIGINT #4889

dkaszews opened this issue Mar 27, 2024 · 9 comments
Labels
Milestone

Comments

@dkaszews
Copy link
Contributor

Xmake Version

xmake v2.8.9+20240321

Operating System Version and Architecture

Ubuntu 22.04 aarch64

Describe Bug

I am using xmake to compile RISC-V code and have created a phony target to run it with spike simulator. To exit spike, you need to first enter interactive mode with Ctrl+C, then run quit. Under xmake, the Ctrl+C just kills xmake, so spike gets detached from but kept alive, making it spam I/O errors and impossible to exit without kill as anything you type goes into shell.

To reproduce, use the test script below:

$ ./trap.sh
Still alive
^CType "quit" to exit
quit

$ xmake run trap
Still alive
^CType "quit" to exit
$
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
quit
Command 'quit' not found
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
trap.sh: line 5: read: read error: 0: Input/output error
Still alive
...

Expected Behavior

Xmake should trap SIGINT for the duration of os.run and wait for the spawned process to exit on its own, or send it SIGTERM/SIGKILL on repeated offences (configurable?). Other programs which trap SIGKILL include:

  • vim - ignores it for most part; may be used with xmake with phony targets running git or opening files
  • bitbake - requires second or third press which will not arrive under xmake; toolchain for Linux images

Project Configuration

xmake.lua

target("trap")
    set_kind("phony")
    -- TODO: Ctrl-C does not kill inner process
    on_run(function (target)
        os.exec("$(projectdir)/trap.sh")
    end)

trap.sh

#!/usr/bin/env bash
function handler { echo 'Type "quit" to exit'; }
trap handler SIGINT
while true; do
    read -t 3 -p $'Still alive\n' x || continue
    echo $x | grep -q 'quit' && break
done

Additional Information and Error Logs

Traping signals in Lua can be seen here: https://stackoverflow.com/a/34409274/2894535

@dkaszews dkaszews added the bug label Mar 27, 2024
@waruqi
Copy link
Member

waruqi commented Mar 28, 2024

Normally, all child processes and xmake belong to the same process group, and they all receive the SIGINT signal, even if xmake doesn't catch it, and all child processes and xmake are exited at the same time.

Therefore, by default, xmake should not trap for SIGINT and then force all child processes to exit, but should let the child processes handle the SIGINT signal themselves.

Then the sub-process you're executing now, which is a special case, caught SIGINT on its own and didn't execute immediately to exit. It needs the user to perform some interaction to continue to exit. I can provide lua's SIGINT signal registration interface, and user can handle how to exit the child process according to their needs.

Also, it might be better if the child process could go to exit by executing other interactive commands instead of SIGINT. like gdb, lldb. we can use exclusive argument to ignore SIGINT.

os.execv("lldb", {}, {exclusive = true}) 
ruki-2:test ruki$ xmake r -d
(lldb) target create "/private/tmp/test/build/macosx/x86_64/release/test"
Current executable set to '/private/tmp/test/build/macosx/x86_64/release/test
' (x86_64).
(lldb) ^C
(lldb) ^C
(lldb) ^C
(lldb) quit

waruqi added a commit that referenced this issue Mar 28, 2024
@waruqi
Copy link
Member

waruqi commented Mar 28, 2024

or you can try this patch, I added os.signal api, user can register signal handler to handle SIGINT. #4890

xmake update -s dev

test.lua

import("core.base.process")

function main()
    local proc
    os.signal(os.SIGINT, function (signo)
        print("sigint")
        if proc then
            proc:kill()
        end
    end)
    proc = process.open("./trap.sh")
    if proc then
        proc:wait()
        proc:close()
    end
end
ruki$ xmake l test.lua
Still alive
Still alive
^CType "quit" to exit
sigint

@waruqi waruqi added this to the v2.9.1 milestone Mar 28, 2024
waruqi added a commit that referenced this issue Mar 28, 2024
@dkaszews
Copy link
Contributor Author

@waruqi Looks good, will have to test it whether it suits my usage. Agreed it would be better if program offered a way to interact with it other than SIGINT, but sometimes you just have to work with what you have.

One more suggestion: it could be useful to provide ability to reset a signal handler to default by calling os.signal(os.SIGINT, nil). On Unix it is done with signal(SIGINT, SIG_DFL);, on Windows with SetConsoleCtrlHandler(NULL, FALSE);.

@dkaszews
Copy link
Contributor Author

I tested it, adding os.signal(os.SIGINT, function() end) works great - xmake is not killed, the signal is forwarded to script (unlike with { exclusive = true }). I can create a pull request implementing signal resetting if you want.

@waruqi
Copy link
Member

waruqi commented Mar 28, 2024

I tested it, adding os.signal(os.SIGINT, function() end) works great - xmake is not killed, the signal is forwarded to script (unlike with { exclusive = true }). I can create a pull request implementing signal resetting if you want.

you can try.

os.signal(os.SIGINT, handler)
os.signal(os.SIGINT, os.SIGDFL) -- clear
os.signal(os.SIGINT, os.SIGIGN) -- ignore

@dkaszews
Copy link
Contributor Author

I implemented it in #4895 , verified with modified xmake.lua:

target("trap")
    set_kind("phony")
    on_run(function (target)
        print('SIGINT trapped and forwarded, will print "trap" and "Type quit to exit"')
        os.signal(os.SIGINT, function() print('trap'); end)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT ignored, will do nothing')
        os.signal(os.SIGINT, os.SIGIGN)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT default, will kill xmake and break running script')
        os.signal(os.SIGINT, os.SIGDFL)
        os.execv("$(projectdir)/trap.sh", {})
    end)

@waruqi
Copy link
Member

waruqi commented Mar 31, 2024

We can use signal module now. #4908

import("core.base.process")
import("core.base.signal")

function main()
    local proc
    signal.register(signal.SIGINT, function (signo)
        print("sigint")
        if proc then
            proc:kill()
        end
    end)
    proc = process.open("./trap.sh")
    if proc then
        proc:wait()
        proc:close()
    end
end
signal.register
signal.ignore
signal.reset

@waruqi waruqi closed this as completed Mar 31, 2024
@dkaszews
Copy link
Contributor Author

dkaszews commented Apr 3, 2024

@waruqi I just got back to trying it out, cannot get it to work inside xmake.lua. I modified the previous test script, but am getting error: attempt to index a nil value (global 'signal'). If I try to add import("core.base.signal") as per your example, then I get an error: ./xmake.lua:2: global 'import' is not callable (a nil value), ditto for require.

target("trap")
    set_kind("phony")
    on_run(function (target)
        print('SIGINT trapped and forwarded')
        signal.register(signal.SIGINT, function() print('trap'); end)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT ignored')
        signal.ignore(signal.SIGINT)
        os.execv("$(projectdir)/trap.sh", {})

        print('SIGINT default')
        signal.reset(signal.SIGINT)
        os.execv("$(projectdir)/trap.sh", {})
    end)

@waruqi
Copy link
Member

waruqi commented Apr 3, 2024

you should call import in on_run

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

No branches or pull requests

2 participants