* [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.