9 ms·
GCC proves an uninitialized variable must be 0 and doesn't warn about it
- zzo38computer 8y agoIt look like a bug to me, since inode->i_nlink may be zero, and then ret is uninitialized (optimizing the final "return ret" to "return 0" should be safe (but is not necessarily worth it, depending on the target instruction set and ABI), although if warnings are enabled, it seem like the warning should still be displayed).
- tux3 8y agoThe compiler is allowed to do that. Since it would be undefined behavior to read the undefined variable, the compiler gets to pick whatever value it wants for it. So after the if, the compiler sees that ret is either 0, or an undefined value. It picks 0 as the undefined value since that's a good optimization, so ret is now always 0, and (in this case) there is no warning.
- atq2119 8y agoRight, the bug in the kernel code snippet is like GP describes, but the gcc bug isn't that the compiled code is incorrect, it's that people expect an "uninitialized variable" warning and the compiler doesn't produce one.
- deleted 8y ago[deleted]
- kazinator 8y ago> It picks 0 as the undefined value since that's a good optimization. No it isn't; it costs an extra instruction to initialize that register or memory location to zero. How C compilers optimize situations involving uninitialized variables is by leaving those variables alone. They do so with the standard's blessing, which grants them that uninitialized automatic variables are "indeterminately-valued" and that if the program uses an indeterminate value, its behavior is undefined. Correctly written programs that perform their own delayed initialization by later assignment benefit from the fact that the compiler didn't add wasteful code to initialize those objects, only to have that compiler-generated initial value be overwritten. > ret is now always 0, and (in this case) there is no warning. It's not correct for diagnostics to pertain to versions of the program that were logically altered by the compiler, rather than to the original program. It may be ISO-conforming, only because ISO C doesn't require those diagnostics in the first place, let alone requiring them to be reliable.
- hyperman1 8y agoActuallym it is: You're right to need an extra instruction, but you gain a register. Storing and reloading this register would take instructions too. I threw this in godbolt, so play around, e.g. initialize ret or invert the condition. Linus' version has 36 lines, most other things I tried end up with 40 or more. https://godbolt.org/z/xiDGam https://godbolt.org/z/xiDGam Update: Added -Wall to godbolt
- kazinator 8y agoHow do you gain a register by emitting xorl %eax, %eax versus simply not emitting that and simply letting the code access the garbage value that's already in %eax?
- tux3 8y agoSo, if the if branch is taken, then the function could really return 0, right? So GCC has to have some instruction somewhere that ensures eax is 0 at the end of the function in case the if was taken. That's why it can't just leave a garbage value in eax. Sometimes it's not actually garbage. And when it really is garbage? Well who cares, might as well return 0.
- kazinator 8y agoI don't follow. If the branch on the inode link count is taken, then the external function shmem_reserve_inode is called; ret becomes its return value then. The only way the tail of the function runs with ret == 0 is if that external function happens to return zero. What I don't somehow see is the advantage of, or requirement for, preparing a zero value in ret that didn't come from shmem_reserve_inode, for a branch of the flow where UB is invoked which causes all behavioral requirements to be off the table.
- tux3 8y agoOh, I see. It's really a behavior of x86 that's causing the confusion then. The gist of it is that ret is not eax. Let me explain. When shmem_reserve_inode is called, under the x86 C ABIs (all of them!), it will place its return value in eax. So after the call, at this precise moment, "ret" is "eax", and we don't know it's value. Then we do "if (ret) return ret", after this line "ret" is still "eax", and this time we know for sure that its value is 0. But eax is not bound to ret until the end of the function. EVERY function we call before returning is allowed to modify eax (whether that function call returns a result or not)! And since we know that ret has the value 0, well we don't even need to keep the value in register eax, do we? We can just use eax for something else and not worry about storing "ret" anywhere, it's just a constant now. >What I don't somehow see is the advantage of, or requirement for, preparing a zero value in ret that didn't come from shmem_reserve_inode That's the subtelty. We don't prepare ret, we prepare eax. Conceptually, at the C language level, at the tail of the function "ret" still contains 0. But at the x86 level, eax DOES NOT necessarily contain 0. (Because we call d_instantiate and other functions before returning, and they're allowed to modify eax). So ret has the right value, but we still need to load the value of ret, into eax before returning. And the most efficient way to do that? Just set eax to 0. I genuinely hope that helps =]
- Taniwha 8y agoNo, it picks 0 because that's required for the code path where the "if (ret) return ret;" doesn't return, for all the other code paths where it reaches the end of the subroutine ret is undefined so 0 is good for all possible paths to that point
- tux3 8y agoSince I'm on a comment spree today: I think we mostly agree, actually, but note that the compiler doesn't have to pick 0 for the undefined value at all. It only does that because that's the most convenient value to pick. (And for the code path where it's deduced to be 0, well it's not really picking, it doesn't have a choice =] ) A compiler with less optimizations could chose to spill the "ret" variable and not pick a value at all, for example. It would just return whatever happens to be lying on the stack if you took the unitialized path.
- Taniwha 8y agoyes I agree, and it's optimised out a stack location for ret so no stack goop to default to My point really is that there are two interesting paths here, one where it's undefined and one where it's 0 ... I guess my point is that 0 is more than just 'convenient' because it's required for one of those paths
- tux3 8y agoFair enough, I can see your point =]
- floatingatoll 8y agoTo summarize various replies in the thread: This is a variation of an unfixed 2004 GCC bug. Clang detects the issue when the right warnings are enabled. Those warnings are disabled currently. A developer is auditing and fixing all such instances found by Clang so that they can reenable those warnings.
- saagarjha 8y agoUnrelated, but it's interesting to see the "new" Linus: no cursing, and no calling people morons.
- deleted 8y ago[deleted]
- sfadm 8y agoNew Linus? He is a guy who sends dozens of mails every month and has cursed in a few handpicked ones. That doesn't make him especially "cursy". I'd say he's about average.
- AdmiralAsshat 8y agoAnd Howard Dean was a fairly soft-spoken politician until one particularly excited outburst at a single rally happened to have been captured on mic. The "actual" frequency of the outbursts is somewhat secondary to the perception of frequency. Every time Linus writes a profanity-laced rant, even if it's email number 83/175 for the day, that particular piece is preserved, archived, and passed around the internet for giggles, further reinforcing whatever pre-existing conception the person referencing the email is arguing for.
- moonshinefe 8y agoI was wondering this as well, as I haven't been following things for a few months. But indeed he's back maintaining the kernel and seems far more civil than previously.
- niteshade 8y agoI agree, he seems to have toned it down: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89501#c12 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89501#c12
- rcdmd 8y agoExplanation here (from 2004): https://gcc.gnu.org/ml/gcc/2004-12/msg00681.html https://gcc.gnu.org/ml/gcc/2004-12/msg00681.html It's not that GCC proves the uninitialized variable must be 0, it's just that CCP sets it to 0 and everything happens to work out in specific cases.
- DannyBee 8y agoThis is not a correct interpretation (I worked on CCP and related optimizations for years :P) It does in fact prove that it is zero, because it is the meet of lattice values (undefined, 0). You can actually solve this particular case by interpreting phi nodes differently than CCP does. CCP does not generally care about what blocks things occur in - it doesn't have to, all definitions dominate all uses, so it is safe to evaluate things in any order. The only thing you stand to lose is optimality because things only go one direction on the lattice (to ensure that it reaches a fixed point). However, in this particular case, you can see if that if you treated the phi nodes as the equivalent form (assignments occurring on the edge of the previous block), and then processed the loop blocks in dominance order, it is obvious the use is uninitialized. It is only when looking at phi nodes as an unordered black box (as ccp does) that you get this particular variant of the issue. This is, of course, just a very cheap form of flow sensitivity. You can also get the same effect by using SSI form in a lot of cases. Most compilers do not bother doing real expensive flow sensitive analysis to try to analyze stuff like this, because almost everything you can do comes with its own fun source of false positives and negatives.
- dope99 8y agoJust curious. I’m learning about compilers and would be interested in which compilers most make the trade-off in favor of optimal code generation over compilation speed.
- chriswarbo 8y agoI'm not sure about C, but I know of a few for other languages. Some compilers perform whole program optimisation, rather than separate compilation of components/modules. This takes longer, but can find some extra optimisations (e.g. if a module provides some functionality, but it's not used in the resulting program, it can be deleted as dead code). MLton does this for StandardML ( http://mlton.org http://mlton.org ) and Stalin does this for Scheme ( https://en.wikipedia.org/wiki/Stalin_(Scheme_implementation) https://en.wikipedia.org/wiki/Stalin_(Scheme_implementation) ) At the very extreme end is the idea of superoptimisation ( https://en.wikipedia.org/wiki/Superoptimization https://en.wikipedia.org/wiki/Superoptimization ). Rather than translating the given code in some way, like a compiler, a superoptimiser treats the given code like a specification or test suite. It performs a brute-force search through the space of all possible programs, looking for any that match the specification. This can find truly optimal code, but it's ridiculously slow. So far its only real-world uses are to find small "peephole" (find/replace) optimisations that can be added to real compilers like LLVM. There's a related idea called supercompilation (which is often confused or conflated with superoptimisation) https://stackoverflow.com/questions/9067545/what-is-supercompilation https://stackoverflow.com/questions/9067545/what-is-supercom... A supercompiler can be thought of as running the given code at compile-time. If we know the values of all the functions and variables in a given piece of code, we can just run it to find out what the answer is. Interestingly, we can also "run" code involving values we don't know: we just pass those values around as opaque black-boxes. This is really useful for collapsing layers of indirection, resolving dynamically dispatched targets, baking-in configurable parameters, etc. The downside is that it can lead to bloated executables. Roughly speaking, supercompilation replaces a few long paths containing many conditionals, with many short paths containing fewer conditionals. It also specialises general-purpose functions into many single-use functions which have particular arguments baked-in.
- wyldfire 8y agoAside: static analysis is a great initial effort but to really get confidence you should use techniques like sanitizers (MSan in this case) or valgrind. (This is general advice, not always applicable to kernels).
- mhh__ 8y agoYou should really use both, however they are only a tool: I don't have any numbers but I imagine Sanitizers have a good detection rate, but one should still write code defensively. There are too many horror stories, be they lethal or expensive, to be risky about memory. Not a kernel developer: Can you test Kernel code with a sanitizer (if that sanitizer doesn't work in a compiled kernel) by compiling into and using the code as blob in userspace code with mocks (Or whatever, unless it's just utility that doesn't need anything other than an interface)?
- saagarjha 8y agoYup, Address Sanitizer can hide or fail to catch bugs occasionally.
- wyldfire 8y ago> but one should still write code defensively. I'm not sure what this means. You should not write your code any differently regardless of whether you used a sanitizer to help gain confidence in your implementation. You should not use sanitizers in production (at least not the current generation). > Can you test Kernel code with a sanitizer In fact you can! Both UBSan [1] and ASan [2] are in use to root out bugs in linux. [1] https://www.kernel.org/doc/html/v4.14/dev-tools/ubsan.html https://www.kernel.org/doc/html/v4.14/dev-tools/ubsan.html [2] https://www.kernel.org/doc/html/v4.14/dev-tools/kasan.html https://www.kernel.org/doc/html/v4.14/dev-tools/kasan.html
- muricula 8y agoLinux has support for kernel ASAN and I think UBSAN too. User mode linux is a also thing and does roughly what you described, and I think you can run it under valgrind too.
- dandigangi 8y agoRandom question - what are the _underscores_ that show up frequently in Linus's responses?
- icedchai 8y agoIt's for emphasis. Some people use asterisks. He uses underscores.
- dullroar 8y agoNote he uses both, even in the same paragraph - asterisks typically mean "bold," whereas underscores typically mean "underlined."
- deleted 8y ago[deleted]
- saagarjha 8y agoExcept in Slack, which somehow interprets this to be italics.
- naikrovek 8y agoMarkdown started that in general. There's no way to underline something in Markdown; underlining just isn't supported.
- kevin_thibedeau 8y agoUnderlining was a substitute for italics before everyone had fancy word processors.
- floatingatoll 8y agoThey were always meant for emphasis, which takes on variable definitions over time. Terminals were bad at italics due to character box layouts and so the literary approach of emphasis through italics mutated into emphasis through underlines. Now that we’re all free of monospaced terminal grids, it looks like we’re drifting back to emphasis through italics.
- ncmncm 8y agoWhat I don't understand is why it doesn't optimize away the check on (inode->i_nlink), since it is assuming the code in the dependent block always runs. It has to be assuming that, because it is the only path that doesn't lead to undefined behavior.
- firethief 8y agoThe optimizer isn't required to eliminate code paths encountering dynamic UB; it is allowed to. It also doesn't have to be consistent in its "assumptions", although inconsistency may be a sign of missing efficiency somewhere
- metaphor 8y ago> The optimizer isn't required to eliminate code paths encountering dynamic UB; it is allowed to. If I understood correctly, Linus had some colorful thoughts[1] on that: If you know something is undefined, you warn about it. You don't silently generate random code that doesn't match the source code just because some paper standard says you "can". [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89501#c12 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89501#c12
- pfdietz 8y agoA simple way to avoid this problem is prohibit, in that code base, local variable declarations without initializers. If the initializer truly was unneeded the compiler would be able to eliminate it in almost all cases. Risking undefined behavior for hypothetical microspeedups is just dumb. If a case occurs where leaving out the initializer is both useful and safe then the initializer could be removed there, after deliberate review.