This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Fix Hurd toolchain class hierarchy
ClosedPublic

Authored by aganea on Feb 28 2020, 11:08 AM.

Details

Summary

As noted in PR45061, a two-stage ThinLTO build fails the clang/test/Driver/hurd.c test because of a static_cast here which wrongly converts an instance of clang::driver::toolchains::Hurd into that of clang::driver::toolchains::Linux. ThinLTO will later devirtualize the ToolChain.getDynamicLinker(Args); call and use Linux::getDynamicLinker() instead, causing the test to generate a wrong -dynamic-linker linker flag (/lib/ld-linux.so.2 instead of /lib/ld.so)

Ideally, and because of the static_cast mentioned above, maybe gnutools::Linker should take a const Generic_ELF & instance, not a const ToolChain &. But that causes other issues and could be done later.

Diff Detail

Event Timeline

aganea created this revision.Feb 28 2020, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2020, 11:08 AM

@hans, do you think this could be included in 10.0?

aganea updated this revision to Diff 247400.Feb 28 2020, 7:20 PM

Fix clang-formatting.

aganea updated this revision to Diff 247542.Mar 1 2020, 7:36 PM
aganea edited the summary of this revision. (Show Details)

The patch did not make sense conceptually. Hurd is not Linux. I think now it makes more sense.

hans added a comment.Mar 2 2020, 12:44 AM

@hans, do you think this could be included in 10.0?

I think it's too late in the process for this, maybe it can be merged for 10.0.1.

kristina accepted this revision.Mar 2 2020, 12:49 AM

The patch did not make sense conceptually. Hurd is not Linux. I think now it makes more sense.

Yes this seems to make more sense now. LGTM.

This revision is now accepted and ready to land.Mar 2 2020, 12:49 AM
This revision was automatically updated to reflect the committed changes.