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

core/panic: don't use LOG_ functions in panic handler #20790

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

benpicco
Copy link
Contributor

Contribution description

The LOG_ functions can be overwritten to log e.g. to network or file system.
Calling them in the panic handler will just lead to another panic, obfuscating the original source of the panic.

Testing procedure

Issues/PRs references

@benpicco benpicco requested a review from maribu July 17, 2024 14:20
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports labels Jul 17, 2024
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 17, 2024
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Any specific reason you opted for printf rather than puts?

@benpicco
Copy link
Contributor Author

It uses format strings and the default LOG_ is just printf()

@riot-ci
Copy link

riot-ci commented Jul 17, 2024

Murdock results

✔️ PASSED

3237ed9 core/panic: don't use LOG_ functions in panic handler

Success Failures Total Runtime
10178 0 10178 17m:35s

Artifacts

@Teufelchen1 Teufelchen1 added this pull request to the merge queue Jul 17, 2024
@mguetschow
Copy link
Contributor

Maybe it would have made sense to add a comments somewhere in that file that LOG_* is not used for a reason. Preventing anyone in the future trying to get rid of printfs from making the same mistake again.

Merged via the queue into RIOT-OS:master with commit 388a7d4 Jul 17, 2024
28 checks passed
@benpicco benpicco deleted the log_panic branch July 17, 2024 18:05
@chrysn
Copy link
Member

chrysn commented Aug 29, 2024

For clarification: How is LOG_ more reliable than stdout? Just like logging can be redirected, stdio can be, eg. by going through BLE or to the VFS, and that'd just run into the same issue.

@Teufelchen1
Copy link
Contributor

@benpicco ^

@kaspar030
Copy link
Contributor

For reference, this creates a hard dependency on libc stdio formatting code. LOG_ was used here in the first place to remove that dependency (so tests/minimal gets smaller).

@benpicco
Copy link
Contributor Author

I'm not so concerned about minimal binary size with DEVELHELP=1

I agree that stdio can have the same problem, but we could introduce a flag field to stdio_provider_t so that we could skip high-level stdio implementations in case of panic.

That could also solve the issue brought up in #20364 (comment)

@kaspar030
Copy link
Contributor

I'm not so concerned about minimal binary size with DEVELHELP=1

This is independent of DEVELHELP, core/panic now always uses printf(). Previously, it was possible to skip linking any stdio code by setting -DLOG_LEVEL=LOG_NONE (for core and non-printf using code).

@benpicco
Copy link
Contributor Author

That's just by chance - any module using stdio would void that again - lets rather solve the problem properly by turning all calls to printf() into a no-op when stdio_null is used.
#20872

@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants