Skip to content

panic: add __attribute__((noreturn)) #if __GNUC__#2581

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.

Sign up for GitHub

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

Merged
jajanusz merged 2 commits into from
Mar 27, 2020
Merged

panic: add __attribute__((noreturn)) #if __GNUC__ #2581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2405394
Testbench: panic() calls abort()
marc-hb Mar 26, 2020
a343042
panic: add __attribute__((noreturn)) #if __GNUC__
marc-hb Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/include/sof/debug/panic.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,20 +12,24 @@
#include <ipc/trace.h>
#include <stdint.h>

#ifdef __clang_analyzer__
#define SOF_NORETURN __attribute__((analyzer_noreturn))
#elif __GNUC__
#define SOF_NORETURN __attribute__((noreturn))
#else
#define SOF_NORETURN
#endif

#ifndef RELATIVE_FILE
#error "This file requires RELATIVE_FILE to be defined. " \
"Add it to CMake's target with sof_append_relative_path_definitions."
#endif

void dump_panicinfo(void *addr, struct sof_ipc_panic_info *panic_info);
void panic_rewind(uint32_t p, uint32_t stack_rewind_frames,
struct sof_ipc_panic_info *panic_info, uintptr_t *data);
#if __clang_analyzer__
void __panic(uint32_t p, char *filename, uint32_t linenum)
__attribute__((analyzer_noreturn));
#else
void __panic(uint32_t p, char *filename, uint32_t linenum);
#endif
struct sof_ipc_panic_info *panic_info, uintptr_t *data)
SOF_NORETURN;
void __panic(uint32_t p, char *filename, uint32_t linenum) SOF_NORETURN;

/* panic dump filename and linenumber of the call */
#define panic(x) __panic((x), (RELATIVE_FILE), (__LINE__))
Expand Down
6 changes: 5 additions & 1 deletion tools/testbench/panic.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -5,5 +5,9 @@
// Author: Tomasz Lauda <tomasz.lauda@linux.intel.com>

#include <sof/debug/panic.h>
#include <stdlib.h>

void __panic(uint32_t p, char *filename, uint32_t linenum) { }
void __panic(uint32_t p, char *filename, uint32_t linenum)
{
abort();
}

@marc-hbmarc-hbMar 19, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I'm not familiar with the testbench so not sure what to do here. @jajanusz would some while(1) sleep be better?

@jajanuszjajanuszMar 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

something like abort() will be better, cos at least program will crash and it's easier to debug than freeze

@jajanuszjajanuszMar 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also tools should be more clever when we switch eventually to c11, cos there is _Noreturn void abort(void);

@jajanuszjajanuszMar 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@marc-hb It should be separate commit - before adding attributes

@jajanuszjajanuszMar 26, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Github shows that it's 2nd commit. I preferred it to be the 1st, cos It may break git bisect for testbench if you have noreturn attribute for function without abort in it, but I'm not sure.

@marc-hbmarc-hbMar 26, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

The addition of the abort() is the first commit. It's hard to tell because commits are ordered sometimes chronologically and sometimes in reverse order and because
github doesn't care about git history (vendor lock-in), but if you click on this link and check the "parent" you can make sure: a343042b959dfbb

@marc-hbmarc-hbMar 26, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Also tools should be more clever when we switch eventually to c11,

https://stackoverflow.com/questions/48615184/does-visual-studio-2017-fully-support-c99

(no)

@jajanuszjajanuszMar 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're right, I forgot that GH doesn't care about order and sorts by time ...