Page MenuHomePhabricator

[Flang] Fix error messages on Windows.

Authored by ijan1 on Aug 6 2021, 8:45 AM.



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.

Diff Detail

Event Timeline

ijan1 requested review of this revision.Aug 6 2021, 8:45 AM
ijan1 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2021, 8:45 AM

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)};
int need{vsnprintf(nullptr, 0, p, ap)};

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);
  return ::ftruncate(fd, at);
ijan1 updated this revision to Diff 365486.Aug 10 2021, 8:28 AM

Changed to be less intrusive.

This revision is now accepted and ready to land.Aug 10 2021, 11:34 AM

Thanks for making the change more local.


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?


Meinersbur added inline comments.Aug 10 2021, 7:25 PM

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.


amccarth added inline comments.Aug 11 2021, 8:53 AM

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

This revision was landed with ongoing or failed builds.Aug 12 2021, 10:51 AM
This revision was automatically updated to reflect the committed changes.