[Driver] Add option to select compiler-rt arch suffix #89775
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Paul T Robinson (pogo59) ChangesCommit b876596 corrected default compiler-rt library names for many targets, this patch restores the arch suffix for PlayStation and Windows which both distribute these libraries with the arch suffix. Full diff: https://github.com/llvm/llvm-project/pull/89775.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 237092ed07e5dc..d104570491203b 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -684,7 +684,9 @@ std::string ToolChain::getCompilerRT(const ArgList &Args, StringRef Component,
if (Path.empty())
Path = P;
}
- if (getTriple().isOSAIX())
+ // Some targets default to the old layout.
+ if (getTriple().isOSAIX() || getTriple().isPS() ||
+ getTriple().isWindowsMSVCEnvironment())
Path.clear();
// Check the filename for the old layout if the new one does not exist.
diff --git a/clang/test/Driver/cl-link.c b/clang/test/Driver/cl-link.c
index ffd0b5ac4bade8..444f0c01b3f999 100644
--- a/clang/test/Driver/cl-link.c
+++ b/clang/test/Driver/cl-link.c
@@ -13,20 +13,20 @@
// ASAN: link.exe
// ASAN: "-debug"
// ASAN: "-incremental:no"
-// ASAN: "{{[^"]*}}clang_rt.asan.lib"
-// ASAN: "-wholearchive:{{.*}}clang_rt.asan.lib"
-// ASAN: "{{[^"]*}}clang_rt.asan_cxx.lib"
-// ASAN: "-wholearchive:{{.*}}clang_rt.asan_cxx.lib"
+// ASAN: "{{[^"]*}}clang_rt.asan-i386.lib"
+// ASAN: "-wholearchive:{{.*}}clang_rt.asan-i386.lib"
+// ASAN: "{{[^"]*}}clang_rt.asan_cxx-i386.lib"
+// ASAN: "-wholearchive:{{.*}}clang_rt.asan_cxx-i386.lib"
// ASAN: "{{.*}}cl-link{{.*}}.obj"
// RUN: %clang_cl -m32 -arch:IA32 --target=i386-pc-win32 /MD /Tc%s -fuse-ld=link -### -fsanitize=address 2>&1 | FileCheck --check-prefix=ASAN-MD %s
// ASAN-MD: link.exe
// ASAN-MD: "-debug"
// ASAN-MD: "-incremental:no"
-// ASAN-MD: "{{.*}}clang_rt.asan_dynamic.lib"
-// ASAN-MD: "{{[^"]*}}clang_rt.asan_dynamic_runtime_thunk.lib"
+// ASAN-MD: "{{.*}}clang_rt.asan_dynamic-i386.lib"
+// ASAN-MD: "{{[^"]*}}clang_rt.asan_dynamic_runtime_thunk-i386.lib"
// ASAN-MD: "-include:___asan_seh_interceptor"
-// ASAN-MD: "-wholearchive:{{.*}}clang_rt.asan_dynamic_runtime_thunk.lib"
+// ASAN-MD: "-wholearchive:{{.*}}clang_rt.asan_dynamic_runtime_thunk-i386.lib"
// ASAN-MD: "{{.*}}cl-link{{.*}}.obj"
// RUN: %clang_cl /LD -fuse-ld=link -### /Tc%s 2>&1 | FileCheck --check-prefix=DLL %s
@@ -40,7 +40,7 @@
// ASAN-DLL: "-dll"
// ASAN-DLL: "-debug"
// ASAN-DLL: "-incremental:no"
-// ASAN-DLL: "{{.*}}clang_rt.asan_dll_thunk.lib"
+// ASAN-DLL: "{{.*}}clang_rt.asan_dll_thunk-i386.lib"
// ASAN-DLL: "{{.*}}cl-link{{.*}}.obj"
// RUN: %clang_cl /Zi /Tc%s -fuse-ld=link -### 2>&1 | FileCheck --check-prefix=DEBUG %s
|
pogo59
changed the title
|
We have a custom toolchain that uses the new style on windows and if you build with runtimes on windows (which is the suggested way) it will end up with the new style without arch suffix. I don't think we can assume this is correct for windows in all setups. I am fine with this change on PlayStation, but I wonder if we should have a switch for this in CMake so the builder can control it instead of assuming stuff. |
|
Microsoft obviously builds and distributes with the arch suffix, supporting both 32-bit and 64-bit in the same distro. So I think they will continue to need it. (I can't actually speak for MS, but it's what I see in the Visual Studio installation.) I don't know enough about how distros work to say what's needed here. Some sort of CMAKE variable I suppose. Then we and MS will set it up one way, and you will set it up the other way. Do you have a concrete suggestion? |
Was someone from Microsoft party to that decision? If not, perhaps it needs to be revisited. We should not be making decisions about how they want to do things. |
|
It's been like that for maybe 2-3 years now and no one has complained about it, so I think that change is solid. I can suggest a CMake change, but last time it was discussed I think @MaskRay was against a new variable, but since we might need to have some different behaviour it might be warrented. |
Are you sure it's this change? There are reports of similar changes showing up in #87866 (comment), caused by ccdebba (#87866)
The old status quo has been that you can build the runtimes with either layout, and clang will use whichever it finds, when invoking the linker. The recent changes, #81037 and #87866, were (as far as I know) only intended to change what is printed as error messages, when neither is found, to help users correct their setup for the new style layout. But those changes also seem to have unexpected effects in changing e.g. what gets emitted as embedded directive, when the compiler doesn't see either of them at compile time (with e.g. distributed build systems), while they might be available at link time.
Not sure who might have been against it; my take on it is at least that the build time cmake variables are tricky, when e.g. one clang binary might be for multiple different targets (e.g. for native compilation on linux, with per-target runtime directories there, but also for cross compilation for windows targets with a different setup for the target runtimes). I'm not against them, as long as both setups remain usable though. |
This is exactly the scenario we had. I will double-check that it is solved by my patch, but it may take a little while. |
As a downstream vendor I can tell you we work around things all the time without bothering to try to fix things upstream (I try to encourage more engagement, but I can't control what people actually do, and upstreaming doesn't always work out). We build these libraries with the arch suffix, even though we have only one target, because we don't want to change the names of libraries when we don't have to. Without someone from MS speaking up, we simply don't know their thinking, we can only observe what they deliver. |
|
My recollection of the past discussions is that we want users to switch to the new hierarchy.
Yes. We try avoiding a CMake variable that customizes this detection.
This matches my understanding. I am not aware of the embedded directive? Do you embed a path to a compiler-rt library to the generated object files? I think while technically a new clang can use an old compiler-rt file hierarchy, If we avoid hard-coded library names in compiler-generated relocatable files (just called "object files" on Windows?). I am fine if more path probing is added to help users migrate, but the end result (in a few years) should be everyone using a new file hierarchy. |
Clang does this, in a number of cases. In the MSVC ecosystem, one usually invokes
I don't think anybody is arguing that a new clang should use an old compiler-rt install, but it should be able to use a new install with the layout according to
We can't avoid this - we already have this situation. See #87866 (comment) - #87866 changed the output of the embedded directives when building with a distributed build system, where the compiler doesn't have access to inspect the lib directory that is going to be used to link things in the end. |
Is Microsoft a "user" who agreed to this change? Was clang-vendors tagged on this discussion about a new file hierarchy? I don't recall it. The library name change is news to me, although if it took place in a Discourse category that I don't follow, possibly our sanitizer folks are aware and I am not.
I probably should not have pulled the PlayStation bit into it. The situation I ran into is a distributed build using a fresh-built clang-cl and the Visual Studio libraries in order to build a Windows app with
Not a full path, but a filename. I see @mstorsjo has a comment also along these lines. The filename is searched for on the usual library paths, which aren't necessarily known or available to the compiler. |
I don't know. There hasn't been enough engagement from Microsoft contributors to Clang Driver and sanitizers. Indeed, we could use clang-vendors more frequently. Some changes are UI-intended though, and only caused distributed build system difference due to something I believe unsupported (see below).
OK, the problem is the magic |
Is there a Release Note for this new requirement on distributed build systems? I don't know whether it would ordinarily be the users or the dbs providers who would implement it, but you've created that external requirement and it needs to be documented. What is -frtlib-defaultlib? I don't see it in include/clang/Driver/Options.td. I will talk to my dbs team and see if we can find a way to appease the Clang driver. |
|
We can add a release note. There is a lot of path probing code in clangDriver. There have been a few cases before and we did not write specific release notes for the driver behavior:
|
Possibly users of those targets do not depend on distributed builds the way our users do, and didn't run into the problem? They don't look like Windows targets, anyway. Distributed builds are very heavily used by game studios and our licensees are typically Windows shops. |
|
I agree that if downstream want to change stuff, they need to engage. We can't guess what microsoft wants to do (or sony) unless we have a discussion about it. This is also documented in the developer policy. If there are missed release notes, they need to be added of course. That said - if I understand your problem correctly @pogo59 you are building clang yourself, but you are using the asan libraries from MSVC? I am not sure that would ever be a supported configuration if the compiler isn't built from the same source as the runtime libraries. If you would have built compiler-rt/asan yourself it should have been found and used correctly. I think it might make sense to make a RFC (again I guess) to remove the old paths and have a grace period where there is a cmake option to force the old layout. This would harmonize so that we have just one layout and remove the biggest problem (in my mind) which is the fallback that can load the wrong runtime library if you are unlucky. |
FWIW this doesn't match my understanding. phosek added the new hierarchy to Fuchsia, maskray put in some work to enable it elsewhere, but it's a huge headache, and we've forcibly turned it off on all platforms on our end. From what I can tell, it doesn't really solve any problems and causes all kinds of confusion. I wish the whole thing didn't exist. |
|
Anyway, for this specific issue it sounds like the change here was unintentional. As suggested by mstorsjo at #87866 (comment), I think it'd be good to revert these changes, and then make a mindful decision about this when relanding. |
|
Honestly - I think going back to one style of runtime path is to be preferred now. But I don't think it's fair to say that it doesn't solve any problems, because we switched to it so that we could ship more platforms in one toolchain package without having to add cfg files and similar things to point it to the right path. There was some kind of collision we ran into when we where using the old path. I do think the current status quo is confusing and bad, so I rather we go back to the old layout if that's what people prefer and I'll have to work around that in that case. But it sounds like this needs a RFC. |
|
And I think it's better to revert it all instead of implementing this half-revert in this PR in that case and then we should discuss how to move forward. |
|
I understand that there is some distributed build system pain and I feel sympathy. Again, clangDriver does file probing in various places. If you want a specific driver decision, provide sufficient files to make it make the best guess for you. If I am also fine if Windows users decide to prefer UI for |
I'm unable to find what code this affects. I don't see it mentioned anywhere in clang/lib or clang/include. It does seem like it should control the behavior of How do CMake variables turn into something that controls code behavior? |
This is the only place where we probe link-time inputs for compile-time decisions as far as I know. |
IIRC - the CMake variable doesn't change any driver code. It just changes the install path of compiler-rt, you can probably find references to it in compiler-rt CMake files. The driver always probes both paths. Before this latest patch it probes the new layout first, then fell back to the old _arch suffix files and it lead to bad behaviour where it unexpectedly fell back (and the error message really didn't make any sense). This new patch was intended to change so the error message made more sense, but seems to have changed some specific behaviours as well. |
|
I think I suggested that we should make the CMake variable change the driver behaviour to reflect the setting in order to remove unexpected fallbacks and that the vendor could select the layout. I still think that's probably the better solution. |
|
Poking around more, it looks like the canonical way to convert a CMake variable to a C++ define is to add an entry to llvm/include/llvm/Config/llvm-config.h.cmake. I'll have a go at doing it that way. |
|
Note that if the code uses LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to decide whether to probe the new or old hierarchy first, these clang/test/Driver I do wish that Windows users migrate to LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on to match |
|
I've redone the decision to be based on the setting of LLVM_ENABLE_PER_TARGET_RUNTIME_DIR and updated the tests to work with either setting as much as possible. Tests that explicitly look for the per-target directory are enabled only if the CMake variable is ON. |
|
One reason why I’m not entirely thrilled (not firmly against, but not thrilled - but I’m not sure if there are much better options either) with this direction, is that it becomes messy when cross compiling. LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is automatically set based on the host OS, e.g. if building on Linux, it defaults to true, while if building on Windows, it defaults to false. When building Clang separately from the runtimes, this decision comes even more as a surprise; if I first build a Linux Clang, then later build the runtimes for Windows (for a Linux hosted cross toolchain targeting Windows), this might mismatch. For the change that currently is suggested, this mismatch might be benign and not matter for this particular setup/direction, but if we extend this pattern to skip all sorts of probing and just assume one layout or the other, it will matter. I guess the solution to that will be that when I build the clang binary first, I need to set LLVM_ENABLE_PER_TARGET_RUNTIME_DIR based on how I’m going to build my runtimes later. That’s probably acceptable, even though less convenient than now. However, if I instead have a Clang binary from e.g. a Linux distribution, and I want to build Windows runtimes to use with it, I would need a way to query which layout is hardcoded into the binary. There are various options for querying paths from the Clang executable, but it’s not very obvious to figure out which layout it will require, or whether it might support both. |
|
Worth noting that this only affects the presumed layout if the driver can't find either layout. In most installations, it will detect which one you have installed, and use it correctly. It's when the driver can't find the libs, and has to guess, that we run into difficulty. I started out by making additional assumptions based on target, and people didn't like that either. Another way to run this would be to invent a driver option whose default is derived from the CMake variable (and possibly target), that will pick the presumed layout instead of essentially hardcoding it. Then if you get into situations where the driver is guessing wrong, you have an easy (or at least easier, depending on your build system) fix: add a command-line option. This is different from the option in #89642 which stops the driver from using the guessed lib name at all, and makes you specify the lib explicitly. My suggestion is an option that tells the driver which layout to use as the fallback. |
pogo59
changed the title
|
I think this is fine. I probably have to carry a one line patch in our tree to make it the default on windows as well. But that's acceptable to me. |
|
It needs a release note and documentation though. |
|
I want to hear about opinions why the old compiler-rt file hierarchy is preferred by some users. Ideally, all operating systems would eventually migrate to For While some level of fragmentation (AIX) is acceptable, I hope these exceptions remain the minority. If possible, I hope that we avoid a driver option. |
|
My understanding is that this change never had a proper Discourse RFC, which means it was quite possible for vendors not to have noticed it. Certainly I (on behalf of Sony) was not aware, and I make some effort to keep an eye on things. There is no good reason to suppose Visual Studio will adopt the change if they are unaware of it. I understand that @tru has adopted the new style downstream for their internal use. The argument I am hearing here is that you are requiring all downstream Windows users to do the same, because otherwise their builds will start failing (as our internal builds have done). But, despite that requirement, it appears that the Windows default is to build with the old style. This is at best an inconsistency, and at worst really broken. I can see a few ways to fix it.
|
|
Yeah I think this should be brought up as an RFC in discourse. I would like to see that we move away from two different layouts and just adopt one (my preference is the new layout, but honestly - I don't feel that strongly about it). |
|
To answer @MaskRay 's question directly, with respect to PlayStation (not Windows, which is the major problem): We have exactly one target, so neither the arch suffix nor a target-specific directory are useful. (Arguably we have two targets, but as we deliver entirely separate SDKs for PS4 and PS5, that's really irrelevant.) I've raised an internal ticket so the relevant people will become aware of what's going on. I'm making a fuss about Windows because internally we do self-hosted builds, so we are essentially vending a Windows-target toolchain to ourselves. And that's the one that is failing. |
I think if you really want a range of opinions, you can get them from an RFC. Not many people will be watching this PR. |
Perhaps @tru can make another comment to https://discourse.llvm.org/t/rfc-time-to-drop-legacy-runtime-paths/64628 and tag specific people. |
|
Thanks for the link. I've put a comment there pointing here, explaining that the current default state (at least on Windows) is broken and that this patch has come to no agreement about how to fix it. |
|
I don't have the bandwidth to get a RFC through right now. If this is broken and no-one wants to take care of getting consensus for something new, I suggest you revert to the previous state. For my toolchain I can continue to carry a patch until this is all sorted. |
Same here; I'm just professionally offended by the brokenness. But I'm not the right person to drive an RFC. If nobody else is willing to, I'll abandon this and we'll do what we need to downstream to hack around it. |
|
Abandoning this. We'll work around the brokenness in our downstream repo. |
Commit b876596 corrected default compiler-rt library names for many targets, this patch restores the arch suffix for PlayStation and Windows which both distribute these libraries with the arch suffix.