[libc++] Drop template layer when using vsnprintf

[joerg]

Summary

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.

[curdeius]

Just a few comments from a quick look.

libcxx/include/__config

1452

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

1453

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.

libcxx/src/filesystem/directory_iterator.cpp

275

@mstorsjo, you probably need to sync on this part regarding https://reviews.llvm.org/D98077.

libcxx/src/filesystem/filesystem_common.h

78

Typo.

[• Quuxplusone]

libcxx/include/__config

1453

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

libcxx/src/filesystem/filesystem_common.h

210

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]

libcxx/include/__config

1452

It seems to work as is with clang-cl.

1453

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.

libcxx/src/filesystem/filesystem_common.h

210

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]

libcxx/src/filesystem/filesystem_common.h

210

...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 https://stackoverflow.com/questions/37454179/how-to-properly-va-end

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?

[curdeius]

libcxx/include/__config

1452

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]

libcxx/include/__config

1452

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]

libcxx/include/__config

1452

That's exactly this.

libcxx/include/__config

 #endif
 
 #if __has_attribute(init_priority)
 # define _LIBCPP_INIT_PRIORITY_MAX __attribute__((init_priority(101)))
 #else
 # define _LIBCPP_INIT_PRIORITY_MAX
 #endif
 
+#ifdef __GNUC__
+#  define _LIBCPP_PRINTFILE(fmtarg, firstvararg) \
+     __attribute__((__format__(__printf__, fmtarg, firstvararg)))
+#else
+#  define _LIBCPP_PRINTFILE(fmtarg, firstvararg)
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

libcxx/src/filesystem/directory_iterator.cpp

directory_iterator& directory_iterator::__increment(error_code* ec) {
   _LIBCPP_ASSERT(__imp_, "Attempting to increment an invalid iterator");
   ErrorHandler<void> err("directory_iterator::operator++()", ec);
 
   error_code m_ec;
   if (!__imp_->advance(m_ec)) {
     path root = move(__imp_->__root_);
     __imp_.reset();
     if (m_ec)
-      err.report(m_ec, "at root \"%s\"", root);
+      err.report(m_ec, "at root \"%s\"", root.c_str());
   }
   return *this;
 }
 
 directory_entry const& directory_iterator::__dereference() const {
   _LIBCPP_ASSERT(__imp_, "Attempting to dereference an invalid iterator");
   return __imp_->__entry_;
 }
while (stack.size() > 0) {
     if (m_ec)
       break;
     stack.pop();
   }
 
   if (m_ec) {
     path root = move(stack.top().__root_);
     __imp_.reset();
-    err.report(m_ec, "at root \"%s\"", root);
+    err.report(m_ec, "at root \"%s\"", root.c_str());
   } else {
     __imp_.reset();
   }
 }
 
 bool recursive_directory_iterator::__try_recursion(error_code* ec) {
   ErrorHandler<void> err("recursive_directory_iterator::operator++()", ec);
 
if (m_ec) {
     const bool allow_eacess =
         bool(__imp_->__options_ & directory_options::skip_permission_denied);
     if (m_ec.value() == EACCES && allow_eacess) {
       if (ec)
         ec->clear();
     } else {
       path at_ent = move(curr_it.__entry_.__p_);
       __imp_.reset();
-      err.report(m_ec, "attempting recursion into \"%s\"", at_ent);
+      err.report(m_ec, "attempting recursion into \"%s\"", at_ent.c_str());
     }
   }
   return false;
 }
 
 _LIBCPP_END_NAMESPACE_FILESYSTEM

libcxx/src/filesystem/filesystem_common.h

 
 #if defined(_LIBCPP_WIN32API)
 // Non anonymous, to allow access from two translation units.
 errc __win_err_to_errc(int err);
 #endif
 
 namespace {
 
-static string format_string_imp(const char* msg, ...) {
-  // we might need a second shot at this, so pre-emptivly make a copy
+static _LIBCPP_PRINTFILE(1, 0) string
+    format_string_impl(const char* msg, va_list& args) {
   struct GuardVAList {
     va_list& target;
     bool active = true;
     GuardVAList(va_list& tgt) : target(tgt), active(true) {}
     void clear() {
       if (active)
         va_end(target);
       active = false;
     }
     ~GuardVAList() {
       if (active)
         va_end(target);
     }
   };
-  va_list args;
-  va_start(args, msg);
-  GuardVAList args_guard(args);
 
+  GuardVAList args_guard(args);
+  // we might need a second shot at this, so pre-emptivly make a copy
   va_list args_cp;
   va_copy(args_cp, args);
   GuardVAList args_copy_guard(args_cp);
 
   std::string result;
 
   array<char, 256> local_buff;
   size_t size_with_null = local_buff.size();
static _LIBCPP_PRINTFILE(1, 0) string
   }
 
   // we did not provide a long enough buffer on our first attempt. The
   // return value is the number of bytes (excluding the null byte) that are
   // needed for formatting.
   size_with_null = static_cast<size_t>(ret) + 1;
   result.__resize_default_init(size_with_null - 1);
   ret = ::vsnprintf(&result[0], size_with_null, msg, args);
+  args_guard.clear();
   _LIBCPP_ASSERT(static_cast<size_t>(ret) == (size_with_null - 1), "TODO");
 
   return result;
 }
 
-const path::value_type* unwrap(path::string_type const& s) { return s.c_str(); }
-const path::value_type* unwrap(path const& p) { return p.native().c_str(); }
-template <class Arg>
-Arg const& unwrap(Arg const& a) {
-  static_assert(!is_class<Arg>::value, "cannot pass class here");
-  return a;
-}
-
-template <class... Args>
-string format_string(const char* fmt, Args const&... args) {
-  return format_string_imp(fmt, unwrap(args)...);
+static _LIBCPP_PRINTFILE(1, 2) string format_string(const char* msg, ...) {
+  va_list args;
+  va_start(args, msg);
+  return format_string_impl(msg, args);
 }
 
 error_code capture_errno() {
   _LIBCPP_ASSERT(errno, "Expected errno to be non-zero");
   return error_code(errno, generic_category());
 }
 
 #if defined(_LIBCPP_WIN32API)
T report(const error_code& ec) const {
     case 1:
       __throw_filesystem_error(what, *p1_, ec);
     case 2:
       __throw_filesystem_error(what, *p1_, *p2_, ec);
     }
     _LIBCPP_UNREACHABLE();
   }
 
-  template <class... Args>
-  T report(const error_code& ec, const char* msg, Args const&... args) const {
+  _LIBCPP_PRINTFILE(3, 0)
+  T report_impl(const error_code& ec, const char* msg, va_list& args) const {
     if (ec_) {
+      va_end(args);
       *ec_ = ec;
       return error_value<T>();
     }
     string what =
-        string("in ") + func_name_ + ": " + format_string(msg, args...);
+        string("in ") + func_name_ + ": " + format_string_impl(msg, args);
     switch (bool(p1_) + bool(p2_)) {
     case 0:
       __throw_filesystem_error(what, ec);
     case 1:
       __throw_filesystem_error(what, *p1_, ec);
     case 2:
       __throw_filesystem_error(what, *p1_, *p2_, ec);
     }
     _LIBCPP_UNREACHABLE();
   }
 
+  _LIBCPP_PRINTFILE(3, 4)
+  T report(const error_code& ec, const char* msg, ...) const {
+    va_list args;
+    va_start(args, msg);
+    return report_impl(ec, msg, args);
+  }
+
   T report(errc const& err) const { return report(make_error_code(err)); }
 
-  template <class... Args>
-  T report(errc const& err, const char* msg, Args const&... args) const {
-    return report(make_error_code(err), msg, args...);
+  _LIBCPP_PRINTFILE(3, 4)
+  T report(errc const& err, const char* msg, ...) const {
+    va_list args;
+    va_start(args, msg);
+    return report_impl(make_error_code(err), msg, args);
   }
 
 private:
   ErrorHandler(ErrorHandler const&) = delete;
   ErrorHandler& operator=(ErrorHandler const&) = delete;
 };
 
 using chrono::duration;

libcxx/src/filesystem/operations.cpp

#else
   if (ret == nullptr)
     ret = "/tmp";
 
   path p(ret);
 #endif
   error_code m_ec;
   file_status st = detail::posix_stat(p, &m_ec);
   if (!status_known(st))
-    return err.report(m_ec, "cannot access path \"" PS_FMT "\"", p);
+    return err.report(m_ec, "cannot access path \"" PS_FMT "\"", p.c_str());
 
   if (!exists(st) || !is_directory(st))
-    return err.report(errc::not_a_directory, "path \"" PS_FMT "\" is not a directory",
-                      p);
+    return err.report(errc::not_a_directory,
+                      "path \"" PS_FMT "\" is not a directory", p.c_str());
 
   return p;
 }
 
 path __weakly_canonical(const path& p, error_code* ec) {
   ErrorHandler<path> err("weakly_canonical", ec, &p);
 
   if (p.empty())