This is an archive of the discontinued LLVM Phabricator instance.

Refactor fortified (__*_chk) library function handling in TLI/LibCallSimplifier.
AbandonedPublic

Authored by ab on Oct 24 2014, 11:49 AM.

Details

Summary

While working on applying an ancient patch (http://reviews.llvm.org/D498), I came across a lot of weirdness in the handling of fortified libcalls. First, they are checked by name rather than using the TLI enum (but one of them was there). Second, the code that simplifies the fortified libcalls into their non-checking counterparts (when the check is useless) is duplicated in both CGP and SimplifyLibCalls.
This series of patches tries to clean it up a bit.

[1/4] Add fortified (__*_chk) library functions to TLI.

This shouldn't have any functional change.
Note that the fortified libfuncs are now part of TLI, but are always
available, because they aren't generated, only optimized into the
non-checking versions.

Diff Detail

Event Timeline

ab updated this revision to Diff 15425.Oct 24 2014, 11:49 AM
ab updated this revision to Diff 15426.
ab retitled this revision from to Refactor fortified (__*_chk) library function handling in TLI/LibCallSimplifier..
ab updated this object.
ab edited the test plan for this revision. (Show Details)
ab added a subscriber: Unknown Object (MLST).

[2/4] [SimplifyLibCalls] Provide a way to only lower fortified calls.

There are currently two versions of the fortified libcall lowering:
this one, and a duplicate in CGP. The former is more general, with the
latter only lowering fortified libcalls to their non-fortified
instrinsic counterparts.

This patch enables both to be merged, by providing a way to
LibCallSimplifier clients to only enable fortified call lowering.

ab updated this revision to Diff 15430.Oct 24 2014, 11:51 AM

[3/4] [CodeGenPrep] Remove LibCallSimplifier duplicate.

A previous patch enabled the OnlyLowerFortified option in the
LibCallSimplifier. This lets us remove the CGP-specific duplicate.

ab updated this revision to Diff 15431.Oct 24 2014, 11:51 AM

[4/4] Fix stpcpy support when building st*cpy libcalls.

Check that the stpcpy/stpncpy libcalls are actually available
before generating them.
Also, set the correct name on the return value rather than always
strncpy.

ab added subscribers: hfinkel, beanz.
ab added a comment.Oct 31 2014, 1:41 PM

Ping! It's (relatively) straightforward refactoring, split into nice and clean commits.

echristo requested changes to this revision.Oct 31 2014, 5:53 PM
echristo added a reviewer: echristo.
echristo added a subscriber: echristo.

Can we not have 4 patches all merged here? How about 4 separate reviews with the 4 patches?

Thanks!

-eric

This revision now requires changes to proceed.Oct 31 2014, 5:53 PM
ab added a comment.Oct 31 2014, 5:58 PM

They're actually separate (each diff has to be chosen manually though, in the revision update history thingy). I prefer that to the alternative (having differentials for many small related patches.)
I can still separate them, though!
-Ahmed

I don't seem to be able to respond to each diff separately?

I'll work through the interface, it'll just take a little longer.

ab added a comment.Oct 31 2014, 6:12 PM

Right, that's the problem; sorry about that.
In the future, if you have a better solution for this kind of small patch series I'd be glad to do that instead; I guess multiple revisions are at least less clunky.

-Ahmed

ab updated this revision to Diff 15945.Nov 7 2014, 4:46 PM
ab edited edge metadata.

Reworked the patch series into just factoring out the fortified stuff into a separate class.
So this now depends on D6179. The rest (tiny NFC stuff) I'll commit directly once these two are upstream.

If it's too unwieldy to review I think it's feasible to split it into three patches.

ab added a comment.Nov 7 2014, 5:28 PM

Next in the how-to-review-many-small-patches saga, my local commit log:

Factor out signature checks for fortifiable libcalls: http://reviews.llvm.org/differential/diff/15947/
Cleanup size_t Type* retrieval: http://reviews.llvm.org/differential/diff/15948/
Factor out the string/memory optimizations: http://reviews.llvm.org/differential/diff/15949/
Factor out the fortified libcall optimizations: http://reviews.llvm.org/differential/diff/15950/
Remove the CGP LibCallSimplifier duplicate: http://reviews.llvm.org/differential/diff/15951/

As is probably obvious by all the churn I'm still not a big fan of making a revision for every single one of them.

ab abandoned this revision.Dec 4 2014, 2:09 PM

My experiment with multiple patches in a single phab revision was a failure: I created separate revisions as D6539, D6540, D6541.