nickdesaulniers requested changes to this revision.
nickdesaulniers added a comment.
This revision now requires changes to proceed.
It's not clear to me why Arch is special here. The path may be different, but
normalizing the triple seems unnecessary. Blocking this until I'm confident
that we're getting to the root cause of the issue here.
================
Comment at: clang/lib/Driver/Distro.cpp:213
+ // that the Linux OS and distro are properly detected in this cases.
+ llvm::Triple NormTargetOrHost =
llvm::Triple(Twine(TargetOrHost.normalize()));
+
----------------
10ne1 wrote:
> nickdesaulniers wrote:
> > Twine has an intentionally non-explicit constructor that accepts a
> > StringRef, which also has an intentionally non-explicit constructor that
> > accepts a std::string, which is what Triple::normalize() returns.
> >
> > Let's be consistent in the explicit construction of these temporary objects
> > by removing the explicit call to the Twine constructor.
> >
> > Also, why is it necessary to normalize the Triple? Can you give an example
> > of the "bad" input and how this "fixes" it?
> I do not claim to fully understand the LLVM toolchain & triplet
> auto-detection code, so maybe this normalization is more of a workaround than
> a real fix. Maybe we need to do the normalization earlier? I do not know, any
> suggestions are welcome.
>
> The behavior I'm seeing is:
>
> If TargetOrHost triplet is "aarch64-linux-gnu" then TargetOrHost.isOSLinux()
> == false and GetDistro returns Distro::UnknownDistro which causes failures
> like the following when building the kernel tools:
>
> ```
> make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- bpf
> DESCEND bpf
>
> Auto-detecting system features:
> ... libbfd: [ OFF ]
> ... disassembler-four-args: [ OFF ]
>
>
> DESCEND bpftool
>
> Auto-detecting system features:
> ... libbfd: [ on ]
> ... disassembler-four-args: [ on ]
> ... zlib: [ OFF ]
> ... libcap: [ OFF ]
> ... clang-bpf-co-re: [ on ]
>
>
> make[2]: *** No rule to make target
> '/home/adi/workspace/cola/GOO0021/chromiumos/src/third_party/kernel/v5.15/tools/include/linux/math.h',
> needed by 'btf.o'. Stop.
> make[1]: *** [Makefile:110: bpftool] Error 2
> make: *** [Makefile:69: bpf] Error 2
> ```
>
> If we do the triple normalization step before detecting the distro, the
> triplet becomes `aarch64-unknown-linux-gnu` which results in
> TargetOrHost.isOSLinux() == true, the distro is properly detected, then the
> system features are ON and the build works.
> If TargetOrHost triplet is "aarch64-linux-gnu" then TargetOrHost.isOSLinux()
> == false
What?! That sounds like a bug. What does Triple::getOS() return in that case?
Otherwise it sounds like `GetDistro` is getting called to early, before
whatever sets the OS has been initialized correctly.
================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377-394
+ if (Distro.IsArchLinux()) {
+ std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
+ if (getVFS().exists(Path))
+ return Path;
+ }
+
if (!GCCInstallation.isValid() || !getTriple().isMIPS())
----------------
10ne1 wrote:
> nickdesaulniers wrote:
> > Do we need the Arch-linux test before the call to
> > `GCCInstallation.isValid()`? Otherwise it seems like this duplicates a fair
> > amount of code computing the `Path`. The initial parts of the Path seem to
> > match; there's only more to it if the Distro is not arch-linux.
> In my testing on ArchLinux, `GCCInstallation.isValid()` is always `true`.
>
> The problem is that if test condition doesn't just test for a valid GCC
> install, it also tests `getTriple().isMIPS()` which is always false on Arch
> Linux which does not support mips, so the sysroot will always be an empty
> string.
>
> If you wish I can create a separate commit / review to untangle `if
> (!GCCInstallation.isValid() || !getTriple().isMIPS())` and try to reduce the
> code duplication, because really having a valid GCC install is independent
> from running on mips. What do you think?
That doesn't make sense.
If `GCCInstallation.isValid()` is always `true` then we should not be returning
the empty string.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134454/new/
https://reviews.llvm.org/D134454
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
