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

Rainer Orth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 13 04:36:12 PST 2020


ro resigned from this revision.
ro added a comment.

In D90524#2393353 <https://reviews.llvm.org/D90524#2393353>, @glaubitz wrote:

> In D90524#2393319 <https://reviews.llvm.org/D90524#2393319>, @ro wrote:
>
>> In D90524#2388214 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D85582>.  Do as you like, I'm out of this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90524/new/

https://reviews.llvm.org/D90524



More information about the cfe-commits mailing list