lib_abort.c: Change tall to user-space exit() into system call _exit()

[pussuw]

Summary

The POSIX standard dictates that during abnormal termination the functions
registered by atexit() are not called, also flushing the streams is
optional. So in this case, it is perfectly legal / better to call the
kernel system call _exit() instead.

GCC stdlib also dictates that a call to abort will not call the atexit() on_exit() registered functions.

This fixes regression issues caused by removal exit() from the kernel.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/abort.html
https://en.cppreference.com/w/c/program/abort

Impact

Fix regression for calls to abort()

Testing

CI and icicle:knsh

[pussuw]

./libs/libc/assert/lib_assert.c:47: abort();
./libs/libc/obstack/lib_obstack_malloc.c:50: abort();
./mm/iob/iob_test.c:182: abort();
./arch/xtensa/src/esp32/esp32_wireless.c:433: abort();
./arch/xtensa/src/esp32/esp32_wifi_adapter.c:4483: abort();
./arch/xtensa/src/esp32/esp32_psram.c:1693: abort();
./arch/xtensa/src/esp32/esp32_psram.c:1753: abort();
./arch/xtensa/src/esp32s2/esp32s2_spiram.c:205: abort();
./arch/xtensa/src/esp32s2/esp32s2_spiram.c:346: abort();
./arch/xtensa/src/esp32s3/esp32s3_spiram.c:116: abort();
./arch/xtensa/src/esp32s3/esp32s3_spiram.c:126: abort();
./arch/xtensa/src/esp32s3/esp32s3_spiram.c:225: abort();
./arch/xtensa/src/esp32s3/esp32s3_spiram.c:236: abort();
./arch/xtensa/src/esp32s3/esp32s3_spiram.c:316: abort();
./arch/xtensa/src/esp32s3/esp32s3_spiram.c:346: abort();
./arch/risc-v/src/bl602/bl602_os_hal.c:193: abort();
./arch/risc-v/src/esp32c3/esp32c3_wifi_adapter.c:4747: abort();
./arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:166: abort();

I think the first 3 are fine, the rest I'm not sure what to do. I think the purpose is to do abnormal termination so _exit(EXIT_FAILURE) should be enough ?

Any comments from @tmedicci and co. from Espressif ?

[tmedicci]

./libs/libc/assert/lib_assert.c:47: abort(); ./libs/libc/obstack/lib_obstack_malloc.c:50: abort(); ./mm/iob/iob_test.c:182: abort(); ./arch/xtensa/src/esp32/esp32_wireless.c:433: abort(); ./arch/xtensa/src/esp32/esp32_wifi_adapter.c:4483: abort(); ./arch/xtensa/src/esp32/esp32_psram.c:1693: abort(); ./arch/xtensa/src/esp32/esp32_psram.c:1753: abort(); ./arch/xtensa/src/esp32s2/esp32s2_spiram.c:205: abort(); ./arch/xtensa/src/esp32s2/esp32s2_spiram.c:346: abort(); ./arch/xtensa/src/esp32s3/esp32s3_spiram.c:116: abort(); ./arch/xtensa/src/esp32s3/esp32s3_spiram.c:126: abort(); ./arch/xtensa/src/esp32s3/esp32s3_spiram.c:225: abort(); ./arch/xtensa/src/esp32s3/esp32s3_spiram.c:236: abort(); ./arch/xtensa/src/esp32s3/esp32s3_spiram.c:316: abort(); ./arch/xtensa/src/esp32s3/esp32s3_spiram.c:346: abort(); ./arch/risc-v/src/bl602/bl602_os_hal.c:193: abort(); ./arch/risc-v/src/esp32c3/esp32c3_wifi_adapter.c:4747: abort(); ./arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:166: abort();

I think the first 3 are fine, the rest I'm not sure what to do. I think the purpose is to do abnormal termination so _exit(EXIT_FAILURE) should be enough ?

Any comments from @tmedicci and co. from Espressif ?

Hi @pussuw !

Yes, I'd say that just calling _exit(EXIT_FAILURE) would fit, but It's needed to be tested properly. I'm away from work today. Tomorrow I'll check this and test it.

@acassis , @gustavonihei and @lucasssvaz , could you please take a look too?

[gustavonihei]

@pussuw It is fine to just change all those abort to PANIC.

[gustavonihei]

@pussuw It is fine to just change all those abort to PANIC.

Except the one from ./arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:166, which must hook to NuttX abort.

[pussuw]

Thank you for going through the list, I'll create a patch momentarily.

I assume changing abort() to PANIC() essentially achieves the same thing here ? abort() in this case crashes the system as only the idle task (if even that?) is running ?

[gustavonihei]

I assume changing abort() to PANIC() essentially achieves the same thing here ?

In the Kernel case abort is redefined to PANIC, so it won’t be called:

nuttx/libs/libc/assert/lib_assert.c

Lines 34 to 38 in 1d41182

	/* If assert() is called from kernel, must not call user API abort */
	#ifdef __KERNEL__
	# define abort PANIC
	#endif

I am afraid it will end up in a recursion, but that would be a different issue.

[pussuw]

I assume changing abort() to PANIC() essentially achieves the same thing here ?

In the Kernel case abort is redefined to PANIC, so it won’t be called:

nuttx/libs/libc/assert/lib_assert.c

Lines 34 to 38 in 1d41182

	/* If assert() is called from kernel, must not call user API abort */
	#ifdef __KERNEL__
	# define abort PANIC
	#endif

I am afraid it will end up in a recursion, but that would be a different issue.

This is a recent addition by me and the visibility of this re-definition is only for lib_assert.c, this is not visible anywhere else. The reason was to prevent calling abort() from assert. This is why I was trying to understand your use-case.

For example, esp32_wireless.c is this a fatal error?
  cal_data = kmm_zalloc(sizeof(esp_phy_calibration_data_t));
  if (!cal_data)
    {
      wlerr("ERROR: Failed to allocate PHY calibration data buffer.");
      abort();
    }

abort() in this case just kills the task/process that was running. Changing that to PANIC will crash the whole system.

I'm trying to understand the use-case, so I don't create a fatal regression for you.

[gustavonihei]

Changing that to PANIC will crash the whole system.

ESP32 wireless drivers call abort with the intent of crashing the system, it is a heritage from IDF.

[pussuw]

I ended up removing the call to the user-space exit() and now abort() calls _exit() instead. I think this is more correct behavior (closer to what abort() is supposed to do).

This should fix the regression sufficiently.

[xiaoxiang781216]

Let's ignore the ci broken which is fixed by #8611 and merge this simple change.