This is an archive of the discontinued LLVM Phabricator instance.

[driver] Search for compatible Android runtime directories
ClosedPublic

Authored by smeenai on Aug 21 2023, 6:55 PM.

Details

Summary

Android triples include a version number, which makes direct triple
comparisons for per-target runtime directory searching not always work.
Instead, look for the triple with the highest compatible version number
and use that per-target runtime directory instead. This maintains the
existing fallback to a triple without any version number, but I'm hoping
we can remove that in the future. https://discourse.llvm.org/t/62717
discusses this further.

The one remaining triple mismatch after this is that Android armv7
triples usually have an environment of androideabi, which Clang
normalizes to android. If you use the androideabi triple when
building the runtimes with a per-target runtimes dir, the directory will
get created with androideabi in its name, but Clang's triple search
uses the normalized triple and will look for an android directory
instead. https://reviews.llvm.org/D140925 will fix that by normalizing
triples when creating the per-target runtimes directories as well.

Diff Detail

Event Timeline

smeenai created this revision.Aug 21 2023, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 6:55 PM
smeenai requested review of this revision.Aug 21 2023, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 6:55 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay added a comment.EditedAug 21 2023, 7:37 PM

// Android target triples contain a target version. If we don't have libraries for the exact target version, we should fall back to the next newest version or a versionless path, if any.

An Android maintainer can override my opinion but my feeling is that this ad-hoc rule probably should be the responsibility of the file hierarchy instead of clangDriver. This really easy to cause mix-and-match situations where the problem is on the user side but the user may blame Clang.

This can be implemented by adding symlinks in the file hierarchy.

// Android target triples contain a target version. If we don't have libraries for the exact target version, we should fall back to the next newest version or a versionless path, if any.

An Android maintainer can override my opinion but my feeling is that this ad-hoc rule probably should be the responsibility of the file hierarchy instead of clangDriver. This really easy to cause mix-and-match situations where the problem is on the user side but the user may blame Clang.

This can be implemented by adding symlinks in the file hierarchy.

That was one of the alternatives I brought up in https://discourse.llvm.org/t/handling-version-numbers-in-per-target-runtime-directories/62717, but symlinks are complicated on Windows, and you'd also have to add build system code to create those symlinks, so I don't think it'll be a clear win.

Are you concerned with falling back to the next newest version or the versionless path (or both)? I think the next newest version is pretty inline with general OS compatibility: if you don't have version N, look for version N - 1, N - 2, etc (since they're forward compatible, see also https://discourse.llvm.org/t/handling-version-numbers-in-per-target-runtime-directories/62717/13). The versionless path is a little less principled IMO (though I guess you could see it as implicitly being compatible with any version) ... maybe we could get rid of that if Chrome is okay moving to a versioned directory.

(Aside: why do we need to produce runtime libraries for different versions in the first place? We want one for the minimum OS version we support, of course, but then there's important additions like version 29 adding ELF TLS support and version 31 adding a thread properties API used by LSAN, so building libraries targeting those important versions and having them be dynamically selected based on your target OS version is super useful. We could build the runtime libraries for every single OS version we wanted to support, but that's a lot of versions (at least 21 through 31, times four architectures), which is wasteful for both build times and toolchain distribution size.)

I think this is a useful feature, for the reasons mentioned on the thread.

Since this is a superset of D115049 (... right?), this should probably revert the changes over there?

Maybe we should dump searched paths in -v mode?

I think this is a useful feature, for the reasons mentioned on the thread.

Since this is a superset of D115049 (... right?), this should probably revert the changes over there?

Wouldn't it break Chrome if I reverted those changes as part of this diff? I was thinking I could land this, Chrome could move over to a versioned dir (assuming that works for it), and then we could undo D115049. I'm happy to make the revert as part of this change if it'll work out for Chrome though.

Maybe we should dump searched paths in -v mode?

As in output all the versioned directories that were found and considered?

This approach LGTM, assuming that https://reviews.llvm.org/D140925 lands as well to ensure that the triple does use androideabi, since it would prevent custom build setups from needing additional help.

(Aside: why do we need to produce runtime libraries for different versions in the first place? We want one for the minimum OS version we support, of course, but then there's important additions like version 29 adding ELF TLS support and version 31 adding a thread properties API used by LSAN, so building libraries targeting those important versions and having them be dynamically selected based on your target OS version is super useful. We could build the runtime libraries for every single OS version we wanted to support, but that's a lot of versions (at least 21 through 31, times four architectures), which is wasteful for both build times and toolchain distribution size.)

I don't think we need all the versions, but today we really only use 21 (lowest level), and 30 (a conservative level for the platform that supports all our apexes). Ideally we'd also pick up the latest platform version, and we probably should explicitly call out 29 for ELF TLS, so that would be ~4 supported levels, but maybe we can use 29 instead of 30 to cut it to ~3 for most architectures.

srhines added a subscriber: pirama.Aug 23 2023, 1:07 AM

This approach LGTM, assuming that https://reviews.llvm.org/D140925 lands as well to ensure that the triple does use androideabi, since it would prevent custom build setups from needing additional help.

@phosek is working on that one, but I think this is useful even without it, because you just need to use android in your triple when building the runtimes, and can use androideabi everywhere else. I'll put up an example runtimes build setup with that to demonstrate.

(Aside: why do we need to produce runtime libraries for different versions in the first place? We want one for the minimum OS version we support, of course, but then there's important additions like version 29 adding ELF TLS support and version 31 adding a thread properties API used by LSAN, so building libraries targeting those important versions and having them be dynamically selected based on your target OS version is super useful. We could build the runtime libraries for every single OS version we wanted to support, but that's a lot of versions (at least 21 through 31, times four architectures), which is wasteful for both build times and toolchain distribution size.)

I don't think we need all the versions, but today we really only use 21 (lowest level), and 30 (a conservative level for the platform that supports all our apexes). Ideally we'd also pick up the latest platform version, and we probably should explicitly call out 29 for ELF TLS, so that would be ~4 supported levels, but maybe we can use 29 instead of 30 to cut it to ~3 for most architectures.

Ah, yeah, https://reviews.llvm.org/D85927 dynamically detects the thread properties API, so we don't need an explicit build for 31. An explicit build with ELF TLS is still required though.

D158798 demonstrates a working Android runtimes setup with per-target runtime directories once this change is in place. It'll be nicer once D140925 lands, but it's workable even without that.

smeenai updated this revision to Diff 555086.Aug 31 2023, 9:23 AM

Kick off pre-merge checks

@thakis I'm inclined to leave the fallback for a triple without any version in place for now, since it seems like Mozilla might be relying on it too, and we can remove it once everyone's moved over to the new fallback mechanism. What do you think?

Works for me. How do you imagine the transition happening? Should we emit some kind of warning if the old fallback is hit?

Works for me. How do you imagine the transition happening? Should we emit some kind of warning if the old fallback is hit?

That's a good call. I'll add that plus a release note.

smeenai updated this revision to Diff 555213.Aug 31 2023, 5:59 PM

Add warning and release note

Ping. I added the warning and release note. I haven't made search paths be dumped in -v mode because I thought it could be pretty noisy (especially since you'll be doing three searches, for resource dir libraries, libc++ per-target headers, and non-resource dir libraries), but I'm happy to do so if people think it'll be useful (or add it under a different flag).

phosek accepted this revision.Sep 18 2023, 1:20 AM

LGTM

This revision is now accepted and ready to land.Sep 18 2023, 1:20 AM
pirama accepted this revision.Sep 18 2023, 2:46 PM

LGTM from the Android platform/NDK side.

MaskRay accepted this revision.Sep 18 2023, 4:23 PM

nits

clang/docs/ReleaseNotes.rst
349

--target=

clang/test/Driver/android-unversioned-fallback-warning.cpp
15

--target=

-target has been deprecated since Clang 3.4

nits

Sorry, this came in right as I committed the diff. I pushed rG915ebb07dfc53486eccf0dc09b6411929a463e98 to address it.