mgorny marked an inline comment as done. mgorny added inline comments.
================ Comment at: clang/test/Driver/config-file3.c:27 -//--- Invocation qqq-clang-g++ tries to find config file qqq-clang-g++.cfg first. +//--- Invocation x86_64-unknown-linux-gnu-clang-g++ tries x86_64-unknown-linux-gnu-clang++.cfg first. // ---------------- arichardson wrote: > sepavloff wrote: > > mgorny wrote: > > > sepavloff wrote: > > > > mgorny wrote: > > > > > sepavloff wrote: > > > > > > mgorny wrote: > > > > > > > sepavloff wrote: > > > > > > > > mgorny wrote: > > > > > > > > > arichardson wrote: > > > > > > > > > > sepavloff wrote: > > > > > > > > > > > Tests must check the case when target prefix is not a > > > > > > > > > > > real triple as in the original test (qqq-clang). > > > > > > > > > > It would be quite important for me that this continues to > > > > > > > > > > work. I made use of that in the CheriBSD toolchain when > > > > > > > > > > creating [[ > > > > > > > > > > https://github.com/CTSRD-CHERI/cheribuild/blob/master/pycheribuild/projects/cross/llvm.py#L499 > > > > > > > > > > | symlinked binaries to easily build for different ABIs]] > > > > > > > > > > such as `cheribsd-riscv64-hybrid-clang++` and > > > > > > > > > > `cheribsd-riscv64-purecap-clang-cpp`. It appears this > > > > > > > > > > previously only worked if the prefix did not start with a > > > > > > > > > > valid triple (which is why I put the OS before the > > > > > > > > > > architecture). I think it would also be nice if the whole > > > > > > > > > > prefix was checked even if it starts with a valid triple, > > > > > > > > > > but this does not need to be changed in this patch (haven't > > > > > > > > > > looked at it in detail so this might actually work). > > > > > > > > > If the prefix is not a valid triple, then clang ignores it > > > > > > > > > and uses host triple instead. And now we're back to square > > > > > > > > > one. If I check both variants, it's too complex. If I don't, > > > > > > > > > it's bad too. > > > > > > > > Our customers use this feature. Target prefix may designate, > > > > > > > > for example, debug build or build with specific framework. > > > > > > > How are we supposed to avoid the "absurd" case where x86_64 > > > > > > > configs are loaded for `-m32` invocation then? > > > > > > In this case overloading target does not makes sense. You need to > > > > > > analyze `RealTriple` in `Driver::loadDefaultConfigFiles` and if it > > > > > > is wrong, use target prefix as if it is real target. > > > > > What do you mean by "wrong"? The target triple is always a correct > > > > > triple. > > > > Not triple, sorry. Target prefix taken from > > > > `ClangNameParts.TargetPrefix`. > > > Now I'm confused. Are you suggesting that we use prefix instead of target > > > if it's… incorrect? > > Basically yes. > > > > We need to support arbitrary target prefixes, because they are used now. If > > `ClangNameParts.TargetPrefix` is not a valid triple, use it to build config > > file names. > > > > We also need to use config files like `x86_64.cfg` where middle components > > are dropped, they are also used. They also need to support target > > overloading. > I can replace my approach with simple shell scripts that invoke clang > --config=..., but IMO it would be nice if the logic was something like the > first check being > `if "explicit --target empty" and "ClangNameParts.TargetPrefix non-empty" and > "ClangNameParts.TargetPrefix not a valid triple" -> load > ClangNameParts.TargetPrefix + ".cfg"` followed by the current logic. > I don't mind either way whether the first check should also load a > driver-specific config file or not. I'd also be happy if the logic was "try > loading ClangNameParts.TargetPrefix if explicit --target empty" regardless of > whether it is a valid triple or not. > Does this approach sound reasonable? Ok, so I guess that the logic would be: if name prefix's (non-normalized) triple does not have valid arch or valid os parts, we use that instead of real triple. ================ Comment at: clang/test/Driver/config-file3.c:60 +// +// RUN: %t/testdmode/x86_64-unknown-linux-gnu-clang-g++ --target=i386-unknown-linux-gnu --config-system-dir= --config-user-dir= -no-canonical-prefixes --version 2>&1 | FileCheck %s -check-prefix FULL1-I386 + ---------------- sepavloff wrote: > We also need a test that checks that in the case of > `--target=i386-unknown-linux-gnu` and absence of > `i386-unknown-linux-gnu-clang++.cfg` clang does not load > `x86_64-unknown-linux-gnu-clang-g++.cfg`. Isn't this what `FULL2 + -m32` tests for? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134337/new/ https://reviews.llvm.org/D134337 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
