This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by muiez on Feb 25 2021, 6:49 AM.

Details

Reviewers
zibi
ldionne
Quuxplusone
Group Reviewers
Restricted Project
Commits
rGf6af5efcec41: [SystemZ][z/OS] vasprintf fix libc++
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.

Diff Detail

Event Timeline

muiez requested review of this revision.Feb 25 2021, 6:49 AM
muiez created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 6:49 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Mar 1 2021, 10:41 AM

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;
}
This revision now requires changes to proceed.Mar 1 2021, 10:41 AM
muiez updated this revision to Diff 328517.Mar 5 2021, 7:11 AM

modify old implementation, formatting

muiez updated this revision to Diff 328520.Mar 5 2021, 7:19 AM

win32 __libcpp_vasprintf fix

muiez added a comment.Mar 8 2021, 6:30 AM

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

Quuxplusone requested changes to this revision.Mar 8 2021, 6:43 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
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.

This revision now requires changes to proceed.Mar 8 2021, 6:43 AM
muiez updated this revision to Diff 329065.Mar 8 2021, 10:32 AM

fix for win32

Okay, my comments have been resolved.

@ldionne May we get a review please? Thanks

@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.)

ldionne accepted this revision.Mar 18 2021, 10:38 AM
This revision is now accepted and ready to land.Mar 18 2021, 10:38 AM
This revision was automatically updated to reflect the committed changes.