Page MenuHomePhabricator

[Analysis] Validate the return type of s(n)printf libcalls
ClosedPublic

Authored by mstorsjo on May 11 2018, 5:48 AM.

Details

Summary

If the sprintf function is static, earlier optimization passes could optimize out the return value altogether, and make it void, which could break optimizations of this libcall that touch the return value.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.May 11 2018, 5:48 AM

I haven't looked in detail at the failure or discussion in D46285 or https://bugs.llvm.org/show_bug.cgi?id=37408 , so I might be missing something.
Why are we getting this far in the 1st place? Shouldn't we have avoided this problem in TargetLibraryInfoImpl::isValidProtoForLibFunc() by checking that the return type is wrong?

I haven't looked in detail at the failure or discussion in D46285 or https://bugs.llvm.org/show_bug.cgi?id=37408 , so I might be missing something.
Why are we getting this far in the 1st place? Shouldn't we have avoided this problem in TargetLibraryInfoImpl::isValidProtoForLibFunc() by checking that the return type is wrong?

Indeed, that's also doable - I'm not familiar with this layer at all so I didn't know about that check. The existing checks for sprintf/snprintf don't check the return type, but by adding a return type check there, it also works fine.

mstorsjo updated this revision to Diff 146320.May 11 2018, 7:00 AM
mstorsjo retitled this revision from [InstCombine] Validate the assumption about return type in optimizeSnPrintFString to [Analysis] Validate the return type of s(n)printf libcalls.
mstorsjo edited the summary of this revision. (Show Details)
spatel added inline comments.May 11 2018, 7:11 AM
lib/Analysis/TargetLibraryInfo.cpp
696 ↗(On Diff #146320)

Is it possible to make this check stronger:
FTy.getReturnType()->isIntegerTy(32) ?

test/Transforms/InstCombine/sprintf-void.ll
13 ↗(On Diff #146320)

I always make this suggestion for instcombine tests: it's better to use the script at utils/update_test_checks.py to auto-generate the CHECK lines than to manually write them.

mstorsjo added inline comments.May 11 2018, 7:59 AM
lib/Analysis/TargetLibraryInfo.cpp
696 ↗(On Diff #146320)

I guess that'd be ok as well, although it would disqualify any ILP64 platform. But I guess that's just a theoretical concern, given the number of ILP64 platforms.

test/Transforms/InstCombine/sprintf-void.ll
13 ↗(On Diff #146320)

Ok, will do.

mstorsjo updated this revision to Diff 146332.May 11 2018, 8:10 AM

Made the requested adjustments.

mstorsjo added inline comments.May 11 2018, 8:11 AM
lib/Analysis/TargetLibraryInfo.cpp
696 ↗(On Diff #146320)

I see that other existing checks for things that should be int also uses isIntegerTy(32), so I changed it as requested.

spatel accepted this revision.May 11 2018, 8:38 AM

LGTM - thanks!
Note (future patches): just glancing at the surrounding cases, it seems like we're missing similar return type checks for other calls.

This revision is now accepted and ready to land.May 11 2018, 8:38 AM

LGTM - thanks!
Note (future patches): just glancing at the surrounding cases, it seems like we're missing similar return type checks for other calls.

Yup. Although I guess it's mostly a question of what optimizations require what details to match; for this particular optimization, it could have been made to handle the case of a void return as well (but it's not necessarily worth to).

xbolva00 accepted this revision.May 11 2018, 8:56 AM
xbolva00 added a comment.EditedMay 11 2018, 9:49 AM

After you merge this, revert also https://reviews.llvm.org/rL332043

This revision was automatically updated to reflect the committed changes.

I commited it again, you do not need to revert your revert.

I'm not convinced this really solves the problem correctly... I mean, obviously we shouldn't crash, but this doesn't deal with the underlying issue: once we transform the internal function, it is no longer the C library function, and that difference may not be reflected in the signature. See http://lists.llvm.org/pipermail/llvm-dev/2017-October/118578.html .

I commited it again, you do not need to revert your revert.

Thanks - the codebase that triggered the error seems to build fine now still with this reapplied.

I'm not convinced this really solves the problem correctly... I mean, obviously we shouldn't crash, but this doesn't deal with the underlying issue: once we transform the internal function, it is no longer the C library function, and that difference may not be reflected in the signature. See http://lists.llvm.org/pipermail/llvm-dev/2017-October/118578.html .

Hm, indeed, there's clearly more aspects to it. I take it there was no further progress since, on the ideas presented there?

No, no progress on the question of the semantics of library functions with internal linkage. (It usually isn't a problem in practice because the functions in question are built with -fno-builtin, so nobody has really spent any time.)