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

Wed Jan 4 02:36:00 PST 2023

philnik added 1 blocking reviewer(s): ldionne.
philnik added a comment.

In D140944#4025344 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140944