[Flang] Fix error messages on Windows.

[ijan1]

Summary

Flang uses positional arguments for messages::say(), such as "%1$s" which is only supported in MS Compilers with the _*printf_p form of the function. This uses a conditional macro to convert the existing vsnprintf used to the one needed in MS-World.

7 tests in D107575 rely on this change.

[Meinersbur]

Using $ index specifier is a POSIX extension and should not have been used in portable code. Using llvm::formatv (#include <llvm/Support/FormatVariadic.h>) would have been the better choice. Unfortunately that would be a larger change.

Instead of redefining a runtime library function, I suggest to #ifdef the code that uses it, which is less intrusive. That is,

#ifdef _MSC_VER
int need{_vsprintf_p(nullptr, 0, p, ap)};
#else
int need{vsnprintf(nullptr, 0, p, ap)};
#endif

Alternatively, define a compatibility wrapper for a platform-specific function, such as in runtime/file.cpp:

inline static int openfile_ftruncate(int fd, OpenFile::FileOffset at) {
#ifdef _WIN32
  return ::_chsize(fd, at);
#else
  return ::ftruncate(fd, at);
#endif
}

[ijan1]

Changed to be less intrusive.

[Meinersbur]

LGTM

[amccarth]

Thanks for making the change more local.

flang/lib/Parser/message.cpp

48

I understand the intent is to determine the size of the buffer needed, but the [Microsoft documentation for _vsprintf_p][1] says,

If the buffer or format parameters are NULL pointers, ... the invalid parameter handler is invoked, as described in Parameter Validation. If execution is allowed to continue, the functions return -1 and set errno to EINVAL.

But since this is passing a null pointer for the buffer, I'd expect this to abort or return -1. Maybe that's a documentation bug. Has this been tested on Windows?

[1]: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/vsprintf-p-vsprintf-p-l-vswprintf-p-vswprintf-p-l?view=msvc-160#remarks

[Meinersbur]

flang/lib/Parser/message.cpp

48

This seems to be a documentation bug. The Windows SDK source explicitly checks for a NULL argument for the buffer and returns the length [1].

The behaviour isn't document for the standard vsprintf function either, see [2] or [3].

The pre-merge buildbot would be running this code in D107575 (if it was rebased). I tested it myself and it is working.

[1] https://github.com/huangqinjin/ucrt/blob/a52376780e631fb0e5d520748addb03651aef0b5/stdio/output.cpp#L169
[2] https://www.cplusplus.com/reference/cstdio/vsprintf/
[3] https://linux.die.net/man/3/vsprintf

[amccarth]

flang/lib/Parser/message.cpp

48

Thanks. I'll file a bug on the Microsoft docs.

flang/lib/Parser/message.cpp

 llvm::raw_ostream &operator<<(llvm::raw_ostream &o, const MessageFixedText &t) {
   std::size_t n{t.text().size()};
   for (std::size_t j{0}; j < n; ++j) {
     o << t.text()[j];
   }
   return o;
 }
 
+#ifdef _MSC_VER
+#define vsnprintf _vsprintf_p
+#endif
+
 void MessageFormattedText::Format(const MessageFixedText *text, ...) {
   const char *p{text->text().begin()};
   std::string asString;
   if (*text->text().end() != '\0') {
     // not NUL-terminated
     asString = text->text().NULTerminatedToString();
     p = asString.c_str();
   }
   va_list ap;
   va_start(ap, text);
   int need{vsnprintf(nullptr, 0, p, ap)};
   CHECK(need >= 0);
   char *buffer{
       static_cast<char *>(std::malloc(static_cast<std::size_t>(need) + 1))};
   CHECK(buffer);
   va_end(ap);
   va_start(ap, text);
   int need2{vsnprintf(buffer, need + 1, p, ap)};
   CHECK(need2 == need);
   va_end(ap);
   string_ = buffer;
   std::free(buffer);