[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
MaskRay wrote: 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 `libclang_rt.asan.a`/`libclang_rt.asan-x86_64.a` tests will need a `REQUIRES: a_feature_signaling_the_config`, otherwise the tests would fail on the other configuration. Alternatively, create dummy files for these clang/test/Driver tests, but this would be huge amount of work. I do wish that Windows users migrate to LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on to match `llvm/runtimes/CMakeLists.txt`. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
tru wrote: 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
tru wrote: > > LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on > > > > 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 > `ToolChain::getCompilerRT`; where I had added the Windows/PS check, seems > like it should check the config variable instead. It's obvious that OS-based > checks are not appropriate, as @tru reports using the new scheme but MSVC > clearly uses the old scheme, both on Windows. > > > > How do CMake variables turn into something that controls code behavior? 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
nico wrote: > Again, clangDriver does file probing in various places. This is the only place where we probe link-time inputs for compile-time decisions as far as I know. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: > LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on 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 `ToolChain::getCompilerRT`; where I had added the Windows/PS check, seems like it should check the config variable instead. It's obvious that OS-based checks are not appropriate, as @tru reports using the new scheme but MSVC clearly uses the old scheme, both on Windows. How do CMake variables turn into something that controls code behavior? https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
MaskRay wrote: I understand that there is some distributed build system pain and I feel sympathy. However, I have pointed out that it is *unsupported* to not provide files for driver probing, therefore I am not sure it is fair to revert the patches improving `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on` users. 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 `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=off` users are willing to accept less test coverage, we can swap the probed paths in `ToolChain::getCompilerRT` if `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=off`. I am also fine if Windows users decide to prefer UI for `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=off` configurations and add a `isOSWindows()` check somewhere. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
tru wrote: 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
tru wrote: 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
nico wrote: Anyway, for this specific issue it sounds like the change here was unintentional. As suggested by mstorsjo at https://github.com/llvm/llvm-project/pull/87866#issuecomment-2073231116, I think it'd be good to revert these changes, and then make a mindful decision about this when relanding. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
nico wrote: > My recollection of the past discussions is that we want users to switch to > the new hierarchy. 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
tru wrote: 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: > 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
MaskRay wrote: > What is -frtlib-defaultlib? I don't see it in include/clang/Driver/Options.td. This is from @nico in a recent change #89642. It seems that Chromium is moving away from driver passing `--dependent-lib=` to ld? https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
MaskRay wrote: We can add a release note. There is a lot of path probing code in clangDriver. clangDriver trusts the result and does its best to construct assembler/linker command lines. This distributed build system use case is an unfortunate case where compilation depends on linking action behaviors like runtime library layouts. There have been a few cases before and we did not write specific release notes for the driver behavior: * https://reviews.llvm.org/D112906 (PPC64 probing glibc 2.34+) * https://reviews.llvm.org/D127812 (AArch64 FMV testing -rtlib=compiler-rt) https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: > I think the distributed build system users need to prepare a file hierarchy > to get the intended --dependent-lib= (like how to prepare --sysroot in > clang/test/Driver tests), or turn off it with the recent -frtlib-defaultlib. 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
MaskRay wrote: > > My recollection of the past discussions is that we want users to switch to > > the new hierarchy. > > 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 don't know. There hasn't been enough engagement from Microsoft contributors to Clang Driver and sanitizers. We as a community have been relying on a few Windows parties (including Chromium, @mstorsjo, @tru) that actively discuss on LLVM. My recollection is that the new compiler-rt file hierarchy has been repeated discussed on Discourses/llvm-project PR/issues in the past few years. If a party does not actively participate in the discussions, others would have to try guessing their intention. Indeed, we could use clang-vendors more frequently. > > 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 `-fsanitize`. > (PlayStation might have a similar issue, I haven't fully investigated that > yet. I think it is likely though.) > > > I am not aware of the embedded directive? Do you embed a path to a > > compiler-rt library to the generated object files? > > 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. OK, the problem is the magic `--dependent-lib=` filename that depends on the old/new file hierarchy. I think the distributed build system users need to prepare a file hierarchy to get the intended `--dependent-lib=` , or turn off it with the recent `-frtlib-defaultlib`. Otherwise, the filesystem probing of Clang Driver may not give the desired result, but it is up to the users to fix it. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: > My recollection of the past discussions is that we want users to switch to > the new hierarchy. 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. > `ToolChain::getCompilerRT` detects both the old and new compiler-rt > directory. Does it not work for PlayStation? 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 `-fsanitize`. (PlayStation might have a similar issue, I haven't fully investigated that yet. I think it is likely though.) > I am not aware of the embedded directive? Do you embed a path to a > compiler-rt library to the generated object files? 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
mstorsjo wrote: > > 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. > > 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? Clang does this, in a number of cases. In the MSVC ecosystem, one usually invokes `link` or `lld-link` directly, instead of using `clang` to drive the link - therefore, in order to pass implicit libraries to link, like asan/profile, directives are embedded into the generated object files, that tells the linker to link in those libraries. > I think while technically a new clang can use an old compiler-rt file > hierarchy, technically it is unsupported: kinda like that a very old libc++ > may not be built with a new Clang. 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 `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR` disabled. > If we avoid hard-coded library names in compiler-generated relocatable files > (just called "object files" on Windows?). there should be no distributed > build system concern. We can't avoid this - we already have this situation. See https://github.com/llvm/llvm-project/pull/87866#issuecomment-2072626122 - #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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
MaskRay wrote: My recollection of the past discussions is that we want users to switch to the new hierarchy. *BSD, Linux, and z/OS have migrated but I am not familiar with the Windows ecosystems. `ToolChain::getCompilerRT` detects both the old and new compiler-rt directory. Does it not work for PlayStation? > 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. Yes. We try avoiding a CMake variable that customizes this detection. We need detection for both old and new file hierarchies for now. > 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. 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, technically it is unsupported: kinda like that a very old libc++ may not be built with a new Clang. I know that changing anything here is tricky as there may be users doing it and we perhaps need to wait a few releases to drop support for the old hierarchy. The intention is that eventually everyone will use the new file hierarchy. If we avoid hard-coded library names in compiler-generated relocatable files (just called "object files" on Windows?). there should be no distributed build system concern. 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: > It's been like that for maybe 2-3 years now and no one has complained about it 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: > 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. 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
mstorsjo wrote: > Commit > [b876596](https://github.com/llvm/llvm-project/commit/b876596a76cdc183439b36455d26883b67f8ee51) > corrected default compiler-rt library names for many targets Are you sure it's this change? There are reports of similar changes showing up in https://github.com/llvm/llvm-project/pull/87866#issuecomment-2072684950, caused by ccdebbae4d77d3efc236af92c22941de5d437e01 (#87866) > It's been like that for maybe 2-3 years now and no one has complained about it 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. > 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. 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
tru wrote: 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: > if you build with runtimes on windows (which is the suggested way) it will > end up with the new style without arch suffix 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
pogo59 wrote: 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? https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
tru wrote: 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. https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver] Restore compiler-rt arch suffix for PS and Windows (PR #89775)
https://github.com/pogo59 edited https://github.com/llvm/llvm-project/pull/89775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
