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

[MaggieYi]

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.

[MaggieYi]

Ping

[vsk]

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.

[MaggieYi]

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]

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

[vsk]

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?

[MaggieYi]

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]

Sure! Thanks.

[MaggieYi]

Thanks, I have updated the test following your comments.

[vsk]

Lgtm

[MaggieYi]

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

[probinson]

In D15222#311896, @MaggieYi wrote:

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

Done. Thanks Maggie!

[MaggieYi]

Thanks Paul for your help.

cfe/trunk/lib/Driver/Tools.cpp

if (Output.isFilename()) {
           CoverageFilename.swap(Pwd);
         }
       }
       CmdArgs.push_back(Args.MakeArgString(CoverageFilename));
     }
   }
 }
 
+static void addPS4ProfileRTArgs(const ToolChain &TC, const ArgList &Args,
+                                ArgStringList &CmdArgs) {
+  if ((Args.hasFlag(options::OPT_fprofile_arcs, options::OPT_fno_profile_arcs,
+                    false) ||
+       Args.hasFlag(options::OPT_fprofile_generate,
+                    options::OPT_fno_profile_instr_generate, false) ||
+       Args.hasFlag(options::OPT_fprofile_generate_EQ,
+                    options::OPT_fno_profile_instr_generate, false) ||
+       Args.hasFlag(options::OPT_fprofile_instr_generate,
+                    options::OPT_fno_profile_instr_generate, false) ||
+       Args.hasFlag(options::OPT_fprofile_instr_generate_EQ,
+                    options::OPT_fno_profile_instr_generate, false) ||
+       Args.hasArg(options::OPT_fcreate_profile) ||
+       Args.hasArg(options::OPT_coverage)))
+    CmdArgs.push_back("--dependent-lib=libclang_rt.profile-x86_64.a");
+}
+
 /// Parses the various -fpic/-fPIC/-fpie/-fPIE arguments.  Then,
 /// smooshes them together with platform defaults, to decide whether
 /// this compile should be using PIC mode or not. Returns a tuple of
 /// (RelocationModel, PICLevel, IsPIE).
 static std::tuple<llvm::Reloc::Model, unsigned, bool>
 ParsePICArgs(const ToolChain &ToolChain, const llvm::Triple &Triple,
              const ArgList &Args) {
   // FIXME: why does this code...and so much everywhere else, use both
#endif
   if (!Args.hasFlag(options::OPT_funique_section_names,
                     options::OPT_fno_unique_section_names, true))
     CmdArgs.push_back("-fno-unique-section-names");
 
   Args.AddAllArgs(CmdArgs, options::OPT_finstrument_functions);
 
   addPGOAndCoverageFlags(C, D, Output, Args, CmdArgs);
 
+  // Add runtime flag for PS4 when PGO or Coverage are enabled.
+  if (getToolChain().getTriple().isPS4CPU())
+    addPS4ProfileRTArgs(getToolChain(), Args, CmdArgs);
+
   // Pass options for controlling the default header search paths.
   if (Args.hasArg(options::OPT_nostdinc)) {
     CmdArgs.push_back("-nostdsysteminc");
     CmdArgs.push_back("-nobuiltininc");
   } else {
     if (Args.hasArg(options::OPT_nostdlibinc))
       CmdArgs.push_back("-nostdsysteminc");
     Args.AddLastArg(CmdArgs, options::OPT_nostdincxx);
void PS4cpu::Assemble::ConstructJob(Compilation &C, const JobAction &JA,
   assert(Input.isFilename() && "Invalid input.");
   CmdArgs.push_back(Input.getFilename());
 
   const char *Exec =
       Args.MakeArgString(getToolChain().GetProgramPath("ps4-as"));
   C.addCommand(llvm::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
 }
 
-static void AddPS4ProfileRT(const ToolChain &TC, const ArgList &Args,
-                            ArgStringList &CmdArgs) {
-  if (!(Args.hasFlag(options::OPT_fprofile_arcs, options::OPT_fno_profile_arcs,
-                     false) ||
-        Args.hasArg(options::OPT_fprofile_generate) ||
-        Args.hasArg(options::OPT_fprofile_instr_generate) ||
-        Args.hasArg(options::OPT_fcreate_profile) ||
-        Args.hasArg(options::OPT_coverage)))
-    return;
-
-  assert(TC.getTriple().isPS4CPU() &&
-         "Profiling libraries are only implemented for the PS4 CPU");
-  CmdArgs.push_back("-lclang_rt.profile-x86_64");
-}
-
 static void AddPS4SanitizerArgs(const ToolChain &TC, ArgStringList &CmdArgs) {
   const SanitizerArgs &SanArgs = TC.getSanitizerArgs();
   if (SanArgs.needsUbsanRt()) {
     CmdArgs.push_back("-lSceDbgUBSanitizer_stub_weak");
   }
   if (SanArgs.needsAsanRt()) {
     CmdArgs.push_back("-lSceDbgAddressSanitizer_stub_weak");
   }
if (Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
     CmdArgs.push_back("--no-demangle");
 
   AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs);
 
   if (Args.hasArg(options::OPT_pthread)) {
     CmdArgs.push_back("-lpthread");
   }
 
-  AddPS4ProfileRT(ToolChain, Args, CmdArgs);
-
   const char *Exec = Args.MakeArgString(ToolChain.GetProgramPath("ps4-ld"));
 
   C.addCommand(llvm::make_unique<Command>(JA, T, Exec, CmdArgs, Inputs));
 }
 
 static void ConstructGoldLinkJob(const Tool &T, Compilation &C,
                                  const JobAction &JA, const InputInfo &Output,
                                  const InputInfoList &Inputs,
static void ConstructGoldLinkJob(const Tool &T, Compilation &C,
   if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nostartfiles)) {
     if (Args.hasArg(options::OPT_shared) || Args.hasArg(options::OPT_pie))
       CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtendS.o")));
     else
       CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtend.o")));
     CmdArgs.push_back(Args.MakeArgString(ToolChain.GetFilePath("crtn.o")));
   }
 
-  AddPS4ProfileRT(ToolChain, Args, CmdArgs);
-
   const char *Exec =
 #ifdef LLVM_ON_WIN32
       Args.MakeArgString(ToolChain.GetProgramPath("ps4-ld.gold.exe"));
 #else
       Args.MakeArgString(ToolChain.GetProgramPath("ps4-ld"));
 #endif
 
   C.addCommand(llvm::make_unique<Command>(JA, T, Exec, CmdArgs, Inputs));

cfe/trunk/test/Driver/ps4-runtime-flags.c

+// REQUIRES: x86-registered-target
+//
+// Test the profile runtime library to be linked for PS4 compiler.
+// Check PS4 runtime flag --dependent-lib which does not append the default library search path.
+//
+// RUN: %clang -target x86_64-scei-ps4 --coverage %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-arcs %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fno-profile-arcs -fprofile-arcs %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fno-profile-arcs %s -### 2>&1 | FileCheck -check-prefix=CHECK-PS4-NO-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-arcs -fno-profile-arcs %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fno-profile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-generate=dir %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fno-profile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate=somefile.profraw %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+//
+// CHECK-PS4-PROFILE: "--dependent-lib=libclang_rt.profile-x86_64.a"
+// CHECK-PS4-NO-PROFILE-NOT: "--dependent-lib=libclang_rt.profile-x86_64.a"