Re: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[Mingwei Zhang]

On Thu, Mar 2, 2023 at 5:50 PM Wang, Wei W <wei.w.wang@xxxxxxxxx> wrote:
>
> On Friday, March 3, 2023 2:12 AM, Mingwei Zhang wrote:
> > > On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > > > I don't get it. Why bothering the type if we just do this?
> > > >
> > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > index 4f26b244f6d0..10455253c6ea 100644
> > > > --- a/include/linux/kvm_host.h
> > > > +++ b/include/linux/kvm_host.h
> > > > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm
> > > > *kvm)
> > > >
> > > >  #define KVM_BUG(cond, kvm, fmt...)                           \
> > > >  ({                                                           \
> > > > -     int __ret = (cond);                                     \
> > > > +     int __ret = !!(cond);                                   \
> > >
> > > This is essentially "bool __ret". No biggie to change it this way.
> >
> > !! will return an int, not a boolean, but it is used as a boolean.
>
> What's the point of defining it as an int when actually being used as a Boolean?
> Original returning of an 'int' is a bug in this sense. Either returning a Boolean or
> the same type (length) as cond is good way to me.

What's the point of using an integer? I think we need to ask the
original author. But I think one of the reasons might be convenience
as the return value. I am not sure if we can return a boolean in the
function. But it should be fine here since it is a macro.

Anyway, returning an 'int' is not a bug. The bug is the casting from
'cond' to the integer that may lose information and this is what you
have captured.
>
> > This is consistent with the original code which _is_ returning an integer.
> >
> > > But I'm inclined to retain the original intention to have the macro
> > > return the value that was passed in:
> > > typeof(cond) __ret = (cond);
> >
> > hmm, I think it is appropriate to retain the original type of 'cond'
> > especially since it may also involve other arithmetic operations. But I doubt it
> > will be very useful. For instance, who is going to write this code?
> >
>
> Maybe there is, maybe not. But it doesn’t hurt anything to leave the
> flexibility there using typeof(cond). As said, I'm also fine to use 'bool ret',
> but probably not 'int' for no good reason.

Right, maybe or maybe not. But using typeof(cond) for the variable
does not always provide benefits. For instance if the 'cond' is
resolved as the 8-byte type like u64, we are wasting space at runtime.
We could have used a shorter type. In addition, throwing this to the
compiler creates complexity and sometimes bugs since the compiler
could have different behaviors.

So, I prefer not having typeof(cond) for KVM_BUG(). But if you have
strong opinions using typeof, go ahead.

Thanks.
-Mingwei