[PATCH] D87187: [Driver] Perform Linux distribution detection just once

[Reid "Away June-Sep" Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/Driver/Distro.cpp:205
+                                    const llvm::Triple &TargetOrHost) {
+  static Distro::DistroType Type = Distro::UninitializedDistro;
+
----------------
rnk wrote:
> aganea wrote:
> > I'm not sure if this is safe. @rnk Do we expect to call `GetDistro` with a different `VFS` or a different `Triple` when compiling with Clang? For GPU targets? Shouldn't we do this higher up, on the call site?
> I think there is a plausible use case for a compile server that does two compiles with different VFSs where you would get different distro detection results. I think this should be a field on the `Driver` object instead of a global variable.
> 
> I know LLD uses lots of globals, but that's not part of the clang design philosophy.
Sorry, I spoke too soon.

For @aganea's concern, my understanding is that the distro is a property of the host machine doing the compilation. Even if we do two compiles in the same process on the same machine with two triples, they should have the same distro, if they are using the same real FS. At least, the FS shouldn't change out from under us in a way that changes the result of distro detection. :) I think it's reasonable to use distro detection to inform header search paths, but we should avoid using it to control properties of the output, like stack protection security features, for example. Otherwise, it becomes impossible to target one Linux distro from another, because the distro does not appear to be controllable by a command line flag.

And, now that I see what is happening with the real FS checks here, I withdraw my concern. Even if you did make this a property of the driver, I would want some global variable somewhere to check that we don't end up doing the distro check twice. It's the only way to really be sure that some passerby doesn't introduce a new distro check. =P


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87187/new/

https://reviews.llvm.org/D87187