This is an archive of the discontinued LLVM Phabricator instance.

[Distro] Bypass distro detection on non-Linux hosts
ClosedPublic

Authored by aganea on Nov 19 2019, 2:43 PM.

Details

Summary

This skips distro detection when we're not running on Linux, or when the target triple is not Linux. This saves a few OS calls for each invocation of clang.exe.

Diff Detail

Event Timeline

aganea created this revision.Nov 19 2019, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2019, 2:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a comment.Nov 19 2019, 3:21 PM

I'd rather avoid the ifdef if possible. I looked to see where these come from:

$ git grep -l Distro ../clang/lib/Driver/
../clang/lib/Driver/CMakeLists.txt
../clang/lib/Driver/Distro.cpp
../clang/lib/Driver/ToolChains/Clang.cpp
../clang/lib/Driver/ToolChains/Cuda.cpp
../clang/lib/Driver/ToolChains/Linux.cpp

I think the only one that matters is in Clang.cpp, but it looks like it should never run on Windows:

if (Args.hasFlag(options::OPT_faddrsig, options::OPT_fno_addrsig,
                 (TC.getTriple().isOSBinFormatELF() ||
                  TC.getTriple().isOSBinFormatCOFF()) &&
                    !TC.getTriple().isPS4() &&
                    !TC.getTriple().isOSNetBSD() &&
                    !Distro(D.getVFS()).IsGentoo() &&
                    !TC.getTriple().isAndroid() &&
                     TC.useIntegratedAs()))
  CmdArgs.push_back("-faddrsig");

Hm, I guess it does happen. I think that condition should be restructured to only do all that BSD, PS4, Android, Gentoo etc logic if the format is ELF, if COFF, then always default to -faddrsig.

aganea added a comment.EditedNov 21 2019, 9:26 AM
In D70467#1752611, @rnk wrote:

Hm, I guess it does happen. I think that condition should be restructured to only do all that BSD, PS4, Android, Gentoo etc logic if the format is ELF, if COFF, then always default to -faddrsig.

We're cross-compiling all our target platforms on Windows. It seems this patch would be still valid in that case:

> clang -target x86_64-linux a.cpp

Would still call DetectDistro(), and unless I'm missing something, we wouldn't want it to lookup C:\etc\lsb-release even if it's there? Or people are really doing that to target a specific distro when compiling from Windows? It doesn't make sense from my POV.
The same argument applies to CUDA: the code in clang/lib/Driver/ToolChains/Cuda.cpp:CudaInstallationDetector() says "HostTriple" but it's actually the Target triple -- you can try my cmd-line above, it hits the Distro detect.
Unless you specify a non-real FS, I wouldn't want DetectDistro to return anything else than Distro::UnknownDistro when running on a non-Linux OS.
Would you have a different opinion or something I don't see?

aganea added a comment.EditedNov 21 2019, 9:45 AM

Actually, I'm not sure the DetectDistro() does what it intends to do when cross-compiling: if I compile on Ubuntu and I forcibly specify -target x86_64-linux on the cmd-line, should it detect which Linux distro I'm on? In the case of CudaInstallationDetector() it seems it should use the host triple, whereas in the case clang/lib/Driver/ToolChains/Linux.cpp:Linux() it adds flags for building the target, so it should take the target triple maybe. Should we pass a triple to DetectDistro?

aganea updated this revision to Diff 230525.Nov 21 2019, 1:35 PM

Removed #ifdef _WIN32
Use the target triple where possible to detect the distro. The Cuda installation detector uses the host triple, in order to use the host tooling.
Skip distro detection when the target or host system is not Linux.

aganea retitled this revision from [Distro] Bypass distro detection on Windows to [Distro] Bypass distro detection on non-Linux hosts.Nov 21 2019, 1:36 PM
aganea edited the summary of this revision. (Show Details)
rnk accepted this revision.Nov 22 2019, 12:19 PM

Actually, I'm not sure the DetectDistro() does what it intends to do when cross-compiling: if I compile on Ubuntu and I forcibly specify -target x86_64-linux on the cmd-line, should it detect which Linux distro I'm on? In the case of CudaInstallationDetector() it seems it should use the host triple, whereas in the case clang/lib/Driver/ToolChains/Linux.cpp:Linux() it adds flags for building the target, so it should take the target triple maybe. Should we pass a triple to DetectDistro?

It does seem wrong to use DetectDistro when the user passes a target triple. DetectDistro inherently detects something about that host, and it seems we don't have a way to pass that information explicitly on the command line. So, cross-compiling from one distro to another might somehow do the wrong thing depending on what distro detection does.

In D70467#1752611, @rnk wrote:

Hm, I guess it does happen. I think that condition should be restructured to only do all that BSD, PS4, Android, Gentoo etc logic if the format is ELF, if COFF, then always default to -faddrsig.

We're cross-compiling all our target platforms on Windows. It seems this patch would be still valid in that case:

> clang -target x86_64-linux a.cpp

Would still call DetectDistro(), and unless I'm missing something, we wouldn't want it to lookup C:\etc\lsb-release even if it's there? Or people are really doing that to target a specific distro when compiling from Windows? It doesn't make sense from my POV.
The same argument applies to CUDA: the code in clang/lib/Driver/ToolChains/Cuda.cpp:CudaInstallationDetector() says "HostTriple" but it's actually the Target triple -- you can try my cmd-line above, it hits the Distro detect.
Unless you specify a non-real FS, I wouldn't want DetectDistro to return anything else than Distro::UnknownDistro when running on a non-Linux OS.
Would you have a different opinion or something I don't see?

True, that makes sense to me.

lgtm

clang/lib/Driver/Distro.cpp
29

I think it's worth adding more to this comment. This is the case where someone is cross-compiling from BSD or Windows to Linux, and it would be meaningless to try to figure out the "distro" of the non-Linux host.

clang/lib/Driver/ToolChains/Linux.cpp
514

So many duplicate detections. =/

This revision is now accepted and ready to land.Nov 22 2019, 12:19 PM
This revision was automatically updated to reflect the committed changes.
aganea marked an inline comment as done.