This is an archive of the discontinued LLVM Phabricator instance.

Disable LibFuncs for stpcpy and stpncpy for Android < 21
ClosedPublic

Authored by lanza on Aug 4 2021, 4:01 PM.

Details

Summary

These functions don't exist in android API levels < 21. A change in
llvm-12 (rG6dbf0cfcf789) caused Oz builds to emit this symbol
assuming it's available and thus is causing link errors. Simply
disable the LibFunc here.

Diff Detail

Event Timeline

lanza created this revision.Aug 4 2021, 4:01 PM
lanza requested review of this revision.Aug 4 2021, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2021, 4:01 PM

Might be helpful to mention rG6dbf0cfcf789 in the commit message.

Is there any way to add a test to this (especially for the triple version conditional), similar to what rGc5638a71d805 did? I imagine it might be complicated because I think our original test case triggered two transforms (sprintf -> strcpy and then strcpy -> stpcpy), but hopefully there's some test for the stpcpy transform that we can piggy-back off.

EDIT: looks like the test modified by rGc5638a71d805 also has segments for the stpcpy transform, so we should just be able to use it.

Adding a bunch of Android folks as reviewers.

lanza updated this revision to Diff 364297.Aug 4 2021, 5:13 PM

Add test

lanza edited the summary of this revision. (Show Details)Aug 4 2021, 5:15 PM
smeenai added inline comments.Aug 4 2021, 5:16 PM
llvm/test/Transforms/InstCombine/sprintf-1.ll
113–118

This is identical to the Windows case, right? Can we just combine the cases and use a more generic label (e.g. NOSTPCPY or something)?

You should also add a test using an Android 21 triple and verify that the stpcpy transform triggers for it.

rprichard accepted this revision.Aug 4 2021, 6:29 PM
llvm/lib/Analysis/TargetLibraryInfo.cpp
595

I assume the problem is that LibFunc_stpcpy is available by default, and we need to override the default, marking it unavailable.

I wondered why the *_unlocked functions (D45736, D75933) seemed to assume the opposite default, but that's because there's code at the start of this function marking them unavailable. It took me a while to find it because LibFunc_getc_unlocked isn't initialized. (LibFunc_getchar_unlocked and various others are, though.) I'll upload a patch if it still looks broken after digging some more.

This revision is now accepted and ready to land.Aug 4 2021, 6:29 PM
This revision was automatically updated to reflect the committed changes.
lanza added a subscriber: tstellar.Aug 4 2021, 8:01 PM

@tstellar This could get picked to llvm-13. It's a bug that was first found with us rolling out llvm-12 internally and is still present in 13.

@lanza Can you file a bug for this and but release-13.0.0 in the blocks field.

probinson added inline comments.
llvm/test/Transforms/InstCombine/sprintf-1.ll
113–118

Yes, please make this a generic label; PS4 is going to have to do the same thing, we just had somebody trip over it. (Not asking you to do the PS4 change, just make the test easier for us to add our case later.)

probinson added inline comments.Aug 6 2021, 6:28 AM
llvm/test/Transforms/InstCombine/sprintf-1.ll
113–118

Ah nvm somehow hadn't noticed this already landed. We'll deal with it.

smeenai added inline comments.Aug 6 2021, 9:25 AM
llvm/test/Transforms/InstCombine/sprintf-1.ll
113–118

This was addressed post-commit by https://reviews.llvm.org/D107526.