Page MenuHomePhabricator

[Patch][Profile] add “--dependent-lib= libclang_rt.profile-x86_64.a” to the CC1 command line when enabling code coverage on PS4
ClosedPublic

Authored by MaggieYi on Dec 4 2015, 2:24 AM.

Details

Summary

Dear All,

This patch adds “--dependent-lib= libclang_rt.profile-x86_64.a” to the compiler command line when enabling code coverage on the PS4 target. A previous PS4 patch (http://reviews.llvm.org/rL250293) added -l libclang_rt.profile-x86_64.a to the linker command line, however, this meant the user needed to add –l when running the linker directly. This patch will improve the user workflow.

Please let me know if this is an acceptable patch.

Regards,

Ying Yi
SN Systems Ltd - Sony Computer Entertainment Group.

Diff Detail

Repository
rL LLVM

Event Timeline

MaggieYi updated this revision to Diff 41850.Dec 4 2015, 2:24 AM
MaggieYi retitled this revision from to [Patch][Profile] add “--dependent-lib= libclang_rt.profile-x86_64.a” to the CC1 command line when enabling code coverage on PS4.
MaggieYi updated this object.
MaggieYi added a subscriber: cfe-commits.
vsk added a subscriber: vsk.Dec 11 2015, 8:58 AM

Thanks, comments inline --

lib/Driver/Tools.cpp
4064 ↗(On Diff #41850)

Profiling instrumentation seems orthogonal to PIC. Could you keep the calls to addPS4ProfileRTArgs in their original locations?

test/Driver/ps4-runtime-flags.c
8 ↗(On Diff #41850)

This tests whether ArgList::hasFlag() works. It doesn't really stress your patch. Please omit it.

10 ↗(On Diff #41850)

^ Ditto.

11 ↗(On Diff #41850)

Looks identical to line 9. Unless I'm missing something, this should be removed.

14 ↗(On Diff #41850)

Omit?

16 ↗(On Diff #41850)

This one too.

22 ↗(On Diff #41850)

And this.

23 ↗(On Diff #41850)

Typo: -fcoverage-mappinge

24 ↗(On Diff #41850)

The rest of these feel a bit redundant, but I haven't looked too closely.

Thanks for reviewing the patch. I try to answer your questions, could you please let me know if this makes sense to you?

Regards,

lib/Driver/Tools.cpp
4064 ↗(On Diff #41850)

Because this patch is adding “--dependent-lib= libclang_rt.profile-x86_64.a” to the compiler command line (CC1), instead of adding –l to the linker command. I had to move the function “addPS4ProfileRTArgs” call from “ConstructPS4LinkJob” to the “ConstructJob” function.

test/Driver/ps4-runtime-flags.c
8 ↗(On Diff #41850)

I believe these do test my patch. My patch has changed the code from “Args.hasArg(options::…)” to “Args.hasFlag(options:: …)”. This changes the behavior from simply requiring a switch (hasArg) to requiring the switches in the correct order (hasFlag).

10 ↗(On Diff #41850)

See response to line 8.

11 ↗(On Diff #41850)

Thanks. I will remove this line and upload a new patch.

14 ↗(On Diff #41850)

See response to line 8.

16 ↗(On Diff #41850)

See response to line 8.

22 ↗(On Diff #41850)

See response to line 8.

23 ↗(On Diff #41850)

Thanks. I will upload a new patch to fix this.

24 ↗(On Diff #41850)

See response to line 8.

MaggieYi updated this revision to Diff 42711.Dec 14 2015, 6:35 AM

Following Vedant's comments, two test issues have been fixed.

vsk added inline comments.Dec 14 2015, 8:40 AM
lib/Driver/Tools.cpp
4067 ↗(On Diff #42711)

Sorry, I don't know why I thought this was in ParsePIC. This seems fine.

test/Driver/ps4-runtime-flags.c
9 ↗(On Diff #42711)

Got it, I see that there's a behavior change. My point was that it isn't necessary to test every combination in {-fX, -fY, -fno-X, -fno-Y, ...} x {-fX, -fY, -fno-X, -fno-Y, ...} to be sure the code works. Most users of hasFlag() aren't tested in this way since it'd cause a test case explosion.

I think lines 7-10 are all that's really needed to test the change from hasArg to hasFlag in your patch. What do you think?

My patch changes 6 compiler flags, which are –coverage, -fprofile-arcs, -fprofile-generate, -fprofile-generate=, -fprofile-instr-generate, -fprofile-instr-generate=. I would like to keep line 7-10 in order to verify the change to using “hasFlag” instead of “hasArg”. For the other switches, I could simplify the tests to only check “–fx” and “–fno-x” in order to cut down on test proliferation.

Do you think that would be acceptable?

vsk added a comment.Dec 15 2015, 7:54 AM

Sure! Thanks.

MaggieYi updated this revision to Diff 42874.Dec 15 2015, 10:16 AM

Thanks, I have updated the test following your comments.

vsk accepted this revision.Dec 15 2015, 1:49 PM
vsk added a reviewer: vsk.

Lgtm

This revision is now accepted and ready to land.Dec 15 2015, 1:49 PM

Could someone commit it for me please (as I do not have commit access)? Thanks

This revision was automatically updated to reflect the committed changes.
probinson edited edge metadata.Dec 16 2015, 9:31 AM

Could someone commit it for me please (as I do not have commit access)? Thanks

Done. Thanks Maggie!

Thanks Paul for your help.