Re: [PATCH bpf-next v2] samples/bpf: Update README.rst and Makefile for manually compiling LLVM and clang
On Mon, Jan 18, 2021 at 11:56 PM Tiezhu Yang wrote: > > The current llvm/clang build procedure in samples/bpf/README.rst is > out of date. See below that the links are not accessible any more. > > $ git clone http://llvm.org/git/llvm.git > Cloning into 'llvm'... > fatal: unable to access 'http://llvm.org/git/llvm.git/': Maximum (20) > redirects followed > $ git clone --depth 1 http://llvm.org/git/clang.git > Cloning into 'clang'... > fatal: unable to access 'http://llvm.org/git/clang.git/': Maximum (20) > redirects followed > > The llvm community has adopted new ways to build the compiler. There are > different ways to build llvm/clang, the Clang Getting Started page [1] has > one way. As Yonghong said, it is better to just copy the build procedure > in Documentation/bpf/bpf_devel_QA.rst to keep consistent. > > I verified the procedure and it is proved to be feasible, so we should > update README.rst to reflect the reality. At the same time, update the > related comment in Makefile. > > [1] https://clang.llvm.org/get_started.html There's also https://www.kernel.org/doc/html/latest/kbuild/llvm.html#getting-llvm (could cross link in rst/sphinx). > > Signed-off-by: Tiezhu Yang > Acked-by: Yonghong Song > --- > > v2: Update the commit message suggested by Yonghong, > thank you very much. > > samples/bpf/Makefile | 2 +- > samples/bpf/README.rst | 17 ++--- > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile > index 26fc96c..d061446 100644 > --- a/samples/bpf/Makefile > +++ b/samples/bpf/Makefile > @@ -208,7 +208,7 @@ TPROGLDLIBS_xdpsock += -pthread -lcap > TPROGLDLIBS_xsk_fwd+= -pthread > > # Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on > cmdline: > -# make M=samples/bpf/ LLC=~/git/llvm/build/bin/llc > CLANG=~/git/llvm/build/bin/clang > +# make M=samples/bpf LLC=~/git/llvm-project/llvm/build/bin/llc > CLANG=~/git/llvm-project/llvm/build/bin/clang > LLC ?= llc > CLANG ?= clang > OPT ?= opt > diff --git a/samples/bpf/README.rst b/samples/bpf/README.rst > index dd34b2d..d1be438 100644 > --- a/samples/bpf/README.rst > +++ b/samples/bpf/README.rst > @@ -65,17 +65,20 @@ To generate a smaller llc binary one can use:: > Quick sniplet for manually compiling LLVM and clang > (build dependencies are cmake and gcc-c++):: > > - $ git clone http://llvm.org/git/llvm.git > - $ cd llvm/tools > - $ git clone --depth 1 http://llvm.org/git/clang.git > - $ cd ..; mkdir build; cd build > - $ cmake .. -DLLVM_TARGETS_TO_BUILD="BPF;X86" Is the BPF target not yet on by default? I frown upon disabling other backends. > - $ make -j $(getconf _NPROCESSORS_ONLN) > + $ git clone https://github.com/llvm/llvm-project.git > + $ mkdir -p llvm-project/llvm/build/install > + $ cd llvm-project/llvm/build > + $ cmake .. -G "Ninja" -DLLVM_TARGETS_TO_BUILD="BPF;X86" \ > +-DLLVM_ENABLE_PROJECTS="clang"\ > +-DBUILD_SHARED_LIBS=OFF \ > +-DCMAKE_BUILD_TYPE=Release\ > +-DLLVM_BUILD_RUNTIME=OFF > + $ ninja > > It is also possible to point make to the newly compiled 'llc' or > 'clang' command via redefining LLC or CLANG on the make command line:: > > - make M=samples/bpf LLC=~/git/llvm/build/bin/llc > CLANG=~/git/llvm/build/bin/clang > + make M=samples/bpf LLC=~/git/llvm-project/llvm/build/bin/llc > CLANG=~/git/llvm-project/llvm/build/bin/clang > > Cross compiling samples > --- > -- > 2.1.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/1611042978-21473-1-git-send-email-yangtiezhu%40loongson.cn. -- Thanks, ~Nick Desaulniers
Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today
On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn wrote: > > There is a movement to try to make more and more of /drivers W=1 > clean. But it will only stay clean if new warnings are quickly > detected and fixed, ideally by the developer adding the new code. > > To allow subdirectories to sign up to being W=1 clean for a given > definition of W=1, export the current set of additional compile flags > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can > then use: > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930) > > To indicate they want to W=1 warnings as defined on 20200930. > > Additional warnings can be added to the W=1 definition. This will not > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will > start appearing unless W=1 is actually added to the command > line. Developers can then take their time to fix any new W=1 warnings, > and then update to the latest KBUILD_CFLAGS_W1_ symbol. I'm not a fan of this approach. Are DATESTAMP configs just going to keep being added? Is it going to complicate isolating the issue from a randconfig build? If we want more things to build warning-free at W=1, then why don't we start moving warnings from W=1 into the default, until this is no W=1 left? That way we're cutting down on the kernel's configuration combinatorial explosion, rather than adding to it? > > Signed-off-by: Andrew Lunn > --- > scripts/Makefile.extrawarn | 34 ++ > 1 file changed, 18 insertions(+), 16 deletions(-) > > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn > index 95e4cdb94fe9..957dca35ae3e 100644 > --- a/scripts/Makefile.extrawarn > +++ b/scripts/Makefile.extrawarn > @@ -20,24 +20,26 @@ export KBUILD_EXTRA_WARN > # > # W=1 - warnings which may be relevant and do not occur too often > # > -ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) > - > -KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter > -KBUILD_CFLAGS += -Wmissing-declarations > -KBUILD_CFLAGS += -Wmissing-format-attribute > -KBUILD_CFLAGS += -Wmissing-prototypes > -KBUILD_CFLAGS += -Wold-style-definition > -KBUILD_CFLAGS += -Wmissing-include-dirs > -KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) > -KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) > -KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) > -KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) > +KBUILD_CFLAGS_W1_20200930 += -Wextra -Wunused -Wno-unused-parameter > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-declarations > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-format-attribute > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-prototypes > +KBUILD_CFLAGS_W1_20200930 += -Wold-style-definition > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-include-dirs > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-but-set-variable) > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-const-variable) > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wpacked-not-aligned) > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wstringop-truncation) > # The following turn off the warnings enabled by -Wextra > -KBUILD_CFLAGS += -Wno-missing-field-initializers > -KBUILD_CFLAGS += -Wno-sign-compare > -KBUILD_CFLAGS += -Wno-type-limits > +KBUILD_CFLAGS_W1_20200930 += -Wno-missing-field-initializers > +KBUILD_CFLAGS_W1_20200930 += -Wno-sign-compare > +KBUILD_CFLAGS_W1_20200930 += -Wno-type-limits > + > +export KBUILD_CFLAGS_W1_20200930 > + > +ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) > > -KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1 > +KBUILD_CPPFLAGS += $(KBUILD_CFLAGS_W1_20200930) -DKBUILD_EXTRA_WARN1 > > else > > -- > 2.28.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/20201001011232.4050282-2-andrew%40lunn.ch. -- Thanks, ~Nick Desaulniers
Re: -Wsizeof-array-div warnings in ethernet drivers
On Tue, Sep 17, 2019 at 6:27 AM Jose Abreu wrote: > > From: David Laight > Date: Sep/17/2019, 11:36:21 (UTC+00:00) > > > From: Jose Abreu > > > Sent: 17 September 2019 08:59 > > > From: Nathan Chancellor > > > Date: Sep/17/2019, 08:32:32 (UTC+00:00) > > > > > > > Hi all, > > > > > > > > Clang recently added a new diagnostic in r371605, -Wsizeof-array-div, > > > > that tries to warn when sizeof(X) / sizeof(Y) does not compute the > > > > number of elements in an array X (i.e., sizeof(Y) is wrong). See that > > > > commit for more details: > > ... > > > > ../drivers/net/ethernet/amd/xgbe/xgbe-dev.c:361:49: warning: expression > > > > does not compute the number of elements in this array; element type is > > > > 'u8' (aka 'unsigned char'), not 'u32' (aka 'unsigned int') > > > > [-Wsizeof-array-div] > > > > unsigned int key_regs = sizeof(pdata->rss_key) / sizeof(u32); > > > >~~ ^ > > ... > > > > What is the reasoning behind having the key being an array of u8s but > > > > seemlingly converting it into an array of u32s? It's not immediately > > > > apparent from reading over the code but I am not familiar with it so I > > > > might be making a mistake. I assume this is intentional? If so, the > > > > warning can be silenced and we'll send patches to do so but we want to > > > > make sure we aren't actually papering over a mistake. > > > > > > This is because we write 32 bits at a time to the reg but internally the > > > driver uses 8 bits to store the array. If you look at > > > dwxgmac2_rss_configure() you'll see that cfg->key is casted to u32 which > > > is the value we use in HW writes. Then the for loop just does the math > > > to check how many u32's has to write. > > > > That stinks of a possible misaligned data access. > > It's possible to happen only if structure field is not aligned. I guess > I can either change all to u32 or just __align the field of the struct Would __aligning the struct still produce the warning? It's good to know that this case is intentional, but I would like to consider other instances of it before we seriously consider turning it off. If the driver can be rewritten to just make use of u32, I would find that preferrable. -- Thanks, ~Nick Desaulniers
Re: [PATCH] netfilter: nft_bitwise: Adjust parentheses to fix memcmp size argument
On Wed, Aug 14, 2019 at 9:58 AM Nathan Chancellor
wrote:
>
> clang warns:
>
> net/netfilter/nft_bitwise.c:138:50: error: size argument in 'memcmp'
> call is a comparison [-Werror,-Wmemsize-comparison]
> if (memcmp(&priv->xor, &zero, sizeof(priv->xor) ||
> ~~^~
> net/netfilter/nft_bitwise.c:138:6: note: did you mean to compare the
> result of 'memcmp' instead?
> if (memcmp(&priv->xor, &zero, sizeof(priv->xor) ||
> ^
>)
> net/netfilter/nft_bitwise.c:138:32: note: explicitly cast the argument
> to size_t to silence this warning
> if (memcmp(&priv->xor, &zero, sizeof(priv->xor) ||
> ^
> (size_t)(
> 1 error generated.
>
> Adjust the parentheses so that the result of the sizeof is used for the
> size argument in memcmp, rather than the result of the comparison (which
> would always be true because sizeof is a non-zero number).
>
> Fixes: bd8699e9e292 ("netfilter: nft_bitwise: add offload support")
> Link: https://github.com/ClangBuiltLinux/linux/issues/638
> Reported-by: kbuild test robot
> Signed-off-by: Nathan Chancellor
oh no! thanks for the patch.
Reviewed-by: Nick Desaulniers
> ---
> net/netfilter/nft_bitwise.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> index 1f04ed5c518c..974300178fa9 100644
> --- a/net/netfilter/nft_bitwise.c
> +++ b/net/netfilter/nft_bitwise.c
> @@ -135,8 +135,8 @@ static int nft_bitwise_offload(struct nft_offload_ctx
> *ctx,
> {
> const struct nft_bitwise *priv = nft_expr_priv(expr);
>
> - if (memcmp(&priv->xor, &zero, sizeof(priv->xor) ||
> - priv->sreg != priv->dreg))
> + if (memcmp(&priv->xor, &zero, sizeof(priv->xor)) ||
> + priv->sreg != priv->dreg)
> return -EOPNOTSUPP;
>
> memcpy(&ctx->regs[priv->dreg].mask, &priv->mask, sizeof(priv->mask));
> --
> 2.23.0.rc2
>
--
Thanks,
~Nick Desaulniers
[PATCH 00/16] treewide: prefer __section from compiler_attributes.h
GCC unescapes escaped string section names while Clang does not. Because
__section uses the `#` stringification operator for the section name, it
doesn't need to be escaped.
This fixes an Oops observed in distro's that use systemd and not
net.core.bpf_jit_enable=1, when their kernels are compiled with Clang.
Instead, we should:
1. Prefer __section(.section_name_no_quotes).
2. Only use __attribute__((__section(".section"))) when creating the
section name via C preprocessor (see the definition of __define_initcall
in arch/um/include/shared/init.h).
This antipattern was found with:
$ grep -e __section\(\" -e __section__\(\" -r
See the discussions in:
https://bugs.llvm.org/show_bug.cgi?id=42950
https://marc.info/?l=linux-netdev&m=156412960619946&w=2
Nick Desaulniers (16):
s390/boot: fix section name escaping
arc: prefer __section from compiler_attributes.h
parisc: prefer __section from compiler_attributes.h
um: prefer __section from compiler_attributes.h
sh: prefer __section from compiler_attributes.h
ia64: prefer __section from compiler_attributes.h
arm: prefer __section from compiler_attributes.h
mips: prefer __section from compiler_attributes.h
sparc: prefer __section from compiler_attributes.h
powerpc: prefer __section and __printf from compiler_attributes.h
x86: prefer __section from compiler_attributes.h
arm64: prefer __section from compiler_attributes.h
include/asm-generic: prefer __section from compiler_attributes.h
include/linux: prefer __section from compiler_attributes.h
include/linux/compiler.h: remove unused KENTRY macro
compiler_attributes.h: add note about __section
arch/arc/include/asm/linkage.h| 8 +++
arch/arc/include/asm/mach_desc.h | 3 +--
arch/arm/include/asm/cache.h | 2 +-
arch/arm/include/asm/mach/arch.h | 4 ++--
arch/arm/include/asm/setup.h | 2 +-
arch/arm64/include/asm/cache.h| 2 +-
arch/arm64/kernel/smp_spin_table.c| 2 +-
arch/ia64/include/asm/cache.h | 2 +-
arch/mips/include/asm/cache.h | 2 +-
arch/parisc/include/asm/cache.h | 2 +-
arch/parisc/include/asm/ldcw.h| 2 +-
arch/powerpc/boot/main.c | 3 +--
arch/powerpc/boot/ps3.c | 6 ++
arch/powerpc/include/asm/cache.h | 2 +-
arch/powerpc/kernel/btext.c | 2 +-
arch/s390/boot/startup.c | 2 +-
arch/sh/include/asm/cache.h | 2 +-
arch/sparc/include/asm/cache.h| 2 +-
arch/sparc/kernel/btext.c | 2 +-
arch/um/kernel/um_arch.c | 6 +++---
arch/x86/include/asm/cache.h | 2 +-
arch/x86/include/asm/intel-mid.h | 2 +-
arch/x86/include/asm/iommu_table.h| 5 ++---
arch/x86/include/asm/irqflags.h | 2 +-
arch/x86/include/asm/mem_encrypt.h| 2 +-
arch/x86/kernel/cpu/cpu.h | 3 +--
include/asm-generic/error-injection.h | 2 +-
include/asm-generic/kprobes.h | 5 ++---
include/linux/cache.h | 6 +++---
include/linux/compiler.h | 31 ---
include/linux/compiler_attributes.h | 10 +
include/linux/cpu.h | 2 +-
include/linux/export.h| 2 +-
include/linux/init_task.h | 4 ++--
include/linux/interrupt.h | 5 ++---
include/linux/sched/debug.h | 2 +-
include/linux/srcutree.h | 2 +-
37 files changed, 62 insertions(+), 83 deletions(-)
--
2.23.0.rc1.153.gdeed80330f-goog
[PATCH 16/16] compiler_attributes.h: add note about __section
The antipattern described can be found with:
$ grep -e __section\(\" -r -e __section__\(\"
Link: https://bugs.llvm.org/show_bug.cgi?id=42950
Signed-off-by: Nick Desaulniers
---
include/linux/compiler_attributes.h | 10 ++
1 file changed, 10 insertions(+)
diff --git a/include/linux/compiler_attributes.h
b/include/linux/compiler_attributes.h
index 6b318efd8a74..f8c008d7f616 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -225,6 +225,16 @@
#define __pure __attribute__((__pure__))
/*
+ * Note: Since this macro makes use of the "stringification operator" `#`, a
+ *quoted string literal should not be passed to it. eg.
+ *prefer:
+ *__section(.foo)
+ *to:
+ *__section(".foo")
+ *unless the section name is dynamically built up, in which case the
+ *verbose __attribute__((__section__(".foo" x))) should be preferred.
+ *See also: https://bugs.llvm.org/show_bug.cgi?id=42950
+ *
* gcc:
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-section-function-attribute
* gcc:
https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-section-variable-attribute
* clang:
https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate
--
2.23.0.rc1.153.gdeed80330f-goog
[PATCH 14/16] include/linux: prefer __section from compiler_attributes.h
Link: https://github.com/ClangBuiltLinux/linux/issues/619
Reported-by: Sedat Dilek
Suggested-by: Josh Poimboeuf
Signed-off-by: Nick Desaulniers
---
include/linux/cache.h | 6 +++---
include/linux/compiler.h| 8
include/linux/cpu.h | 2 +-
include/linux/export.h | 2 +-
include/linux/init_task.h | 4 ++--
include/linux/interrupt.h | 5 ++---
include/linux/sched/debug.h | 2 +-
include/linux/srcutree.h| 2 +-
8 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/include/linux/cache.h b/include/linux/cache.h
index 750621e41d1c..3f4df9eef1e1 100644
--- a/include/linux/cache.h
+++ b/include/linux/cache.h
@@ -28,7 +28,7 @@
* but may get written to during init, so can't live in .rodata (via "const").
*/
#ifndef __ro_after_init
-#define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
+#define __ro_after_init __section(.data..ro_after_init)
#endif
#ifndef cacheline_aligned
@@ -45,8 +45,8 @@
#ifndef __cacheline_aligned
#define __cacheline_aligned\
- __attribute__((__aligned__(SMP_CACHE_BYTES), \
-__section__(".data..cacheline_aligned")))
+ __aligned(SMP_CACHE_BYTES) \
+ __section(.data..cacheline_aligned)
#endif /* __cacheline_aligned */
#ifndef __cacheline_aligned_in_smp
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f0fd5636fddb..5e88e7e33abe 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -24,7 +24,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int
val,
long __r; \
static struct ftrace_likely_data\
__aligned(4)\
- __section("_ftrace_annotated_branch") \
+ __section(_ftrace_annotated_branch) \
__f = { \
.data.func = __func__, \
.data.file = __FILE__, \
@@ -60,7 +60,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int
val,
#define __trace_if_value(cond) ({ \
static struct ftrace_branch_data\
__aligned(4)\
- __section("_ftrace_branch") \
+ __section(_ftrace_branch) \
__if_trace = { \
.func = __func__, \
.file = __FILE__, \
@@ -118,7 +118,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int
val,
".popsection\n\t"
/* Annotate a C jump table to allow objtool to follow the code flow */
-#define __annotate_jump_table __section(".rodata..c_jump_table")
+#define __annotate_jump_table __section(.rodata..c_jump_table)
#else
#define annotate_reachable()
@@ -298,7 +298,7 @@ unsigned long read_word_at_a_time(const void *addr)
* visible to the compiler.
*/
#define __ADDRESSABLE(sym) \
- static void * __section(".discard.addressable") __used \
+ static void * __section(.discard.addressable) __used \
__PASTE(__addressable_##sym, __LINE__) = (void *)&sym;
/**
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index fcb1386bb0d4..186bbd79d6ce 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -166,7 +166,7 @@ void cpu_startup_entry(enum cpuhp_state state);
void cpu_idle_poll_ctrl(bool enable);
/* Attach to any functions which should be considered cpuidle. */
-#define __cpuidle __attribute__((__section__(".cpuidle.text")))
+#define __cpuidle __section(.cpuidle.text)
bool cpu_in_idle(unsigned long pc);
diff --git a/include/linux/export.h b/include/linux/export.h
index fd8711ed9ac4..808c1a0c2ef9 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -104,7 +104,7 @@ struct kernel_symbol {
* discarded in the final link stage.
*/
#define __ksym_marker(sym) \
- static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
+ static int __ksym_marker_##sym[0] __section(.discard.ksym) __used
#define __EXPORT_SYMBOL(sym, sec) \
__ksym_marker(sym); \
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 6049baa5b8bc..50139505da34 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -51,12 +51,12 @@ extern struct cred init_cred;
/* Attach to the init_task data structure for proper alignment */
#ifdef CONFIG_ARCH_TASK_STRUCT_ON_STACK
-#define __init_task_data __attribu
[PATCH 15/16] include/linux/compiler.h: remove unused KENTRY macro
This macro is not used throughout the kernel. Delete it rather than
update the __section to be a fully spelled out
__attribute__((__section__())) to avoid
https://bugs.llvm.org/show_bug.cgi?id=42950.
Signed-off-by: Nick Desaulniers
---
include/linux/compiler.h | 23 ---
1 file changed, 23 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 5e88e7e33abe..f01c1e527f85 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -136,29 +136,6 @@ void ftrace_likely_update(struct ftrace_likely_data *f,
int val,
} while (0)
#endif
-/*
- * KENTRY - kernel entry point
- * This can be used to annotate symbols (functions or data) that are used
- * without their linker symbol being referenced explicitly. For example,
- * interrupt vector handlers, or functions in the kernel image that are found
- * programatically.
- *
- * Not required for symbols exported with EXPORT_SYMBOL, or initcalls. Those
- * are handled in their own way (with KEEP() in linker scripts).
- *
- * KENTRY can be avoided if the symbols in question are marked as KEEP() in the
- * linker script. For example an architecture could KEEP() its entire
- * boot/exception vector code rather than annotate each function and data.
- */
-#ifndef KENTRY
-# define KENTRY(sym) \
- extern typeof(sym) sym; \
- static const unsigned long __kentry_##sym \
- __used \
- __section("___kentry" "+" #sym )\
- = (unsigned long)&sym;
-#endif
-
#ifndef RELOC_HIDE
# define RELOC_HIDE(ptr, off) \
({ unsigned long __ptr; \
--
2.23.0.rc1.153.gdeed80330f-goog
Re: [PATCH v3 13/17] mvpp2: no need to check return value of debugfs_create functions
On Mon, Aug 12, 2019 at 12:01 PM Greg Kroah-Hartman wrote: > > On Mon, Aug 12, 2019 at 10:55:51AM -0700, Nick Desaulniers wrote: > > On Sat, Aug 10, 2019 at 3:17 AM Greg Kroah-Hartman > > wrote: > > > > > > When calling debugfs functions, there is no need to ever check the > > > return value. The function can work or not, but the code logic should > > > never do something different based on this. > > > > Maybe adding this recommendation to the comment block above the > > definition of debugfs_create_dir() in fs/debugfs/inode.c would help > > prevent this issue in the future? What failure means, and how to > > proceed can be tricky; more documentation can only help in this > > regard. > > If it was there, would you have read it? :) Absolutely; I went looking for it, which is why I haven't added my reviewed by tag, because it's not clear from the existing comment block how callers should handle the return value, particularly as you describe in this commit's commit message. > > I'll add it to the list for when I revamp the debugfs documentation that > is already in the kernel, that very few people actually read... > > thanks, > > greg k-h -- Thanks, ~Nick Desaulniers
Re: [PATCH v3 13/17] mvpp2: no need to check return value of debugfs_create functions
On Sat, Aug 10, 2019 at 3:17 AM Greg Kroah-Hartman
wrote:
>
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
Maybe adding this recommendation to the comment block above the
definition of debugfs_create_dir() in fs/debugfs/inode.c would help
prevent this issue in the future? What failure means, and how to
proceed can be tricky; more documentation can only help in this
regard.
>
> Cc: "David S. Miller"
> Cc: Maxime Chevallier
> Cc: Nick Desaulniers
> Cc: Nathan Huckleberry
> Cc: netdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> .../ethernet/marvell/mvpp2/mvpp2_debugfs.c| 19 +--
> 1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
> b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
> index 274fb07362cb..4a3baa7e0142 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
> @@ -452,8 +452,6 @@ static int mvpp2_dbgfs_flow_port_init(struct dentry
> *parent,
> struct dentry *port_dir;
>
> port_dir = debugfs_create_dir(port->dev->name, parent);
> - if (IS_ERR(port_dir))
> - return PTR_ERR(port_dir);
>
> port_entry = &port->priv->dbgfs_entries->port_flow_entries[port->id];
>
> @@ -480,8 +478,6 @@ static int mvpp2_dbgfs_flow_entry_init(struct dentry
> *parent,
> sprintf(flow_entry_name, "%02d", flow);
>
> flow_entry_dir = debugfs_create_dir(flow_entry_name, parent);
> - if (!flow_entry_dir)
> - return -ENOMEM;
>
> entry = &priv->dbgfs_entries->flow_entries[flow];
>
> @@ -514,8 +510,6 @@ static int mvpp2_dbgfs_flow_init(struct dentry *parent,
> struct mvpp2 *priv)
> int i, ret;
>
> flow_dir = debugfs_create_dir("flows", parent);
> - if (!flow_dir)
> - return -ENOMEM;
>
> for (i = 0; i < MVPP2_N_PRS_FLOWS; i++) {
> ret = mvpp2_dbgfs_flow_entry_init(flow_dir, priv, i);
> @@ -539,8 +533,6 @@ static int mvpp2_dbgfs_prs_entry_init(struct dentry
> *parent,
> sprintf(prs_entry_name, "%03d", tid);
>
> prs_entry_dir = debugfs_create_dir(prs_entry_name, parent);
> - if (!prs_entry_dir)
> - return -ENOMEM;
>
> entry = &priv->dbgfs_entries->prs_entries[tid];
>
> @@ -578,8 +570,6 @@ static int mvpp2_dbgfs_prs_init(struct dentry *parent,
> struct mvpp2 *priv)
> int i, ret;
>
> prs_dir = debugfs_create_dir("parser", parent);
> - if (!prs_dir)
> - return -ENOMEM;
>
> for (i = 0; i < MVPP2_PRS_TCAM_SRAM_SIZE; i++) {
> ret = mvpp2_dbgfs_prs_entry_init(prs_dir, priv, i);
> @@ -688,8 +678,6 @@ static int mvpp2_dbgfs_port_init(struct dentry *parent,
> struct dentry *port_dir;
>
> port_dir = debugfs_create_dir(port->dev->name, parent);
> - if (IS_ERR(port_dir))
> - return PTR_ERR(port_dir);
>
> debugfs_create_file("parser_entries", 0444, port_dir, port,
> &mvpp2_dbgfs_port_parser_fops);
> @@ -716,15 +704,10 @@ void mvpp2_dbgfs_init(struct mvpp2 *priv, const char
> *name)
> int ret, i;
>
> mvpp2_root = debugfs_lookup(MVPP2_DRIVER_NAME, NULL);
> - if (!mvpp2_root) {
> + if (!mvpp2_root)
> mvpp2_root = debugfs_create_dir(MVPP2_DRIVER_NAME, NULL);
> - if (IS_ERR(mvpp2_root))
> - return;
> - }
>
> mvpp2_dir = debugfs_create_dir(name, mvpp2_root);
> - if (IS_ERR(mvpp2_dir))
> - return;
>
> priv->dbgfs_dir = mvpp2_dir;
> priv->dbgfs_entries = kzalloc(sizeof(*priv->dbgfs_entries),
> GFP_KERNEL);
> --
> 2.22.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH net v2 1/2] ipv6: constify rt6_nexthop()
On Mon, Jun 24, 2019 at 10:22 AM David Miller wrote: > > From: Nick Desaulniers > Date: Mon, 24 Jun 2019 10:17:03 -0700 > > > On Mon, Jun 24, 2019 at 10:06 AM David Miller wrote: > >> And you mean just changing to 'const' fixes something, how? > > > > See the warning in the above link (assuming now you have access). > > Assigning a non-const variable the result of a function call that > > returns const discards the const qualifier. > > Ok thanks for clarifying. > > However I was speaking in terms of this fixing a functional bug rather > than a loss of const warning. The author stated that this patch was no functional change. Nicolas, it can be helpful to include compiler warnings in the commit message when sending warning fixes, but it's not a big deal. Thanks for sending the patches. -- Thanks, ~Nick Desaulniers
Re: [PATCH net v2 1/2] ipv6: constify rt6_nexthop()
On Mon, Jun 24, 2019 at 10:06 AM David Miller wrote: > > From: Nick Desaulniers > Date: Mon, 24 Jun 2019 09:45:14 -0700 > > > https://groups.google.com/forum/#!searchin/clang-built-linux/const%7Csort:date/clang-built-linux/umkS84jS9m8/GAVVEgNYBgAJ > > Inaccessible... > > This group either doesn't exist, or you don't have permission > to access it. If you're sure this group exists, contact the > Owner of the group and ask them to give you access. Sorry, I set up the mailing list not too long ago, seem to have a long tail of permissions related issues. I confirmed that the link was borked in an incognito window. Via https://support.google.com/a/answer/9325317#Visibility I was able to change the obscure setting. I now confirmed the above link works in an incognito window. Thanks for reporting; can you please triple check? > > And you mean just changing to 'const' fixes something, how? See the warning in the above link (assuming now you have access). Assigning a non-const variable the result of a function call that returns const discards the const qualifier. -- Thanks, ~Nick Desaulniers
Re: [PATCH net v2 1/2] ipv6: constify rt6_nexthop()
On Mon, Jun 24, 2019 at 7:01 AM Nicolas Dichtel
wrote:
>
> There is no functional change in this patch, it only prepares the next one.
>
> rt6_nexthop() will be used by ip6_dst_lookup_neigh(), which uses const
> variables.
>
> Signed-off-by: Nicolas Dichtel
Also, I think this fixes an issues reported by 0day:
https://groups.google.com/forum/#!searchin/clang-built-linux/const%7Csort:date/clang-built-linux/umkS84jS9m8/GAVVEgNYBgAJ
Reported-by: kbuild test robot
Acked-by: Nick Desaulniers
> ---
> drivers/net/vrf.c| 2 +-
> include/net/ip6_route.h | 4 ++--
> net/bluetooth/6lowpan.c | 4 ++--
> net/ipv6/ip6_output.c| 2 +-
> net/netfilter/nf_flow_table_ip.c | 2 +-
> 5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index 11b9525dff27..311b0cc6eb98 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -350,8 +350,8 @@ static int vrf_finish_output6(struct net *net, struct
> sock *sk,
> {
> struct dst_entry *dst = skb_dst(skb);
> struct net_device *dev = dst->dev;
> + const struct in6_addr *nexthop;
> struct neighbour *neigh;
> - struct in6_addr *nexthop;
> int ret;
>
> nf_reset(skb);
> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index 4790beaa86e0..ee7405e759ba 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -262,8 +262,8 @@ static inline bool ip6_sk_ignore_df(const struct sock *sk)
>inet6_sk(sk)->pmtudisc == IPV6_PMTUDISC_OMIT;
> }
>
> -static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt,
> - struct in6_addr *daddr)
> +static inline const struct in6_addr *rt6_nexthop(const struct rt6_info *rt,
> +const struct in6_addr *daddr)
> {
> if (rt->rt6i_flags & RTF_GATEWAY)
> return &rt->rt6i_gateway;
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 19d27bee285e..1555b0c6f7ec 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -160,10 +160,10 @@ static inline struct lowpan_peer
> *peer_lookup_dst(struct lowpan_btle_dev *dev,
> struct in6_addr *daddr,
> struct sk_buff *skb)
> {
> - struct lowpan_peer *peer;
> - struct in6_addr *nexthop;
> struct rt6_info *rt = (struct rt6_info *)skb_dst(skb);
> int count = atomic_read(&dev->peer_count);
> + const struct in6_addr *nexthop;
> + struct lowpan_peer *peer;
I see the added const, but I'm not sure why the declarations were
reordered? Here and below. Doesn't matter for code review (doesn't
necessitate a v2).
>
> BT_DBG("peers %d addr %pI6c rt %p", count, daddr, rt);
>
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 834475717110..21efcd02f337 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -59,8 +59,8 @@ static int ip6_finish_output2(struct net *net, struct sock
> *sk, struct sk_buff *
> {
> struct dst_entry *dst = skb_dst(skb);
> struct net_device *dev = dst->dev;
> + const struct in6_addr *nexthop;
> struct neighbour *neigh;
> - struct in6_addr *nexthop;
> int ret;
>
> if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) {
> diff --git a/net/netfilter/nf_flow_table_ip.c
> b/net/netfilter/nf_flow_table_ip.c
> index 241317473114..cdfc33517e85 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -439,9 +439,9 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
> struct nf_flowtable *flow_table = priv;
> struct flow_offload_tuple tuple = {};
> enum flow_offload_tuple_dir dir;
> + const struct in6_addr *nexthop;
> struct flow_offload *flow;
> struct net_device *outdev;
> - struct in6_addr *nexthop;
> struct ipv6hdr *ip6h;
> struct rt6_info *rt;
>
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] net: stmmac: Avoid one more sometimes uninitialized Clang warning
On Thu, Mar 7, 2019 at 8:02 PM Nathan Chancellor
wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c:111:2: error: variable
> 'ns' is used uninitialized whenever 'if' condition is false
> [-Werror,-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c:111:2: error: variable
> 'ns' is used uninitialized whenever '&&' condition is false
> [-Werror,-Wsometimes-uninitialized]
>
> Clang is concerned with the use of stmmac_do_void_callback (which
> stmmac_get_systime wraps), as it may fail to initialize these values if
> the if condition was ever false (meaning the callback doesn't exist).
> It's not wrong because the callback is what initializes ns. While it's
> unlikely that the callback is going to disappear at some point and make
> that condition false, we can easily avoid this warning by zero
> initializing the variable.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/384
> Fixes: df103170854e ("net: stmmac: Avoid sometimes uninitialized Clang
> warnings")
Was initially confused by the link (thought I caught the 3 previous
instances in code review of df103170854e). I see now that I did not
report stmmac_ptp.c (only stmmac_main.c) properly. Had I, this could
have been bundled up into df103170854e indeed. Sorry. Thanks for
circling back and cleaning up this additional instance.
Reviewed-by: Nick Desaulniers
> Suggested-by: Nick Desaulniers
> Signed-off-by: Nathan Chancellor
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 2293e21f789f..cc60b3fb0892 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -105,7 +105,7 @@ static int stmmac_get_time(struct ptp_clock_info *ptp,
> struct timespec64 *ts)
> struct stmmac_priv *priv =
> container_of(ptp, struct stmmac_priv, ptp_clock_ops);
> unsigned long flags;
> - u64 ns;
> + u64 ns = 0;
>
> spin_lock_irqsave(&priv->ptp_lock, flags);
> stmmac_get_systime(priv, priv->ptpaddr, &ns);
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] net: ethernet: sun: Zero initialize class in default case in niu_add_ethtool_tcam_entry
On Thu, Mar 7, 2019 at 3:29 PM Nathan Chancellor wrote: > > When building with -Wsometimes-uninitialized, Clang warns: > > drivers/net/ethernet/sun/niu.c:7466:5: warning: variable 'class' is used > uninitialized whenever switch default is taken > [-Wsometimes-uninitialized] > > The default case can never happen because i can only be 0 to 3 > (NIU_L3_PROG_CLS is defined as 4). To make this clear to Clang, > just zero initialize class in the default case (use the macro > CLASS_CODE_UNRECOG to make it clear this shouldn't happen). Good choice. Thanks for the patch. Reviewed-by: Nick Desaulniers > > Link: https://github.com/ClangBuiltLinux/linux/issues/403 > Signed-off-by: Nathan Chancellor > --- > drivers/net/ethernet/sun/niu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c > index d84501441edd..6f99437a6962 100644 > --- a/drivers/net/ethernet/sun/niu.c > +++ b/drivers/net/ethernet/sun/niu.c > @@ -7464,6 +7464,7 @@ static int niu_add_ethtool_tcam_entry(struct niu *np, > class = CLASS_CODE_USER_PROG4; > break; > default: > + class = CLASS_CODE_UNRECOG; > break; > } > ret = tcam_user_ip_class_set(np, class, 0, > -- > 2.21.0 > -- Thanks, ~Nick Desaulniers
Re: [PATCH] cfg80211: Change an 'else if' into an 'else' in cfg80211_calculate_bitrate_he
On Thu, Mar 7, 2019 at 3:57 PM Nathan Chancellor
wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> net/wireless/util.c:1223:11: warning: variable 'result' is used
> uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
>
> Clang can't evaluate at this point that WARN(1, ...) always returns true
> because __ret_warn_on is defined as !!(condition), which isn't
> immediately evaluated as 1. Change this branch to else so that it's
> clear to Clang that we intend to bail out here.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/382
> Suggested-by: Nick Desaulniers
> Signed-off-by: Nathan Chancellor
Thanks for the quick fix!
Reviewed-by: Nick Desaulniers
> ---
> net/wireless/util.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index e4b8db5e81ec..75899b62bdc9 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -1220,9 +1220,11 @@ static u32 cfg80211_calculate_bitrate_he(struct
> rate_info *rate)
> else if (rate->bw == RATE_INFO_BW_HE_RU &&
> rate->he_ru_alloc == NL80211_RATE_INFO_HE_RU_ALLOC_26)
> result = rates_26[rate->he_gi];
> - else if (WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
> - rate->bw, rate->he_ru_alloc))
> + else {
> + WARN(1, "invalid HE MCS: bw:%d, ru:%d\n",
> + rate->bw, rate->he_ru_alloc);
> return 0;
> + }
>
> /* now scale to the appropriate MCS */
> tmp = result;
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] iwlwifi: mvm: Change an 'else if' into an 'else' in iwl_mvm_send_add_bcast_sta
On Thu, Mar 7, 2019 at 4:03 PM Nathan Chancellor
wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/net/wireless/intel/iwlwifi/mvm/sta.c:2114:12: warning: variable
> 'queue' is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
>
> Clang can't evaluate at this point that WARN(1, ...) always returns true
> because __ret_warn_on is defined as !!(condition), which isn't
> immediately evaluated as 1. Change this branch to else so that it's
> clear to Clang that we intend to bail out here.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/399
> Signed-off-by: Nathan Chancellor
Thanks for the simple fix.
Reviewed-by: Nick Desaulniers
> ---
> drivers/net/wireless/intel/iwlwifi/mvm/sta.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> index 498c315291cf..360724ec41a6 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
> @@ -2111,8 +2111,10 @@ int iwl_mvm_send_add_bcast_sta(struct iwl_mvm *mvm,
> struct ieee80211_vif *vif)
> queue = mvm->probe_queue;
> else if (vif->type == NL80211_IFTYPE_P2P_DEVICE)
> queue = mvm->p2p_dev_queue;
> - else if (WARN(1, "Missing required TXQ for adding bcast
> STA\n"))
> + else {
> + WARN(1, "Missing required TXQ for adding bcast
> STA\n");
> return -EINVAL;
> + }
>
> bsta->tfd_queue_msk |= BIT(queue);
>
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] net: stmmac: Avoid sometimes uninitialized Clang warnings
On Thu, Mar 7, 2019 at 10:00 AM Nathan Chancellor
wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:495:3: warning: variable
> 'ns' is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:495:3: warning: variable
> 'ns' is used uninitialized whenever '&&' condition is false
> [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:532:3: warning: variable
> 'ns' is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:532:3: warning: variable
> 'ns' is used uninitialized whenever '&&' condition is false
> [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:741:3: warning: variable
> 'sec_inc' is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:741:3: warning: variable
> 'sec_inc' is used uninitialized whenever '&&' condition is false
> [-Wsometimes-uninitialized]
>
> Clang is concerned with the use of stmmac_do_void_callback (which
> stmmac_get_timestamp and stmmac_config_sub_second_increment wrap),
> as it may fail to initialize these values if the if condition was ever
> false (meaning the callbacks don't exist). It's not wrong because the
> callbacks (get_timestamp and config_sub_second_increment respectively)
> are the ones that initialize the variables. While it's unlikely that the
> callbacks are ever going to disappear and make that condition false, we
> can easily avoid this warning by zero initialize the variables.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/384
> Suggested-by: Nick Desaulniers
> Reviewed-by: Nick Desaulniers
V2 LGTM, thanks for resending.
> Signed-off-by: Nathan Chancellor
> ---
>
> v1 -> v2:
>
> * Respect reverse Christmas tree variable ordering in
> stmmac_hwtstamp_set.
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e2a13ec2e30b..97c5e1aad88f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -480,7 +480,7 @@ static void stmmac_get_tx_hwtstamp(struct stmmac_priv
> *priv,
>struct dma_desc *p, struct sk_buff *skb)
> {
> struct skb_shared_hwtstamps shhwtstamp;
> - u64 ns;
> + u64 ns = 0;
>
> if (!priv->hwts_tx_en)
> return;
> @@ -519,7 +519,7 @@ static void stmmac_get_rx_hwtstamp(struct stmmac_priv
> *priv, struct dma_desc *p,
> {
> struct skb_shared_hwtstamps *shhwtstamp = NULL;
> struct dma_desc *desc = p;
> - u64 ns;
> + u64 ns = 0;
>
> if (!priv->hwts_rx_en)
> return;
> @@ -564,8 +564,8 @@ static int stmmac_hwtstamp_set(struct net_device *dev,
> struct ifreq *ifr)
> u32 snap_type_sel = 0;
> u32 ts_master_en = 0;
> u32 ts_event_en = 0;
> + u32 sec_inc = 0;
> u32 value = 0;
> - u32 sec_inc;
> bool xmac;
>
> xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
> --
> 2.21.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 post to this group, send email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/20190307180027.6226-1-natechancellor%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
--
Thanks,
~Nick Desaulniers
Re: [PATCH] net: stmmac: Avoid sometimes uninitialized Clang warnings
On Thu, Mar 7, 2019 at 9:49 AM David Miller wrote: > > From: Nathan Chancellor > Date: Thu, 7 Mar 2019 09:21:01 -0700 > > > @@ -565,7 +565,7 @@ static int stmmac_hwtstamp_set(struct net_device *dev, > > struct ifreq *ifr) > > u32 ts_master_en = 0; > > u32 ts_event_en = 0; > > u32 value = 0; > > - u32 sec_inc; > > + u32 sec_inc = 0; > > bool xmac; > > Please don't break the reverse christmas tree ordering here, thank you. Every codebase has its own unique code style. *wonders how clang-format could or could not handle that rule* Thanks for sending the patch Nathan; if you could resend with the declaration order changed so that the line width decreases going down in the order, you can add my Reviewed-by tag. -- Thanks, ~Nick Desaulniers
Re: Clang warning in drivers/net/ethernet/intel/igc/igc_ethtool.c
On Fri, Feb 8, 2019 at 6:34 AM Michal Kubecek wrote:
>
> On Thu, Feb 07, 2019 at 10:09:21PM -0700, Nathan Chancellor wrote:
> > Hi all,
> >
> > After commit 8c5ad0dae93c ("igc: Add ethtool support"), Clang warns:
> >
> > drivers/net/ethernet/intel/igc/igc_ethtool.c:9:19: warning: variable
> > 'igc_priv_flags_strings' is not needed and will not be emitted
> > [-Wunneeded-internal-declaration]
> > static const char igc_priv_flags_strings[][ETH_GSTRING_LEN] = {
> > ^
> > 1 warning generated.
> >
> > igc_priv_flags_strings is only used in an ARRAY_SIZE macro, which is a
> > compile time evaluation, so no reference to it is being emitted in the
> > final assembly. Is it actually needed and was forgotten to be used
> > somewhere or could it be eliminated so that Clang no longer warns?
>
> That's because the driver provides get_priv_flags() and set_priv_flags()
> callbacks in its ethtool_ops to allow querying and setting legacy-rx
> private flag but it does not provide get_sset_count() and get_strings()
> to provide list of private flags to userspace ethtool.
So the variable declaration should get a `__unused` annotation then
(and maybe a comment)?
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] iwlwifi: mvm: Use div_s64 instead of do_div in iwl_mvm_debug_range_resp
On Thu, Feb 21, 2019 at 12:08 AM Nathan Chancellor
wrote:
>
> Clang warns:
>
> drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:465:2: warning:
> comparison of distinct pointer types ('typeof ((rtt_avg)) *' (aka 'long
> long *') and 'uint64_t *' (aka 'unsigned long long *'))
> [-Wcompare-distinct-pointer-types]
> do_div(rtt_avg, );
> ^
> include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
> (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
>~~ ^ ~~~
> 1 warning generated.
>
> do_div expects an unsigned dividend. Use div_s64, which expects a signed
> dividend.
>
> Fixes: 937b10c0de68 ("iwlwifi: mvm: add debug prints for FTM")
> Link: https://github.com/ClangBuiltLinux/linux/issues/372
> Suggested-by: Arnd Bergmann
> Signed-off-by: Nathan Chancellor
> ---
>
> v1 -> v2:
>
> * Fix logic (as the return value of div{,64}_s64 must be used), thanks
> to Arnd for the review.
oh boy, sorry I missed that in the initial code review, thanks Arnd
for the sharp eye!
Reviewed-by: Nick Desaulniers
Side tangent: we see this kind of difference in APIs a lot (modifying
the parameter vs returning a new value or making a copy then modifying
that) in C++ when a call site isn't passing the explicit address of
some variable or an identifier that's clearly a pointer. Ex.
int foo;
bar(foo);
Doesn't tell you whether bar mutates foo or not without looking at the
definition of bar, as it could be:
void bar(int x);
or
void bar(int& x);
I miss the convention in Ruby of using `!` suffixes on methods to
differentiate between such cases. ex:
"hello".capitalize
vs
"hello".capitalize!
both return the same value, but the one with the ! mutates the
existing object, while the one without creates a new object. And
that's a very standard convention throughout the standard library.
Whether or not people follow that convention is always another story.
One thing I'm curious about, is "why does do_div exist?" When should I
use do_div vs div_u64 (not div_s64 as is used in this patch)?
>
> drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> index e9822a3ec373..94132cfd1f56 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> @@ -460,9 +460,7 @@ static int iwl_mvm_ftm_range_resp_valid(struct iwl_mvm
> *mvm, u8 request_id,
> static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm, u8 index,
> struct cfg80211_pmsr_result *res)
> {
> - s64 rtt_avg = res->ftm.rtt_avg * 100;
> -
> - do_div(rtt_avg, );
> + s64 rtt_avg = div_s64(res->ftm.rtt_avg * 100, );
>
> IWL_DEBUG_INFO(mvm, "entry %d\n", index);
> IWL_DEBUG_INFO(mvm, "\tstatus: %d\n", res->status);
> --
> 2.21.0.rc1
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] iwlwifi: mvm: Use div64_s64 instead of do_div in iwl_mvm_debug_range_resp
On Tue, Feb 19, 2019 at 10:21 AM Nathan Chancellor
wrote:
>
> Clang warns:
>
> drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c:465:2: warning:
> comparison of distinct pointer types ('typeof ((rtt_avg)) *' (aka 'long
> long *') and 'uint64_t *' (aka 'unsigned long long *'))
> [-Wcompare-distinct-pointer-types]
> do_div(rtt_avg, );
> ^
> include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
> (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
>~~ ^ ~~~
> 1 warning generated.
>
> do_div expects an unsigned dividend. Use div64_s64, which expects a
> signed dividend.
Eh, IIRC, signed vs unsigned division has implications for rounding
towards zero or not, but I doubt that the round trip time average (RTT
avg) should ever be negative. General rule of thumb for C is to keep
arithmetic signed (even when working with non zero values), so rather
than make the literal () a unsigned long, I agree with your change
to keep the division signed as well. Thanks for the fix.
Reviewed-by: Nick Desaulniers
>
> Fixes: 937b10c0de68 ("iwlwifi: mvm: add debug prints for FTM")
> Link: https://github.com/ClangBuiltLinux/linux/issues/372
> Signed-off-by: Nathan Chancellor
> ---
> drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> index e9822a3ec373..92b22250eb7d 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/ftm-initiator.c
> @@ -462,7 +462,7 @@ static void iwl_mvm_debug_range_resp(struct iwl_mvm *mvm,
> u8 index,
> {
> s64 rtt_avg = res->ftm.rtt_avg * 100;
>
> - do_div(rtt_avg, );
> + div64_s64(rtt_avg, 6666);
>
> IWL_DEBUG_INFO(mvm, "entry %d\n", index);
> IWL_DEBUG_INFO(mvm, "\tstatus: %d\n", res->status);
> --
> 2.21.0.rc1
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2] isdn: avm: Fix string plus integer warning from Clang
On Wed, Jan 9, 2019 at 9:42 PM Nathan Chancellor wrote: > > A recent commit in Clang expanded the -Wstring-plus-int warning, showing > some odd behavior in this file. > > drivers/isdn/hardware/avm/b1.c:426:30: warning: adding 'int' to a string does > not append to the string [-Wstring-plus-int] > cinfo->version[j] = "\0\0" + 1; > ~~~^~~ > drivers/isdn/hardware/avm/b1.c:426:30: note: use array indexing to silence > this warning > cinfo->version[j] = "\0\0" + 1; >^ > & [ ] > 1 warning generated. > > This is equivalent to just "\0". Nick pointed out that it is smarter to > use "" instead of "\0" because "" is used elsewhere in the kernel and > can be deduplicated at the linking stage. > > Link: https://github.com/ClangBuiltLinux/linux/issues/309 > Suggested-by: Nick Desaulniers Thanks for sending the v2. I'll leave review to the maintainers to confirm my suspicions about the code's intent. > Signed-off-by: Nathan Chancellor > --- > > v1 -> v2: > > * Use "" instead of "\0", as they are equivalent, but "" can be > deduplicated by the linker, as pointed out by Nick. > > drivers/isdn/hardware/avm/b1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/isdn/hardware/avm/b1.c b/drivers/isdn/hardware/avm/b1.c > index 4ac378e48902..40ca1e8fa09f 100644 > --- a/drivers/isdn/hardware/avm/b1.c > +++ b/drivers/isdn/hardware/avm/b1.c > @@ -423,7 +423,7 @@ void b1_parse_version(avmctrl_info *cinfo) > int i, j; > > for (j = 0; j < AVM_MAXVERSION; j++) > - cinfo->version[j] = "\0\0" + 1; > + cinfo->version[j] = ""; > for (i = 0, j = 0; > j < AVM_MAXVERSION && i < cinfo->versionlen; > j++, i += cinfo->versionbuf[i] + 1) > -- > 2.20.1 > -- Thanks, ~Nick Desaulniers
Re: [PATCH] iavf: Use printf instead of gnu_printf for iavf_debug_d
On Wed, Jan 9, 2019 at 8:22 PM Nathan Chancellor
wrote:
>
> Clang warns:
>
> In file included from drivers/net/ethernet/intel/iavf/iavf_main.c:4:
> In file included from drivers/net/ethernet/intel/iavf/iavf.h:37:
> In file included from drivers/net/ethernet/intel/iavf/iavf_type.h:8:
> drivers/net/ethernet/intel/iavf/iavf_osdep.h:49:18: warning: 'format'
> attribute argument not supported: gnu_printf [-Wignored-attributes]
> __attribute__ ((format(gnu_printf, 3, 4)));
> ^
> 1 warning generated.
>
> We can convert from gnu_printf to printf without any side effects for
> two reasons:
>
> 1. All iavf_debug instances use standard printf formats, as pointed out
>by Miguel Ojeda at the below link, meaning gnu_printf is not strictly
>required.
>
> 2. However, GCC has aliased printf to gnu_printf on Linux since at least
>2010 based on git history.
Thanks for this fix!
FWIW, using godbolt to see which version something was added in is
better than giving a year of the commit. I don't think it matters for
this case, as the kernel already makes use of `printf` as you do in
this change, but food for thought for next time.
>
>From gcc/c-family/c-format.c:
>
>/* Attributes such as "printf" are equivalent to those such as
> "gnu_printf" unless this is overridden by a target. */
>static const target_ovr_attr gnu_target_overrides_format_attributes[] =
>{
> { "gnu_printf", "printf" },
> { "gnu_scanf","scanf" },
> { "gnu_strftime", "strftime" },
> { "gnu_strfmon", "strfmon" },
> { NULL, NULL }
>};
>
> The mentioned override only happens on Windows (mingw32). Changing from
> gnu_printf to printf is a no-op for GCC and stops Clang from warning.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/111
> Suggested-by: Miguel Ojeda
> Signed-off-by: Nathan Chancellor
> ---
> drivers/net/ethernet/intel/iavf/iavf_osdep.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> index e6e0b0328706..c90cafb526d0 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> @@ -46,7 +46,7 @@ struct iavf_virt_mem {
>
> #define iavf_debug(h, m, s, ...) iavf_debug_d(h, m, s, ##__VA_ARGS__)
> extern void iavf_debug_d(void *hw, u32 mask, char *fmt_str, ...)
> - __attribute__ ((format(gnu_printf, 3, 4)));
> + __printf(3, 4);
And you make use of the __attribute__ wrapper from
include/linux/compiler_attributes.h, cool.
Reviewed-by: Nick Desaulniers
--
Thanks,
~Nick Desaulniers
Re: [PATCH] isdn: avm: Fix string plus integer warning from Clang
On Mon, Jan 7, 2019 at 9:07 PM Nathan Chancellor wrote: > > A recent commit in Clang expanded the -Wstring-plus-int warning, showing > some odd behavior in this file. > > drivers/isdn/hardware/avm/b1.c:426:30: warning: adding 'int' to a string does > not append to the string [-Wstring-plus-int] > cinfo->version[j] = "\0\0" + 1; > ~~~^~~ > drivers/isdn/hardware/avm/b1.c:426:30: note: use array indexing to silence > this warning > cinfo->version[j] = "\0\0" + 1; >^ > & [ ] > 1 warning generated. > > This is equivalent to just "\0" so fix it to clean up the warning. Hard to tell what the intent of this was. I also assume that they meant to set all version strings to the empty string. I think just `= ""` would be better, as otherwise that creates a new data section containing two zero bytes vs the empty string which already probably exists throughout the kernel and can be deduplicated by the linker. https://godbolt.org/z/qf3Rn8 Sorry I gave the quick LGTM in https://github.com/ClangBuiltLinux/linux/issues/309#issuecomment-452106468, that was my mistake. Assuming there's no other comments (maybe from the maintainers) on what the intent of this code actually is, would you mind sending a V2? (Sorry again for the quick LGTM on this version). > > Link: https://github.com/ClangBuiltLinux/linux/issues/309 > Signed-off-by: Nathan Chancellor > --- > drivers/isdn/hardware/avm/b1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/isdn/hardware/avm/b1.c b/drivers/isdn/hardware/avm/b1.c > index 4ac378e48902..7fa8141e2019 100644 > --- a/drivers/isdn/hardware/avm/b1.c > +++ b/drivers/isdn/hardware/avm/b1.c > @@ -423,7 +423,7 @@ void b1_parse_version(avmctrl_info *cinfo) > int i, j; > > for (j = 0; j < AVM_MAXVERSION; j++) > - cinfo->version[j] = "\0\0" + 1; > + cinfo->version[j] = "\0"; > for (i = 0, j = 0; > j < AVM_MAXVERSION && i < cinfo->versionlen; > j++, i += cinfo->versionbuf[i] + 1) > -- > 2.20.1 > -- Thanks, ~Nick Desaulniers
Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR
Thanks for the patch and sorry for the delay; was totally unplugged
for the holidays.
On Wed, Jan 2, 2019 at 12:57 PM Michael S. Tsirkin wrote:
>
> Since commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h
> mutually exclusive") clang no longer reuses the OPTIMIZER_HIDE_VAR macro
> from compiler-gcc - instead it gets the version in
> include/linux/compiler.h. Unfortunately that version doesn't actually
> prevent compiler from optimizing out the variable.
Good catch. Did you find this via eyeballing the code, a test, or some
other way?
>
> Fix up by moving the macro out from compiler-gcc.h to compiler.h.
> Compilers without incline asm support will keep working
> since it's protected by an ifdef.
>
> Also fix up comments to match reality since we are no longer overriding
> any macros.
>
> Build-tested with gcc and clang.
>
> Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually
> exclusive")
> Cc: Eli Friedman
> Cc: Joe Perches
> Cc: Nick Desaulniers
> Cc: Linus Torvalds
> Signed-off-by: Michael S. Tsirkin
Also for more context, see:
commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against
dead store elimination")
> ---
> include/linux/compiler-clang.h | 5 ++---
> include/linux/compiler-gcc.h | 4
> include/linux/compiler-intel.h | 4 +---
> include/linux/compiler.h | 4 +++-
> 4 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 3e7dafb3ea80..7ddaeb5182e3 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -3,9 +3,8 @@
> #error "Please don't include directly, include
> instead."
> #endif
>
> -/* Some compiler specific definitions are overwritten here
> - * for Clang compiler
> - */
> +/* Compiler specific definitions for Clang compiler */
> +
> #define uninitialized_var(x) x = *(&(x))
>
> /* same as gcc, this was present in clang-2.6 so we can assume it works
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 2010493e1040..72054d9f0eaa 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -58,10 +58,6 @@
> (typeof(ptr)) (__ptr + (off)); \
> })
>
> -/* Make the optimizer believe the variable can be manipulated arbitrarily. */
> -#define OPTIMIZER_HIDE_VAR(var)
> \
> - __asm__ ("" : "=r" (var) : "0" (var))
> -
> /*
> * A trick to suppress uninitialized variable warning without generating any
> * code
> diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h
> index 517bd14e1222..b17f3cd18334 100644
> --- a/include/linux/compiler-intel.h
> +++ b/include/linux/compiler-intel.h
> @@ -5,9 +5,7 @@
>
> #ifdef __ECC
>
> -/* Some compiler specific definitions are overwritten here
> - * for Intel ECC compiler
> - */
> +/* Compiler specific definitions for Intel ECC compiler */
>
> #include
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 06396c1cf127..1ad367b4cd8d 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -152,7 +152,9 @@ void ftrace_likely_update(struct ftrace_likely_data *f,
> int val,
> #endif
>
> #ifndef OPTIMIZER_HIDE_VAR
> -#define OPTIMIZER_HIDE_VAR(var) barrier()
> +/* Make the optimizer believe the variable can be manipulated arbitrarily. */
> +#define OPTIMIZER_HIDE_VAR(var)
> \
> + __asm__ ("" : "=r" (var) : "0" (var))
> #endif
This should be fine, thanks for the cleanup! For now, we're not yet
confident to turn on Clang's integrated assembler for the kernel, but
I'll make sure to revisit this should we, in case Clang is then able
to optimize this out.
+ Eric, who might know of a better trick for what we're trying to
accomplish with this macro.
Reviewed-by: Nick Desaulniers
+ Miguel
Miguel, would you mind taking this into your compiler-attributes tree?
--
Thanks,
~Nick Desaulniers
Re: [PATCH net] perf/bpf: fix a clang compilation issue
great, thanks! On Mon, Sep 11, 2017 at 2:28 PM, David Miller wrote: > From: Yonghong Song > Date: Thu, 7 Sep 2017 18:36:15 -0700 > >> clang does not support variable length array for structure member. >> It has the following error during compilation: >> >> kernel/trace/trace_syscalls.c:568:17: error: fields must have a constant >> size: >> 'variable length array in structure' extension will never be supported >> unsigned long args[sys_data->nb_args]; >> ^ >> >> The fix is to use a fixed array length instead. >> >> Reported-by: Nick Desaulniers >> Signed-off-by: Yonghong Song > > Applied. -- Thanks, ~Nick Desaulniers
Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning
bumping for review (resending, had gmail set to html mode) On Mon, Aug 14, 2017 at 10:36 AM, Nick Desaulniers wrote: > Minor nit for the commit message that can get fixed up when being merged: > > On Fri, Aug 11, 2017 at 11:16 AM, Nick Desaulniers > wrote: > >> if (x) >> return >> ... >> >> rather than: >> >> if (!x == 0) > > should remove the `!`, ex: > > if (x == 0) > >> ... >> else >> return > > -- > Thanks, > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers
Re: [PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning
Minor nit for the commit message that can get fixed up when being merged: On Fri, Aug 11, 2017 at 11:16 AM, Nick Desaulniers wrote: > if (x) > return > ... > > rather than: > > if (!x == 0) should remove the `!`, ex: if (x == 0) > ... > else > return -- Thanks, ~Nick Desaulniers
[PATCH v2] netfilter: nf_nat_h323: fix logical-not-parentheses warning
Clang produces the following warning:
net/ipv4/netfilter/nf_nat_h323.c:553:6: error:
logical not is only applied to the left hand side of this comparison
[-Werror,-Wlogical-not-parentheses]
if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
^
add parentheses after the '!' to evaluate the comparison first
add parentheses around left hand side expression to silence this warning
There's not necessarily a bug here, but it's cleaner to return early,
ex:
if (x)
return
...
rather than:
if (!x == 0)
...
else
return
Also added a return code check that seemed to be missing in one
instance.
Signed-off-by: Nick Desaulniers
---
Changes in v2:
* Reorder if/else blocks to return early.
* Also handle this for set_h245_addr(), not just set_h225_addr().
* Add return code check for the Gnomemeeting case.
net/ipv4/netfilter/nf_nat_h323.c | 57 +---
1 file changed, 30 insertions(+), 27 deletions(-)
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 574f7ebba0b6..ac8342dcb55e 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -252,16 +252,16 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct
nf_conn *ct,
if (set_h245_addr(skb, protoff, data, dataoff, taddr,
&ct->tuplehash[!dir].tuple.dst.u3,
htons((port & htons(1)) ? nated_port + 1 :
- nated_port)) == 0) {
- /* Save ports */
- info->rtp_port[i][dir] = rtp_port;
- info->rtp_port[i][!dir] = htons(nated_port);
- } else {
+ nated_port))) {
nf_ct_unexpect_related(rtp_exp);
nf_ct_unexpect_related(rtcp_exp);
return -1;
}
+ /* Save ports */
+ info->rtp_port[i][dir] = rtp_port;
+ info->rtp_port[i][!dir] = htons(nated_port);
+
/* Success */
pr_debug("nf_nat_h323: expect RTP %pI4:%hu->%pI4:%hu\n",
&rtp_exp->tuple.src.u3.ip,
@@ -370,15 +370,15 @@ static int nat_h245(struct sk_buff *skb, struct nf_conn
*ct,
/* Modify signal */
if (set_h225_addr(skb, protoff, data, dataoff, taddr,
&ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
- /* Save ports */
- info->sig_port[dir] = port;
- info->sig_port[!dir] = htons(nated_port);
- } else {
+ htons(nated_port))) {
nf_ct_unexpect_related(exp);
return -1;
}
+ /* Save ports */
+ info->sig_port[dir] = port;
+ info->sig_port[!dir] = htons(nated_port);
+
pr_debug("nf_nat_q931: expect H.245 %pI4:%hu->%pI4:%hu\n",
&exp->tuple.src.u3.ip,
ntohs(exp->tuple.src.u.tcp.port),
@@ -462,24 +462,27 @@ static int nat_q931(struct sk_buff *skb, struct nf_conn
*ct,
/* Modify signal */
if (set_h225_addr(skb, protoff, data, 0, &taddr[idx],
&ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
- /* Save ports */
- info->sig_port[dir] = port;
- info->sig_port[!dir] = htons(nated_port);
-
- /* Fix for Gnomemeeting */
- if (idx > 0 &&
- get_h225_addr(ct, *data, &taddr[0], &addr, &port) &&
- (ntohl(addr.ip) & 0xff00) == 0x7f00) {
- set_h225_addr(skb, protoff, data, 0, &taddr[0],
- &ct->tuplehash[!dir].tuple.dst.u3,
- info->sig_port[!dir]);
- }
- } else {
+ htons(nated_port))) {
nf_ct_unexpect_related(exp);
return -1;
}
+ /* Save ports */
+ info->sig_port[dir] = port;
+ info->sig_port[!dir] = htons(nated_port);
+
+ /* Fix for Gnomemeeting */
+ if (idx > 0 &&
+ get_h225_addr(ct, *data, &taddr[0], &addr, &port) &&
+ (ntohl(addr.ip) & 0xff00) == 0x7f00) {
+ if (set_h225_addr(skb, protoff, data, 0, &taddr[0],
+ &ct->tuplehash[!dir].tuple.dst.u3,
+ info->sig_port[!dir])) {
+ nf_ct_unexpect_related(exp);
+ return -1;
+ }
+ }
+
/* Success */
pr_debug("nf_nat_ras: expect Q.931 %pI4:%hu->%pI4:%hu\n",
&exp->tuple.src.u3.ip,
@@ -550,9
Re: [PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning
bumping for review
On Mon, Jul 31, 2017 at 11:39 AM, Nick Desaulniers
wrote:
> Clang produces the following warning:
>
> net/ipv4/netfilter/nf_nat_h323.c:553:6: error:
> logical not is only applied to the left hand side of this comparison
> [-Werror,-Wlogical-not-parentheses]
> if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
> ^
> add parentheses after the '!' to evaluate the comparison first
> add parentheses around left hand side expression to silence this warning
>
> There's not necessarily a bug here, but it's cleaner to use the form:
>
> if (x != 0)
>
> rather than:
>
> if (!x == 0)
>
> Signed-off-by: Nick Desaulniers
> ---
> Also, it's even cleaner to use the form:
>
> if (x)
>
> but then if the return codes change from treating 0 as success (unlikely),
> then all call sites must be updated.
>
> I'm happy to send v2 that changes to that form, and updates the other call
> sites to be:
>
> if (set_h225_addr())
> handle_failures()
> else
> handle_success()
>
> net/ipv4/netfilter/nf_nat_h323.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/netfilter/nf_nat_h323.c
> b/net/ipv4/netfilter/nf_nat_h323.c
> index 574f7ebba0b6..d8fb251fa6e3 100644
> --- a/net/ipv4/netfilter/nf_nat_h323.c
> +++ b/net/ipv4/netfilter/nf_nat_h323.c
> @@ -550,9 +550,9 @@ static int nat_callforwarding(struct sk_buff *skb, struct
> nf_conn *ct,
> }
>
> /* Modify signal */
> - if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
> - &ct->tuplehash[!dir].tuple.dst.u3,
> - htons(nated_port)) == 0) {
> + if (set_h225_addr(skb, protoff, data, dataoff, taddr,
> + &ct->tuplehash[!dir].tuple.dst.u3,
> + htons(nated_port)) != 0) {
> nf_ct_unexpect_related(exp);
> return -1;
> }
> --
> 2.14.0.rc0.400.g1c36432dff-goog
>
--
Thanks,
~Nick Desaulniers
[PATCH] netfilter: nf_nat_h323: fix logical-not-parentheses warning
Clang produces the following warning:
net/ipv4/netfilter/nf_nat_h323.c:553:6: error:
logical not is only applied to the left hand side of this comparison
[-Werror,-Wlogical-not-parentheses]
if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
^
add parentheses after the '!' to evaluate the comparison first
add parentheses around left hand side expression to silence this warning
There's not necessarily a bug here, but it's cleaner to use the form:
if (x != 0)
rather than:
if (!x == 0)
Signed-off-by: Nick Desaulniers
---
Also, it's even cleaner to use the form:
if (x)
but then if the return codes change from treating 0 as success (unlikely),
then all call sites must be updated.
I'm happy to send v2 that changes to that form, and updates the other call
sites to be:
if (set_h225_addr())
handle_failures()
else
handle_success()
net/ipv4/netfilter/nf_nat_h323.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 574f7ebba0b6..d8fb251fa6e3 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -550,9 +550,9 @@ static int nat_callforwarding(struct sk_buff *skb, struct
nf_conn *ct,
}
/* Modify signal */
- if (!set_h225_addr(skb, protoff, data, dataoff, taddr,
- &ct->tuplehash[!dir].tuple.dst.u3,
- htons(nated_port)) == 0) {
+ if (set_h225_addr(skb, protoff, data, dataoff, taddr,
+ &ct->tuplehash[!dir].tuple.dst.u3,
+ htons(nated_port)) != 0) {
nf_ct_unexpect_related(exp);
return -1;
}
--
2.14.0.rc0.400.g1c36432dff-goog
