* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime
@ 2021-09-15 9:03 Xiang W
2021-09-15 9:03 ` [PATCH V2 2/2] lib: sbi: Add runtime bug detection for csr_read_num/csr_write_num/misa_string Xiang W
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Xiang W @ 2021-09-15 9:03 UTC (permalink / raw)
To: opensbi
Two macros are mainly added. One is called BUG(), which is used to put
in unreachable branches. One named BUG_ON, used for assertion.
Signed-off-by: Xiang W <wxjstz@126.com>
---
include/sbi/sbi_console.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h
index e24ba5f..e75a279 100644
--- a/include/sbi/sbi_console.h
+++ b/include/sbi/sbi_console.h
@@ -11,6 +11,7 @@
#define __SBI_CONSOLE_H__
#include <sbi/sbi_types.h>
+#include <sbi/riscv_asm.h>
struct sbi_console_device {
/** Name of the console device */
@@ -51,4 +52,16 @@ struct sbi_scratch;
int sbi_console_init(struct sbi_scratch *scratch);
+#define BUG() do { \
+ sbi_printf("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
+ while (1) \
+ wfi(); \
+ __builtin_unreachable(); \
+} while (0)
+
+#define BUG_ON(cond) do { \
+ if (!(cond)) \
+ BUG(); \
+} while (0)
+
#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2 2/2] lib: sbi: Add runtime bug detection for csr_read_num/csr_write_num/misa_string 2021-09-15 9:03 [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Xiang W @ 2021-09-15 9:03 ` Xiang W 2021-09-15 9:27 ` [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Andreas Schwab 2021-09-15 12:39 ` 杜东 2 siblings, 0 replies; 9+ messages in thread From: Xiang W @ 2021-09-15 9:03 UTC (permalink / raw) To: opensbi Signed-off-by: Xiang W <wxjstz@126.com> --- lib/sbi/riscv_asm.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c index 4c24a51..281e50a 100644 --- a/lib/sbi/riscv_asm.c +++ b/lib/sbi/riscv_asm.c @@ -11,6 +11,7 @@ #include <sbi/riscv_encoding.h> #include <sbi/sbi_error.h> #include <sbi/sbi_platform.h> +#include <sbi/sbi_console.h> /* determine CPU extension, return non-zero support */ int misa_extension_imp(char ext) @@ -75,6 +76,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz) out[pos++] = '8'; break; default: + BUG(); return; } } @@ -134,6 +136,7 @@ unsigned long csr_read_num(int csr_num) #endif default: + BUG(); break; }; @@ -197,6 +200,7 @@ void csr_write_num(int csr_num, unsigned long val) switchcase_csr_write_16(CSR_MHPMEVENT16, val) default: + BUG(); break; }; -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime 2021-09-15 9:03 [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Xiang W 2021-09-15 9:03 ` [PATCH V2 2/2] lib: sbi: Add runtime bug detection for csr_read_num/csr_write_num/misa_string Xiang W @ 2021-09-15 9:27 ` Andreas Schwab 2021-09-15 10:07 ` Xiang W 2021-09-15 12:39 ` 杜东 2 siblings, 1 reply; 9+ messages in thread From: Andreas Schwab @ 2021-09-15 9:27 UTC (permalink / raw) To: opensbi On Sep 15 2021, Xiang W wrote: > +#define BUG_ON(cond) do { \ > + if (!(cond)) \ > + BUG(); \ Isn't that backwards? Andreas. -- Andreas Schwab, schwab at linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime 2021-09-15 9:27 ` [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Andreas Schwab @ 2021-09-15 10:07 ` Xiang W 0 siblings, 0 replies; 9+ messages in thread From: Xiang W @ 2021-09-15 10:07 UTC (permalink / raw) To: opensbi ? 2021-09-15???? 11:27 +0200?Andreas Schwab??? > On Sep 15 2021, Xiang W wrote: > > > +#define BUG_ON(cond) do { \ > > +???????if (!(cond))????\ > > +???????????????BUG(); \ > > Isn't that backwards? I can't understand what you mean. Regards, Xiang W > > Andreas. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime 2021-09-15 9:03 [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Xiang W 2021-09-15 9:03 ` [PATCH V2 2/2] lib: sbi: Add runtime bug detection for csr_read_num/csr_write_num/misa_string Xiang W 2021-09-15 9:27 ` [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Andreas Schwab @ 2021-09-15 12:39 ` 杜东 2021-09-15 13:56 ` Xiang W 2 siblings, 1 reply; 9+ messages in thread From: 杜东 @ 2021-09-15 12:39 UTC (permalink / raw) To: opensbi ----- Original Message ----- > From: "Xiang W" <wxjstz@126.com> > To: "opensbi" <opensbi@lists.infradead.org> > Cc: "atish patra" <atish.patra@wdc.com>, "anup patel" <anup.patel@wdc.com>, "Xiang W" <wxjstz@126.com> > Sent: Wednesday, September 15, 2021 5:03:29 PM > Subject: [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime > Two macros are mainly added. One is called BUG(), which is used to put > in unreachable branches. One named BUG_ON, used for assertion. > > Signed-off-by: Xiang W <wxjstz@126.com> > --- > include/sbi/sbi_console.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h > index e24ba5f..e75a279 100644 > --- a/include/sbi/sbi_console.h > +++ b/include/sbi/sbi_console.h > @@ -11,6 +11,7 @@ > #define __SBI_CONSOLE_H__ > > #include <sbi/sbi_types.h> > +#include <sbi/riscv_asm.h> > > struct sbi_console_device { > /** Name of the console device */ > @@ -51,4 +52,16 @@ struct sbi_scratch; > > int sbi_console_init(struct sbi_scratch *scratch); > > +#define BUG() do { \ > + sbi_printf("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \ > + while (1) \ > + wfi(); \ > + __builtin_unreachable(); \ > +} while (0) > + > +#define BUG_ON(cond) do { \ > + if (!(cond)) \ > + BUG(); \ > +} while (0) > + If the BUG_ON has a similar semantics as BUG_ON in Linux, it should be: + if (cond) \ + BUG(); \ > #endif > -- > 2.30.2 > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi Regards, Dong ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime 2021-09-15 12:39 ` 杜东 @ 2021-09-15 13:56 ` Xiang W 2021-09-15 14:41 ` Mitchell Horne 0 siblings, 1 reply; 9+ messages in thread From: Xiang W @ 2021-09-15 13:56 UTC (permalink / raw) To: opensbi ? 2021-09-15???? 20:39 +0800?????? > > > ----- Original Message ----- > > From: "Xiang W" <wxjstz@126.com> > > To: "opensbi" <opensbi@lists.infradead.org> > > Cc: "atish patra" <atish.patra@wdc.com>, "anup patel" < > > anup.patel at wdc.com>, "Xiang W" <wxjstz@126.com> > > Sent: Wednesday, September 15, 2021 5:03:29 PM > > Subject: [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at > > runtime > > > Two macros are mainly added. One is called BUG(), which is used to > > put > > in unreachable branches. One named BUG_ON, used for assertion. > > > > Signed-off-by: Xiang W <wxjstz@126.com> > > --- > > include/sbi/sbi_console.h | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h > > index e24ba5f..e75a279 100644 > > --- a/include/sbi/sbi_console.h > > +++ b/include/sbi/sbi_console.h > > @@ -11,6 +11,7 @@ > > #define __SBI_CONSOLE_H__ > > > > #include <sbi/sbi_types.h> > > +#include <sbi/riscv_asm.h> > > > > struct sbi_console_device { > > ????????/** Name of the console device */ > > @@ -51,4 +52,16 @@ struct sbi_scratch; > > > > int sbi_console_init(struct sbi_scratch *scratch); > > > > +#define BUG() do { \ > > +???????sbi_printf("BUG: failure at %s:%d/%s()!\n", __FILE__, > > __LINE__, __func__); \ > > +???????while (1)???????\ > > +???????????????wfi();??\ > > +???????__builtin_unreachable(); \ > > +} while (0) > > + > > +#define BUG_ON(cond) do { \ > > +???????if (!(cond))????\ > > +???????????????BUG(); \ > > +} while (0) > > + > > If the BUG_ON has a similar semantics as BUG_ON in Linux, it should > be: > ? + if (cond)???\ > ? +?????????????BUG(); \ > I want to implement BUG_ON like assert. If the meaning of linux is like this, I think it can be used as a reference Regards? Xiang W > > #endif > > -- > > 2.30.2 > > > > > > -- > > opensbi mailing list > > opensbi at lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > > Regards, > Dong > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime 2021-09-15 13:56 ` Xiang W @ 2021-09-15 14:41 ` Mitchell Horne 2021-09-15 14:49 ` Jessica Clarke 0 siblings, 1 reply; 9+ messages in thread From: Mitchell Horne @ 2021-09-15 14:41 UTC (permalink / raw) To: opensbi On Wed, Sep 15, 2021 at 10:56 AM Xiang W <wxjstz@126.com> wrote: > > ? 2021-09-15???? 20:39 +0800?????? > > > > > > ----- Original Message ----- > > > From: "Xiang W" <wxjstz@126.com> > > > To: "opensbi" <opensbi@lists.infradead.org> > > > Cc: "atish patra" <atish.patra@wdc.com>, "anup patel" < > > > anup.patel at wdc.com>, "Xiang W" <wxjstz@126.com> > > > Sent: Wednesday, September 15, 2021 5:03:29 PM > > > Subject: [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at > > > runtime > > > > > Two macros are mainly added. One is called BUG(), which is used to > > > put > > > in unreachable branches. One named BUG_ON, used for assertion. > > > > > > Signed-off-by: Xiang W <wxjstz@126.com> > > > --- > > > include/sbi/sbi_console.h | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h > > > index e24ba5f..e75a279 100644 > > > --- a/include/sbi/sbi_console.h > > > +++ b/include/sbi/sbi_console.h > > > @@ -11,6 +11,7 @@ > > > #define __SBI_CONSOLE_H__ > > > > > > #include <sbi/sbi_types.h> > > > +#include <sbi/riscv_asm.h> > > > > > > struct sbi_console_device { > > > /** Name of the console device */ > > > @@ -51,4 +52,16 @@ struct sbi_scratch; > > > > > > int sbi_console_init(struct sbi_scratch *scratch); > > > > > > +#define BUG() do { \ > > > + sbi_printf("BUG: failure at %s:%d/%s()!\n", __FILE__, > > > __LINE__, __func__); \ > > > + while (1) \ > > > + wfi(); \ > > > + __builtin_unreachable(); \ > > > +} while (0) > > > + > > > +#define BUG_ON(cond) do { \ > > > + if (!(cond)) \ > > > + BUG(); \ > > > +} while (0) > > > + > > > > If the BUG_ON has a similar semantics as BUG_ON in Linux, it should > > be: > > + if (cond) \ > > + BUG(); \ > > > I want to implement BUG_ON like assert. If the meaning of linux is like > this, I think it can be used as a reference > Maybe it should be named ASSERT or SBI_ASSERT then? It does not seem like your other patch even uses this however. In my opinion, BUG() and BUG_ON() are confusing names to begin with; they do not obviously describe their semantics. If you insist on using these names, their behaviour should match Linux. Cheers, Mitchell > Regards? > Xiang W > > > #endif > > > -- > > > 2.30.2 > > > > > > > > > -- > > > opensbi mailing list > > > opensbi at lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/opensbi > > > > Regards, > > Dong > > > > > > -- > opensbi mailing list > opensbi at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime 2021-09-15 14:41 ` Mitchell Horne @ 2021-09-15 14:49 ` Jessica Clarke 2021-09-16 4:38 ` Xiang W 0 siblings, 1 reply; 9+ messages in thread From: Jessica Clarke @ 2021-09-15 14:49 UTC (permalink / raw) To: opensbi On 15 Sep 2021, at 15:41, Mitchell Horne <mhorne@freebsd.org> wrote: > > On Wed, Sep 15, 2021 at 10:56 AM Xiang W <wxjstz@126.com> wrote: >> >> ? 2021-09-15???? 20:39 +0800?????? >>> >>> >>> ----- Original Message ----- >>>> From: "Xiang W" <wxjstz@126.com> >>>> To: "opensbi" <opensbi@lists.infradead.org> >>>> Cc: "atish patra" <atish.patra@wdc.com>, "anup patel" < >>>> anup.patel at wdc.com>, "Xiang W" <wxjstz@126.com> >>>> Sent: Wednesday, September 15, 2021 5:03:29 PM >>>> Subject: [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at >>>> runtime >>> >>>> Two macros are mainly added. One is called BUG(), which is used to >>>> put >>>> in unreachable branches. One named BUG_ON, used for assertion. >>>> >>>> Signed-off-by: Xiang W <wxjstz@126.com> >>>> --- >>>> include/sbi/sbi_console.h | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/include/sbi/sbi_console.h b/include/sbi/sbi_console.h >>>> index e24ba5f..e75a279 100644 >>>> --- a/include/sbi/sbi_console.h >>>> +++ b/include/sbi/sbi_console.h >>>> @@ -11,6 +11,7 @@ >>>> #define __SBI_CONSOLE_H__ >>>> >>>> #include <sbi/sbi_types.h> >>>> +#include <sbi/riscv_asm.h> >>>> >>>> struct sbi_console_device { >>>> /** Name of the console device */ >>>> @@ -51,4 +52,16 @@ struct sbi_scratch; >>>> >>>> int sbi_console_init(struct sbi_scratch *scratch); >>>> >>>> +#define BUG() do { \ >>>> + sbi_printf("BUG: failure at %s:%d/%s()!\n", __FILE__, >>>> __LINE__, __func__); \ >>>> + while (1) \ >>>> + wfi(); \ >>>> + __builtin_unreachable(); \ >>>> +} while (0) >>>> + >>>> +#define BUG_ON(cond) do { \ >>>> + if (!(cond)) \ >>>> + BUG(); \ >>>> +} while (0) >>>> + >>> >>> If the BUG_ON has a similar semantics as BUG_ON in Linux, it should >>> be: >>> + if (cond) \ >>> + BUG(); \ >>> >> I want to implement BUG_ON like assert. If the meaning of linux is like >> this, I think it can be used as a reference >> > > Maybe it should be named ASSERT or SBI_ASSERT then? It does not seem > like your other patch even uses this however. > > In my opinion, BUG() and BUG_ON() are confusing names to begin with; > they do not obviously describe their semantics. If you insist on using > these names, their behaviour should match Linux. Also BUG is a bit of a lazy interface; line numbers change over time, really you should be using some kind of panic function that takes a format string with a meaningful message. That also gives the user some hope of knowing what went wrong and maybe working around or fixing it. And why not reuse sbi_hart_hang rather than inlining another copy of it? Jess ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime 2021-09-15 14:49 ` Jessica Clarke @ 2021-09-16 4:38 ` Xiang W 0 siblings, 0 replies; 9+ messages in thread From: Xiang W @ 2021-09-16 4:38 UTC (permalink / raw) To: opensbi ? 2021-09-15???? 15:49 +0100?Jessica Clarke??? > On 15 Sep 2021, at 15:41, Mitchell Horne <mhorne@freebsd.org> wrote: > > > > On Wed, Sep 15, 2021 at 10:56 AM Xiang W <wxjstz@126.com> wrote: > > > > > > ? 2021-09-15???? 20:39 +0800?????? > > > > > > > > > > > > ----- Original Message ----- > > > > > From: "Xiang W" <wxjstz@126.com> > > > > > To: "opensbi" <opensbi@lists.infradead.org> > > > > > Cc: "atish patra" <atish.patra@wdc.com>, "anup patel" < > > > > > anup.patel at wdc.com>, "Xiang W" <wxjstz@126.com> > > > > > Sent: Wednesday, September 15, 2021 5:03:29 PM > > > > > Subject: [PATCH V2 1/2] lib: sbi: add some macros to detect > > > > > BUG at > > > > > runtime > > > > > > > > > Two macros are mainly added. One is called BUG(), which is > > > > > used to > > > > > put > > > > > in unreachable branches. One named BUG_ON, used for > > > > > assertion. > > > > > > > > > > Signed-off-by: Xiang W <wxjstz@126.com> > > > > > --- > > > > > include/sbi/sbi_console.h | 13 +++++++++++++ > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > diff --git a/include/sbi/sbi_console.h > > > > > b/include/sbi/sbi_console.h > > > > > index e24ba5f..e75a279 100644 > > > > > --- a/include/sbi/sbi_console.h > > > > > +++ b/include/sbi/sbi_console.h > > > > > @@ -11,6 +11,7 @@ > > > > > #define __SBI_CONSOLE_H__ > > > > > > > > > > #include <sbi/sbi_types.h> > > > > > +#include <sbi/riscv_asm.h> > > > > > > > > > > struct sbi_console_device { > > > > > ?????? /** Name of the console device */ > > > > > @@ -51,4 +52,16 @@ struct sbi_scratch; > > > > > > > > > > int sbi_console_init(struct sbi_scratch *scratch); > > > > > > > > > > +#define BUG() do { \ > > > > > +?????? sbi_printf("BUG: failure at %s:%d/%s()!\n", __FILE__, > > > > > __LINE__, __func__); \ > > > > > +?????? while (1)?????? \ > > > > > +?????????????? wfi();? \ > > > > > +?????? __builtin_unreachable(); \ > > > > > +} while (0) > > > > > + > > > > > +#define BUG_ON(cond) do { \ > > > > > +?????? if (!(cond))??? \ > > > > > +?????????????? BUG(); \ > > > > > +} while (0) > > > > > + > > > > > > > > If the BUG_ON has a similar semantics as BUG_ON in Linux, it > > > > should > > > > be: > > > > ?+ if (cond)?? \ > > > > ?+???????????? BUG(); \ > > > > > > > I want to implement BUG_ON like assert. If the meaning of linux > > > is like > > > this, I think it can be used as a reference > > > > > > > Maybe it should be named ASSERT or SBI_ASSERT then? It does not > > seem > > like your other patch even uses this however. > > > > In my opinion, BUG() and BUG_ON() are confusing names to begin > > with; > > they do not obviously describe their semantics. If you insist on > > using > > these names, their behaviour should match Linux. > > Also BUG is a bit of a lazy interface; line numbers change over time, > really you should be using some kind of panic function that takes a > format string with a meaningful message. That also gives the user > some > hope of knowing what went wrong and maybe working around or fixing > it. > And why not reuse sbi_hart_hang rather than inlining another copy of > it? Thanks for the suggestion Regards? Xiang W > > Jess ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-09-16 4:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-15 9:03 [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Xiang W 2021-09-15 9:03 ` [PATCH V2 2/2] lib: sbi: Add runtime bug detection for csr_read_num/csr_write_num/misa_string Xiang W 2021-09-15 9:27 ` [PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime Andreas Schwab 2021-09-15 10:07 ` Xiang W 2021-09-15 12:39 ` 杜东 2021-09-15 13:56 ` Xiang W 2021-09-15 14:41 ` Mitchell Horne 2021-09-15 14:49 ` Jessica Clarke 2021-09-16 4:38 ` Xiang W
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.