On Wed, Apr 17, 2013 at 6:43 PM, Jordan Rose <[email protected]> wrote: > > On Apr 17, 2013, at 18:35 , David Blaikie <[email protected]> wrote: > > On Wed, Apr 17, 2013 at 6:14 PM, Jordan Rose <[email protected]> wrote: > > > On Apr 17, 2013, at 17:15 , Anna Zaks <[email protected]> wrote: > > + // This is not valid C++; dynamic_cast with a reference type will throw > an > + // exception if the pointer does not match the expected type. However, > our > + // implementation of dynamic_cast will pass through a null pointer...or a > + // "null reference"! So this branch is actually possible. > + if (&val == 0) { //expected-note {{Assuming pointer value is null}} > > > I know this is just a test, but the comment is bogus. Instead it's something > like this: > > // This is not valid C++; if 'ptr' were null, creating 'ref' would be > illegal. > // However, this is not checked at runtime, so this branch is actually > possible. > > > Shouldn't the SA just be diagnosing the initialization of the > reference 'val' as problematic, rather than the later use? The code is > UB because of that reference initialization. > > > Yes and no. We actually tried forbidding the creation of null lvalues for a > while, but it turned out that not only does that seem to be legal,
The specifics are a bit tricky here, but my understand is the dereference itself is not UB, but initializing the reference is. (at least that's how UBSan has been implemented & I'll take Richard's word for it - though I think the wording got better/more clear here in C++11, too) > people > actually use it to implement a homegrown 'offsetof'. I don't know if that falls afoul of the above UB - I suspect it doesn't, but just by a narrow margin. So a correct checker would not fail on this while still failing on the reference binding. > The next problem here is that we don't actually find out that the pointer is > null until the if-case. At this point the reference has already been > defined, and whatever path we've taken to get to the purported null > dereference might have relied on using the reference. Even if we realized > there was a problem, it could be difficult to report it in a sensible way. > > What we could do is check the reference at initialization time in > UndefinedAssignmentChecker, like we do for return values > (ReturnUndefChecker) and function parameters (CallAndMessageChecker). At > this point, if we can't prove a pointer is null (to warn about it), we'd > just assume it's non-null. > > We've discussed this before, and I think we decided against it because > people do make this mistake, and strictly following the C++ standard here > would cause us not to warn about it at all. (Most compilers do not insert > runtime checks for this undefined behavior.) The ideal case would be to make > the assumption at bind time, and then show that the branch is dead, but the > analyzer doesn't currently have a good way to assert that something is true > on all paths, rather than just some path...which is critical for dead-code > checking. Really? That seems strange. There's no constant condition evaluation? (even things as simple as "if (false)" - which this should be equivalent to) > Anyway, yes, it's less than ideal, but until we reach the better solution we > should do a good job with what we have. Okey dokey. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
