[libcxx-commits] [PATCH] D140944: [libc++] Optimize _LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY

Wed Jan 4 01:58:48 PST 2023

vitalybuka added a comment.

In D140944#4024959 <https://reviews.llvm.org/D140944#4024959>, @philnik wrote:

> In D140944#4024901 <https://reviews.llvm.org/D140944#4024901>, @vitalybuka wrote:
>
>> In D140944#4024899 <https://reviews.llvm.org/D140944#4024899>, @vitalybuka wrote:
>>
>>> In D140944#4024885 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140944/new/

https://reviews.llvm.org/D140944