This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Disable calls to *_finite and other glibc-only functions on Android.
ClosedPublic

Authored by chh on Jan 29 2018, 3:43 PM.

Details

Summary

Since r322087, glibc's finite lib calls are generated when possible.
However, they are not supported on Android. This change also
disables other functions not available on Android.

Diff Detail

Event Timeline

chh created this revision.Jan 29 2018, 3:43 PM
efriedma added inline comments.
lib/Analysis/TargetLibraryInfo.cpp
413

Instead of going through this process repeatedly, could we just change this line to "if (!T.isOSLinux() || T.isAndroid())"?

srhines added inline comments.Jan 30 2018, 1:04 AM
lib/Analysis/TargetLibraryInfo.cpp
413

I agree that we should just be nuking all of the *_finite() functions, but many of these other functions are indeed available on Android, so we can't just change this single guard here.

Chih-hung, we should follow up with the bionic folks to ensure that these functions are available for all NDK platforms too. In cases like fseeko64/etc., where it only appears in API level 24, it also wouldn't be correct to lower to any of these libcalls for a target API level < 24.

minseong.kim added inline comments.Jan 30 2018, 4:10 AM
lib/Analysis/TargetLibraryInfo.cpp
413

In terms of having the separate list of unsupported libcalls for Android, it looks reasonable to me as the two lists of libcalls might not be exactly the same now and in the future.

chh updated this revision to Diff 132064.Jan 30 2018, 3:28 PM
chh retitled this revision from [Analysis] Disable log/log2/log10 finite lib calls on Android with -ffast-math. to [Analysis] Disable calls to *_finite and other glibc-only functions on Android..
chh edited the summary of this revision. (Show Details)
chh marked 3 inline comments as done.
chh added inline comments.
lib/Analysis/TargetLibraryInfo.cpp
413

I keep only the memalign function, which is available on Android.

efriedma added inline comments.Jan 30 2018, 3:38 PM
lib/Analysis/TargetLibraryInfo.cpp
414

Maybe instead of !T.isOSLinux() || T.isAndroid(), this should be !T.isOSLinux() || !T.isGNUEnvironment(), so it's clear this is the set of functions specifically available on glibc?

421

If would probably be more clear to write this as "if (!T.isAndroid())". Or maybe move this out into its own if statement.

chh added inline comments.Jan 30 2018, 6:42 PM
lib/Analysis/TargetLibraryInfo.cpp
414

I tried to use !T.isGNUEnvironment() instead of T.isAndroid().
The Transforms/InferFunctionAttrs/annotate.ll test failed because it expects x86_64-unknown-linux to be treated as isOSLinux() here.
I am not sure which clang targets use unknown-linux plus glibc.
Shall we only limit Android target for now?

efriedma added inline comments.Jan 30 2018, 6:44 PM
lib/Analysis/TargetLibraryInfo.cpp
414

Oh, didn't think of that.

Sure, let's limit to Android for now.

chh updated this revision to Diff 132209.Jan 31 2018, 10:29 AM
chh marked an inline comment as done.
This revision is now accepted and ready to land.Jan 31 2018, 10:32 AM
This revision was automatically updated to reflect the committed changes.