[SystemZ][z/OS] vasprintf fix libc++

[muiez]

Summary

The aim is to use the correct vasprintf implementation for z/OS libc++, where a copy of va_list ap is needed. In particular, it avoids the potential that the initial internal call to vsnprintf will modify ap and the subsequent call to vsnprintf will use that modified ap.

[ldionne]

We have the same issue in __libcpp_vasprintf in libcxx/src/support/win32/locale_win32.cpp - we should fix both in one go.

libcxx/include/__support/ibm/xlocale.h

269–283

Is there a reason for calling vsnprintf twice unconditionally now instead of doing the same thing as before, but providing a copy of the va_list for the first call?

In other words, why not do:

static inline
int vasprintf(char **strp, const char *fmt, va_list ap)
{
  const size_t buff_size = 256;
  if ((*strp = (char *)malloc(buff_size)) == NULL)
  {
    return -1;
  }

  va_list ap_copy;
  va_copy(ap_copy, ap);
  int str_size = vsnprintf(*strp, buff_size, fmt,  ap_copy);
  va_end(ap_copy);

  if (str_size >= buff_size)
  {
    if ((*strp = (char *)realloc(*strp, str_size + 1)) == NULL)
    {
      return -1;
    }
    str_size = vsnprintf(*strp, str_size + 1, fmt,  ap);
  }
  return str_size;
}

[muiez]

modify old implementation, formatting

[muiez]

win32 __libcpp_vasprintf fix

[muiez]

Any ideas as to why the CI build failed with mainly FileNotFoundError for Apple system? The changes seem unrelated.

[• Quuxplusone]

libcxx/src/support/win32/locale_win32.cpp

122 ↗

(On Diff #328520)

I don't think you need this diff. We receive a va_list; we pass it (once) to __libcpp_vasprintf. One source, one sink. No copy should be needed.

However, you do need to modify __libcpp_vasprintf in src/support/win32/support.cpp per Louis' comment.

[muiez]

fix for win32

[• Quuxplusone]

Okay, my comments have been resolved.

[muiez]

Ping :) @ldionne

[muiez]

@ldionne May we get a review please? Thanks

[• Quuxplusone]

@ldionne is on vacation this month, so don't expect anything from him until April, presumably.
@curdeius perhaps?
However, (1) since Louis did have specific "requested changes," it might be appropriate to wait for him before landing this, anyway.
(2) this varargs stuff is similar to D98097 — just in a different part of the codebase — and you might want to compare the two snippets and see if they could be even further harmonized. (Perhaps by leaving review comments on D98097.)

libcxx/include/__support/ibm/xlocale.h

 // -*- C++ -*-
 //===-----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP_SUPPORT_IBM_XLOCALE_H
 #define _LIBCPP_SUPPORT_IBM_XLOCALE_H
 
+#include <stdarg.h>
 #include <__support/ibm/locale_mgmt_aix.h>
 
 #include "cstdlib"
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 

 }
 static inline
 unsigned long strtoul_l(const char *__nptr, char **__endptr,
     int __base, locale_t locale) {
   return strtoul(__nptr, __endptr, __base);
 }
 
 static inline
-int vasprintf(char **strp, const char *fmt, va_list ap)
-{
+int vasprintf(char **strp, const char *fmt, va_list ap) {
   const size_t buff_size = 256;
-  int str_size;
-  if ((*strp = (char *)malloc(buff_size)) == NULL)
-  {
+  if ((*strp = (char *)malloc(buff_size)) == NULL) {
     return -1;
   }
-  if ((str_size = vsnprintf(*strp, buff_size, fmt,  ap)) >= buff_size)
-  {
-    if ((*strp = (char *)realloc(*strp, str_size + 1)) == NULL)
-    {
+
+  va_list ap_copy;
+  va_copy(ap_copy, ap);
+  int str_size = vsnprintf(*strp, buff_size, fmt,  ap_copy);
+  va_end(ap_copy);
+
+  if ((size_t) str_size >= buff_size) {
+    if ((*strp = (char *)realloc(*strp, str_size + 1)) == NULL) {
       return -1;
     }
     str_size = vsnprintf(*strp, str_size + 1, fmt,  ap);
   }
   return str_size;
 }
 
 #ifdef __cplusplus
 }
 #endif
 #endif // _LIBCPP_SUPPORT_IBM_XLOCALE_H

libcxx/src/support/win32/support.cpp

 
 // Like sprintf, but when return value >= 0 it returns
 // a pointer to a malloc'd string in *sptr.
 // If return >= 0, use free to delete *sptr.
 int __libcpp_vasprintf( char **sptr, const char *__restrict format, va_list ap )
 {
     *sptr = NULL;
     // Query the count required.
-    int count = _vsnprintf( NULL, 0, format, ap );
+    va_list ap_copy;
+    va_copy(ap_copy, ap);
+    int count = _vsnprintf( NULL, 0, format, ap_copy );
+    va_end(ap_copy);
     if (count < 0)
         return count;
     size_t buffer_size = static_cast<size_t>(count) + 1;
     char* p = static_cast<char*>(malloc(buffer_size));
     if ( ! p )
         return -1;
     // If we haven't used exactly what was required, something is wrong.
     // Maybe bug in vsnprintf. Report the error and return.