Reaching the end of a value-returning function in C++

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.

Thanks!
Richard

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.

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".

Shouldn't -Wreturn-type have caught this?

-- Sean Silva

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.

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 ?

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.
If you operate on source code with such a policy, the usefulness of the initial change becomes questionable.

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".

Shouldn't -Wreturn-type have caught this?

It slipped by due to warnings getting suppressed inside system headers.

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?)

FWIW, I endorse using ‘unreachable’ here outside of -O0.

John.

Compared to ‘unreachable’, I prefer always emitting a call to @llvm.trap.

Please keep in mind that there’s debugging and investigation of crash reports from -Os/O2 code as well…
I didn’t yet see an argument that there’s enough optimization opportunity in practical terms to justify the havoc that ‘unreachable’ will cause with a buggy function.
Valid code is, in reality, going to use ‘unreachable’ marks and ‘noreturn’ functions, so all we are going to achieve is “speed up” buggy code, relinquishing any hope of finding the bug or figuring out what is going on in general.

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?)

FWIW, I endorse using 'unreachable' here outside of -O0.

Compared to 'unreachable', I prefer always emitting a call to @llvm.trap.

Please keep in mind that there's debugging and investigation of crash
reports from -Os/O2 code as well..
I didn't yet see an argument that there's enough optimization opportunity in
practical terms to justify the havoc that 'unreachable' will cause with a
buggy function.
Valid code is, in reality, going to use 'unreachable' marks and 'noreturn'
functions, so all we are going to achieve is "speed up" buggy code,

Peanut gallery: I'm not really sure why this would be true. Once a
small function like that is inlined the unreachable might allow us to
prove certain properties of the parameters to the function, for
example. ((pre: x != 0) int func(int x) { if (x > 0) return 1; if (x <
0) return -1; } - now when you inline that function & you've written
correct code that meets the precondition, without having to assert it
anywhere, LLVM will be able to propagate that back & correctly assume
x != 0 for other parts of the caller that might benefit from such an
assumption)

full disclosure: not entirely true. We drop unreachable blocks
relatively early on, in SimplyCFG - so we don't actually prove too
much from them. (this swings both ways, though - if you're seeing
interesting things happen to invalid code that make them hard to
debug, I don't see any reason to believe that interesting things could
happen to valid code that make it more efficient)

Is there a case where we wouldn’t actually warn before doing this? Buggy C++ system headers?

John.

Actually Richard did fix a couple issue in libc++ headers (missing “return *this” in swaps/assignment operators for what I could see), so yes, c++ system headers (and possibly c platform headers ?)

– Matthieu

Okay.

It shouldn’t happen with C platform headers because I believe this is limited to returns of C++ types (references and things with non-trivial constructors), right?

I guess I don’t really have a problem with using @llvm.trap here unconditionally, absent any sign that anyone is using this as a portable idiom for unreachability.

John.

If you believe that the warning will catch all such bugs, then the “optimization” is actually useless (as in “it won’t get used”).

If, on the other hand you believe that warnings are not panacea and there’s a chance that such a bug will slip by (already did), which is when the ‘unreachable’ will kick in, then at best we are obfuscating the bug and and at worst we are creating a mess.
Is there a case where this is worth it ?

-Argyrios

Yes, this was my reasoning; see my earlier response.

John.

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?)

FWIW, I endorse using ‘unreachable’ here outside of -O0.

Compared to ‘unreachable’, I prefer always emitting a call to @llvm.trap.

Please keep in mind that there’s debugging and investigation of crash reports from -Os/O2 code as well…
I didn’t yet see an argument that there’s enough optimization opportunity in practical terms to justify the havoc that ‘unreachable’ will cause with a buggy function.
Valid code is, in reality, going to use ‘unreachable’ marks and ‘noreturn’ functions, so all we are going to achieve is “speed up” buggy code, relinquishing any hope of finding the bug or figuring out what is going on in general.

Is there a case where we wouldn’t actually warn before doing this? Buggy C++ system headers?

John.

If you believe that the warning will catch all such bugs, then the “optimization” is actually useless (as in “it won’t get used”).

If, on the other hand you believe that warnings are not panacea and there’s a chance that such a bug will slip by (already did), which is when the ‘unreachable’ will kick in, then at best we are obfuscating the bug and and at worst we are creating a mess.
Is there a case where this is worth it ?

Ping, are there objections to avoid using unreachable, but trap instead ?

No objection from me.

I think we should use unreachable.

This is a very real optimization that can have significant performance
impacts. Notably, it allows us to delete substantial amounts of code
that the C and C++ standard says *will not be executed*. We should
take full advantage of this.

We cannot curtail every optimization opportunity because somewhere
someone might have written buggy code. Instead we should build tools
to find such buggy code effectively. The behavior at -O0 and the use
of -fcatch-undefined-behavior both seem reasonable ways to produce a
trap and catch the behavior. Absent those, I think we should be very
aggressive about simplifying these control flow construts as much as
the standard allows us to simplify it.

To answer why we need the semantic unreachable to get these
optimization opportunities: in a word, inlining. When inlining
collapses the CFG of a function, having the unreachable hint can be
essential to selecting the proper 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?)

FWIW, I endorse using 'unreachable' here outside of -O0.

Compared to 'unreachable', I prefer always emitting a call to @llvm.trap.

Please keep in mind that there's debugging and investigation of crash
reports from -Os/O2 code as well..
I didn't yet see an argument that there's enough optimization opportunity in
practical terms to justify the havoc that 'unreachable' will cause with a
buggy function.
Valid code is, in reality, going to use 'unreachable' marks and 'noreturn'
functions, so all we are going to achieve is "speed up" buggy code,
relinquishing any hope of finding the bug or figuring out what is going on
in general.

Is there a case where we wouldn't actually warn before doing this? Buggy
C++ system headers?

John.

If you believe that the warning will catch all such bugs, then the
"optimization" is actually useless (as in "it won't get used").

If, on the other hand you believe that warnings are not panacea and there's
a chance that such a bug will slip by (already did), which is when the
'unreachable' will kick in, then at best we are obfuscating the bug and and
at worst we are creating a mess.
Is there a case where this is worth it ?

Ping, are there objections to avoid using unreachable, but trap instead ?

No objection from me.

I think we should use unreachable.

This is a very real optimization that can have significant performance
impacts. Notably, it allows us to delete substantial amounts of code
that the C and C++ standard says *will not be executed*. We should
take full advantage of this.

Is it a real optimization in practice, though? This situation can't be formed
or exposed by optimization; you actually have to have a single function
with a reachable implicit return site. Where is this supposed real code
that does this intentionally but cannot use noreturn attributes and
unreachable markers?

To answer why we need the semantic unreachable to get these
optimization opportunities: in a word, inlining. When inlining
collapses the CFG of a function, having the unreachable hint can be
essential to selecting the proper representation.

Can you expand on this? How does inlining create this opportunity?

John.