Page MenuHomePhabricator

[libc++] Drop template layer when using vsnprintf

Authored by joerg on Feb 18 2021, 12:14 PM.


Group Reviewers
Restricted Project

The filesystem implementation used variadic templates to convert path
arguments to C strings, but in the process, format string checks for
vsnprintf was lost. The latter is more useful than the former, so prefer
the simpler code. While here, fix a small bug in the va_list handling as
the initial list could be va_end'd twice, once by the second vsnprintf
and another time by the guard itself.

Diff Detail

Event Timeline

joerg requested review of this revision.Feb 18 2021, 12:14 PM
joerg created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2021, 12:14 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Just a few comments from a quick look.


Does clang-cl support this attribute? If yes, then it should be #if defined(__GNUC_) || defined(__clang__).


The macro name is misleading for me. It doesn't print, and it doesn't print files. It annotates a printf-like function though.
IMO it should contain something like format and attribute in the name, but I haven't given it a long thought.


@mstorsjo, you probably need to sync on this part regarding



Quuxplusone added inline comments.

Looks like the typical name would be _LIBCPP_FORMAT_PRINTF. (Compare _LIBCPP_NOALIAS, _LIBCPP_NORETURN, _LIBCPP_ALWAYS_INLINE, etc.)


It looks like you're trying to make sure that va_end is called even during exception-based stack unwinding, is that right? If so, I think you should do the RAII thing and create a proper struct type that calls va_end in its destructor. Hey, it looks like GuardVaList might already be that RAII type! Use it on line 209, and on line 218, and on line 113.

Don't call va_end manually on line 189 — let these GuardVaList objects deal with that cleanup.

Basically, make sure every time you call va_start or va_copy, you follow it immediately with a transfer of ownership to a GuardVaList.

joerg added inline comments.Mar 5 2021, 4:05 PM

It seems to work as is with clang-cl.


Yeah, C&P of a type. It should have been _LIBCPP_PRINTFLIKE. I'm not sure _LIBCPP_FORMAT_PRINTF is better, but I'm not really attached to the name either way.


I don't think there is any advantage to moving GuardVaList out. If anything, calling format_string_impl first and building the result afterwards seems like the way forward as it removes the only possible exception path.

Quuxplusone requested changes to this revision.Mar 5 2021, 4:41 PM
Quuxplusone added inline comments.

...Actually, I take back the "RAII guard" idea. From OSX's man stdarg:

Note that each call to va_start() must be matched by a call to va_end(), from within the same function.

And see

So you should just figure out a way to make report_impl noexcept, and then do both va_start and va_end from within this function. (format_string_impl will continue to call both va_copy and va_end from within the same function; but again, it shouldn't use the RAII type to do it; it should just be made noexcept.)

Also, please pass va_list ap by value, not reference; that way we match what libc does with vprintf, which we know is OK. Pass-by-reference is probably fine in practice, but why take the chance?

This revision now requires changes to proceed.Mar 5 2021, 4:41 PM
curdeius added inline comments.Mar 8 2021, 12:27 AM

I just tested it and __GNUC__ is *not* defined by clang-cl.
Please adjust.
Or @Quuxplusone, please update this in D98097, depending on how you both want to proceed.

Quuxplusone added inline comments.Mar 8 2021, 5:37 AM

I would like to proceed with D98097, and @joerg has said D98097 works for him as well. It's still waiting on libc++ approval though, so, ping! :)
I'm only 75% sure I understand what you're requesting here. I think you're saying that all Clangs do support __attribute__((format(printf))) with the right semantics, and clang-cl is an example of a Clang that doesn't define __GNUC__, so therefore we should change this to #if defined(__GNUC__) || defined(__clang__). Right? I'll do that in D98097.

curdeius added inline comments.Mar 8 2021, 5:50 AM

That's exactly this.

joerg abandoned this revision.Mar 8 2021, 11:38 AM