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

[John McCall]

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.

John.