[PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Matthias Kaehlcke]

empty_child_inc/dec() use the ternary operator for conditional
operations. The conditions involve the post/pre in/decrement
operator and the operation is only performed when the condition
is *not* true. This is hard to parse for humans, use a regular
'if' construct instead and perform the in/decrement separately.

This also fixes two warnings that are emitted about the value
of the ternary expression being unused, when building the kernel
with clang + "kbuild: Remove unnecessary -Wno-unused-value"
(https://lore.kernel.org/patchwork/patch/1089869/):

CC      net/ipv4/fib_trie.o
net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
        ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
I have no good understanding of the fib_trie code, but the
disentangled code looks wrong, and it should be equivalent to the
cryptic version, unless I messed it up. In empty_child_inc()
'full_children' is only incremented when 'empty_children' is -1. I
suspect a bug in the cryptic code, but am surprised why it hasn't
blown up yet. Or is it intended behavior that is just
super-counterintuitive?

For now I'm leaving it at disentangling the cryptic expressions,
if there is a bug we can discuss what action to take.
---
 net/ipv4/fib_trie.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 868c74771fa9..cf7788e336b7 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -338,12 +338,18 @@ static struct tnode *tnode_alloc(int bits)
 
 static inline void empty_child_inc(struct key_vector *n)
 {
-	++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
+	tn_info(n)->empty_children++;
+
+	if (!tn_info(n)->empty_children)
+		tn_info(n)->full_children++;
 }
 
 static inline void empty_child_dec(struct key_vector *n)
 {
-	tn_info(n)->empty_children-- ? : tn_info(n)->full_children--;
+	if (!tn_info(n)->empty_children)
+		tn_info(n)->full_children--;
+
+	tn_info(n)->empty_children--;
 }
 
 static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Doug Anderson]

Hi,

On Tue, Jun 18, 2019 at 2:14 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> empty_child_inc/dec() use the ternary operator for conditional
> operations. The conditions involve the post/pre in/decrement
> operator and the operation is only performed when the condition
> is *not* true. This is hard to parse for humans, use a regular
> 'if' construct instead and perform the in/decrement separately.
>
> This also fixes two warnings that are emitted about the value
> of the ternary expression being unused, when building the kernel
> with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> (https://lore.kernel.org/patchwork/patch/1089869/):
>
> CC      net/ipv4/fib_trie.o
> net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
>         ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> I have no good understanding of the fib_trie code, but the
> disentangled code looks wrong, and it should be equivalent to the
> cryptic version, unless I messed it up. In empty_child_inc()
> 'full_children' is only incremented when 'empty_children' is -1. I
> suspect a bug in the cryptic code, but am surprised why it hasn't
> blown up yet. Or is it intended behavior that is just
> super-counterintuitive?
>
> For now I'm leaving it at disentangling the cryptic expressions,
> if there is a bug we can discuss what action to take.
> ---
>  net/ipv4/fib_trie.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

I have no knowledge of this code either but Matthias's patch looks
sane to me and I agree with the disentangling before making functional
changes.

My own personal belief is that this is pointing out a bug somewhere.
Since "empty_children" ends up being an unsigned type it doesn't feel
like it was by-design that -1 is ever a value that should be in there.

In any case:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Matthias Kaehlcke]

On Tue, Jun 18, 2019 at 02:45:34PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 18, 2019 at 2:14 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > empty_child_inc/dec() use the ternary operator for conditional
> > operations. The conditions involve the post/pre in/decrement
> > operator and the operation is only performed when the condition
> > is *not* true. This is hard to parse for humans, use a regular
> > 'if' construct instead and perform the in/decrement separately.
> >
> > This also fixes two warnings that are emitted about the value
> > of the ternary expression being unused, when building the kernel
> > with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> > (https://lore.kernel.org/patchwork/patch/1089869/):
> >
> > CC      net/ipv4/fib_trie.o
> > net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> >         ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > I have no good understanding of the fib_trie code, but the
> > disentangled code looks wrong, and it should be equivalent to the
> > cryptic version, unless I messed it up. In empty_child_inc()
> > 'full_children' is only incremented when 'empty_children' is -1. I
> > suspect a bug in the cryptic code, but am surprised why it hasn't
> > blown up yet. Or is it intended behavior that is just
> > super-counterintuitive?
> >
> > For now I'm leaving it at disentangling the cryptic expressions,
> > if there is a bug we can discuss what action to take.
> > ---
> >  net/ipv4/fib_trie.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> I have no knowledge of this code either but Matthias's patch looks
> sane to me and I agree with the disentangling before making functional
> changes.

I in terms of the -stable process it might make sense to either
disentangle & fix in a single step, or first fix the cryptic code
(shudder!) and then disentangle it. I guess if we make it a series
disentangle & fix could be separate steps.

I'm open to whatever maintainers & stable folks prefer.

> My own personal belief is that this is pointing out a bug somewhere.
> Since "empty_children" ends up being an unsigned type it doesn't feel
> like it was by-design that -1 is ever a value that should be in there.

good point that 'empty_children' is unsigned, that indeed reinforces
the bug theory.

> In any case:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks!

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Nathan Chancellor]

On Tue, Jun 18, 2019 at 02:14:40PM -0700, Matthias Kaehlcke wrote:
> empty_child_inc/dec() use the ternary operator for conditional
> operations. The conditions involve the post/pre in/decrement
> operator and the operation is only performed when the condition
> is *not* true. This is hard to parse for humans, use a regular
> 'if' construct instead and perform the in/decrement separately.
> 
> This also fixes two warnings that are emitted about the value
> of the ternary expression being unused, when building the kernel
> with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> (https://lore.kernel.org/patchwork/patch/1089869/):
> 
> CC      net/ipv4/fib_trie.o
> net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
>         ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> 

As an FYI, this is also being fixed in clang:

https://bugs.llvm.org/show_bug.cgi?id=42239

https://reviews.llvm.org/D63369

> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> I have no good understanding of the fib_trie code, but the
> disentangled code looks wrong, and it should be equivalent to the
> cryptic version, unless I messed it up. In empty_child_inc()
> 'full_children' is only incremented when 'empty_children' is -1. I
> suspect a bug in the cryptic code, but am surprised why it hasn't
> blown up yet. Or is it intended behavior that is just
> super-counterintuitive?
> 
> For now I'm leaving it at disentangling the cryptic expressions,
> if there is a bug we can discuss what action to take.
> ---
>  net/ipv4/fib_trie.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 868c74771fa9..cf7788e336b7 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -338,12 +338,18 @@ static struct tnode *tnode_alloc(int bits)
>  
>  static inline void empty_child_inc(struct key_vector *n)
>  {
> -	++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> +	tn_info(n)->empty_children++;
> +
> +	if (!tn_info(n)->empty_children)
> +		tn_info(n)->full_children++;
>  }
>  
>  static inline void empty_child_dec(struct key_vector *n)
>  {
> -	tn_info(n)->empty_children-- ? : tn_info(n)->full_children--;
> +	if (!tn_info(n)->empty_children)
> +		tn_info(n)->full_children--;
> +
> +	tn_info(n)->empty_children--;
>  }
>  
>  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Matthias Kaehlcke]

On Tue, Jun 18, 2019 at 04:04:20PM -0700, Nathan Chancellor wrote:
> On Tue, Jun 18, 2019 at 02:14:40PM -0700, Matthias Kaehlcke wrote:
> > empty_child_inc/dec() use the ternary operator for conditional
> > operations. The conditions involve the post/pre in/decrement
> > operator and the operation is only performed when the condition
> > is *not* true. This is hard to parse for humans, use a regular
> > 'if' construct instead and perform the in/decrement separately.
> > 
> > This also fixes two warnings that are emitted about the value
> > of the ternary expression being unused, when building the kernel
> > with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> > (https://lore.kernel.org/patchwork/patch/1089869/):
> > 
> > CC      net/ipv4/fib_trie.o
> > net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> >         ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> > 
> 
> As an FYI, this is also being fixed in clang:
> 
> https://bugs.llvm.org/show_bug.cgi?id=42239
> 
> https://reviews.llvm.org/D63369

Great, thanks!

In this case it was actually useful to get the warning, even though it
didn't point out the actual bug. I think in general it would be
preferable to avoid such constructs, even when they are correct. But
then again, it's the reviewers/maintainers task to avoid unnecessarily
cryptic code from slipping in, and this just happens to be one instance
where the compiler could have helped.

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Alexander Duyck]

On Tue, Jun 18, 2019 at 4:22 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Jun 18, 2019 at 04:04:20PM -0700, Nathan Chancellor wrote:
> > On Tue, Jun 18, 2019 at 02:14:40PM -0700, Matthias Kaehlcke wrote:
> > > empty_child_inc/dec() use the ternary operator for conditional
> > > operations. The conditions involve the post/pre in/decrement
> > > operator and the operation is only performed when the condition
> > > is *not* true. This is hard to parse for humans, use a regular
> > > 'if' construct instead and perform the in/decrement separately.
> > >
> > > This also fixes two warnings that are emitted about the value
> > > of the ternary expression being unused, when building the kernel
> > > with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> > > (https://lore.kernel.org/patchwork/patch/1089869/):
> > >
> > > CC      net/ipv4/fib_trie.o
> > > net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> > >         ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> > >
> >
> > As an FYI, this is also being fixed in clang:
> >
> > https://bugs.llvm.org/show_bug.cgi?id=42239
> >
> > https://reviews.llvm.org/D63369
>
> Great, thanks!
>
> In this case it was actually useful to get the warning, even though it
> didn't point out the actual bug. I think in general it would be
> preferable to avoid such constructs, even when they are correct. But
> then again, it's the reviewers/maintainers task to avoid unnecessarily
> cryptic code from slipping in, and this just happens to be one instance
> where the compiler could have helped.

So it took me a bit to remember/understand it as well since I haven't
touched the code in over 4 years, however part of that is because the
comment for this code is actually buried down in put_child.
Essentially this is just meant to be an add w/ carry and a sub w/
borrow to address a potential overflow if bits == KEYLENGTH.

If you want you can add:
Fixes: 95f60ea3e99a ("fib_trie: Add collapse() and should_collapse() to resize")
Acked-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Nick Desaulniers]

On Tue, Jun 18, 2019 at 4:04 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Jun 18, 2019 at 02:14:40PM -0700, Matthias Kaehlcke wrote:
> > empty_child_inc/dec() use the ternary operator for conditional
> > operations. The conditions involve the post/pre in/decrement
> > operator and the operation is only performed when the condition
> > is *not* true. This is hard to parse for humans, use a regular
> > 'if' construct instead and perform the in/decrement separately.
> >
> > This also fixes two warnings that are emitted about the value
> > of the ternary expression being unused, when building the kernel
> > with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> > (https://lore.kernel.org/patchwork/patch/1089869/):
> >
> > CC      net/ipv4/fib_trie.o
> > net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> >         ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> >
>
> As an FYI, this is also being fixed in clang:
>
> https://bugs.llvm.org/show_bug.cgi?id=42239
>
> https://reviews.llvm.org/D63369

While I'm glad we found and fixed a bug in Clang; I'm still for fixing
the underlying code from a readability perspective.  If it takes
longer than a few seconds to understand a statement to the non-author,
the code as written may be too clever.

As a side note, I'm going to try to see if MAINTAINERS and
scripts/get_maintainers.pl supports regexes on the commit messages in
order to cc our mailing list (clang-built-linux
<clang-built-linux@googlegroups.com>) automatically; but in the
meantime please try to remember to cc the list manually.  I usually
find it quickly from the bookmarked https://clangbuiltlinux.github.io/
which lists it there) (but I should still automate this).

>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > I have no good understanding of the fib_trie code, but the
> > disentangled code looks wrong, and it should be equivalent to the
> > cryptic version, unless I messed it up. In empty_child_inc()
> > 'full_children' is only incremented when 'empty_children' is -1. I
> > suspect a bug in the cryptic code, but am surprised why it hasn't
> > blown up yet. Or is it intended behavior that is just
> > super-counterintuitive?
> >
> > For now I'm leaving it at disentangling the cryptic expressions,
> > if there is a bug we can discuss what action to take.
> > ---
> >  net/ipv4/fib_trie.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> > index 868c74771fa9..cf7788e336b7 100644
> > --- a/net/ipv4/fib_trie.c
> > +++ b/net/ipv4/fib_trie.c
> > @@ -338,12 +338,18 @@ static struct tnode *tnode_alloc(int bits)
> >
> >  static inline void empty_child_inc(struct key_vector *n)
> >  {
> > -     ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> > +     tn_info(n)->empty_children++;
> > +
> > +     if (!tn_info(n)->empty_children)
> > +             tn_info(n)->full_children++;
> >  }
> >
> >  static inline void empty_child_dec(struct key_vector *n)
> >  {
> > -     tn_info(n)->empty_children-- ? : tn_info(n)->full_children--;
> > +     if (!tn_info(n)->empty_children)
> > +             tn_info(n)->full_children--;
> > +
> > +     tn_info(n)->empty_children--;

This change looks correct to me, so thanks for the patch and:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

(Bikeshed; I prefer pre-inc/dec to post-inc/dec unless you really care
about the previous value. Should be irrelevant with a sufficiently
smart compiler though. ;) )
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Joe Perches]

On Tue, 2019-06-18 at 16:23 -0700, Nick Desaulniers wrote:
> As a side note, I'm going to try to see if MAINTAINERS and
> scripts/get_maintainers.pl supports regexes on the commit messages in
> order to cc our mailing list

Neither.  Why should either?

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Nick Desaulniers]

On Wed, Jun 19, 2019 at 2:36 AM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2019-06-18 at 16:23 -0700, Nick Desaulniers wrote:
> > As a side note, I'm going to try to see if MAINTAINERS and
> > scripts/get_maintainers.pl supports regexes on the commit messages in
> > order to cc our mailing list
>
> Neither.  Why should either?

Looks like `K:` is exactly what I'm looking for.  Joe, how does:
https://github.com/ClangBuiltLinux/linux/commit/a0a64b8d65c4e7e033f49e48cc610d6e4002927e
look?  Is there a maintainer for MAINTAINERS or do I just send the
patch to Linus?

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[Joe Perches]

On Wed, 2019-06-19 at 10:41 -0700, Nick Desaulniers wrote:
> On Wed, Jun 19, 2019 at 2:36 AM Joe Perches <joe@perches.com> wrote:
> > On Tue, 2019-06-18 at 16:23 -0700, Nick Desaulniers wrote:
> > > As a side note, I'm going to try to see if MAINTAINERS and
> > > scripts/get_maintainers.pl supports regexes on the commit messages in
> > > order to cc our mailing list
> > 
> > Neither.  Why should either?
> 
> Looks like `K:` is exactly what I'm looking for.

Using K: for commit message content isn't the intended
use, but if it works for you, fine by me.

> Joe, how does:
> https://github.com/ClangBuiltLinux/linux/commit/a0a64b8d65c4e7e033f49e48cc610d6e4002927e
> look?

You might consider using

K:	\b(?i:clang|llvm)\b

to get case insensitive matches.

> Is there a maintainer for MAINTAINERS or do I just send the
> patch to Linus?

Generally MAINTAINER patches go via Andrew Morton or
indirectly along with other changes via a pull request
to Linus.

Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions

[David Miller]

From: Matthias Kaehlcke <mka@chromium.org>
Date: Tue, 18 Jun 2019 14:14:40 -0700

> empty_child_inc/dec() use the ternary operator for conditional
> operations. The conditions involve the post/pre in/decrement
> operator and the operation is only performed when the condition
> is *not* true. This is hard to parse for humans, use a regular
> 'if' construct instead and perform the in/decrement separately.
> 
> This also fixes two warnings that are emitted about the value
> of the ternary expression being unused, when building the kernel
> with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> (https://lore.kernel.org/patchwork/patch/1089869/):
> 
> CC      net/ipv4/fib_trie.o
> net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
>         ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Applied to net-next, thanks.