Page MenuHomePhabricator

[LLDB] Remove a now redundant windows specific workaround
ClosedPublic

Authored by mstorsjo on Sep 20 2019, 1:45 PM.

Details

Summary

vsnprintf(NULL, 0, ...) works for measuring the needed string size on all supported Windows variants; it's supported since at least MSVC 2015 (and LLVM requires a newer version than that), and works on both msvcrt.dll (since at least XP) and UCRT with MinGW.

The previous use of ifdefs was wrong as well, as __MINGW64__ only is defined for 64 bit targets, and the define without trailing underscores is never defined automatically (neither by clang nor by gcc).

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Sep 20 2019, 1:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 1:45 PM

Judging by the microsoft docs, vsnprintf is avaliable at least since VS 2017 (the lowest version supported by llvm). Is it possible that we can just delete this #ifdef ?

Judging by the microsoft docs, vsnprintf is avaliable at least since VS 2017 (the lowest version supported by llvm). Is it possible that we can just delete this #ifdef ?

Yes, it would seem so.

With MSVC, you got an MSVC-specific version of the CRT, and with the versions of MSVC we require, it is a C99 compliant one.

With MinGW, you can link either against the UCRT (the same modern CRT as MSVC uses these days) or msvcrt.dll (the old legacy one, shipped as a part of the OS). But I checked that this use of vsnprintf(NULL, 0, ...) does seem to work with the old msvcrt.dll since at least XP (and llvm requires a much newer version of Windows anyway), so I would say that it should be safe.

mstorsjo updated this revision to Diff 221276.Sep 23 2019, 3:20 AM
mstorsjo retitled this revision from [LLDB] Check for the __MINGW32__ define instead of __MINGW64 to [LLDB] Remove a now redundant windows specific workaround.
mstorsjo edited the summary of this revision. (Show Details)

Updated the patch to simply remove the now redundant workaround.

labath accepted this revision.Sep 23 2019, 4:01 AM

Awesome. Let's give that a spin.

This revision is now accepted and ready to land.Sep 23 2019, 4:01 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 5:03 AM