On Thu, Dec 3, 2015 at 1:34 PM, Philip Oakley <philipoakley@xxxxxxx> wrote: > From: "Junio C Hamano" <gitster@xxxxxxxxx> >> >> Stefan Naewe <stefan.naewe@xxxxxxxxx> writes: >> >>> Two functions dereference a tree pointer before checking >> >> >> Reading them a bit carefully, a reader would notice that they >> actually do not dereference the pointer at all. It just computes >> another pointer and that is done by adding the offset of object >> member in the tree struct. Well compiler people want their compiler to produce the best output, meaning the compiled code goes fast. So if you ask a compiler-writer, this may qualify enough for being a dereference, because it looks like a dereference. Assuming this is a dereference, you can further reason about upcoming if (pointer) As the pointer was already dereferenced, it can be assumed not NULL. (the pointer being NULL would be undefined behavior, in which the compiler can do whatever it wants, i.e. that case can be ignored) So with the strong assumption of the pointer being not NULL, you can optimize away an if (pointer) as that is "always" false. In case the pointer is NULL, we have had undefined behavior, so the compiler is allowed to generate wrong code. Which is why the if(pointer) is removed from the compiled binary, as less instructions make the code go faster. > > But you can't do that computation (in the error case under consideration). > Null can't be added to anything (as far as the implications of the standards > go). These are horrid gotchas because they go against the grain of all that > binary arithmetic and simplifications we learnt long ago. > > That said, the fact that we know it can't be null does save the day, until > that is, the compiler [via some coding of an interpretation] decides that it > could be null and thus undefined etc etc (which one would argue as poor > logic, but standards have no truck with such arguments;-). > > There were some discussion on undefined behaviour way back (2013-08-08) when > Stephan Beller looked at STACK's checking of the Git code, see for example > http://article.gmane.org/gmane.comp.version-control.git/231945/ > "3 issues have been discovered using the STACK tool > The paper regarding that tool can be found at > https://pdos.csail.mit.edu/papers/stack:sosp13.pdf"; (link updated) Yeah that tool would detect such a bug. I can see if I can get it to run frequently and post results somewhere. IIRC it was quite a pain to get it working correctly on Git and then reasoning for the resulting patch. > > All their source code is publicly available at > http://css.csail.mit.edu/stack/ Thanks for pointing to that tool again. :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html