[Driver] Enable getOSLibDir() lib32 workaround for SPARC on Linux

[glaubitz]

Summary

This fixes the Builtins-sparc-linux testsuite failures on Linux
SPARC which occur because clang cannot find the 32-bit runtime
libraries when -m32 is passed on the command line. The same
workaround is already being used on X86 and PPC.

Also, switch the CHECK-DEBIAN-SPARC tests to use debian_multiarch_tree
as both sparc and sparc64 are using the MultiArch mechanism on modern Debian
systems the same way as x86_64, powerpc64el and others. Thus, switch the
CHECK-DEBIAN-SPARC32 and CHECK-DEBIAN-SPARC64 tests to use the files from
the debian_multiarch_tree directory for the header and linker path tests.

Finally, rename CHECK-DEBIAN-SPARC32 to CHECK-DEBIAN-SPARC to match the naming
scheme of the Debian MultiArch checks for the other Debian architectures.

[glaubitz]

Hmm, I'm not sure why the CHECK-DEBIAN-SPARC32 test is failing but I assume the expected output needs to be updated.

[glaubitz]

The tests are fixed by this change: https://reviews.llvm.org/D90549.

The tests CHECK-DEBIAN-SPARC32 is outdated and does not reflect the fact that sparc and sparc64 use MultiArch on Debian as well.

[glaubitz]

Ping.

[ro]

As it happens, I'd arrived at exactly the same patch when I tried a build on a Debian/sparc64 system in the GCC compile farm (gcc202), submitted as D85582.

A couple of questions/caveats:

• I wouldn't call this patch a workaround, as you do in the summary: if clang cannot find the 32-bit libs, it's broken and needs to be fixed.
• Please reformat Linux.cpp as the pre-merge check requested.
• I'd be willing to accept this patch, but for one I'm not certain about LLVM policy about who may or may not do so.
• Besides, I believe it's a mistake to split the bug fix and the testsuite change into to different patches. More about that in D90549.

[glaubitz]

Hi Rainer!

Thanks for your review.

In D90524#2382555, @ro wrote:

As it happens, I'd arrived at exactly the same patch when I tried a build on a Debian/sparc64 system in the GCC compile farm (gcc202), submitted as D85582.

A couple of questions/caveats:

• I wouldn't call this patch a workaround, as you do in the summary: if clang cannot find the 32-bit libs, it's broken and needs to be fixed.

I'm calling it a workaround because it's already called like that in the existing code comment (well, they call it "hack"):

FIXME: This is a bit of a hack. We should really unify this code for
reasoning about oslibdir spellings with the lib dir spellings in the
// GCCInstallationDetector, but that is a more significant refactoring.

• Please reformat Linux.cpp as the pre-merge check requested.

OK.

• I'd be willing to accept this patch, but for one I'm not certain about LLVM policy about who may or may not do so.
• Besides, I believe it's a mistake to split the bug fix and the testsuite change into to different patches. More about that in D90549.

OK.

[glaubitz]

Update as requested in previous review, also merge with D90549.

[glaubitz]

I think it should be good for merging now. I addressed all remarks. I'm still convinced that "workaround" is the proper term though.

[glaubitz]

Ping. It would be nice to get this finally merged so that the testsuite noise finally goes down on the sparc64 Linux worker.

[ro]

In D90524#2388214, @glaubitz wrote:

I think it should be good for merging now. I addressed all remarks. I'm still convinced that "workaround" is the proper term though.

Quite the contrary: the comment you cited

// FIXME: This is a bit of a hack. We should really unify this code for
// reasoning about oslibdir spellings with the lib dir spellings in the
// GCCInstallationDetector, but that is a more significant refactoring.

pretty clearly is about how/where support for that layout is implemented in the clang Driver code, not about the layout itself.

Besides, you haven't explained why it's appropriate to no longer test support for the old (pre-Debian 9,I believe) directory layout. However, as I said I don't feel qualified to review that part, so you'll need another reviewer for that, no matter if only testing the new layout or both old and new ones.

[ro]

In D90524#2393312, @glaubitz wrote:

Ping. It would be nice to get this finally merged so that the testsuite noise finally goes down on the sparc64 Linux worker.

Please be a little more patient: one ping a week is considered appropriate, but after only two days is a bit over the top.

[glaubitz]

In D90524#2393319, @ro wrote:

In D90524#2388214, @glaubitz wrote:

I think it should be good for merging now. I addressed all remarks. I'm still convinced that "workaround" is the proper term though.

Quite the contrary: the comment you cited

// FIXME: This is a bit of a hack. We should really unify this code for
// reasoning about oslibdir spellings with the lib dir spellings in the
// GCCInstallationDetector, but that is a more significant refactoring.

pretty clearly is about how/where support for that layout is implemented in the clang Driver code, not about the layout itself.

I don't understand that argument. I call it "workaround", the source comment calls it "hack". It's clearly not to stay forever as it's an ugly
workaround, but until a proper fix comes around, I would like to add "sparc" here as well so the testsuite failures drop from over
400 to just below 70.

Besides, you haven't explained why it's appropriate to no longer test support for the old (pre-Debian 9,I believe) directory layout. However, as I said I don't feel qualified to review that part, so you'll need another reviewer for that, no matter if only testing the new layout or both old and new ones.

Debian 8 doesn't even support sparc as the port was dropped in this release:

https://cdimage.debian.org/cdimage/archive/8.0.0/

The last sparc release was 7.11.0 and that's Wheezy which is long out of support:

https://cdimage.debian.org/cdimage/archive/7.11.0/

And, as I said, MultiArch was and is the same for all architectures, including sparc/sparc64. It does not make sense to test sparc here differently than the other
Debian architectures. There is no special sparc configuration in Debian and I think I can make that statement as Debian's primary maintainer for the sparc64
port.

[glaubitz]

In D90524#2393320, @ro wrote:

In D90524#2393312, @glaubitz wrote:

Ping. It would be nice to get this finally merged so that the testsuite noise finally goes down on the sparc64 Linux worker.

Please be a little more patient: one ping a week is considered appropriate, but after only two days is a bit over the top.

The problem is that LLVM is a very fast moving target and when waiting long for changes to be merged, one constantly runs
into the risk of having to rebase patches.

I would like to move forward with other changes and having unmerged changes open takes away attention.

[ro]

In D90524#2393353, @glaubitz wrote:

In D90524#2393319, @ro wrote:

In D90524#2388214, @glaubitz wrote:

I think it should be good for merging now. I addressed all remarks. I'm still convinced that "workaround" is the proper term though.

Quite the contrary: the comment you cited

// FIXME: This is a bit of a hack. We should really unify this code for
// reasoning about oslibdir spellings with the lib dir spellings in the
// GCCInstallationDetector, but that is a more significant refactoring.

pretty clearly is about how/where support for that layout is implemented in the clang Driver code, not about the layout itself.

I don't understand that argument. I call it "workaround", the source comment calls it "hack". It's clearly not to stay forever as it's an ugly
workaround, but until a proper fix comes around, I would like to add "sparc" here as well so the testsuite failures drop from over
400 to just below 70.

I think we're talking past each other, but as I've said, i'm not able to review the rest of this patch, so I won't belabour the point.

Besides, you haven't explained why it's appropriate to no longer test support for the old (pre-Debian 9,I believe) directory layout. However, as I said I don't feel qualified to review that part, so you'll need another reviewer for that, no matter if only testing the new layout or both old and new ones.

Debian 8 doesn't even support sparc as the port was dropped in this release:

https://cdimage.debian.org/cdimage/archive/8.0.0/

The last sparc release was 7.11.0 and that's Wheezy which is long out of support:

https://cdimage.debian.org/cdimage/archive/7.11.0/

And, as I said, MultiArch was and is the same for all architectures, including sparc/sparc64. It does not make sense to test sparc here differently than the other
Debian architectures. There is no special sparc configuration in Debian and I think I can make that statement as Debian's primary maintainer for the sparc64
port.

I had an extremely hard time researching the history of directory layouts for my patch D85582. Do as you like, I'm out of this.

[ro]

In D90524#2393366, @glaubitz wrote:

In D90524#2393320, @ro wrote:

In D90524#2393312, @glaubitz wrote:

Ping. It would be nice to get this finally merged so that the testsuite noise finally goes down on the sparc64 Linux worker.

Please be a little more patient: one ping a week is considered appropriate, but after only two days is a bit over the top.

The problem is that LLVM is a very fast moving target and when waiting long for changes to be merged, one constantly runs
into the risk of having to rebase patches.

True, but it's the same for everyone. Constantly pinging at short intervals won't get you faster reviews, but just creates noise and annoys possible reviewers.

[glaubitz]

! In D90524#2393635, @ro wrote:

I had an extremely hard time researching the history of directory layouts for my patch D85582. Do as you like, I'm out of this.

Please feel free to reach out to the corresponding debian-$ARCH mailing list for such questions. Debian-specific layouts are not
necessarily obvious at first glance to people not very familiar with the distribution.

[ro]

In D90524#2393651, @glaubitz wrote:

Please feel free to reach out to the corresponding debian-$ARCH mailing list for such questions. Debian-specific layouts are not
necessarily obvious at first glance to people not very familiar with the distribution.

I could have. However, my only interest in Linux/SPARC was to have a comparison point for my Solaris/SPARC work to determine which test failures occur on all SPARC targets and which ones are Solaris-specific. I simply don't have to time to dig deeper into Linux issues.

[glaubitz]

In D90524#2393746, @ro wrote:

In D90524#2393651, @glaubitz wrote:

Please feel free to reach out to the corresponding debian-$ARCH mailing list for such questions. Debian-specific layouts are not
necessarily obvious at first glance to people not very familiar with the distribution.

I could have. However, my only interest in Linux/SPARC was to have a comparison point for my Solaris/SPARC work to determine which test failures occur on all SPARC targets and which ones are Solaris-specific. I simply don't have to time to dig deeper into Linux issues.

No worries. I'm glad someone is taking care of the Solaris parts and appreciate fixes from other parties.

Please don't see my comments as dismissive in any way, they aren't meant that way. I appreciate the feedback.

[ro]

In D90524#2393747, @glaubitz wrote:

No worries. I'm glad someone is taking care of the Solaris parts and appreciate fixes from other parties.

My pleasure: hopefully our work will also be for the benefit of the other party. I certainly do have some patches in the queue that will fix bugs also occuring on Linux/SPARC.

[phosek]

clang/test/Driver/linux-ld.c

1292 ↗

(On Diff #303846)

What's the reason for changing the version from 4.9 to 4.5? Not that it really matters, but I'm curious.

[glaubitz]

I changed it to 4.5 to be consisted with the other Multi-Arch tests for x86, MIPS and PowerPC. That's all.

Debian Multi-Arch on sparc/sparc64 behaves the exact same way as it does on mips*, powerpc* and x86, so I think it makes sense to use the exact same path patterns.

[phosek]

LGTM

[glaubitz]

Thanks so much! Would you mind pushing that change for me? I don't have commit access at the moment.

Thanks!

[MaskRay]

clang/test/Driver/linux-header-search.cpp

266 ↗

(On Diff #303846)

I'll add some -SAME directives because that helps FileCheck to provide a friendly error message when things go off.

clang/lib/Driver/ToolChains/Linux.cpp

     return Triple.isArch32Bit() ? "lib" : "lib64";
   }
 
-  // It happens that only x86 and PPC use the 'lib32' variant of oslibdir, and
-  // using that variant while targeting other architectures causes problems
+  // It happens that only x86, PPC and SPARC use the 'lib32' variant of oslibdir,
+  // and using that variant while targeting other architectures causes problems
   // because the libraries are laid out in shared system roots that can't cope
   // with a 'lib32' library search path being considered. So we only enable
   // them when we know we may need it.

   // reasoning about oslibdir spellings with the lib dir spellings in the
   // GCCInstallationDetector, but that is a more significant refactoring.
   if (Triple.getArch() == llvm::Triple::x86 ||
-      Triple.getArch() == llvm::Triple::ppc)
+      Triple.getArch() == llvm::Triple::ppc ||
+      Triple.getArch() == llvm::Triple::sparc)
     return "lib32";
 
   if (Triple.getArch() == llvm::Triple::x86_64 &&