Add attribute noreturn to functions that throw

[hiraditya]

Summary

Worked in collaboration with Sebastian Pop.

We add noreturn to all the functions in libcxx which would throw (when exceptions are enabled). That helps the compiler make useful optimization decisions.
The only place we have not inserted the noreturn is include/__split_buffer because there is no function definition of throw_length_error/throw_out_of_range, so it was not obvious if these functions would definitely throw.

[hiraditya]

Add attribute noreturn the all the functions in libcxx which throw.

[mclow.lists]

I think a macro would be better here; something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS rather than putting __attribute__((__noreturn__)) everywhere.

Besides, it makes it clear *why* these bits are being marked noreturn.

What do you think?

[sebpop]

In D21232#458989, @mclow.lists wrote:

I think a macro would be better here; something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS rather than putting __attribute__((__noreturn__)) everywhere.

Besides, it makes it clear *why* these bits are being marked noreturn.

What do you think?

Much nicer: thanks for the idea.
We will amend the patch and resubmit.

[hiraditya]

Replaced attribute with macro which would be defined conditionally.

[hiraditya]

In D21232#458989, @mclow.lists wrote:

I think a macro would be better here; something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS rather than putting __attribute__((__noreturn__)) everywhere.

Besides, it makes it clear *why* these bits are being marked noreturn.

What do you think?

Done. Thanks for the idea.

[mclow.lists]

This looks much better, but the macro name is misleading. The routines are not "always noreturn", they're "noreturn in some circumstances" - specifically when exceptions are disabled.

That's why I suggested "something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS", and I still think that's a good idea.
I'm not going to insist on that name, but something more descriptive than _LIBCPP_ATTR_NORETURN, please.

[hiraditya]

Renamed.

[hiraditya]

In D21232#459779, @mclow.lists wrote:

This looks much better, but the macro name is misleading. The routines are not "always noreturn", they're "noreturn in some circumstances" - specifically when exceptions are disabled.

That's why I suggested "something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS", and I still think that's a good idea.
I'm not going to insist on that name, but something more descriptive than _LIBCPP_ATTR_NORETURN, please.

Will do that

In D21232#459779, @mclow.lists wrote:

This looks much better, but the macro name is misleading. The routines are not "always noreturn", they're "noreturn in some circumstances" - specifically when exceptions are disabled.

That's why I suggested "something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS", and I still think that's a good idea.
I'm not going to insist on that name, but something more descriptive than _LIBCPP_ATTR_NORETURN, please.

Thanks for the suggestion, I agree LIBCPP_NORETURN_IF_NO_EXCEPTIONS makes more sense.

[sebpop]

In D21232#460075, @hiraditya wrote:

Thanks for the suggestion, I agree LIBCPP_NORETURN_IF_NO_EXCEPTIONS makes more sense.

I think the name should be LIBCPP_NORETURN_IF_EXCEPTIONS:
we add the attribute "noreturn" when exceptions are enabled.

[sebpop]

In D21232#460255, @sebpop wrote:

In D21232#460075, @hiraditya wrote:

Thanks for the suggestion, I agree LIBCPP_NORETURN_IF_NO_EXCEPTIONS makes more sense.

I think the name should be LIBCPP_NORETURN_IF_EXCEPTIONS:
we add the attribute "noreturn" when exceptions are enabled.

Or even better LIBCPP_NORETURN_ON_EXCEPTIONS

[hiraditya]

Renamed as per Sebastian's suggestion.

[hiraditya]

Ping.

[EricWF]

Sorry to jump in late here, but libc++ already has a NORETURN macro. It'l _LIBCPP_NORETURN. Please use that instead.

You'll also want to apply this change to __libcpp_throw in <exception>.

[EricWF]

Disregard my last comment. I forgot these functions aren't always noreturn.

libcxx/include/__config

308 ↗

(On Diff #61556)

This should probably be

#define _LIBCPP_NORETURN_ON_EXCEPTIONS _LIBCPP_NORETURN

[hiraditya]

Addressed Eric's comment.

[hiraditya]

Thanks for the feedback.

[hiraditya]

Ping

[hiraditya]

In D21232#458989, @mclow.lists wrote:

I think a macro would be better here; something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS rather than putting __attribute__((__noreturn__)) everywhere.

Besides, it makes it clear *why* these bits are being marked noreturn.

What do you think?

Hi Marshall,
I have updated the macro name and addressed the suggestions of Sebastian, and Eric. Please accept the patch if it looks good to you.

Thanks,

[hiraditya]

In D21232#458989, @mclow.lists wrote:

I think a macro would be better here; something like LIBCPP_NORETURN_IF_NO_EXCEPTIONS rather than putting __attribute__((__noreturn__)) everywhere.

Besides, it makes it clear *why* these bits are being marked noreturn.

What do you think?

ping

[mclow.lists]

I'm confused here.
I thought that the goal of this patch was to add noreturn to functions that throw in the case when exceptions are disabled.
which is why I suggested the macro name LIBCPP_NORETURN_IF_NO_EXCEPTIONS

But that's not what the code does. It marks the functions as noreturn only when exceptions are enabled.

What are we trying to accomplish here?

[hiraditya]

In D21232#524293, @mclow.lists wrote:

I'm confused here.
I thought that the goal of this patch was to add noreturn to functions that throw in the case when exceptions are disabled.
which is why I suggested the macro name LIBCPP_NORETURN_IF_NO_EXCEPTIONS

But that's not what the code does. It marks the functions as noreturn only when exceptions are enabled.

What are we trying to accomplish here?

I think we should mark these functions as noreturn in both the cases (except when assert is a nop). I don't know how the find if assertions are enabled, so I added noreturn only when exceptions are enabled.

[mclow.lists]

I don't know how the find if assertions are enabled, so I added noreturn only when exceptions are enabled.

I think the plan is to replace the assertions with a call to terminate() (or maybe abort()). We certainly don't want those functions returning via the normal mechanism.

[mclow.lists]

My understanding of noreturn is that control will never return to the caller. That's not the case if the call throws an exception - control returns to the caller, searching for catch blocks.

[hiraditya]

In D21232#524375, @mclow.lists wrote:

My understanding of noreturn is that control will never return to the caller. That's not the case if the call throws an exception - control returns to the caller, searching for catch blocks.

From https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

The noreturn keyword does not affect the exceptional path when that applies: a noreturn-marked function may still return to the caller by throwing an exception or calling longjmp.

[mclow.lists]

The noreturn keyword does not affect the exceptional path when that applies: a noreturn-marked function may still return to the caller by throwing an exception or calling longjmp.

I learn something new every day :-)
In that case, we can lose the _LIBCPP_NORETURN_ON_EXCEPTIONS macro altogether, and just mark those functions with _LIBCPP_NORETURN - which I think was the original proposal.

Sorry for the confusion.

[mclow.lists]

libcxx/include/__locale

1185–1188

Also, I'm pretty sure that the _LIBCPP_NORETURN needs to go on the beginning, not the end.

[kparzysz]

http://en.cppreference.com/w/cpp/language/attributes

[[noreturn]] Indicates that the function does not return.

This attribute applies to function declarations only. The behavior is undefined if the function with this attribute actually returns.
The following standard functions have this attribute: std::_Exit, std::abort, std::exit, std::quick_exit, std::unexpected, std::terminate, std::rethrow_exception, std::throw_with_nested, std::nested_exception::rethrow_nested

[hiraditya]

In D21232#524625, @kparzysz wrote:

http://en.cppreference.com/w/cpp/language/attributes

[[noreturn]] Indicates that the function does not return.

This attribute applies to function declarations only. The behavior is undefined if the function with this attribute actually returns.
The following standard functions have this attribute: std::_Exit, std::abort, std::exit, std::quick_exit, std::unexpected, std::terminate, std::rethrow_exception, std::throw_with_nested, std::nested_exception::rethrow_nested

From: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf
7.6.3 Noreturn attribute [dcl.attr.noreturn]

#2 If a function f is called where f was previously declared with the noreturn attribute and f eventually
returns, the behavior is undefined. [ Note: The function may terminate by throwing an exception. —end
note ] [ Note: Implementations are encouraged to issue a warning if a function marked [[noreturn]] might
return. —end note ]

[hiraditya]

Added _LIBCPP_NORETURN in the beginning of the declaration.

[mclow.lists]

With that one change libc++ compiles and passes all it's tests
LGTM!

libcxx/include/future

517 ↗

(On Diff #69171)

This one, too - needs to be: _LIBCPP_NORETURN inline _LIBCPP_ALWAYS_INLINE

Which compiler are you using to test these patches?

[mclow.lists]

See also D23855

[sebpop]

libcxx/include/regex

964 ↗

(On Diff #69171)

Why throw_regex_error is just "_LIBCPP_ALWAYS_INLINE"
whereas throw_future_error was declared as "inline _LIBCPP_ALWAYS_INLINE"?

I think we need to add inline as well before throw_regex_error.

[hiraditya]

Added inline

[sebpop]

LGTM. Thanks!

[hiraditya]

Rebased against master after D23855

libcxx/include/__locale

 codecvt_byname<_InternT, _ExternT, _StateT>::~codecvt_byname()
 {
 }
 
 _LIBCPP_EXTERN_TEMPLATE2(class _LIBCPP_TYPE_VIS codecvt_byname<char, char, mbstate_t>)
 _LIBCPP_EXTERN_TEMPLATE2(class _LIBCPP_TYPE_VIS codecvt_byname<wchar_t, char, mbstate_t>)
 _LIBCPP_EXTERN_TEMPLATE2(class _LIBCPP_TYPE_VIS codecvt_byname<char16_t, char, mbstate_t>)
 _LIBCPP_EXTERN_TEMPLATE2(class _LIBCPP_TYPE_VIS codecvt_byname<char32_t, char, mbstate_t>)
 
+_LIBCPP_NORETURN _LIBCPP_FUNC_VIS void __throw_runtime_error(const char*);
+
 template <size_t _Np>
 struct __narrow_to_utf8
 {
     template <class _OutputIterator, class _CharT>
     _OutputIterator
     operator()(_OutputIterator __s, const _CharT* __wb, const _CharT* __we) const;
 };
 
 template <>

libcxx/include/system_error

public:
 
     _LIBCPP_ALWAYS_INLINE
     const error_code& code() const _NOEXCEPT {return __ec_;}
 
 private:
     static string __init(const error_code&, string);
 };
 
-_LIBCPP_FUNC_VIS void __throw_system_error(int ev, const char* what_arg);
+_LIBCPP_NORETURN _LIBCPP_FUNC_VIS
+void __throw_system_error(int ev, const char* what_arg);
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_SYSTEM_ERROR