Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 1:33 PM Finn Thain wrote: > > Or do you think that a codebase can somehow satisfy multiple checkers and > their divergent interpretations of the language spec? Have we found any cases yet that are divergent? I don't think so. It sounds to me like GCC's cases it warns for is a subset of Clang's. Having additional coverage with Clang then should ensure coverage for both. > > This is not a shiny new warning; it's already on for GCC and has existed > > in both compilers for multiple releases. > > > > Perhaps you're referring to the compiler feature that lead to the > ill-fated, tree-wide /* fallthrough */ patch series. > > When the ink dries on the C23 language spec and the implementations figure > out how to interpret it then sure, enforce the warning for new code -- the > cost/benefit analysis is straight forward. However, the case for patching > existing mature code is another story. I don't think we need to wait for the ink to dry on the C23 language spec to understand that implicit fallthrough is an obvious defect of the C language. While the kernel is a mature codebase, it's not immune to bugs. And its maturity has yet to slow its rapid pace of development. -- Thanks, ~Nick Desaulniers
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 8:24 AM Jakub Kicinski wrote: > > Applying a real patch set and then getting a few follow ups the next day > for trivial coding things like fallthrough missing or static missing, > just because I didn't have the full range of compilers to check with > before applying makes me feel pretty shitty, like I'm not doing a good > job. YMMV. I understand. Everyone feels that way, except maybe Bond villains and robots. My advice in that case is don't take it personally. We're working with a language that's more error prone relative to others. While one would like to believe they are flawless, over time they can't beat the aggregate statistics. A balance between Imposter Syndrome and Dunning Kruger is walked by all software developers, and the fear of making mistakes in public is one of the number one reasons folks don't take the plunge contributing to open source software or even the kernel. My advice to them is "don't sweat the small stuff." -- Thanks, ~Nick Desaulniers
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
m you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
>
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
>
> FWIW, this series has found at least one bug so far:
> https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
So looks like the bulk of these are:
switch (x) {
case 0:
++x;
default:
break;
}
I have a patch that fixes those up for clang:
https://reviews.llvm.org/D91895
There's 3 other cases that don't quite match between GCC and Clang I
observe in the kernel:
switch (x) {
case 0:
++x;
default:
goto y;
}
y:;
switch (x) {
case 0:
++x;
default:
return;
}
switch (x) {
case 0:
++x;
default:
;
}
Based on your link, and Nathan's comment on my patch, maybe Clang
should continue to warn for the above (at least the `default: return;`
case) and GCC should change? While the last case looks harmless,
there were only 1 or 2 across the tree in my limited configuration
testing; I really think we should just add `break`s for those.
--
Thanks,
~Nick Desaulniers
Re: [RFC] treewide: cleanup unreachable breaks
On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > From: Tom Rix > > > > This is a upcoming change to clean up a new warning treewide. > > I am wondering if the change could be one mega patch (see below) or > > normal patch per file about 100 patches or somewhere half way by collecting > > early acks. > > Please break it up into one-patch-per-subsystem, like normal, and get it > merged that way. > > Sending us a patch, without even a diffstat to review, isn't going to > get you very far... Tom, If you're able to automate this cleanup, I suggest checking in a script that can be run on a directory. Then for each subsystem you can say in your commit "I ran scripts/fix_whatever.py on this subdir." Then others can help you drive the tree wide cleanup. Then we can enable -Wunreachable-code-break either by default, or W=2 right now might be a good idea. Ah, George (gbiv@, cc'ed), did an analysis recently of `-Wunreachable-code-loop-increment`, `-Wunreachable-code-break`, and `-Wunreachable-code-return` for Android userspace. From the review: ``` Spoilers: of these, it seems useful to turn on -Wunreachable-code-loop-increment and -Wunreachable-code-return by default for Android ... While these conventions about always having break arguably became obsolete when we enabled -Wfallthrough, my sample turned up zero potential bugs caught by this warning, and we'd need to put a lot of effort into getting a clean tree. So this warning doesn't seem to be worth it. ``` Looks like there's an order of magnitude of `-Wunreachable-code-break` than the other two. We probably should add all 3 to W=2 builds (wrapped in cc-option). I've filed https://github.com/ClangBuiltLinux/linux/issues/1180 to follow up on. -- Thanks, ~Nick Desaulniers
Re: [Xen-devel] [PATCH v3] xen-pciback: optionally allow interrupt enable flag writes
gt; 99 if (!(cmd->val & PCI_COMMAND_INVALIDATE) &&
>100 (value & PCI_COMMAND_INVALIDATE)) {
>101 if (unlikely(verbose_request))
>102 printk(KERN_DEBUG
>103 DRV_NAME ": %s: enable
> memory-write-invalidate\n",
>104 pci_name(dev));
>105 err = pci_set_mwi(dev);
>106 if (err) {
>107 pr_warn("%s: cannot enable
> memory-write-invalidate (%d)\n",
>108 pci_name(dev), err);
>109 value &= ~PCI_COMMAND_INVALIDATE;
>110 }
>111 } else if ((cmd->val & PCI_COMMAND_INVALIDATE) &&
>112 !(value & PCI_COMMAND_INVALIDATE)) {
>113 if (unlikely(verbose_request))
>114 printk(KERN_DEBUG
>115 DRV_NAME ": %s: disable
> memory-write-invalidate\n",
>116 pci_name(dev));
>117 pci_clear_mwi(dev);
>118 }
>119
>120 if (dev_data && dev_data->allow_interrupt_control) {
> > 121 if ((cmd->val ^ val) & PCI_COMMAND_INTX_DISABLE) {
>122 if (value & PCI_COMMAND_INTX_DISABLE) {
>123 pci_intx(dev, 0);
>124 } else {
>125 /* Do not allow enabling INTx
> together with MSI or MSI-X. */
>126 switch
> (xen_pcibk_get_interrupt_type(dev)) {
>127 case INTERRUPT_TYPE_NONE:
>128 case INTERRUPT_TYPE_INTX:
>129 pci_intx(dev, 1);
>130 break;
>131 default:
>132 return PCIBIOS_SET_FAILED;
>133 }
>134 }
>135 }
>136 }
>137
>138 cmd->val = value;
>139
>140 if (!xen_pcibk_permissive && (!dev_data ||
> !dev_data->permissive))
>141 return 0;
>142
>143 /* Only allow the guest to control certain bits. */
>144 err = pci_read_config_word(dev, offset, );
>145 if (err || val == value)
>146 return err;
>147
>148 value &= PCI_COMMAND_GUEST;
>149 value |= val & ~PCI_COMMAND_GUEST;
>150
>151 return pci_write_config_word(dev, offset, value);
>152 }
>153
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org Intel Corporation
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/202001112351.gy4c3aUU%25lkp%40intel.com.
--
Thanks,
~Nick Desaulniers
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/blkfront: Adjust indentation in xlvbd_alloc_gendisk
On Mon, Dec 9, 2019 at 12:14 PM Nathan Chancellor
wrote:
>
> Clang warns:
>
> ../drivers/block/xen-blkfront.c:1117:4: warning: misleading indentation;
> statement is not part of the previous 'if' [-Wmisleading-indentation]
> nr_parts = PARTS_PER_DISK;
> ^
> ../drivers/block/xen-blkfront.c:1115:3: note: previous statement is here
> if (err)
> ^
>
> This is because there is a space at the beginning of this line; remove
> it so that the indentation is consistent according to the Linux kernel
> coding style and clang no longer warns.
>
> While we are here, the previous line has some trailing whitespace; clean
> that up as well.
>
> Fixes: c80a420995e7 ("xen-blkfront: handle Xen major numbers other than
> XENVBD")
> Link: https://github.com/ClangBuiltLinux/linux/issues/791
> Signed-off-by: Nathan Chancellor
> ---
> drivers/block/xen-blkfront.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index a74d03913822..c02be06c5299 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1113,8 +1113,8 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
While you're here, would you please also removing the single space
before the labels in this function?
In vim:
/^ [a-zA-Z]
turns up 5 labels with this.
> if (!VDEV_IS_EXTENDED(info->vdevice)) {
> err = xen_translate_vdev(info->vdevice, , );
> if (err)
> - return err;
> - nr_parts = PARTS_PER_DISK;
> + return err;
> + nr_parts = PARTS_PER_DISK;
> } else {
> minor = BLKIF_MINOR_EXT(info->vdevice);
> nr_parts = PARTS_PER_EXT_DISK;
> --
> 2.24.0
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/20191209201444.33243-1-natechancellor%40gmail.com.
--
Thanks,
~Nick Desaulniers
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] x86: xen: remove the use of VLAIS
Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
frowned upon by others.
https://lkml.org/lkml/2013/9/23/500
Here, the VLAIS was used because the size of the bitmap returned from
xen_mc_entry() depended on possibly (based on kernel configuration)
runtime sized data. Rather than declaring args as a VLAIS then calling
sizeof on *args, we calculate the appropriate sizeof args manually.
Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
(thanks to a helpful checkpatch warning from an earlier version of this
patch).
Suggested-by: Juergen Gross <jgr...@suse.com>
Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com>
---
Changes in v2:
* Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
* Update commit message to remove mention of pointer.
* Update sizeof calculation to work with array rather than pointer.
arch/x86/xen/mmu_pv.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 4d62c07..d850762 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask
*cpus,
{
struct {
struct mmuext_op op;
-#ifdef CONFIG_SMP
- DECLARE_BITMAP(mask, num_processors);
-#else
DECLARE_BITMAP(mask, NR_CPUS);
-#endif
} *args;
struct multicall_space mcs;
+ const size_t mc_entry_size = sizeof(args->op) +
+ sizeof(args->mask[0]) * BITS_TO_LONGS(num_possible_cpus());
trace_xen_mmu_flush_tlb_others(cpus, info->mm, info->start, info->end);
if (cpumask_empty(cpus))
return; /* nothing to do */
- mcs = xen_mc_entry(sizeof(*args));
+ mcs = xen_mc_entry(mc_entry_size);
args = mcs.args;
args->op.arg2.vcpumask = to_cpumask(args->mask);
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/xen/time: fix section mismatch for xen_init_time_ops()
On Tue, Jan 2, 2018 at 7:00 AM, Boris Ostrovsky <boris.ostrov...@oracle.com> wrote: > On 01/02/2018 09:32 AM, Andrew Cooper wrote: >> On 02/01/18 14:24, Juergen Gross wrote: >>> On 02/01/18 15:18, Boris Ostrovsky wrote: >>>> On 12/23/2017 09:50 PM, Nick Desaulniers wrote: >>>>> The header declares this function as __init but is defined in __ref >>>>> section. >>>>> >>>>> Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com> >>>> AFAIK section attributes in header files are ignored by compiler anyway >>>> so I'd remove all of them. >>> Hmm, I'm not sure all future compilers will ignore the section >>> attributes. include/linux/init.h explictily mentions where to put >>> the attrubute in a prototype, so I'd rather keep it. >> Attributes in the declaration are for static analysis tools such as sparse. >> >> How else are you going to work out whether a section mismatch has occurred? > > Isn't this done based on definitions? > > Tons of __init routines don't have the attribute specified in header > files. In fact, even in this file (arch/x86/xen/xen-ops.h) there are > some that don't have it. > > -boris What are the next steps for getting this patch merged? This is the only function for which I get a compiler warning (with Clang). Do you require a patch instead that changes more function attributes, or can that be a follow up patch? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86: xen: remove the use of VLAIS
Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
frowned upon by others.
https://lkml.org/lkml/2013/9/23/500
Here, the VLAIS was used because the size of the bitmap returned from
xen_mc_entry() depended on possibly (based on kernel configuration)
runtime sized data. Rather than declaring args as a VLAIS then calling
sizeof on *args, we can define the variable length array (mask) to be a
pointer, and calculate the appropriate sizeof args manually. Further, we
can get rid of the #ifdef's and rely on num_possible_cpus() (thanks to a
helpful checkpatch warning from an earlier version of this patch).
Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com>
---
arch/x86/xen/mmu_pv.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 4d62c07..966976c 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask
*cpus,
{
struct {
struct mmuext_op op;
-#ifdef CONFIG_SMP
- DECLARE_BITMAP(mask, num_processors);
-#else
- DECLARE_BITMAP(mask, NR_CPUS);
-#endif
+ unsigned long *mask;
} *args;
struct multicall_space mcs;
+ const size_t mc_entry_size = sizeof(args->op) +
+ sizeof(*args->mask) * BITS_TO_LONGS(num_possible_cpus());
trace_xen_mmu_flush_tlb_others(cpus, info->mm, info->start, info->end);
if (cpumask_empty(cpus))
return; /* nothing to do */
- mcs = xen_mc_entry(sizeof(*args));
+ mcs = xen_mc_entry(mc_entry_size);
args = mcs.args;
args->op.arg2.vcpumask = to_cpumask(args->mask);
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/xen/time: fix section mismatch for xen_init_time_ops()
The header declares this function as __init but is defined in __ref section. Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com> --- arch/x86/xen/xen-ops.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h index 75011b8..3b34745 100644 --- a/arch/x86/xen/xen-ops.h +++ b/arch/x86/xen/xen-ops.h @@ -72,7 +72,7 @@ u64 xen_clocksource_read(void); void xen_setup_cpu_clockevents(void); void xen_save_time_memory_area(void); void xen_restore_time_memory_area(void); -void __init xen_init_time_ops(void); +void __ref xen_init_time_ops(void); void __init xen_hvm_init_time_ops(void); irqreturn_t xen_debug_interrupt(int irq, void *dev_id); -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
