This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Only use _unlocked stdio functions on linux
ClosedPublic

Authored by mstorsjo on May 17 2018, 12:26 AM.

Details

Summary

The existing comment said that the functions were available only on GNU/Linux (and on certain Android versions), but only checked T.isGNUEnvironment() which also is true on MinGW (for arch-windows-gnu triplets), which doesn't have such functions.

Existing checks in the initialize function in TargetLibraryInfo.cpp also use only T.isOSLinux() to check for glibc features.

This fixes use of stdio on MinGW.

Diff Detail

Event Timeline

mstorsjo created this revision.May 17 2018, 12:26 AM
xbolva00 accepted this revision.EditedMay 17 2018, 12:29 AM

Oh, thanks for fix. I associated gnu == linux, forgot on mingw.

This revision is now accepted and ready to land.May 17 2018, 12:29 AM
xbolva00 added inline comments.May 17 2018, 12:30 AM
lib/Analysis/TargetLibraryInfo.cpp
485

I would leave isLinux && isGNU...

mstorsjo added inline comments.May 17 2018, 12:46 AM
lib/Analysis/TargetLibraryInfo.cpp
485

Yeah, I thought about that as well - but the rest of this function just plainly does isOSLinux everywhere, even if comments talk about glibc specifically. But I can make it Linux && GNU here for extra clarity.

srhines added inline comments.May 17 2018, 12:49 AM
lib/Analysis/TargetLibraryInfo.cpp
485

isOSLinux is true for Android, isn't it? Doesn't that break the rest of the case for < API 28 on Android?

xbolva00 added inline comments.May 17 2018, 12:52 AM
lib/Analysis/TargetLibraryInfo.cpp
485

Probably yes.

E.g. musl implements them as aliases to locked variants..
https://github.com/esmil/musl/blob/194f9cf93da8ae62491b7386edf481ea8565ae4e/src/stdio/fwrite.c#L37

xbolva00 added inline comments.May 17 2018, 12:53 AM
lib/Analysis/TargetLibraryInfo.cpp
485

I think this is okay:

if ((T.isOSLinux() && T.isGNUEnvironment()) || (T.isAndroid() && !T.isAndroidVersionLT(28))) {

mstorsjo added inline comments.May 17 2018, 12:55 AM
lib/Analysis/TargetLibraryInfo.cpp
485

isOSLinux is true for Android, isn't it? Doesn't that break the rest of the case for < API 28 on Android?

Oh right, yes, we clearly should make it linux&&GNU then.

This revision was automatically updated to reflect the committed changes.

Just for info.. Windows has also some kind of unlocked stdio via _nolock functions :)

Just for info.. Windows has also some kind of unlocked stdio via _nolock functions :)

Just for info.. Windows has also some kind of unlocked stdio via _nolock functions :)

Yes, I saw that as well :-) However it doesn't seem to be available in the system DLL "msvcrt.dll" which mingw targets by default. (Nowadays one can target the more modern ucrtbase.dll by default as well, and there they'd be available.)

Just for info.. Windows has also some kind of unlocked stdio via _nolock functions :)

Yes, I saw that as well :-) However it doesn't seem to be available in the system DLL "msvcrt.dll" which mingw targets by default. (Nowadays one can target the more modern ucrtbase.dll by default as well, and there they'd be available.)

... except that shouldn't be an issue; if we'd would want to use them, mingw-w64 could just provide wrappers that map down back to the normal locked ones, for msvcrt.dll. So nevermind about that.

So yes, one could take advantage of that if one would have extra effort to spend on it :-)