[PATCH V2 1/2] lib: sbi: add some macros to detect BUG at runtime

[Jessica Clarke]

On 15 Sep 2021, at 15:41, Mitchell Horne <mhorne at freebsd.org> wrote:
> 
> On Wed, Sep 15, 2021 at 10:56 AM Xiang W <wxjstz at 126.com> wrote:
>> 
>> 在 2021-09-15星期三的 20:39 +0800，杜东写道：
>>> 
>>> 
>>> ----- Original Message -----
>>>> From: "Xiang W" <wxjstz at 126.com>
>>>> To: "opensbi" <opensbi at lists.infradead.org>
>>>> Cc: "atish patra" <atish.patra at wdc.com>, "anup patel" <
>>>> anup.patel at wdc.com>, "Xiang W" <wxjstz at 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 at 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