Page MenuHomePhabricator

Add attribute noreturn to functions that throw
ClosedPublic

Authored by hiraditya on Jun 10 2016, 10:36 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

hiraditya updated this revision to Diff 60373.Jun 10 2016, 10:36 AM
hiraditya retitled this revision from to Add attribute noreturn to functions that throw in std::vector.
hiraditya updated this object.
hiraditya added subscribers: laxmansole, sebpop, llvm-commits.
hiraditya updated this revision to Diff 60855.Jun 15 2016, 9:36 AM
hiraditya retitled this revision from Add attribute noreturn to functions that throw in std::vector to Add attribute noreturn to functions that throw.
hiraditya updated this object.

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

mclow.lists edited edge metadata.Jun 15 2016, 11:14 AM

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?

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 updated this revision to Diff 60934.Jun 15 2016, 4:52 PM
hiraditya edited edge metadata.

Replaced attribute with macro which would be defined conditionally.

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.

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.

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

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.

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.

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 updated this revision to Diff 61556.Jun 22 2016, 8:20 AM

Renamed as per Sebastian's suggestion.

EricWF edited edge metadata.Jun 29 2016, 11:40 PM

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

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 updated this revision to Diff 62398.Jun 30 2016, 12:24 PM
hiraditya edited edge metadata.

Addressed Eric's comment.

hiraditya marked an inline comment as done.Jun 30 2016, 12:24 PM

Thanks for the feedback.

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,

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

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

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.

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.

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.

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 added inline comments.Aug 24 2016, 1:13 PM
libcxx/include/__locale
1187 ↗(On Diff #62398)

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

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 added a comment.EditedAug 24 2016, 2:11 PM

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 updated this revision to Diff 69171.Aug 24 2016, 2:20 PM

Added _LIBCPP_NORETURN in the beginning of the declaration.

hiraditya marked an inline comment as done.Aug 24 2016, 2:21 PM

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?

sebpop added inline comments.Aug 24 2016, 8:04 PM
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 updated this revision to Diff 69222.Aug 25 2016, 4:28 AM
hiraditya marked 2 inline comments as done.

Added inline

sebpop accepted this revision.Aug 25 2016, 4:57 AM
sebpop added a reviewer: sebpop.

LGTM. Thanks!

This revision is now accepted and ready to land.Aug 25 2016, 4:57 AM
hiraditya updated this revision to Diff 69407.Aug 26 2016, 11:13 AM
hiraditya edited edge metadata.

Rebased against master after D23855

This revision was automatically updated to reflect the committed changes.