[cfe-commits] r148874 - in /cfe/trunk: lib/Sema/SemaExprCXX.cpp test/Analysis/reference.cpp

[Chandler Carruth]

On Mon, Jan 30, 2012 at 1:10 AM, John McCall <rjmccall at apple.com> wrote:

> On Jan 29, 2012, at 1:16 PM, Nico Weber wrote:
> > Chandler / Matthieu: I tried that - it gets rid of the copy
> > constructor error, but since the volatile is casted away, the
> > Wnull-dereference warning reappears.
> >
> > John: abort() works, and that's what I'm using locally for now. I
> > don't know if all compilers are smart enough to not warn about a
> > missing return statement if the function body contains nothing but
> > abort() – but I suppose that would be other compiler's bug then.
> >
> > Another example with the same problem was in a container class, which
> > looked like:
> >
> > template<class T> class container {
> >  T Get(int index) {  if (i < 0 || i >= size_) return *(const T*)NULL;
> > return *((T*)storage_)[i]; }
> > };
> >
> > The idea here is to turn a potentially exploitable invalid read into
> > at worst a harmless invalid read. Here the semantics change slightly
> > with the abort() from 'maybe crash on invalid index' to 'definitely
> > crash on invalid index' (at least for Chromium, that's fine).
> >
> > So is abort() the recommended solution? Should the diagnostic text
> > recommend abort() when this warning occurs as part of a return
> > expression?
>
> If you want a portable, reliable execution-time failure, abort()'s the tool
> for the job.  I know there are compilers that don't have a generic
> no-return annotation that nonetheless recognize abort().  __builtin_trap()
> has a slight advantage in that it generally makes the debugger stop
> in the current function instead of somewhere inside abort() (or even
> further downwind, e.g. in kill()).
>
> We could add abort() as a third, more portable option to the diagnostic;
> I don't see any reason to only suggest it in a return-statement context.
> This is one of those places where it would be nice to have the diagnostic
> link to a webpage with a longer discussion of the benefits of the
> different alternatives.


While I agree, this comes up *very* rarely, so it may not be worth it to
slave over the diagnostic.

Maybe it'd be worth having a blurb about this on the existing Clang docs
that we can point people to when the question arises, but I genuinely hope
we're not growing new uses of this at anything like a rapid pace.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120130/9d523af1/attachment.html>