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

[Ying Yi via cfe-commits]

MaggieYi added a comment.

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

Regards,


================
Comment at: lib/Driver/Tools.cpp:4064
@@ -4046,1 +4063,3 @@
 
+  // Add runtime flag for PS4 when PGO or Coverage are enabled.
+  if (getToolChain().getTriple().isPS4CPU())
----------------
vsk wrote:
> Profiling instrumentation seems orthogonal to PIC. Could you keep the calls to `addPS4ProfileRTArgs` in their original locations?
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.

================
Comment at: test/Driver/ps4-runtime-flags.c:8
@@ +7,3 @@
+// 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
----------------
vsk wrote:
> This tests whether ArgList::hasFlag() works. It doesn't really stress your patch. Please omit it.
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).

================
Comment at: test/Driver/ps4-runtime-flags.c:10
@@ +9,3 @@
+// 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 -fno-profile-arcs %s -### 2>&1 | FileCheck -check-prefix=CHECK-PS4-NO-PROFILE %s
----------------
vsk wrote:
> ^ Ditto.
See response to line 8.

================
Comment at: test/Driver/ps4-runtime-flags.c:11
@@ +10,3 @@
+// 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 -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
----------------
vsk wrote:
> Looks identical to line 9. Unless I'm missing something, this should be removed.
Thanks. I will remove this line and upload a new patch.

================
Comment at: test/Driver/ps4-runtime-flags.c:14
@@ +13,3 @@
+// 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 -fno-profile-generate -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
----------------
vsk wrote:
> Omit?
See response to line 8.

================
Comment at: test/Driver/ps4-runtime-flags.c:16
@@ +15,3 @@
+// 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 -fno-profile-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-generate -fno-profile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s
----------------
vsk wrote:
> This one too.
See response to line 8.

================
Comment at: test/Driver/ps4-runtime-flags.c:22
@@ +21,3 @@
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate=somefile.profraw %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fno-profile-instr-generate -fprofile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate -fcoverage-mappinge %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
----------------
vsk wrote:
> And this.
See response to line 8.

================
Comment at: test/Driver/ps4-runtime-flags.c:23
@@ +22,3 @@
+// RUN: %clang -target x86_64-scei-ps4 -fno-profile-instr-generate -fprofile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-PROFILE %s
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate -fcoverage-mappinge %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
----------------
vsk wrote:
> Typo: `-fcoverage-mappinge`
Thanks. I will upload a new patch to fix this.

================
Comment at: test/Driver/ps4-runtime-flags.c:24
@@ +23,3 @@
+// RUN: %clang -target x86_64-scei-ps4 -fprofile-instr-generate -fcoverage-mappinge %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 -fno-profile-instr-generate %s -### 2>&1 | FileCheck --check-prefix=CHECK-PS4-NO-PROFILE %s
----------------
vsk wrote:
> The rest of these feel a bit redundant, but I haven't looked too closely.
See response to line 8.


http://reviews.llvm.org/D15222