Page MenuHomePhabricator

[Flang] Fix error messages on Windows.
ClosedPublic

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

Details

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.

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)};
#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 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.

flang/lib/Parser/message.cpp
44

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 added inline comments.Aug 10 2021, 7:25 PM
flang/lib/Parser/message.cpp
44

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 added inline comments.Aug 11 2021, 8:53 AM
flang/lib/Parser/message.cpp
44

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.