[libc++] Optimize _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY

[vitalybuka]

Summary

Sanitizers, e.g. ubsan can uninline this call.
https://godbolt.org/z/s51sfezb5

This holds unused printf parameters and bloats binaries.

[philnik]

Could you provide some numbers? This doesn't look great, but I don't think we care that much about binary size with sanitizers. It's not great anyways, so it would have to be quite a big difference to make it worth adding _LIBCPP_ALWAYS_INLINE. Also, please add a comment why it is added. We don't do that everywhere we use the attribute currently, but we should. So let's not make it worse.

[vitalybuka]

In D140944#4024885, @philnik wrote:

Could you provide some numbers? This doesn't look great, but I don't think we care that much about binary size with sanitizers. It's not great anyways, so it would have to be quite a big difference to make it worth adding _LIBCPP_ALWAYS_INLINE. Also, please add a comment why it is added. We don't do that everywhere we use the attribute currently, but we should. So let's not make it worse.

Actually at google we care about size with sanitizers.
server apps often close to 2GB of linker memory model limit
mobile always care about size

With asan+ubsan combo _LIBCPP_ENABLE_ASSERTIONS=1 costs us 1.6%
with this patch it's just 0.2%

[vitalybuka]

In D140944#4024899, @vitalybuka wrote:

In D140944#4024885, @philnik wrote:

Could you provide some numbers? This doesn't look great, but I don't think we care that much about binary size with sanitizers. It's not great anyways, so it would have to be quite a big difference to make it worth adding _LIBCPP_ALWAYS_INLINE. Also, please add a comment why it is added. We don't do that everywhere we use the attribute currently, but we should. So let's not make it worse.

Actually at google we care about size with sanitizers.
server apps often close to 2GB of linker memory model limit
mobile always care about size

With asan+ubsan combo _LIBCPP_ENABLE_ASSERTIONS=1 costs us 1.6%
with this patch it's just 0.2%

actually it's
_LIBCPP_ENABLE_ASSERTIONS=1 _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY=1
and _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY is there exactly to save size

[philnik]

In D140944#4024901, @vitalybuka wrote:

In D140944#4024899, @vitalybuka wrote:

In D140944#4024885, @philnik wrote:

Could you provide some numbers? This doesn't look great, but I don't think we care that much about binary size with sanitizers. It's not great anyways, so it would have to be quite a big difference to make it worth adding _LIBCPP_ALWAYS_INLINE. Also, please add a comment why it is added. We don't do that everywhere we use the attribute currently, but we should. So let's not make it worse.

Actually at google we care about size with sanitizers.
server apps often close to 2GB of linker memory model limit
mobile always care about size

With asan+ubsan combo _LIBCPP_ENABLE_ASSERTIONS=1 costs us 1.6%
with this patch it's just 0.2%

actually it's
_LIBCPP_ENABLE_ASSERTIONS=1 _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY=1
and _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY is there exactly to save size

_LIBCPP_HAS_NO_VERBOSE_ABORTIN_LIBRARY exists to make deploying to a target that doesn't have __libcpp_verbose_abort in the dylib possible, not to save size. You can use a custom verbose abort handler for that. I expect that we'll remove the inline version at some point (Not that this will happen soon).
Enabling sanitizers probably has a much larger size impact on it's own. While 1.6% vs. 0.2% is a large improvement, I'm not sure it's enough to justify adding _LIBCPP_ALWAYS_INLINE. Do you know why this doesn't get inlined? I would expect clang to be able to see through this, even with sanitizers. Fixing this in clang would probably have a much larger impact on overall binary size.

[vitalybuka]

In D140944#4024959, @philnik wrote:

In D140944#4024901, @vitalybuka wrote:

In D140944#4024899, @vitalybuka wrote:

In D140944#4024885, @philnik wrote:

Could you provide some numbers? This doesn't look great, but I don't think we care that much about binary size with sanitizers. It's not great anyways, so it would have to be quite a big difference to make it worth adding _LIBCPP_ALWAYS_INLINE. Also, please add a comment why it is added. We don't do that everywhere we use the attribute currently, but we should. So let's not make it worse.

Actually at google we care about size with sanitizers.
server apps often close to 2GB of linker memory model limit
mobile always care about size

With asan+ubsan combo _LIBCPP_ENABLE_ASSERTIONS=1 costs us 1.6%
with this patch it's just 0.2%

actually it's
_LIBCPP_ENABLE_ASSERTIONS=1 _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY=1
and _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY is there exactly to save size

_LIBCPP_HAS_NO_VERBOSE_ABORTIN_LIBRARY exists to make deploying to a target that doesn't have __libcpp_verbose_abort in the dylib possible, not to save size. You can use a custom verbose abort handler for that. I expect that we'll remove the inline version at some point (Not that this will happen soon).
Enabling sanitizers probably has a much larger size impact on it's own. While 1.6% vs. 0.2% is a large improvement, I'm not sure it's enough to justify adding _LIBCPP_ALWAYS_INLINE.

Yes, impact of sanitizers is larger, but I don't know anything that can save 1% with as simple as _LIBCPP_ALWAYS_INLINE.

Regardless of sanitizers, inlined callback is very desirable.
Some production binaries use libcxx for hardening, e.g. Chromium https://source.chromium.org/chromium/chromium/src/+/main:base/nodebug_assertion.cc;l=12?q=__libcpp_verbose_abort&ss=chromium
However without inlining it still holds all strings, filenames for verbose messaging. I guess Chromium can tolerate it because of ThinLTO. It would be nice to have general approach, and it's easy to achieve.

There is ugly workaround with _LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED and defining inline libcpp_verbose_abort in config_site.
It would be nice to have something more convenient. Seems like _LIBCPP_HAS_NO_VERBOSE_ABORTIN_LIBRARY is good candidate even if original intention was just deploying.

What is your concerns regarding _LIBCPP_ALWAYS_INLINE here, seems like it's going to be inlined in any other possible config anyway?

Do you know why this doesn't get inlined? I would expect clang to be able to see through this, even with sanitizers. Fixing this in clang would probably have a much larger impact on overall binary size.

Probably there is no easy fix in compiler itself. I see two issues with the current __libcpp_verbose_abort:

1. Compiler does not inline. I suspect with no ubsan libcpp_verbose_abort calls 1 function, with ubsan it's 2 function calls: abort()+ubsan_handle_builtin_unreachable(). Inliner CostModel selects to not inline. Inliner CostModel is tuned heuristics and likely improving this case will degrade others.
2. Compiler does not eliminate unused printf parameters. I believe it's because compiler can't do that with linkonce_odr as it does not know which definition linker will pick. However if we "static inline" it will be internal linkage, so it can drop parameters. it does not help to inline __libcpp_verbose_abort but constants are gone and I see similar savings as with _LIBCPP_ALWAYS_INLINE. Still I like _LIBCPP_ALWAYS_INLINE as more explicit so we don't care about what is going to change in optimization pipeline.

[vitalybuka]

rebase

[philnik]

In D140944#4025344, @vitalybuka wrote:

Yes, impact of sanitizers is larger, but I don't know anything that can save 1% with as simple as _LIBCPP_ALWAYS_INLINE.

Regardless of sanitizers, inlined callback is very desirable.
Some production binaries use libcxx for hardening, e.g. Chromium https://source.chromium.org/chromium/chromium/src/+/main:base/nodebug_assertion.cc;l=12?q=__libcpp_verbose_abort&ss=chromium
However without inlining it still holds all strings, filenames for verbose messaging. I guess Chromium can tolerate it because of ThinLTO. It would be nice to have general approach, and it's easy to achieve.

There is ugly workaround with _LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED and defining inline libcpp_verbose_abort in config_site.
It would be nice to have something more convenient. Seems like _LIBCPP_HAS_NO_VERBOSE_ABORTIN_LIBRARY is good candidate even if original intention was just deploying.

What is your concerns regarding _LIBCPP_ALWAYS_INLINE here, seems like it's going to be inlined in any other possible config anyway?

I'm not a fan of adding macros like _LIBCPP_ALWAYS_INLINE because it easily bit-rots and indicates a problem with the compiler optimizations. Fixing that is generally the better way to achieve these kinds of things.

Do you know why this doesn't get inlined? I would expect clang to be able to see through this, even with sanitizers. Fixing this in clang would probably have a much larger impact on overall binary size.

Probably there is no easy fix in compiler itself. I see two issues with the current __libcpp_verbose_abort:

1. Compiler does not inline. I suspect with no ubsan libcpp_verbose_abort calls 1 function, with ubsan it's 2 function calls: abort()+ubsan_handle_builtin_unreachable(). Inliner CostModel selects to not inline. Inliner CostModel is tuned heuristics and likely improving this case will degrade others.
2. Compiler does not eliminate unused printf parameters. I believe it's because compiler can't do that with linkonce_odr as it does not know which definition linker will pick. However if we "static inline" it will be internal linkage, so it can drop parameters. it does not help to inline __libcpp_verbose_abort but constants are gone and I see similar savings as with _LIBCPP_ALWAYS_INLINE. Still I like _LIBCPP_ALWAYS_INLINE as more explicit so we don't care about what is going to change in optimization pipeline.

I think I'd like to have @ldionne's opinion here before approving, but I guess it's indeed simple enough for such a large impact.

[vitalybuka]

Looks like we will have even more libcpp_verbose_abort with D141222
@ldionne Can we have a way to easily throw away string like with this inline libcpp_verbose_abort?

[ldionne]

Thanks for pinging me here, I was OOO when the discussion happened.

First, it is true that _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY was meant to allow back-deployment only, and I don't think we should abuse it for other purposes. If you want to have an abort handler that is guaranteed to be inlined, the path forward would be to provide a way that users can define their abort handler with whatever attributes (and signature while we're at it) they want, and allow them to override the default one at compile-time. That approach would be in-line with how this was meant to be used.

I fully agree that the current mechanism is lacking in that respect. In fact, it's an open issue I was already aware of and planning to fix before I saw this patch, since we do have codesize-sensitive users that want __libcpp_verbose_abort to boil down to essentially a single instruction. This patch just made me re-bump the priority of fixing this issue, so I just opened D141326 to propose a way to solve this problem. Please let me know what you think.

[ldionne]

FWIW my opinion is that D141326 is basically a more general version of this. If you agree, I think it's safe to abandon this one and try to converge on D141326.

libcxx/include/__verbose_abort

 // We can't provide a great implementation because it needs to be pretty much
 // dependency-free (this is included everywhere else in the library).
 #if defined(_LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY) && !defined(_LIBCPP_AVAILABILITY_CUSTOM_VERBOSE_ABORT_PROVIDED)
 
 extern "C" void abort();
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-_LIBCPP_NORETURN _LIBCPP_ATTRIBUTE_FORMAT(__printf__, 1, 2) _LIBCPP_HIDE_FROM_ABI inline
+_LIBCPP_NORETURN _LIBCPP_ALWAYS_INLINE _LIBCPP_ATTRIBUTE_FORMAT(__printf__, 1, 2) _LIBCPP_HIDE_FROM_ABI inline
 void __libcpp_verbose_abort(const char *, ...) {
   ::abort();
   __builtin_unreachable(); // never reached, but needed to tell the compiler that the function never returns
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #else