Kernel prepatch 3.11-rc4 [LWN.net]

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 12:11 UTC (Mon) by error27 (subscriber, #8346)
In reply to: Kernel prepatch 3.11-rc4 by NAR
Parent article: Kernel prepatch 3.11-rc4

Don't do that. Or if you prefer: Don't that, do.

These bugs are extremely rare since the compiler normally warns about them if you don't use the double parenthesis. Probably we get one or two of these per year in the kernel. Meanwhile Yoda code makes the code ugly and hides more bugs than it fixes. We don't write Yoda code in the kernel.

Along side the GCC warning I have a static checker that looks for bugs like:

if ((x = 10)) {

I will amend it to look for:

if ((x = &foo)) {

since that is obviously never false. Also this should trigger an unreachable code warning... Hm...

to post comments

Kernel prepatch 3.11-rc4

Posted Aug 5, 2013 12:20 UTC (Mon) by error27 (subscriber, #8346) [Link] (8 responses)

Also in the kernel checkpatch.pl doesn't let people do:

if ((rc = lbmRead(log, log->page, &bp)))

Banning these is better than writing Yoda code because it makes the code easier to read and less error prone at the same time.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:23 UTC (Tue) by dgm (subscriber, #49227) [Link] (7 responses)

How do you catch this, then:

  some_function(x = what_ever, more_args);

specially when "x = ..." comes from macro expansion?

Yoda conditions are VERY useful. You don't even need an static checker. In forcing yourself to slowdown to write the condition reversed, _you_ will catch the mistake before it even reaches the compiler. Worse is better.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:13 UTC (Tue) by mpr22 (subscriber, #60784) [Link] (5 responses)

rvalue-first comparison is a clever and useful workaround, and in the guts of a macro, it's clearly the right thing. It's also deeply jarring when I read it, so I'm not sure it's the right thing in the main body of the code.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:59 UTC (Tue) by khim (subscriber, #9252) [Link] (4 responses)

It's obviously wrong thing to do because code readability is more important then code writability: it's written once by one guy but then read by thousands and sometimes refactored (to refactor something you need to first read old code and then write new code thus balance is unchanged).

Tradeoffs may be different in code which you are writting for yourself to only run it once... but you would not believe how often such code survives for years, so I'm not sure there are places where I would like to see Yoda conditions in practice.

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:21 UTC (Wed) by dgm (subscriber, #49227) [Link] (3 responses)

What's the value of code that's easy to read, but incorrect?

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:32 UTC (Wed) by mpr22 (subscriber, #60784) [Link] (2 responses)

Low, possibly negative. But once you fix it, it is code that is both correct and easy to read, making it superior to the code that is correct and hard to read.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 13:22 UTC (Wed) by dgm (subscriber, #49227) [Link] (1 responses)

For this you first have to spot the error. Using one style you get help from the compiler, while with the other you need angry users and some debugger voodoo.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 15:25 UTC (Wed) by mpr22 (subscriber, #60784) [Link]

The compiler can be (fairly easily) modified to alert the user to the errors that the order of comparison operands people like me find natural makes possible. People-like-me's brains cannot be easily modified to find the order of comparison operands people-like-you find sufficiently cautious natural.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 10:58 UTC (Tue) by error27 (subscriber, #8346) [Link]

We don't often use a compare as a function parameter in the kernel so it's not a problem in practice. We don't like it when people use assignments as a parameter. If I spot an assignment like that in a patch, I will tell the submitter to fix it in a follow on patch. But now I will write a Smatch check to spot it automatically so I can complain every time.

I have reviewed the kernel patches so far this year. Out of 39,745 total patches, there are 21 patches where only a '=' character changed. There is only one '=' vs '==' typo and this ARM bug will be the second bug like that this year. The remaining 20 bugs from the list are almost all off-by-one bugs.

The '=' vs '==' patch was: 9e48854c58 "drm/tilcdc: Fix an incorrect condition". It was found with by checkpatch.pl. Checkpatch is a mandatory step for patches and it reflects a problem in the process that it was skipped for this patch.

It should also have been caught by GCC except the authors of that driver put extra parenthesis around everything as if they are programming in LISP. In the kernel, LISP-isms are considered bad style. We don't have an automated process to complain about this, but I do try to educate submitters to drivers/staging/ about this.

The tilcdc bug would have been caught by Smatch as well but it is an ARM thing. The new CONFIG_COMPILE_TEST might help with this in the future.

Apparently the driver was tested, but I have to think that it wasn't tested very well. The maintainer, Rob Clark, did not have a board to test it on at the time.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 7:32 UTC (Tue) by kugel (subscriber, #70540) [Link] (5 responses)

How can a yoda condition possible "hide more bugs than it fixes"?

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 10:01 UTC (Tue) by khim (subscriber, #9252) [Link] (4 responses)

Easy: when you see on Yoda condition it's harder for you to notice that code does something syntactically correct yet semantically wrong. Uses address of wrong variable (which is pushed to the end of expression with Yoda condition), for example.

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:25 UTC (Wed) by dgm (subscriber, #49227) [Link] (3 responses)

How can you tell if an expression is correct if you don't read them in full?

Kernel prepatch 3.11-rc4

Posted Aug 7, 2013 9:32 UTC (Wed) by mpr22 (subscriber, #60784) [Link] (2 responses)

You can't. Which is why making expressions hard to read is a bad idea.

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 13:24 UTC (Wed) by dgm (subscriber, #49227) [Link] (1 responses)

Which of those expressions is harder to read:

A == B

B == A

Kernel prepatch 3.11-rc4

Posted Aug 14, 2013 15:18 UTC (Wed) by mathstuf (subscriber, #69389) [Link]

Let's use some actual examples:

> i == container.end()
> container.end() == i

> result == (2 * idx * idx)
> (2 * idx * idx) == result

> pivot < container.size() / 2
> container.size() / 2 > pivot

> speed > threshold
> threshold < speed

In all of these cases except the last, I would naturally write the first line[1]. I think they read more naturally. To reduce errors, I have const on every variable possible (containers I have to build are the exception (I can't wait for initializer lists)), so my code tends to follow SSA a lot[2].

[1]I try and order all orderings to use '<' since I think "(a > min) && (a < max)" is harder to parse and using one comparison everywhere helps keep the mental overhead low.
[2]It helps in debugging since all intermediates are named and you don't get things like i->first.second.second in the middle of lines and have to backtrack what the iterator's pair nesting is all the time.

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 9:55 UTC (Tue) by khim (subscriber, #9252) [Link] (3 responses)

Also this should trigger an unreachable code warning...

You would not believe how often similar code is used in metaprogramming to strip away code which is not needed in some cases because we are using regular variable and not something more complex.

Sure, new warnings are added to compiler but that's not an easy process: even simple things like open(2) do something like this in modern system (try to look on /usr/include/bits/fcntl2.h in your system sometime, you'll be surprised to see how open(2) is implemented in modern GlibC).

Kernel prepatch 3.11-rc4

Posted Aug 6, 2013 11:05 UTC (Tue) by error27 (subscriber, #8346) [Link]

In Smatch, I print a list of unreachable code and then run a script over it to remove the kernel specific false positives.

Kernel prepatch 3.11-rc4

Posted Aug 10, 2013 11:15 UTC (Sat) by kleptog (subscriber, #1183) [Link] (1 responses)

I had a look and indeed, open(2) is implemented quite differently from what I expected. It implements error checking on the arguments and there's even a (not very well documented) __errordecl() function which allows you to add new error messages to gcc. I can probably find a use for that.

Thanks for the tip.

Kernel prepatch 3.11-rc4

Posted Aug 10, 2013 11:38 UTC (Sat) by khim (subscriber, #9252) [Link]

Small correction: __errordecl is not function, it's a macro defined in /usr/include/sys/cdefs.h and there are also similar __warndecl ...