| Message ID | 20210915090330.23354-1-wxjstz@126.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2,1/2] lib: sbi: add some macros to detect BUG at runtime | expand |
On Sep 15 2021, Xiang W wrote: > +#define BUG_ON(cond) do { \ > + if (!(cond)) \ > + BUG(); \ Isn't that backwards? Andreas.
在 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. >
----- 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi Regards, Dong
在 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@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@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > > Regards, > Dong >
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@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@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/opensbi > > > > Regards, > > Dong > > > > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/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@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
在 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@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
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
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(+)