10ne1 added inline comments.
================
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()));
+
----------------
nickdesaulniers wrote:
> 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.
I've done some further debugging and turns out there was an error in my test
environment due to the Linux kernel tools/ cleanup patch not being correctly
applied.
After fixing that I can confirm the values for the triple are correct and this
normalization is not required anymore:
```
llvm::dbgs() << "TargetOrHost.getOS() = " << TargetOrHost.getOS() << "\n";
llvm::dbgs() << "TargetOrHost.isOSLinux() = " << TargetOrHost.isOSLinux() <<
"\n";
```
produces:
```
TargetOrHost.isOSLinux() = 1
TargetOrHost.getOS() = 9
```
which is what we expect.
I will drop this specific triplet code and update the diff, the only remaining
open issue is the sysroot detection change below. Thanks for your patience!
================
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())
----------------
nickdesaulniers wrote:
> 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.
It does makes sense if you read the condition carefully:
```
if (!GCCInstallation.isValid() || !getTriple().isMIPS())
```
results in:
```
if ( ! true || ! false )
```
which means an empty string is always returned because Arch does not support
mips even though a valid GCC install is present.
I think I'll just go ahead and clean up this code and update the diff to drop
the triple normalization.
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
