On Red Hat operating systems (RHEL, Fedora Linux), the gcc triple is
$arch-redhat-linux. The gnu environment is the default environment, but
is not explcitly listed in the triple.
Details
Details
- Reviewers
MaskRay aaronpuchert
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/include/llvm/ADT/Triple.h | ||
|---|---|---|
| 532 ↗ | (On Diff #376415) | Since getEnvironment is also public, shouldn't we fix it there? Otherwise I fear this might lead to inconsistencies. (That is, getEnvironment() returning UnknownEnvironment, but isGNUEnvironment() returning true.) Perhaps we should even do something like this: bool isGNUEnvironment() const {
return isGNU(getEnvironment());
}
static bool isGNU(EnvironmentType Env) {
// ...
}to clarify that isGNUEnvironment really only depends on the environment. Possibly we should even set Environment = Triple::GNU; in the constructor for vendors known to use GNU by default without declaring it? That's currently done in some cases, though I'm not sure if the members should contain the properties as declared, or the actual values. |
| 536 ↗ | (On Diff #376415) | The Triple:: qualification should not be needed inside the class. Though it's used throughout the class, so maybe we should address that separately. |
Comment Actions
This whole thing feels a bit wrong to me, but if we need something like this, there's three things that stand out to me. Firstly, @aaronpuchert already commented that SUSE has the same issue, so this shouldn't be limited to Red Hat. Depending on what GCC does, possibly even things like x86_64-linux and x86_64-unknown-linux should be treated as x86_64-unknown-linux-gnu. Maybe the vendor check could just be removed entirely? Secondly, we appear to not distinguish between unknown environment and missing environment, this should probably only be done for missing environment. That is, we probably shouldn't start treating x86_64-linux-uclibc, which we do not support, as x86_64-linux-gnu. Thirdly, this should almost certainly have a check for OS, we should only be doing this for Linux.
Comment Actions
@hvdijk There are two things I'm trying to fix with this patch:
- I want a triple that behaves the same as gcc's x86_64-redhat-linux and can also be used to select the gcc install at /usr/lib/x86_64-redhat-linux
- I want to be able to use gcc -dumpmachine to determine the host architecture rather than config.guess.
I really want to fix 1, but if I can fix 2 too that would be a bonus.
Another solution for 1 (but not 2), would be to update the driver to select the x86_64-redhat-linux gcc install when the triple is x86_64-redhat-linux-gnu. Do you think this would be better?
Comment Actions
Yeah, I saw your comments on D109837. It's unfortunate that changing the GCC target name is a no go, that would have been the nicest fix IMO.
- I want to be able to use gcc -dumpmachine to determine the host architecture rather than config.guess.
I really want to fix 1, but if I can fix 2 too that would be a bonus.
Another solution for 1 (but not 2), would be to update the driver to select the x86_64-redhat-linux gcc install when the triple is x86_64-redhat-linux-gnu. Do you think this would be better?
That should already happen, x86_64-redhat-linux is in X86_64Triples (https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Gnu.cpp), so any GCC install that uses x86_64-redhat-linux as the triple name should be picked up by LLVM/clang regardless of whether the LLVM target includes -gnu.
So that means, I *think*, that if the CMake logic to read gcc -dumpmachine's output could change x86_64-redhat-linux (and x86_64-suse-linux and others) to append -gnu, the core LLVM code wouldn't need updating. I've been holding back on commenting in more detail as I am not currently in a position to test that this actually works, but I should be able to try soon.
Comment Actions
It picks the first triple from the list that is installed on your system. So if you also have an x86_64-linux-gnu gcc installation on your system, it will pick that one over x86_64-redhat-linux.
So that means, I *think*, that if the CMake logic to read gcc -dumpmachine's output could change x86_64-redhat-linux (and x86_64-suse-linux and others) to append -gnu, the core LLVM code wouldn't need updating. I've been holding back on commenting in more detail as I am not currently in a position to test that this actually works, but I should be able to try soon.
Doing this will have the same problem I described above, clang will still pick x86_64-linux-gnu over x86_64-redhat-linux.
Comment Actions
There seem to be efforts to clean those up, see e.g. D109727.
Is that something people do? But yeah, just picking the first one from the list doesn't sound great.
Comment Actions
Ah, okay, but that is how LLVM behaves today as well, is it not? LLVM can either be configured for x86_64-redhat-linux, in which case it will not be seen as a GNU environment, or it can be configured for x86_64-redhat-linux-gnu, in which case the x86_64-redhat-linux installation will be found but will not be preferred. Whichever way is used by Red Hat today, as long as that remains the same it should not get in the way of D109837, and that can then be improved further here in whatever way works best. I interpreted your comments on that other diff as making this is a prerequisite for it, but that should not be necessary, I think?
So you would like to have a triple that causes the x86_64-redhat-linux GCC installation to be preferred over all others, but that still ensures LLVM sees it as a GNU triple. There are basically two alternatives then, the first is making sure that x86_64-redhat-linux is seen as a GNU triple, which is what you are going for here. A possibly radical alternative suggestion: could we instead change the way the GCC installation to use is determined? This might seem like overkill if the Red Hat special case would be the only benefit, but I think it might actually be a welcome improvement for other systems as well to reduce unnecessary stat-ing when the GCC target name and LLVM target name do not use the same spelling.
Comment Actions
Maybe @marxin can comment on that?
I'm not sure if we can assume that linux implies gnu right away. We can probably assume that for some vendors though. So in general I think this patch is a good idea, modulo some details.
Comment Actions
Can't speak for RedHat, but on SUSE we have
> gcc -dumpmachine x86_64-suse-linux > clang -dumpmachine x86_64-unknown-linux-gnu
and that I think is the problem for config.guess. (RedHat is probably similar, since before this change there is no vendor for it at all.)
A possibly radical alternative suggestion: could we instead change the way the GCC installation to use is determined?
Some kind of map from llvm::Triple to a path/directory for the GCC installation? Should be feasible, but it wouldn't harmonize the -dumpmachine output.
Comment Actions
Yes, there is an x86_64 cross compiler package in Fedora that installs to /usr/lib/x86_64-linux-gnu. It's easy to accidentally install this if you do something like dnf install *gcc*.
Comment Actions
Not anymore. I've switched the default target triple to <arch>-suse-linux some time ago (except for ARM 6 & 7, where we specify the ABI), and haven't gotten any complaints about it.
There is only one issue that I have observed, and patching isGNUEnvironment fixes it: we get a function call for fma even if it's available in hardware, apparently because Sema::AddKnownFunctionAttributes in SemaDecl.cpp doesn't add __attribute__(const)) if isGNUEnvironment returns false.
| llvm/include/llvm/ADT/Triple.h | ||
|---|---|---|
| 532 ↗ | (On Diff #376415) | I've been thinking about this a bit more and think what you did here is actually correct. The triple doesn't explicitly specify GNU, but we're "implicitly" in a GNU environment. And if one wants to check whether they're in some kind of GNU environment, they'd probably use this function instead of enumerating all the GNU* environments anyway. Most of the occurrences of Triple::GNU are actually in connection with Triple::Win32. Places where there might be issues:
|
| 532–536 ↗ | (On Diff #376415) | Perhaps we should make this a switch? |
Comment Actions
We switched to using <arch>-redhat-linux-gnu as the default triple and we patch clang to translate the triple from <arch>-redhat-linux-gnu to <arch>-redhat-linux only when searching for the gcc installation, but not for anything else. Switching clang/llvm to treat <arch>-redhat-linux or <arch>-suse-linux as a gnu environment would be better, but that would also cause a behavior change for anyone using those triples.
Comment Actions
That seems like a valid solution as well.
Switching clang/llvm to treat <arch>-redhat-linux or <arch>-suse-linux as a gnu environment would be better, but that would also cause a behavior change for anyone using those triples.
Do you mean that the triple is treated as GNU, but users might not want that, e.g. because they use a different runtime? I think users would explicitly specify if they wanted a different runtime than GNU on RedHat/SUSE.
Or do you mean a difference in terms of which GCC installation gets picked up? Though I'm wondering why you wouldn't expect the equally named GCC triple.
Comment Actions
I just mean that making <arch>-redhat-linux imply GNU might subtly change behavior for those triples if people are using them now, but maybe that doesn't really matter that much.
Comment Actions
Looks fine to me, but maybe you can wait a bit for the other reviewers in case there are still concerns.
| llvm/include/llvm/TargetParser/Triple.h | ||
|---|---|---|
| 568–578 | You might use the opportunity to drop the redundant Triple::. | |
Comment Actions
In D110900#3044136, I suggested another approach that should reduce the number of file system accesses, which you implemented in D110900#3044545 (D111207). Apologies for never responding to that; at first glance the way you implemented that looks like it increases the number of file system accesses, and I don't think we should do that. Given that the only implemented alternative is this one, I'm not going to argue against this.
| llvm/include/llvm/TargetParser/Triple.h | ||
|---|---|---|
| 579 | In D110900#4160169 you suggested that this should be done for SUSE as well, do you still think so? | |
| llvm/lib/TargetParser/Triple.cpp | ||
| 199 | This list was in alphabetical order and should probably remain so. | |
| llvm/include/llvm/TargetParser/Triple.h | ||
|---|---|---|
| 579 | Yes, in fact we ship with a patch that does that in openSUSE now. It mostly works without that, but some things were missing, see D110900#4160110. If we can agree on this, I'd open a separate change on top of this for SUSE. (Unless @tstellar would like to include it right away.) | |
| llvm/include/llvm/TargetParser/Triple.h | ||
|---|---|---|
| 579 | I think we should add SUSE here in a separate change. | |
| llvm/include/llvm/TargetParser/Triple.h | ||
|---|---|---|
| 579 |
@hvdijk Your suggestion may be this: " Can you elaborate? I agree that decreasing the number of filesystem stats is preferable... | |
Comment Actions
My thinking there was that, taking x86_64-redhat-linux-gnu as an example, we know that the GCC installation we are looking for is going to use a target name of exactly x86_64-redhat-linux, so we can search for only that and skip the search for other target names, and skip the file system accesses associated with the search for other target names. And if we do it that way, there is no longer a drawback to using x86_64-redhat-linux-gnu as the LLVM triple, so we can simply use that and avoid the special vendor-based exceptions in isGNUEnvironment() that we have in this diff.
But, I don't actually mind doing it like in this diff; please consider it only as an option, and if you don't like it, just stick with the perfectly good diff we have here already.
| llvm/include/llvm/TargetParser/Triple.h | ||
|---|---|---|
| 579 | This is unrelated to what is being discussed here, will respond in a non-inline comment. | |
Comment Actions
This is exactly what we are doing in Fedora right now: https://src.fedoraproject.org/rpms/clang/blob/rawhide/f/0001-Driver-Add-a-gcc-equivalent-triple-to-the-list-of-tr.patch. The downside of doing this is that it adds even more vendor specific code the driver which we are trying to avoid.
Now that we have improved config file support, another option would be to distribute a default config file that has --gcc-install-dir=/usr/lib/gcc/x86_64-redhat-linux/13/ .
Comment Actions
Is it? Correct me if I am misreading, but that looks like it adds x86_64-redhat-linux to the list of GCC target names to consider, but still continues to search for all other GCC target names LLVM recognises as well.
Comment Actions
Yes, you are right. It does the triple conversion, but then doesn't only search for the x86_64-redhat-linux triple.
Comment Actions
I'd like to get there, but with x86_64-suse-linux as target triple, the same as GCC.
And if we do it that way, there is no longer a drawback to using x86_64-redhat-linux-gnu as the LLVM triple, so we can simply use that and avoid the special vendor-based exceptions in isGNUEnvironment() that we have in this diff.
With the vendor-based exceptions we can have both compilers use the same triple and Clang can simply look for a directory matching its own triple. So we don't need to maintain a map "Clang triple → GCC triple", and it might be less confusing for users. GCC is the primary compiler on RedHat/SUSE and thus it seems good UX to use the same terminology. "When in Rome, do as the Romans do."
You might use the opportunity to drop the redundant Triple::.