[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles

[Martin Pelikán via Phabricator via cfe-commits]

pelikan accepted this revision.
pelikan added a comment.
This revision is now accepted and ready to land.

Most of my comments are minor enough I'd be OK if this went in.  But please 
consider them before committing.




Comment at: clang/include/clang/Driver/XRayArgs.h:29
   std::vector Modes;
+  XRayInstrSet XRayInstrumentationBundle;
   bool XRayInstrument = false;

Since the class already has "XRay" in its name, I would rename the member to 
just "InstrumentationBundle", just as most of the other members are.



Comment at: clang/include/clang/Frontend/CodeGenOptions.h:111
 
+  enum XRayInstrumentationTypes {
+XRay_Function,

Now I fail to spot where is this enum used.  IIUC this should work even when 
it's not here, as the code uses the things in namespace XRayInstrKind.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:469
 /// AlwaysEmitXRayCustomEvents - Return true if we should emit IR for calls to
 /// the __xray_customevent(...) builin calls, when doing XRay instrumentation.
 bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {

typo: builin -> builtin



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:471
 bool CodeGenFunction::AlwaysEmitXRayCustomEvents() const {
-  return CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents;
+  return CGM.getCodeGenOpts().XRayInstrumentFunctions &&
+ (CGM.getCodeGenOpts().XRayAlwaysEmitCustomEvents ||

I kind of don't like how the "-fxray-instrument" variable is called 
"XRayInstrumentFunctions" because that's not what it means any more.  I think 
in a later diff, we should clean this up.  Or maybe even clean up some of the 
old flags whose functionality has been superseded by this.  But the logic here 
is fine.

Same with the misleading "ShouldXRayInstrumentFunction()" which controls custom 
events too, and not just functions.



Comment at: clang/lib/Driver/XRayArgs.cpp:107-109
+  } else
+D.Diag(clang::diag::err_drv_invalid_value)
+<< "-fxray-instrumentation-bundle=" << P;

nitpick:  I'd rewrite it to 

  if (!Valid) {
D.Diag
continue; // or break or return
  }
  
  

but the current code logic is fine.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:457
+auto Mask = parseXRayInstrValue(B);
+if (Mask == 0)
+  if (B != "none")

did you mean: "(Mask == None)"?


https://reviews.llvm.org/D44970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45570: [XRay] [clang] use compiler-rt's symbol visibility rules

[Martin Pelikán via Phabricator via cfe-commits]

pelikan created this revision.
pelikan added a reviewer: dberris.

Depends on https://reviews.llvm.org/D38993.


Repository:
  rC Clang

https://reviews.llvm.org/D45570

Files:
  lib/Driver/ToolChains/CommonArgs.cpp


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -706,7 +706,8 @@
   return !StaticRuntimes.empty() || !NonWholeStaticRuntimes.empty();
 }
 
-bool tools::addXRayRuntime(const ToolChain&TC, const ArgList &Args, 
ArgStringList &CmdArgs) {
+bool tools::addXRayRuntime(const ToolChain&TC, const ArgList &Args,
+   ArgStringList &CmdArgs) {
   if (Args.hasArg(options::OPT_shared))
 return false;
 
@@ -716,6 +717,10 @@
 for (const auto &Mode : TC.getXRayArgs().modeList())
   CmdArgs.push_back(TC.getCompilerRTArgString(Args, Mode, false));
 CmdArgs.push_back("-no-whole-archive");
+
+SmallString<128> XRay(TC.getCompilerRT(Args, "xray"));
+CmdArgs.push_back(Args.MakeArgString("--dynamic-list=" + XRay + ".syms"));
+CmdArgs.push_back(Args.MakeArgString("--version-script=" + XRay + 
".vers"));
 return true;
   }
 


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -706,7 +706,8 @@
   return !StaticRuntimes.empty() || !NonWholeStaticRuntimes.empty();
 }
 
-bool tools::addXRayRuntime(const ToolChain&TC, const ArgList &Args, ArgStringList &CmdArgs) {
+bool tools::addXRayRuntime(const ToolChain&TC, const ArgList &Args,
+   ArgStringList &CmdArgs) {
   if (Args.hasArg(options::OPT_shared))
 return false;
 
@@ -716,6 +717,10 @@
 for (const auto &Mode : TC.getXRayArgs().modeList())
   CmdArgs.push_back(TC.getCompilerRTArgString(Args, Mode, false));
 CmdArgs.push_back("-no-whole-archive");
+
+SmallString<128> XRay(TC.getCompilerRT(Args, "xray"));
+CmdArgs.push_back(Args.MakeArgString("--dynamic-list=" + XRay + ".syms"));
+CmdArgs.push_back(Args.MakeArgString("--version-script=" + XRay + ".vers"));
 return true;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45570: [XRay] [clang] use compiler-rt's symbol visibility rules

[Martin Pelikán via Phabricator via cfe-commits]

pelikan updated this revision to Diff 142174.
pelikan added a comment.

while there, clang-format the code I touched


Repository:
  rC Clang

https://reviews.llvm.org/D45570

Files:
  lib/Driver/ToolChains/CommonArgs.cpp


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -706,16 +706,20 @@
   return !StaticRuntimes.empty() || !NonWholeStaticRuntimes.empty();
 }
 
-bool tools::addXRayRuntime(const ToolChain&TC, const ArgList &Args, 
ArgStringList &CmdArgs) {
-  if (Args.hasArg(options::OPT_shared))
-return false;
+bool tools::addXRayRuntime(const ToolChain &TC, const ArgList &Args,
+   ArgStringList &CmdArgs) {
+  if (Args.hasArg(options::OPT_shared)) return false;
 
   if (TC.getXRayArgs().needsXRayRt()) {
 CmdArgs.push_back("-whole-archive");
 CmdArgs.push_back(TC.getCompilerRTArgString(Args, "xray", false));
 for (const auto &Mode : TC.getXRayArgs().modeList())
   CmdArgs.push_back(TC.getCompilerRTArgString(Args, Mode, false));
 CmdArgs.push_back("-no-whole-archive");
+
+SmallString<128> XRay(TC.getCompilerRT(Args, "xray"));
+CmdArgs.push_back(Args.MakeArgString("--dynamic-list=" + XRay + ".syms"));
+CmdArgs.push_back(Args.MakeArgString("--version-script=" + XRay + 
".vers"));
 return true;
   }
 


Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -706,16 +706,20 @@
   return !StaticRuntimes.empty() || !NonWholeStaticRuntimes.empty();
 }
 
-bool tools::addXRayRuntime(const ToolChain&TC, const ArgList &Args, ArgStringList &CmdArgs) {
-  if (Args.hasArg(options::OPT_shared))
-return false;
+bool tools::addXRayRuntime(const ToolChain &TC, const ArgList &Args,
+   ArgStringList &CmdArgs) {
+  if (Args.hasArg(options::OPT_shared)) return false;
 
   if (TC.getXRayArgs().needsXRayRt()) {
 CmdArgs.push_back("-whole-archive");
 CmdArgs.push_back(TC.getCompilerRTArgString(Args, "xray", false));
 for (const auto &Mode : TC.getXRayArgs().modeList())
   CmdArgs.push_back(TC.getCompilerRTArgString(Args, Mode, false));
 CmdArgs.push_back("-no-whole-archive");
+
+SmallString<128> XRay(TC.getCompilerRT(Args, "xray"));
+CmdArgs.push_back(Args.MakeArgString("--dynamic-list=" + XRay + ".syms"));
+CmdArgs.push_back(Args.MakeArgString("--version-script=" + XRay + ".vers"));
 return true;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44970: [XRay][clang] Add flag to choose instrumentation bundles

[Martin Pelikán via Phabricator via cfe-commits]

pelikan added a comment.

I would probably add more tests for the different configurations, but I suspect 
more diffs are coming after this.




Comment at: clang/include/clang/Driver/Options.td:1112
+  Group, Flags<[CC1Option]>,
+  HelpText<"Select which bundle of XRay instrumentation points to emit. 
Options: all, none, function-extents, custom-only.">;
+

I'd suggest making them "fn-only" and "custom-only", or just plain "function" 
and "custom".



Comment at: clang/include/clang/Driver/Options.td:1112
+  Group, Flags<[CC1Option]>,
+  HelpText<"Select which bundle of XRay instrumentation points to emit. 
Options: all, none, function-extents, custom-only.">;
+

pelikan wrote:
> I'd suggest making them "fn-only" and "custom-only", or just plain "function" 
> and "custom".
I also don't get why "none" is an option here, if it's equivalent to 
-fnoxray-instrument.  Why duplicate the functionality and add more points the 
users will have to debug?



Comment at: clang/include/clang/Frontend/CodeGenOptions.h:110
 
+  enum XRayInstrumentationPointBundle {
+XRay_All, // Always emit all the instrumentation points.

To me, this feels like a bitfield would be a better match.
All = Function | Custom
None = 0



Comment at: clang/lib/Driver/XRayArgs.cpp:62
+  << (std::string(XRayInstrumentOption) +
+  " on non-supported target OS");
 }

I would also print the triple.  Something like:

"-fomit-bugs is not supported on target win64-ppc-none"

will be much more informative, especially when you collect logs from build 
machines on lots of architectures (like Linux distro/BSD package builders do).



Comment at: clang/lib/Driver/XRayArgs.cpp:86
+Args.getLastArg(options::OPT_fxray_instrumentation_bundle)) {
+  StringRef B = A->getValue();
+  if (B != "all" && B != "none" && B != "function-extents" &&

echristo wrote:
> How about a more descriptive name here and a string switch below?
+1


https://reviews.llvm.org/D44970



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29704: [XRay] [clang] Allow logging the first argument of a function call.

[Martin Pelikán via Phabricator via cfe-commits]

pelikan created this revision.

Functions with the "xray_log_args" attribute will tell LLVM to emit a special
XRay sled for compiler-rt to copy any call arguments to your logging handler.


https://reviews.llvm.org/D29704

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/CodeGenFunction.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/xray-log-args.cpp
  test/Sema/xray-log-args-oob.c
  test/Sema/xray-log-args-oob.cpp

Index: test/Sema/xray-log-args-oob.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-oob.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+void foo [[clang::xray_log_args(1)]] (int);
+struct [[clang::xray_log_args(1)]] a { int x; }; // expected-warning {{'xray_log_args' attribute only applies to functions and methods}}
+
+void fop [[clang::xray_log_args(1)]] (); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void foq [[clang::xray_log_args(-1)]] (); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void fos [[clang::xray_log_args(0)]] (); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
Index: test/Sema/xray-log-args-oob.c
===
--- /dev/null
+++ test/Sema/xray-log-args-oob.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c11
+void foo(int) __attribute__((xray_log_args(1)));
+struct __attribute__((xray_log_args(1))) a { int x; }; // expected-warning {{'xray_log_args' attribute only applies to functions and methods}}
+
+void fop() __attribute__((xray_log_args(1))); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void foq() __attribute__((xray_log_args(-1))); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void fos() __attribute__((xray_log_args(0))); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
Index: test/CodeGen/xray-log-args.cpp
===
--- /dev/null
+++ test/CodeGen/xray-log-args.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 %s -fxray-instrument -std=c++11 -x c++ -emit-llvm -o - -triple x86_64-unknown-linux-gnu | FileCheck %s
+
+// Make sure that the LLVM attribute for XRay-annotated functions do show up.
+[[clang::xray_always_instrument,clang::xray_log_args(1)]] void foo(int a) {
+// CHECK: define void @_Z3fooi(i32 %a) #0
+};
+
+[[clang::xray_log_args(1)]] void bar(int a) {
+// CHECK: define void @_Z3bari(i32 %a) #1
+};
+
+// CHECK: #0 = {{.*}}"function-instrument"="xray-always"{{.*}}"xray-log-args"="1"
+// CHECK-NOT: #1 = {{.*}}"xray-log-args"="1"
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4397,6 +4397,19 @@
 Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleXRayLogArgsAttr(Sema &S, Decl *D,
+  const AttributeList &Attr) {
+  uint64_t ArgumentCount;
+  if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
+   ArgumentCount))
+return;
+
+  // It isn't a parameter index [0;n), it's a count [1;n] - hence the + 1.
+  D->addAttr(::new (S.Context)
+ XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgumentCount,
+ Attr.getAttributeSpellingListIndex()));
+}
+
 //===--===//
 // Checker-specific attribute handlers.
 //===--===//
@@ -6255,6 +6268,9 @@
   case AttributeList::AT_XRayInstrument:
 handleSimpleAttribute(S, D, Attr);
 break;
+  case AttributeList::AT_XRayLogArgs:
+handleXRayLogArgsAttr(S, D, Attr);
+break;
   }
 }
 
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -779,6 +779,10 @@
 Fn->addFnAttr("function-instrument", "xray-always");
   if (XRayAttr->neverXRayInstrument())
 Fn->addFnAttr("function-instrument", "xray-never");
+  if (const auto *LogArgs = D->getAttr()) {
+Fn->addFnAttr("xray-log-args",
+  llvm::utostr(LogArgs->getArgumentCount()));
+  }
 } else {
   Fn->addFnAttr(
   "xray-instruction-threshold",
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -2804,13 +2804,15 @@
 
 def XRayDocs : Documentation {
   let Category = DocCatFunction;
-  let Heading = "xray_always_instrument (clang::xray_always_instrument), xray_never_instrument (clang::xray_never_instrument)"

[PATCH] D29704: [XRay] [clang] Allow logging the first argument of a function call.

[Martin Pelikán via Phabricator via cfe-commits]

pelikan updated this revision to Diff 90431.
pelikan marked an inline comment as done.
pelikan added a comment.

- clarify comment and rename variable so it'll all fit.


https://reviews.llvm.org/D29704

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/CodeGenFunction.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/xray-log-args.cpp
  test/Sema/xray-log-args-oob.c
  test/Sema/xray-log-args-oob.cpp

Index: test/Sema/xray-log-args-oob.cpp
===
--- /dev/null
+++ test/Sema/xray-log-args-oob.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c++11 -x c++
+void foo [[clang::xray_log_args(1)]] (int);
+struct [[clang::xray_log_args(1)]] a { int x; }; // expected-warning {{'xray_log_args' attribute only applies to functions and methods}}
+
+void fop [[clang::xray_log_args(1)]] (); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void foq [[clang::xray_log_args(-1)]] (); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void fos [[clang::xray_log_args(0)]] (); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
Index: test/Sema/xray-log-args-oob.c
===
--- /dev/null
+++ test/Sema/xray-log-args-oob.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -std=c11
+void foo(int) __attribute__((xray_log_args(1)));
+struct __attribute__((xray_log_args(1))) a { int x; }; // expected-warning {{'xray_log_args' attribute only applies to functions and methods}}
+
+void fop() __attribute__((xray_log_args(1))); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void foq() __attribute__((xray_log_args(-1))); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
+
+void fos() __attribute__((xray_log_args(0))); // expected-error {{'xray_log_args' attribute parameter 1 is out of bounds}}
Index: test/CodeGen/xray-log-args.cpp
===
--- /dev/null
+++ test/CodeGen/xray-log-args.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 %s -fxray-instrument -std=c++11 -x c++ -emit-llvm -o - -triple x86_64-unknown-linux-gnu | FileCheck %s
+
+// Make sure that the LLVM attribute for XRay-annotated functions do show up.
+[[clang::xray_always_instrument,clang::xray_log_args(1)]] void foo(int a) {
+// CHECK: define void @_Z3fooi(i32 %a) #0
+};
+
+[[clang::xray_log_args(1)]] void bar(int a) {
+// CHECK: define void @_Z3bari(i32 %a) #1
+};
+
+// CHECK: #0 = {{.*}}"function-instrument"="xray-always"{{.*}}"xray-log-args"="1"
+// CHECK-NOT: #1 = {{.*}}"xray-log-args"="1"
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4424,6 +4424,19 @@
 Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleXRayLogArgsAttr(Sema &S, Decl *D,
+  const AttributeList &Attr) {
+  uint64_t ArgCount;
+  if (!checkFunctionOrMethodParameterIndex(S, D, Attr, 1, Attr.getArgAsExpr(0),
+   ArgCount))
+return;
+
+  // ArgCount isn't a parameter index [0;n), it's a count [1;n] - hence + 1.
+  D->addAttr(::new (S.Context)
+ XRayLogArgsAttr(Attr.getRange(), S.Context, ++ArgCount,
+ Attr.getAttributeSpellingListIndex()));
+}
+
 //===--===//
 // Checker-specific attribute handlers.
 //===--===//
@@ -6285,6 +6298,9 @@
   case AttributeList::AT_XRayInstrument:
 handleSimpleAttribute(S, D, Attr);
 break;
+  case AttributeList::AT_XRayLogArgs:
+handleXRayLogArgsAttr(S, D, Attr);
+break;
   }
 }
 
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -779,6 +779,10 @@
 Fn->addFnAttr("function-instrument", "xray-always");
   if (XRayAttr->neverXRayInstrument())
 Fn->addFnAttr("function-instrument", "xray-never");
+  if (const auto *LogArgs = D->getAttr()) {
+Fn->addFnAttr("xray-log-args",
+  llvm::utostr(LogArgs->getArgumentCount()));
+  }
 } else {
   Fn->addFnAttr(
   "xray-instruction-threshold",
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -2862,13 +2862,15 @@
 
 def XRayDocs : Documentation {
   let Category = DocCatFunction;
-  let Heading = "xray_always_instrument (clang::xray_always_instrument), xray_never_instrument (clang::xray_never_instrument)";
+  let Heading = "xray_always_