[xray][clang] Always add xray-skip-entry/exit and xray-ignore-loops attrs

[ianlevesque]

Summary

The function attributes xray-skip-entry, xray-skip-exit, and
xray-ignore-loops were only being applied if a function had an
xray-instrument attribute, but they should apply if xray is enabled
globally too.

[MaskRay]

clang/lib/Frontend/CompilerInvocation.cpp

1102

Args.hasArg(OPT_fxray_ignore_loops);

There is no need to check a fno option in cc1, if the option is always enabled/disabled by default.

[ianlevesque]

clang/lib/CodeGen/CodeGenFunction.cpp

832

@hiraditya I realize on a previous diff you suggested dropping the brackets. I'll do that here too.

clang/lib/Frontend/CompilerInvocation.cpp

1102

Will update.

[ianlevesque]

Address code review nits

[ianlevesque]

@hiraditya or @smeenai can i get a merge?

[smeenai]

I can commit this for you, but is it possible to write a test case?

[ianlevesque]

Now with 100% more tests.

[smeenai]

Is it worth adding a test that a function with an explicit xray-instrument attribute also has these other attributes applied?

clang/test/CodeGen/xray-attributes-skip-entry-exit.cpp

8

I don't see the NOCUSTOM or NOTYPED prefixes defined?

12

Same with the FUNCTION prefix.

clang/test/CodeGen/xray-ignore-loops.cpp

2

From what I've seen, CodeGen tests don't usually invoke the driver directly. They have a test that the driver passes the correct flags to cc1 (as you do below), and then they just test that cc1 does the right thing with that flag.

[ianlevesque]

Address code review feedback on the tests.

[ianlevesque]

In D73842#1869890, @smeenai wrote:

Is it worth adding a test that a function with an explicit xray-instrument attribute also has these other attributes applied?

The existing tests from when the feature was added covered that case well. It was the driver passing the flag and the case without the xray-instrument attribute that weren't working.

[smeenai]

LGTM. I'll commit this for you.

clang/include/clang/Driver/XRayArgs.h

class XRayArgs {
   std::vector<std::string> ExtraDeps;
   std::vector<std::string> Modes;
   XRayInstrSet InstrumentationBundle;
   bool XRayInstrument = false;
   int InstructionThreshold = 200;
   bool XRayAlwaysEmitCustomEvents = false;
   bool XRayAlwaysEmitTypedEvents = false;
   bool XRayRT = true;
+  bool XRayIgnoreLoops = false;
 
 public:
   /// Parses the XRay arguments from an argument list.
   XRayArgs(const ToolChain &TC, const llvm::opt::ArgList &Args);
   void addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
                llvm::opt::ArgStringList &CmdArgs, types::ID InputType) const;
 
   bool needsXRayRt() const { return XRayInstrument && XRayRT; }
   llvm::ArrayRef<std::string> modeList() const { return Modes; }
   XRayInstrSet instrumentationBundle() const { return InstrumentationBundle; }
 };
 
 } // namespace driver
 } // namespace clang
 
 #endif // LLVM_CLANG_DRIVER_XRAYARGS_H

clang/lib/CodeGen/CodeGenFunction.cpp

if (const auto *XRayAttr = D->getAttr<XRayInstrumentAttr>()) {
         if (XRayAttr->alwaysXRayInstrument() && ShouldXRayInstrumentFunction())
           Fn->addFnAttr("function-instrument", "xray-always");
         if (XRayAttr->neverXRayInstrument())
           Fn->addFnAttr("function-instrument", "xray-never");
         if (const auto *LogArgs = D->getAttr<XRayLogArgsAttr>())
           if (ShouldXRayInstrumentFunction())
             Fn->addFnAttr("xray-log-args",
                           llvm::utostr(LogArgs->getArgumentCount()));
-        if (!CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
-                XRayInstrKind::FunctionExit)) {
-          Fn->addFnAttr("xray-skip-exit");
-        }
-        if (!CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
-                XRayInstrKind::FunctionEntry)) {
-          Fn->addFnAttr("xray-skip-entry");
-        }
       }
     } else {
       if (ShouldXRayInstrumentFunction() && !CGM.imbueXRayAttrs(Fn, Loc))
         Fn->addFnAttr(
             "xray-instruction-threshold",
             llvm::itostr(CGM.getCodeGenOpts().XRayInstructionThreshold));
-      if (CGM.getCodeGenOpts().XRayIgnoreLoops) {
-        Fn->addFnAttr("xray-ignore-loops");
-      }
+
+    if (ShouldXRayInstrumentFunction()) {
+      if (CGM.getCodeGenOpts().XRayIgnoreLoops)
+        Fn->addFnAttr("xray-ignore-loops");
+
+      if (!CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
+              XRayInstrKind::FunctionExit))
+        Fn->addFnAttr("xray-skip-exit");
+
+      if (!CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
+              XRayInstrKind::FunctionEntry))
+        Fn->addFnAttr("xray-skip-entry");
     }
 
     unsigned Count, Offset;
     if (const auto *Attr = D->getAttr<PatchableFunctionEntryAttr>()) {
       Count = Attr->getCount();
       Offset = Attr->getOffset();
     } else {
       Count = CGM.getCodeGenOpts().PatchableFunctionEntryCount;

clang/lib/Driver/XRayArgs.cpp

XRayArgs::XRayArgs(const ToolChain &TC, const ArgList &Args) {
   if (Args.hasFlag(options::OPT_fxray_always_emit_typedevents,
                    options::OPT_fno_xray_always_emit_typedevents, false))
     XRayAlwaysEmitTypedEvents = true;
 
   if (!Args.hasFlag(options::OPT_fxray_link_deps,
                     options::OPT_fnoxray_link_deps, true))
     XRayRT = false;
 
+  if (Args.hasFlag(options::OPT_fxray_ignore_loops,
+                   options::OPT_fno_xray_ignore_loops, false))
+    XRayIgnoreLoops = true;
+
   auto Bundles =
       Args.getAllArgValues(options::OPT_fxray_instrumentation_bundle);
   if (Bundles.empty())
     InstrumentationBundle.Mask = XRayInstrKind::All;
   else
     for (const auto &B : Bundles) {
       llvm::SmallVector<StringRef, 2> BundleParts;
       llvm::SplitString(B, BundleParts, ",");
void XRayArgs::addArgs(const ToolChain &TC, const ArgList &Args,
   CmdArgs.push_back(XRayInstrumentOption);
 
   if (XRayAlwaysEmitCustomEvents)
     CmdArgs.push_back("-fxray-always-emit-customevents");
 
   if (XRayAlwaysEmitTypedEvents)
     CmdArgs.push_back("-fxray-always-emit-typedevents");
 
+  if (XRayIgnoreLoops)
+    CmdArgs.push_back("-fxray-ignore-loops");
+
   CmdArgs.push_back(Args.MakeArgString(Twine(XRayInstructionThresholdOption) +
                                        Twine(InstructionThreshold)));
 
   for (const auto &Always : AlwaysInstrumentFiles) {
     SmallString<64> AlwaysInstrumentOpt("-fxray-always-instrument=");
     AlwaysInstrumentOpt += Always;
     CmdArgs.push_back(Args.MakeArgString(AlwaysInstrumentOpt));
   }

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.XRayInstrumentFunctions =
       Args.hasArg(OPT_fxray_instrument);
   Opts.XRayAlwaysEmitCustomEvents =
       Args.hasArg(OPT_fxray_always_emit_customevents);
   Opts.XRayAlwaysEmitTypedEvents =
       Args.hasArg(OPT_fxray_always_emit_typedevents);
   Opts.XRayInstructionThreshold =
       getLastArgIntValue(Args, OPT_fxray_instruction_threshold_EQ, 200, Diags);
-  Opts.XRayIgnoreLoops =
-      Args.hasArg(OPT_fxray_ignore_loops, OPT_fno_xray_ignore_loops, false);
+  Opts.XRayIgnoreLoops = Args.hasArg(OPT_fxray_ignore_loops);
 
   auto XRayInstrBundles =
       Args.getAllArgValues(OPT_fxray_instrumentation_bundle);
   if (XRayInstrBundles.empty())
     Opts.XRayInstrumentationBundle.Mask = XRayInstrKind::All;
   else
     for (const auto &A : XRayInstrBundles)
       parseXRayInstrumentationBundle("-fxray-instrumentation-bundle=", A, Args,

clang/test/CodeGen/xray-attributes-skip-entry-exit.cpp

+// RUN: %clang_cc1 -fxray-instrument \
+// RUN:     -fxray-instrumentation-bundle=function-entry -x c++ \
+// RUN:     -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN:     | FileCheck --check-prefixes CHECK,NOCUSTOM,NOTYPED,SKIPEXIT %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN:     -fxray-instrumentation-bundle=function-exit -x c++ \
+// RUN:     -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN:     | FileCheck --check-prefixes CHECK,NOCUSTOM,NOTYPED,SKIPENTRY %s
+// RUN: %clang_cc1 -fxray-instrument \
+// RUN:     -fxray-instrumentation-bundle=function-entry,function-exit -x c++ \
+// RUN:     -std=c++11 -triple x86_64-unknown-unknown -emit-llvm -o - %s \
+// RUN:     | FileCheck --check-prefixes CHECK,FUNCTION,NOCUSTOM,NOTYPED %s
+
+// CHECK: define void @_Z13justAFunctionv() #[[ATTR:[0-9]+]] {
+void justAFunction() {
+}
+
+// SKIPENTRY: attributes #[[ATTR]] = {{.*}} "xray-skip-entry" {{.*}}
+// SKIPEXIT: attributes #[[ATTR]] = {{.*}} "xray-skip-exit" {{.*}}

clang/test/CodeGen/xray-ignore-loops.cpp

 // RUN: %clang_cc1 -fxray-instrument -fxray-ignore-loops -x c++ -std=c++11 -emit-llvm -o - %s -triple x86_64-unknown-linux-gnu | FileCheck %s
+// RUN: %clang -fxray-instrument -fxray-ignore-loops -x c++ -std=c++11 -S -emit-llvm -o - %s -target x86_64-unknown-linux-gnu | FileCheck %s
 
 int foo() {
   return 1;
 }
 
-// CHECK: define i32 @_Z3foov() #[[ATTRS:[0-9]+]] {
+// CHECK: define{{.*}} i32 @_Z3foov() #[[ATTRS:[0-9]+]] {
 // CHECK-DAG: attributes #[[ATTRS]] = {{.*}} "xray-ignore-loops" {{.*}}

clang/test/Driver/XRay/xray-ignore-loops-flags.cpp

+// This test ensures that when we invoke the clang compiler, that the -cc1
+// options include the -fxray-ignore-loops flag we provide in the
+// invocation.
+//
+// RUN: %clang -fxray-instrument -fxray-ignore-loops -target x86_64-linux- -### \
+// RUN:     -x c++ -std=c++11 -emit-llvm -c -o - %s 2>&1 \
+// RUN:     | FileCheck %s
+// CHECK:  -fxray-ignore-loops
+//
+// REQUIRES: x86_64 || x86_64h
 No newline at end of file