Enable abort support #15392

RealJohnSmith · 2024-07-02T11:12:25Z

Summary

Add abort setup code (nlr_set_abort) to standard runtime. This makes the standard runtime respond to abort signal without any further modifications.

When aborted, the program exits with 137 exit code (configurable, same as posix sig abort), to differentiate from a normal shutdown.
When exited by exception/crash, the program will exit with exit code 1 (configurable)
When exited by exception KeyboardInterrupt, the program will exit with exit code 130 (configurable, same as posix sig int)

This PR updates previous PR/discussion at: #14419 but in this new PR, the code is rebased on top of the current master.

Testing

Tested on ESP32, should work in other places. The changes are guarded by #if abort is enabled in the runtime -> only people who want the abort functionality will get this change.

Trade-offs and Alternatives

This code may return non-zero exit code in scenarios where previous version did return zero exit code. The exit codes are configurable and the defaults are zero, so not to introduce any breaking changes.

github-actions · 2024-07-02T11:21:40Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

codecov · 2024-07-02T12:05:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.41%. Comparing base (91f4a6b) to head (a931243).
Report is 49 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15392      +/-   ##
==========================================
- Coverage   98.43%   98.41%   -0.03%     
==========================================
  Files         161      163       +2     
  Lines       21281    21576     +295     
==========================================
+ Hits        20948    21234     +286     
- Misses        333      342       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dpgeorge · 2024-07-03T14:21:32Z

shared/runtime/pyexec.c

+            ret = pyexec_abort;
+        } else
+        #endif
+        if (mp_obj_is_subclass_fast(MP_OBJ_FROM_PTR(((mp_obj_base_t *)nlr.ret_val)->type), MP_OBJ_FROM_PTR(&mp_type_SystemExit))) { // system exit


This updated SystemExit handling code (the second commit in the PR) adds quite a bit of code size to existing builds, which don't really use the functionality.

How useful is this new code? Does it make sense to guard it with a compile-time configuration flag?

Or perhaps to make it even more flexible, use a macro similar to other port-specific hooks.

I.e. MICROPY_VM_HOOK_ABORT. This way, the code can be customized without modifying pyexec.c.

I am not sure how to do the part with the hook. What do you mean, can you please explain?

It doesnt feel like a lot of code size to me, but I can add some compile time guards, that is not a problem

I implemented the guard using a new macro "MICROPY_ENABLE_EXIT_CODE_HANDLING"

if set to 0, the behavior should be the same as before. As well as code size. This means, the exit code is written into the variable pyexec_system_exit internally, before the SystemExit exception is raised (e.g. soft reset method).

if set to 1, the values defined in macros such as PYEXEC_KEYBOARD_INTERRUPT are used for the appropriate exit value.

I am not sure how to do the part with the hook. What do you mean, can you please explain?

Maybe I am remembering wrong, but in the previous PR, it seems like at one point, there was some extra printing or something in the abort path. I don't see that now. So this is something we could add later if needed.

What you did with MICROPY_ENABLE_EXIT_CODE_HANDLING looks good to me.

RealJohnSmith · 2024-07-17T21:37:46Z

What do you think, @dpgeorge @dlech? Is this ok for merge?

dlech · 2024-07-08T20:03:30Z

shared/runtime/pyexec.c

+            ret = pyexec_abort;
+        } else
+        #endif
+        if (mp_obj_is_subclass_fast(MP_OBJ_FROM_PTR(((mp_obj_base_t *)nlr.ret_val)->type), MP_OBJ_FROM_PTR(&mp_type_SystemExit))) { // system exit


I am not sure how to do the part with the hook. What do you mean, can you please explain?

Maybe I am remembering wrong, but in the previous PR, it seems like at one point, there was some extra printing or something in the abort path. I don't see that now. So this is something we could add later if needed.

What you did with MICROPY_ENABLE_EXIT_CODE_HANDLING looks good to me.

dlech · 2024-08-04T18:01:19Z

shared/runtime/pyexec.c

+#if MICROPY_ENABLE_VM_ABORT
+int pyexec_abort = 0; // kept at 0 for backward compatibility
+#endif
+int pyexec_keyboard_interrupt = 0; // kept at 0 for backward compatibility


Do we still need pyexec_keyboard_interrupt? It doesn't look like it is being used anywhere.

we dont, I forgot it there

dlech · 2024-08-04T18:07:21Z

shared/runtime/pyexec.c

@@ -45,6 +45,11 @@

 pyexec_mode_kind_t pyexec_mode_kind = PYEXEC_MODE_FRIENDLY_REPL;
 int pyexec_system_exit = 0;
+#if MICROPY_ENABLE_VM_ABORT
+int pyexec_abort = 0; // kept at 0 for backward compatibility


Since abort handling wasn't implemented before, I'm not sure we need to worry about backwards compatibility here.

Ok, removing the comment. The original value was 0 and I think we should keep it there.

dlech · 2024-08-04T18:07:49Z

shared/runtime/pyexec.h

+#if MICROPY_ENABLE_VM_ABORT
+// Set this to the value (eg PYEXEC_ABORT) that will be propagated through
+// the pyexec functions if the VM is aborted for immediate exit.
+// It will reset to 0 at the start of each execution (eg each REPL entry).


I don't see where this is reset to 0 in the code.

Here:

micropython/shared/runtime/pyexec.c

Lines 82 to 83 in d2bfc85

// by default a SystemExit exception returns 0

pyexec_system_exit = 0;

This is the original part (e.g. relevant without the MICROPY_ENABLE_EXIT_CODE_HANDLING) I didn't modify it in any way.

Signed-off-by: John Smith <[email protected]>

RealJohnSmith · 2024-08-08T16:51:52Z

If I understand correctly the conflict that occurred since the last time, the pyexec_system_exit was removed. I removed it as well, which also solves the comments you had

RealJohnSmith · 2024-08-08T16:52:39Z

Can we merge it now?

dpgeorge · 2024-08-29T05:33:10Z

Superseded by #15730.

RealJohnSmith force-pushed the enable-abort-support branch from 253df70 to 3ce8061 Compare July 2, 2024 11:58

dpgeorge mentioned this pull request Jul 2, 2024

runtime/abort: Add support for vm_abort in standard runtime #14419

Closed

dpgeorge added the py-core Relates to py/ directory in source label Jul 2, 2024

dpgeorge reviewed Jul 3, 2024

View reviewed changes

RealJohnSmith force-pushed the enable-abort-support branch 3 times, most recently from a212069 to d2bfc85 Compare July 8, 2024 18:17

RealJohnSmith requested review from dlech and dpgeorge August 2, 2024 14:03

dlech reviewed Aug 4, 2024

View reviewed changes

runtime/exitcodes: Set exit code according to the SystemExit exception.

a931243

Signed-off-by: John Smith <[email protected]>

RealJohnSmith force-pushed the enable-abort-support branch from d2bfc85 to a931243 Compare August 8, 2024 16:50

RealJohnSmith requested a review from dlech August 12, 2024 07:30

RealJohnSmith closed this Aug 27, 2024

RealJohnSmith mentioned this pull request Aug 27, 2024

runtime/exitcodes: Set exit code according to the SystemExit exception. #15730

Closed

RealJohnSmith mentioned this pull request Sep 17, 2024

runtime/exitcodes: Set exit code according to the SystemExit exception. #15863

Open

	// by default a SystemExit exception returns 0
	pyexec_system_exit = 0;

Uh oh!

Enable abort support #15392

Enable abort support #15392

Uh oh!

Conversation

RealJohnSmith commented Jul 2, 2024

Summary

Testing

Trade-offs and Alternatives

Uh oh!

github-actions bot commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RealJohnSmith commented Jul 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RealJohnSmith commented Aug 8, 2024

Uh oh!

RealJohnSmith commented Aug 8, 2024

Uh oh!

dpgeorge commented Aug 29, 2024

Uh oh!

Uh oh!

github-actions bot commented Jul 2, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading