[cfe-dev] Reaching the end of a value-returning function in C++

[Richard Smith]

On Mon, Oct 15, 2012 at 11:39 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com>wrote:

> On Oct 14, 2012, at 5:26 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Sun, Oct 14, 2012 at 4:07 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> On Fri, Oct 12, 2012 at 7:07 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com>wrote:
>>
>>> On Oct 4, 2012, at 4:56 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>
>>> > As of r165273, clang produces 'unreachable' if we flow off the end of
>>> a value-returning function (without returning a value) in C++. This is
>>> undefined behavior, but we used to just return undef, instead of trying to
>>> exploit it. Please let me know if this causes problems for you, and we can
>>> weaken it to an acceptable level.
>>>
>>> FYI, this caused some libc++ tests to crash (or not terminate). To
>>> highlight one example, there was this method:
>>>
>>> > template <class _CharT, class _Traits>
>>> > inline _LIBCPP_INLINE_VISIBILITY
>>> > basic_filebuf<_CharT, _Traits>&
>>> > basic_filebuf<_CharT, _Traits>::operator=(basic_filebuf&& __rhs)
>>> > {
>>> >     close();
>>> >     swap(__rhs);
>>> > }
>>>
>>> Notice the absence of a "return *this".
>>>
>>> At the test
>>> (test/input.output/file.streams/fstreams/filebuf.assign/move_assign.pass.cpp)
>>> the method was used like this:
>>>
>>>  std::filebuf f2;
>>>  f2 = move(f);
>>>
>>> Notice that the returned value from 'operator=' is ignored.
>>>
>>> There was a crash with a stack trace showing bad access inside
>>> "libunwind.dylib`_Unwind_Resume"; the stack trace didn't provide much hint
>>> at what the culprit is.
>>>
>>> I don't think producing 'unreachable' always by default is a good idea;
>>> this is too aggressive to do at the frontend level and it makes debugging
>>> (on a "-O0 -g" build) a nightmare.
>>
>>
>> Yeah, we were worried that this might be a problem for people who don't
>> use -Wreturn-type. How would you feel about inserting a call to llvm.trap
>> before the unreachable if we're building without optimizations?
>>
>
> I've implemented this in r165914. We can tune this further if there are
> still debugability problems.
>
>
> Stepping back a bit, could you elaborate on the real benefit of the
> initial change and with what kind of source code you saw it ?
>

This change was made as a cleanup when adding the
-fcatch-undefined-behavior check. Internal discussion concluded that
returning undef is not helpful, and 'unreachable' is a more appropriate
representation.


> Unless I'm missing something, this will benefit functions that are not
> checked with -Wreturn-type and are supposed to be unreachable in some path
> but are not marked as such.
> I'd prefer that these functions are actually marked as 'unreachable' in
> source code, instead of depending on the compiler implicitly assuming that
> in order to get such an optimization.
>

I agree, but if they're not marked 'unreachable' in the source code, what
IR would you want to produce for code paths which fall off the end?
@llvm.trap() at -O0 and unreachable otherwise seems reasonable to me; would
you prefer something else? (Perhaps always emitting a call to @llvm.trap?)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121015/df255ff9/attachment.html>