Report every abnormal end of the standalone driver - #5
SMI-Lab-Inha wants to merge 4 commits into
Conversation
A process that is interrupted or hits a fatal fault now writes one line to stderr naming the cause and the simulated time of the last committed step, then lets the event continue to the runtime's own handler, so the exit status is unchanged. On Windows a console-control handler, a vectored handler for fatal faults and an unhandled-exception filter (with a stack guarantee, so a stack overflow is reported) are used; on Linux and macOS signal handlers on an alternate stack. Only the driver installs them; the shared library leaves the host process alone. Every non-zero exit the driver makes itself now ends stderr with the closing line "CableDyn_driver: ended with exit code <n>". A process ended from outside (taskkill /F, End task, kill -9) runs none of its own code and cannot report anything; the missing closing line identifies it, since taskkill /F leaves exit code 1, the code of a refused input. The Python runner reports such a run as ended early, with the time its output reached. Tests: a stack overflow on a 1 MiB stack, a null-pointer write and SIGTERM each end the process with the report; the march records the time of its last committed step; the driver's failure checks require the closing line.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12992a9eff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| alt.ss_sp = cabledyn_altstack; | ||
| alt.ss_size = sizeof cabledyn_altstack; | ||
| alt.ss_flags = 0; | ||
| sigaltstack(&alt, NULL); |
There was a problem hiding this comment.
When OpenMP is enabled, the parallel line and element loops can fault on worker threads, but sigaltstack() configures only the calling thread; the Windows SetThreadStackGuarantee() call has the same limitation. Since installation occurs only on the main driver thread, a stack overflow in an OpenMP worker still attempts to run this handler on the exhausted worker stack and can terminate without the promised report. Each participating thread needs its own alternate/guaranteed stack.
Useful? React with 👍 / 👎.
| break; | ||
| } | ||
| } | ||
| raise(sig); |
There was a problem hiding this comment.
For a hardware-generated SIGSEGV, SIGBUS, SIGILL, or SIGFPE, re-delivering the signal with raise() creates a new user-generated signal. The restored runtime handler or core dump therefore receives the nested raise() context rather than the original signal metadata and fault address, degrading the backtrace needed to diagnose the fatal error. Install an SA_SIGINFO handler and preserve or chain the original siginfo_t/context instead of replacing the fault with a new signal.
Useful? React with 👍 / 👎.
… report Every OpenMP parallel region of the library now starts with CD_Fatal_Thread_Init, which gives the calling thread room to report its own stack overflow: a stack guarantee on Windows, an alternate signal stack (freed when the thread ends) on Linux and macOS. It does nothing until the driver has installed the report, and costs one thread-local test after a thread's first call. Previously only the installing thread had that room, so an overflow on a worker thread could still end the process without a word. On Linux and macOS the handlers are now three-argument (SA_SIGINFO) handlers. A hardware fault returns from the handler under the previous disposition, so the faulting instruction raises the same fault with its own address and context; a previous three-argument handler is called with the original siginfo and context. Only asynchronous signals and faults sent by kill or raise are sent again. Tests: a stack overflow on an OpenMP worker with a 1 MiB stack is reported (it ended silently without the per-thread setup); a forked child with an earlier SA_SIGINFO handler faults at a known address and that handler receives the kernel's own siginfo, the child ends by the fault's signal, and the report is written once; and every OpenMP parallel region in src/ and app/ is checked to begin with the per-thread call.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 367d7b42df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((cabledyn_previous[k].sa_flags & SA_SIGINFO) == 0 && | ||
| cabledyn_previous[k].sa_handler == SIG_IGN) { |
There was a problem hiding this comment.
On Linux, an inherited disposition can validly combine SIG_IGN with SA_SIGINFO; this condition fails to recognize it as ignored and installs the CableDyn handler instead. When that signal arrives, the chaining path treats the SIG_IGN sentinel as a three-argument callback and jumps to address 1, turning an ignored SIGTERM or SIGHUP into a segmentation fault. Check sa_handler == SIG_IGN regardless of the SA_SIGINFO flag before installing the replacement.
Useful? React with 👍 / 👎.
The previous disposition of each signal is now classified by one helper that compares sa_handler with SIG_IGN, SIG_DFL and SIG_ERR whatever the SA_SIGINFO flag says (sa_handler and sa_sigaction share storage). The report is not installed on a signal that was ignored, with or without SA_SIGINFO, or whose disposition cannot be classified, and it chains only to a real handler function. Previously an inherited SIG_IGN stored with SA_SIGINFO set was taken for a handler: the report was installed, and the ignored signal was then passed to the SIG_IGN sentinel as a function. If restoring the previous disposition fails, the default action is set so the handler is never entered again. Test (POSIX, forked children): SIGTERM set to SIG_IGN with SA_SIGINFO, and to plain SIG_IGN, leaves the child running to a normal exit with no report; SIG_DFL is reported once and ends the child by SIGTERM; a one-argument handler is reported once and then runs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b965006e3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (disposition(&cabledyn_previous[k]) == DISPOSITION_SIGINFO_HANDLER) { | ||
| /* A previous three-argument handler receives the original siginfo and context. */ | ||
| cabledyn_previous[k].sa_sigaction(sig, info, context); |
There was a problem hiding this comment.
When a previous three-argument handler uses SA_RESETHAND, a nonempty sa_mask, or the default self-blocking behavior, calling sa_sigaction directly bypasses those kernel-enforced semantics. For example, if a returning SIGTERM handler has SA_RESETHAND, the first signal should reset it so the second terminates the process, but this path leaves it installed and invokes it again; it also permits reentrancy because the CableDyn action uses SA_NODEFER. Apply the saved action's flags/mask when chaining rather than treating the callback as an ordinary function.
Useful? React with 👍 / 👎.
…n a crash The report no longer calls a previous signal handler itself. It re-installs the saved action exactly as it was (handler, flags and mask) and lets the kernel deliver the signal again: a hardware fault returns so the instruction re-executes, and an asynchronous signal is raised. The previous handler therefore runs under its own SA_RESETHAND, sa_mask and self-blocking, as it would have without the report. Tests (POSIX, forked children): a previous handler with SA_RESETHAND gets the first SIGTERM and a second SIGTERM ends the process; a previous handler with SIGUSR1 in its sa_mask runs with SIGUSR1 and SIGTERM blocked. The fault-context test still sees the kernel's own siginfo. test_modal and stack_modal ended once on a hosted Windows runner with "Exception: SegFault" and no output, and passed on a re-run of the same commit; the toolchain CI resolves passes here. They now install the abnormal-end report, so a recurrence names the fault (stack overflow or access violation), and print the OpenBLAS build and kernel the host selected before any case runs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d7b17b9dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| diagnostic = stderr.strip() or stdout.strip() | ||
| tail = "".join(f"{line}\n" for line in diagnostic.splitlines()[-20:]) | ||
| return ( | ||
| f"{tail}CableDyn driver ended with exit code {returncode} without reporting a " | ||
| f"result{reached}: the process was ended from outside (for example by Task Manager, " | ||
| "taskkill /F or kill -9) or by a fault it could not report." |
There was a problem hiding this comment.
When the Python package is upgraded while --executable, CABLEDYN_DRIVER, or PATH still selects an older CableDyn binary, an ordinary validation or solve failure has no new closing line and is therefore reported as an external kill or unreportable fault. There is no native-version compatibility check before run(), and the Python wheel and executable are distributed separately, so this produces a misleading diagnosis for a supported executable-selection path; preserve the generic failure interpretation when marker support has not been established.
Useful? React with 👍 / 👎.
| DO e = 1, n_elem | ||
| CALL CD_Fatal_Thread_Init() ! this thread can report its own stack overflow |
There was a problem hiding this comment.
In each PARALLEL DO, this call executes once per iteration rather than once per participating thread. That adds an out-of-line C call and TLS/global checks to every production element evaluation even when a library host never installed fatal reporting; this is especially significant here because the surrounding axial kernel is documented as roughly 30 ns and is parallelized specifically for throughput. Use an explicit parallel region that initializes each worker once before the work-sharing loop rather than putting the initialization in the loop body.
Useful? React with 👍 / 👎.
1 participant
When the standalone driver ends abnormally, it now says so.
CableDyn_driver: fatal error: stack overflow (exception 0xC00000FD) after the step at simulated time t = 7534.600 s. The run did not finish …CableDyn_driver: ended with exit code <n>. A process ended from outside (for example withtaskkill /F) leaves no closing line. That is the only way to tell it apart from a refused input, which also exits 1.cabledyn-runandcabledyn-studynow separate three cases: a reported failure, an abnormal end, and a process ended from outside. Each states the time the output reached.doc/standalone_driver.rst, with updates to the CLI and troubleshooting pages. The docs note thattaskkill /IMends every run on the machine.Tests. New CTest cases for overflow, null access and termination reports during and before the march. All existing non-zero-exit checks now require the closing line. The POSIX branch has been desk-checked only, so CI on Linux and macOS is its first compile.