[libcxx-commits] [PATCH] D113137: [libc++] Fix lifetime issues of temporaries.

Mon Nov 8 13:47:54 PST 2021

ldionne accepted this revision.
ldionne added a subscriber: vitaut.
ldionne added a comment.
This revision is now accepted and ready to land.

Thanks for the fix. I'd still like to discuss whether we want to file a LWG issue or if I'm misunderstanding the (lack of) importance of this in the user visible API.

================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.context/format.context/ctor.pass.cpp:54
+  auto format_arg_store = std::make_format_args<std::basic_format_context<OutIt, CharT>>(true, CharT('a'), 42, string);
+  std::basic_format_args args{format_arg_store};

----------------
Mordante wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > Style: `std::basic_format_args args = format_arg_store;` (it's not an aggregate or sequence-container)
> > > > 
> > > > Do I understand correctly that `std::make_format_args` does not actually return a `std::format_args`? Who came up with that? 😛 
> > > > 
> > > > But does this fix the dangling? https://en.cppreference.com/w/cpp/utility/format/make_format_args insists that "A formatting argument has reference semantics for user-defined types and does not extend the lifetime of `args`," so do you really need to do something like
> > > > ```
> > > > auto args_tuple = std::make_tuple(true, CharT('a'), 42, string);
> > > > auto args = std::apply([](auto&&... ts) {
> > > >     return std::make_format_args<std::basic_format_context<OutIt, CharT>>(static_cast<decltype(ts)>(ts)...);
> > > > }, std::tuple<bool&&, CharT&&, int&&, std::basic_string<CharT>&>(args_tuple));
> > > > ```
> > > > ? (Or consider turning each of the following blocks into its own function, and just calling them like `test_foo(std::make_format_args<...>(...));` which would eliminate the lifetime issues in a more realistic way: basically by treating `format_args` as a parameter-only type.)
> > > > Style: `std::basic_format_args args = format_arg_store;` (it's not an aggregate or sequence-container)
> > > > 
> > > > Do I understand correctly that `std::make_format_args` does not actually return a `std::format_args`? Who came up with that? 😛 
> > > 
> > > Indeed
> > > ```
> > > Returns an object that stores an array of formatting arguments and can be implicitly converted to std::basic_format_args<Context>. 
> > > ```
> > > The `basic_format_args` stores a pointer to the data of this array. Before the change that array was a temporary, after the change it's no longer a temporary. (The format functions are implemented like `return vformat_to(out, fmt.str, make_format_args(args...));`. So the temporary's lifetime isn't an issue there.)
> > > 
> > > > But does this fix the dangling? https://en.cppreference.com/w/cpp/utility/format/make_format_args insists that "A formatting argument has reference semantics for user-defined types and does not extend the lifetime of `args`," so do you really need to do something like
> > > > ```
> > > > auto args_tuple = std::make_tuple(true, CharT('a'), 42, string);
> > > > auto args = std::apply([](auto&&... ts) {
> > > >     return std::make_format_args<std::basic_format_context<OutIt, CharT>>(static_cast<decltype(ts)>(ts)...);
> > > > }, std::tuple<bool&&, CharT&&, int&&, std::basic_string<CharT>&>(args_tuple));
> > > > ```
> > > > ? (Or consider turning each of the following blocks into its own function, and just calling them like `test_foo(std::make_format_args<...>(...));` which would eliminate the lifetime issues in a more realistic way: basically by treating `format_args` as a parameter-only type.)
> > > 
> > > No this should work; the values of the fundamental types are copied in the array so they won't dangle. The emphasis in the note is `user-defined types`. Using a temporary would be an issue for `std::string`, it's stored as a `std::string_view` in the array. That's why the argument `string` isn't a temporary.
> > > No, this should work; the values of the fundamental types are copied in the array so they won't dangle. The emphasis in the note is user-defined types. Using a temporary would be an issue for std::string, it's stored as a std::string_view in the array.
> > 
> > Okay, everything you say is true for libc++'s implementation — we copy `int`s but merely reference/view `std::string`s — but does the Standard actually mandate that that happens, and if so, how? (Does it actually mandate value semantics for a whitelist of primitive types, reference semantics for "everything else"? Is there any gray area? e.g. what happens for `int128_t`, `std::byte`, non-const-`void*`, `void(*)()`?)  Is it //possible// that a "maliciously conforming" implementation might capture your `int` argument by reference, even if libc++ never will? We do want the test suite to work even for non-libc++ implementations... although I think it's quite defensible in this case to say "we'll cross that bridge if we ever come to it."
> > 
> > (My vague unease with this is probably refactoring-related. What if somebody six months from now notices that we test `make_format_args` with a `string` lvalue but never with a `string` rvalue: will it be easy for them to add a test for `string` rvalues, or will they have to reinvent all the stuff we're going through now?)
> > 
> > Anyway, I'm not blocking this; I still say if you're happy with it (and Louis isn't //un//happy with it) then it should be shipped.
> The value is stored in an exposition only `variant` in  `basic_format_arg` http://eel.is/c++draft/format.arg. (Actually it will never be stored in a `variant` since optimizations are required http://eel.is/c++draft/format#args-1. I'm working on that requirement, but nothing public yet.)
> 
> In our implementation the values are always lvalue references. P2418 allows rvalue references, but Charlie noticed some issues with the wording. So for now I prefer to keep it as is. I did some work on P2418, but I wait for an LWG-issue to see the final direction of that paper. (P2418 has been accepted at the last plenary.)
> 
> So for now I'm happy and when I implement P2418 I need to see what the exact impact rvalue references have. (I expect no changes for this test, since I haven't implemented the handle class yet.)
This sounds like a very very dangerous API to me -- it's taking stuff by reference but that's not obvious at all. And even worse, `basic_format_args(const format-arg-store<Context, Args...>&)` is implicit.

@Mordante Do you think this issue is worth solving? Perhaps users will never actually use this API? But shouldn't we **at least** make the constructor `explicit`? I don't have nearly as much context as you do on `std::format`, so I'm curious to hear what you think. Also CCing @vitaut 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113137