| Message ID | ad7a9addf043ee7fda8907eff9f83c9709b575dc.camel@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/06/2018 19:23, Joe Perches wrote: > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote: >> On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> On 15/06/2018 20:45, Nick Desaulniers wrote: >>>>> >>>>>> In any case I think it it preferable to fix the code over disabling >>>>>> the warning, unless the warning is bogus or there are just too many >>>>>> occurrences. >>>>> >>>>> Maybe. >>>> >>>> Spurious warning today, actual bug tomorrow? I prefer to not to >>>> disable warnings wholesale. They don't need to find actual bugs to be >>>> useful. Flagging code that can be further specified does not hurt. >>>> Part of the effort to compile the kernel with different compilers is >>>> to add warning coverage, not remove it. That said, there may be >>>> warnings that are never useful (or at least due to some invariant that >>>> only affects the kernel). I cant think of any off the top of my head, >>>> but I'm also not sure this is one. >>> >>> This one really makes the code uglier though, so I'm not really inclined >>> to applying the patch. >> >> Note that of the three variables (w, u, x), only u is used later on. >> What about declaring them as negated with the cast, that way there's >> no cast in a ternary? > > It'd be simpler to cast in the BYTE_MASK macro itself I don't think that would work, as the ~ would be done on a zero-extended signed int. Paolo > Ex: >> >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >> index d594690d8b95..53673ad4b295 100644 >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -4261,8 +4261,9 @@ static void update_permission_bitmask(struct >> kvm_vcpu *vcpu, >> { >> unsigned byte; >> >> - const u8 x = BYTE_MASK(ACC_EXEC_MASK); >> - const u8 w = BYTE_MASK(ACC_WRITE_MASK); >> + const u8 x_not = (u8)~BYTE_MASK(ACC_EXEC_MASK); >> + const u8 w_not = (u8)~BYTE_MASK(ACC_WRITE_MASK); >> + const u8 u_not = (u8)~BYTE_MASK(ACC_USER_MASK); >> const u8 u = BYTE_MASK(ACC_USER_MASK); >> >> >> bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0; >> @@ -4278,11 +4279,11 @@ static void update_permission_bitmask(struct >> kvm_vcpu *vcpu, >> */ >> >> /* Faults from writes to non-writable pages */ >> - u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0; >> + u8 wf = (pfec & PFERR_WRITE_MASK) ? w_not : 0; >> /* Faults from user mode accesses to supervisor pages */ >> - u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0; >> + u8 uf = (pfec & PFERR_USER_MASK) ? u_not : 0; >> /* Faults from fetches of non-executable pages*/ >> - u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0; >> + u8 ff = (pfec & PFERR_FETCH_MASK) ? x_not : 0; >> /* Faults from kernel mode fetches of user pages */ >> u8 smepf = 0; >> /* Faults from kernel mode accesses of user pages */ >> >> >> Maybe you have a better naming scheme than *_not ? What do you think? > > It'd be nicer to cast in the BYTE_MASK macro > and using "unsigned byte;" is misleading at best. > > --- > arch/x86/kvm/mmu.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index d594690d8b95..201711aa99b9 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -4246,15 +4246,14 @@ reset_ept_shadow_zero_bits_mask(struct kvm_vcpu *vcpu, > boot_cpu_data.x86_phys_bits, execonly); > } > > -#define BYTE_MASK(access) \ > - ((1 & (access) ? 2 : 0) | \ > - (2 & (access) ? 4 : 0) | \ > - (3 & (access) ? 8 : 0) | \ > - (4 & (access) ? 16 : 0) | \ > - (5 & (access) ? 32 : 0) | \ > - (6 & (access) ? 64 : 0) | \ > - (7 & (access) ? 128 : 0)) > - > +#define BYTE_MASK(access) \ > + ((u8)(((access) & 1 ? 2 : 0) | \ > + ((access) & 2 ? 4 : 0) | \ > + ((access) & 3 ? 8 : 0) | \ > + ((access) & 4 ? 16 : 0) | \ > + ((access) & 5 ? 32 : 0) | \ > + ((access) & 6 ? 64 : 0) | \ > + ((access) & 7 ? 128 : 0))) > > static void update_permission_bitmask(struct kvm_vcpu *vcpu, > struct kvm_mmu *mmu, bool ept) >
On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote: > On 19/06/2018 19:23, Joe Perches wrote: > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote: > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > On 15/06/2018 20:45, Nick Desaulniers wrote: > > > > > > > > > > > > > In any case I think it it preferable to fix the code over disabling > > > > > > > the warning, unless the warning is bogus or there are just too many > > > > > > > occurrences. > > > > > > > > > > > > Maybe. > > > > > > > > > > Spurious warning today, actual bug tomorrow? I prefer to not to > > > > > disable warnings wholesale. They don't need to find actual bugs to be > > > > > useful. Flagging code that can be further specified does not hurt. > > > > > Part of the effort to compile the kernel with different compilers is > > > > > to add warning coverage, not remove it. That said, there may be > > > > > warnings that are never useful (or at least due to some invariant that > > > > > only affects the kernel). I cant think of any off the top of my head, > > > > > but I'm also not sure this is one. > > > > > > > > This one really makes the code uglier though, so I'm not really inclined > > > > to applying the patch. > > > > > > Note that of the three variables (w, u, x), only u is used later on. > > > What about declaring them as negated with the cast, that way there's > > > no cast in a ternary? > > > > It'd be simpler to cast in the BYTE_MASK macro itself > > I don't think that would work, as the ~ would be done on a zero-extended > signed int. True, but the whole concept is dubious. The implicit casts are all over the place, not just in the file.
On Tue, Jun 19, 2018 at 11:07:47AM -0700, Joe Perches wrote: > On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote: > > On 19/06/2018 19:23, Joe Perches wrote: > > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote: > > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > > > On 15/06/2018 20:45, Nick Desaulniers wrote: > > > > > > > > > > > > > > > In any case I think it it preferable to fix the code over disabling > > > > > > > > the warning, unless the warning is bogus or there are just too many > > > > > > > > occurrences. > > > > > > > > > > > > > > Maybe. > > > > > > > > > > > > Spurious warning today, actual bug tomorrow? I prefer to not to > > > > > > disable warnings wholesale. They don't need to find actual bugs to be > > > > > > useful. Flagging code that can be further specified does not hurt. > > > > > > Part of the effort to compile the kernel with different compilers is > > > > > > to add warning coverage, not remove it. That said, there may be > > > > > > warnings that are never useful (or at least due to some invariant that > > > > > > only affects the kernel). I cant think of any off the top of my head, > > > > > > but I'm also not sure this is one. > > > > > > > > > > This one really makes the code uglier though, so I'm not really inclined > > > > > to applying the patch. > > > > > > > > Note that of the three variables (w, u, x), only u is used later on. > > > > What about declaring them as negated with the cast, that way there's > > > > no cast in a ternary? > > > > > > It'd be simpler to cast in the BYTE_MASK macro itself > > > > I don't think that would work, as the ~ would be done on a zero-extended > > signed int. > > True, but the whole concept is dubious. > The implicit casts are all over the place, > not just in the file. Would that have been any different with the solution you proposed (if it worked)? Apparently both gcc and clang limit the warning to the potentially more problematic case where a value with sign bit is promoted.
On Tue, 2018-06-19 at 11:36 -0700, Matthias Kaehlcke wrote: > On Tue, Jun 19, 2018 at 11:07:47AM -0700, Joe Perches wrote: > > On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote: > > > On 19/06/2018 19:23, Joe Perches wrote: > > > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote: > > > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > > > > > On 15/06/2018 20:45, Nick Desaulniers wrote: > > > > > > > > > > > > > > > > > In any case I think it it preferable to fix the code over disabling > > > > > > > > > the warning, unless the warning is bogus or there are just too many > > > > > > > > > occurrences. > > > > > > > > > > > > > > > > Maybe. > > > > > > > > > > > > > > Spurious warning today, actual bug tomorrow? I prefer to not to > > > > > > > disable warnings wholesale. They don't need to find actual bugs to be > > > > > > > useful. Flagging code that can be further specified does not hurt. > > > > > > > Part of the effort to compile the kernel with different compilers is > > > > > > > to add warning coverage, not remove it. That said, there may be > > > > > > > warnings that are never useful (or at least due to some invariant that > > > > > > > only affects the kernel). I cant think of any off the top of my head, > > > > > > > but I'm also not sure this is one. > > > > > > > > > > > > This one really makes the code uglier though, so I'm not really inclined > > > > > > to applying the patch. > > > > > > > > > > Note that of the three variables (w, u, x), only u is used later on. > > > > > What about declaring them as negated with the cast, that way there's > > > > > no cast in a ternary? > > > > > > > > It'd be simpler to cast in the BYTE_MASK macro itself > > > > > > I don't think that would work, as the ~ would be done on a zero-extended > > > signed int. > > > > True, but the whole concept is dubious. > > The implicit casts are all over the place, > > not just in the file. > > Would that have been any different with the solution you proposed (if > it worked)? > > Apparently both gcc and clang limit the warning to the potentially > more problematic case where a value with sign bit is promoted. I think the warning is exactly equivalent to -Wsign-conversion and we don't normally compile the kernel with that either. Trying to allow a "make W=3" to be compiler warning message free is also silly. I think it's better to make the warning emitted only at a W=3 level instead.
On Tue, Jun 19, 2018 at 12:11:27PM -0700, Joe Perches wrote: > On Tue, 2018-06-19 at 11:36 -0700, Matthias Kaehlcke wrote: > > On Tue, Jun 19, 2018 at 11:07:47AM -0700, Joe Perches wrote: > > > On Tue, 2018-06-19 at 19:35 +0200, Paolo Bonzini wrote: > > > > On 19/06/2018 19:23, Joe Perches wrote: > > > > > On Tue, 2018-06-19 at 10:08 -0700, Nick Desaulniers wrote: > > > > > > On Tue, Jun 19, 2018 at 8:19 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > > > > > > > On 15/06/2018 20:45, Nick Desaulniers wrote: > > > > > > > > > > > > > > > > > > > In any case I think it it preferable to fix the code over disabling > > > > > > > > > > the warning, unless the warning is bogus or there are just too many > > > > > > > > > > occurrences. > > > > > > > > > > > > > > > > > > Maybe. > > > > > > > > > > > > > > > > Spurious warning today, actual bug tomorrow? I prefer to not to > > > > > > > > disable warnings wholesale. They don't need to find actual bugs to be > > > > > > > > useful. Flagging code that can be further specified does not hurt. > > > > > > > > Part of the effort to compile the kernel with different compilers is > > > > > > > > to add warning coverage, not remove it. That said, there may be > > > > > > > > warnings that are never useful (or at least due to some invariant that > > > > > > > > only affects the kernel). I cant think of any off the top of my head, > > > > > > > > but I'm also not sure this is one. > > > > > > > > > > > > > > This one really makes the code uglier though, so I'm not really inclined > > > > > > > to applying the patch. > > > > > > > > > > > > Note that of the three variables (w, u, x), only u is used later on. > > > > > > What about declaring them as negated with the cast, that way there's > > > > > > no cast in a ternary? > > > > > > > > > > It'd be simpler to cast in the BYTE_MASK macro itself > > > > > > > > I don't think that would work, as the ~ would be done on a zero-extended > > > > signed int. > > > > > > True, but the whole concept is dubious. > > > The implicit casts are all over the place, > > > not just in the file. > > > > Would that have been any different with the solution you proposed (if > > it worked)? > > > > Apparently both gcc and clang limit the warning to the potentially > > more problematic case where a value with sign bit is promoted. > > I think the warning is exactly equivalent to -Wsign-conversion > and we don't normally compile the kernel with that either. I disagree with "exactly equivalent". With -Wsign-conversion a warning is generated whenever a signed type is assigned to an unsigned variable or viceversa. -Wconstant-conversion is only issued when a *constant value* is assigned to an incompatible type. > Trying to allow a "make W=3" to be compiler warning message free > is also silly. > > I think it's better to make the warning emitted only at a W=3 > level instead. Another difference with -Wsign-conversion is that enabling it would probably result in thousands of warnings. Do you have evidence that there is a significant number of spurious -Wconstant-conversion warnings?
On Tue, 2018-06-19 at 14:10 -0700, Matthias Kaehlcke wrote: > On Tue, Jun 19, 2018 at 12:11:27PM -0700, Joe Perches wrote: > > -Wconstant-conversion is only issued when a *constant value* is assigned to > an incompatible type. > > > Trying to allow a "make W=3" to be compiler warning message free > > is also silly. > > > > I think it's better to make the warning emitted only at a W=3 > > level instead. > > Another difference with -Wsign-conversion is that enabling it would > probably result in thousands of warnings. Do you have evidence that > there is a significant number of spurious -Wconstant-conversion > warnings? Not my issue really. You're advocating for making the code more complex/ugly for a condition where the result is identical. Do you have evidence this is the only location in an allyesconfig compilation?
On Tue, Jun 19, 2018 at 02:55:05PM -0700, Joe Perches wrote: > On Tue, 2018-06-19 at 14:10 -0700, Matthias Kaehlcke wrote: > > On Tue, Jun 19, 2018 at 12:11:27PM -0700, Joe Perches wrote: > > > -Wconstant-conversion is only issued when a *constant value* is assigned to > > an incompatible type. > > > > > Trying to allow a "make W=3" to be compiler warning message free > > > is also silly. > > > > > > I think it's better to make the warning emitted only at a W=3 > > > level instead. > > > > Another difference with -Wsign-conversion is that enabling it would > > probably result in thousands of warnings. Do you have evidence that > > there is a significant number of spurious -Wconstant-conversion > > warnings? > > Not my issue really. Well, you advocate to disable a possibly useful warning globally ... > You're advocating for making the code more complex/ugly for a > condition where the result is identical. My goal is no to make the code (slightly) more complex/ugly but have the rest of the kernel benefit from a possibly useful warning. > Do you have evidence this is the only location in > an allyesconfig compilation? I never claimed that this is the only location, but that it is not an extremely noisy warning that can not be fixed without a major effort. Since you asked: v4.16 with an x86 allyesconfig and a few drivers disabled since they don't build with clang (yet): 16 occurrences of -Wconstant-conversion, including the ones fixed by this patch. Certainly no need for an endless churn of patches, and given the limited number it also doesn't seem likely that new instances will be added on a regular base.
On Tue, 2018-06-19 at 16:45 -0700, Matthias Kaehlcke wrote: > On Tue, Jun 19, 2018 at 02:55:05PM -0700, Joe Perches wrote: > > Well, you advocate to disable a possibly useful warning globally ... > > You're advocating for making the code more complex/ugly for a > > condition where the result is identical. > My goal is no to make the code (slightly) more complex/ugly but have > the rest of the kernel benefit from a possibly useful warning. For what case in the kernel is this warning useful?
On Tue, Jun 19, 2018 at 05:18:02PM -0700, Joe Perches wrote: > On Tue, 2018-06-19 at 16:45 -0700, Matthias Kaehlcke wrote: > > On Tue, Jun 19, 2018 at 02:55:05PM -0700, Joe Perches wrote: > > > Well, you advocate to disable a possibly useful warning globally ... > > > You're advocating for making the code more complex/ugly for a > > > condition where the result is identical. > > My goal is no to make the code (slightly) more complex/ugly but have > > the rest of the kernel benefit from a possibly useful warning. > > For what case in the kernel is this warning useful? It's less about finding errors that are currently in the kernel (I don't know if there are any) and more about preventing new ones from creeping in. Code similar to the example from the link shared by Nick could easily be part of some kernel driver: https://wiki.sei.cmu.edu/confluence/display/c/EXP14-C.+Beware+of+integer+promotion+when+performing+bitwise+operations+on+integer+types+smaller+than+int
On 20/06/2018 01:45, Matthias Kaehlcke wrote: > > v4.16 with an x86 allyesconfig and a few drivers disabled since they > don't build with clang (yet): > > 16 occurrences of -Wconstant-conversion, including the ones fixed by > this patch. Certainly no need for an endless churn of patches, and > given the limited number it also doesn't seem likely that new > instances will be added on a regular base. Thanks for providing numbers! The next question is: how many of these 16 occurrences are actual bugs? This is what measures the usefulness of the warning. Paolo
On Wed, Jun 20, 2018 at 10:02:15AM +0200, Paolo Bonzini wrote: > On 20/06/2018 01:45, Matthias Kaehlcke wrote: > > > > v4.16 with an x86 allyesconfig and a few drivers disabled since they > > don't build with clang (yet): > > > > 16 occurrences of -Wconstant-conversion, including the ones fixed by > > this patch. Certainly no need for an endless churn of patches, and > > given the limited number it also doesn't seem likely that new > > instances will be added on a regular base. > > Thanks for providing numbers! The next question is: how many of these > 16 occurrences are actual bugs? Probably none. > This is what measures the usefulness of the warning. I very much doubt that this is a useful metric given the limited number of occurrences. Let's assume that 1 out of 20 patches not doing the cast have an actual problem. For 16 occurrences this means there's a 44% chance that none of these instances is an error (18.5% for 1 out 10). This is without taking into account that some of the instances are in the same file and likey similar (as in the KVM case), and that most of the code has undergone years of testing in the field.
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index d594690d8b95..201711aa99b9 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -4246,15 +4246,14 @@ reset_ept_shadow_zero_bits_mask(struct kvm_vcpu *vcpu, boot_cpu_data.x86_phys_bits, execonly); } -#define BYTE_MASK(access) \ - ((1 & (access) ? 2 : 0) | \ - (2 & (access) ? 4 : 0) | \ - (3 & (access) ? 8 : 0) | \ - (4 & (access) ? 16 : 0) | \ - (5 & (access) ? 32 : 0) | \ - (6 & (access) ? 64 : 0) | \ - (7 & (access) ? 128 : 0)) - +#define BYTE_MASK(access) \ + ((u8)(((access) & 1 ? 2 : 0) | \ + ((access) & 2 ? 4 : 0) | \ + ((access) & 3 ? 8 : 0) | \ + ((access) & 4 ? 16 : 0) | \ + ((access) & 5 ? 32 : 0) | \ + ((access) & 6 ? 64 : 0) | \ + ((access) & 7 ? 128 : 0))) static void update_permission_bitmask(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, bool ept)