[libcxx-commits] [PATCH] D121514: [libc++][format] Improve format-arg-store.

Eric Fiselier via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Mar 14 07:13:16 PDT 2022


EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

I've got a bunch of concerns about the `memcpy` magic happening here. How is it `constexpr`, how does it handle lifetimes, is it fully tested?



================
Comment at: libcxx/include/__format/format_arg_store.h:157
+  requires(same_as<typename _Context::char_type, wchar_t> && same_as<_CharT, char>)
+consteval auto __make_storage_type() {
+  return __storage_type<wchar_t>{};
----------------
These overloads are constrained in entirely different ways. Is it possible to use a single method to constrain each of them? 


================
Comment at: libcxx/include/__format/format_arg_store.h:245
+template <class _Context, class _Tp, class... _Args>
+consteval void __get_meta_data(uint32_t __offset, __arg_meta_data* __meta_data) {
+  auto __s = __make_storage_type<_Context, _Tp>();
----------------
Why do some of these functions take output parameters and other return by value? An overload set should be cohesive in how you use it.


================
Comment at: libcxx/include/__format/format_arg_store.h:264
+template <class _Context, class _Tp, class... _Args>
+consteval size_t __get_storage_size(size_t __size) {
+  auto __s = __make_storage_type<_Context, _Tp>();
----------------
Why not use a fold expression here rather than recursively instantiating all these function templates.
Please find a way to make the number of instantiations `N`, not `N**2`


================
Comment at: libcxx/include/__format/format_arg_store.h:287
+
+// Most exposition only constructors in [format.arg] are noexcept.
+// But all can be, so use noexcept for this function.
----------------
This comment doesn't make sense to me? If the constructors can possibly be non-noexcept, does that mean they can throw? 


================
Comment at: libcxx/include/__format/format_arg_store.h:291
+_LIBCPP_HIDE_FROM_ABI constexpr void __store_data(char* __data, _Tp&& __v, _Args&&... __args) noexcept {
+  using _Sp = typename decltype(__make_storage_type<_Context, remove_cvref_t<_Tp>>())::type;
+  if constexpr (same_as<_Sp, basic_string_view<typename _Context::char_type>>) {
----------------
I'm very uncomfortable with this function. How does it handle alignment? non-trivial types? Is it opaque to sanitizers?
Does it leave us open to buffer overflow bugs/attacks? 


================
Comment at: libcxx/include/__format/format_arg_store.h:300
+    basic_string_view<typename _Context::char_type> __s{_VSTD::data(__v), _VSTD::size(__v)};
+    _VSTD::memcpy(static_cast<void*>(__data), static_cast<const void*>(_VSTD::addressof(__s)), sizeof(_Sp));
+  } else if constexpr (same_as<_Sp, typename basic_format_arg<_Context>::handle>) {
----------------
How is any of this `constexpr`?
https://godbolt.org/z/esKTzK38b

If I'm right that this isn't, it means we need a lot more tests before this lands. Because it should have been caught by tests.




================
Comment at: libcxx/include/__format/format_arg_store.h:305
+  } else {
+    __storage<_Sp> __s{_VSTD::forward<_Tp>(__v)};
+    _VSTD::memcpy(static_cast<void*>(__data), static_cast<const void*>(_VSTD::addressof(__s.__value)), sizeof(_Sp));
----------------
This needs to `static_assert(is_trivially_copyable<_Sp>);` and do we really need to instantiate another type just to memcpy this thing?
What are the requirements on `_Sp` generally?


================
Comment at: libcxx/include/__format/format_arg_store.h:306
+    __storage<_Sp> __s{_VSTD::forward<_Tp>(__v)};
+    _VSTD::memcpy(static_cast<void*>(__data), static_cast<const void*>(_VSTD::addressof(__s.__value)), sizeof(_Sp));
+  }
----------------
`sizeof(__storage<_Sp>)` may not always be equal to `sizeof(_Sp)`.


================
Comment at: libcxx/include/__format/format_args.h:45
   template <class... _Args>
-  _LIBCPP_HIDE_FROM_ABI basic_format_args(
-      const __format_arg_store<_Context, _Args...>& __store) noexcept
-      : __size_(sizeof...(_Args)), __data_(__store.__args.data()) {}
+  _LIBCPP_HIDE_FROM_ABI basic_format_args(const __format_arg_store<_Context, _Args...>& __store) noexcept
+      : __size_(sizeof...(_Args)) {
----------------
Should this be explicit?


================
Comment at: libcxx/include/__format/format_args.h:65
 private:
   size_t __size_{0};
+  const __format::__arg_meta_data* __meta_data_{nullptr};
----------------
Is there a reason to pack the members in this order? It seems likely to generate padding for certain arg types


================
Comment at: libcxx/include/__format/format_args.h:66
   size_t __size_{0};
-  const basic_format_arg<_Context>* __data_{nullptr};
+  const __format::__arg_meta_data* __meta_data_{nullptr};
+  const char* __data_{nullptr};
----------------
Have we shipped this in a release? 
Because I'm not sure we can change the A-BI here then.


================
Comment at: libcxx/include/format:193
 template <class _Context = format_context, class... _Args>
-_LIBCPP_HIDE_FROM_ABI __format_arg_store<_Context, _Args...>
-make_format_args(const _Args&... __args) {
-  return {basic_format_arg<_Context>(__args)...};
+_LIBCPP_HIDE_FROM_ABI auto make_format_args(const _Args&... __args) {
+  return _VSTD::__make_format_args<_Context>(__args...);
----------------
This isn't specified as returning auto so you should spell out the return type. Making the compiler deduce it has costs and can potentially break SFINAE.




================
Comment at: libcxx/include/format:199
 template <class... _Args>
-_LIBCPP_HIDE_FROM_ABI __format_arg_store<wformat_context, _Args...>
-make_wformat_args(const _Args&... __args) {
+_LIBCPP_HIDE_FROM_ABI auto make_wformat_args(const _Args&... __args) {
   return _VSTD::make_format_args<wformat_context>(__args...);
----------------
This isn't specified as returning `auto` so you should spell out the return type. Making the compiler deduce it has costs and can potentially break SFINAE.


================
Comment at: libcxx/test/libcxx/utilities/format/format.arguments/format.arg/format_arg_store.pass.cpp:1
+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
We typically avoid testing internal details like this. Can you say more about why we really


================
Comment at: libcxx/test/libcxx/utilities/format/format.arguments/format.arg/format_arg_store.pass.cpp:89
+
+  CharT* cstring = const_cast<CharT*>(CSTR("hello C"));
+  const CharT* const_cstring = CSTR("hello const C");
----------------
This is undefined behavior. Don't


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121514



More information about the libcxx-commits mailing list