[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,M]

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:77
   "invalid unwind library name in argument '%0'">;
+def err_drv_incompatible_options : Error<"'%0' and '%1' cannot be used 
together">;
 def err_drv_incompatible_unwindlib : Error<

We already have `err_drv_argument_not_allowed_with`, which can be used instead 
of adding this.



Comment at: clang/include/clang/Driver/Options.td:1703
   HelpText<"Recognize and construct Pascal-style string literals">;
+def fpatchable_function_entry : Joined<["-"], "fpatchable-function-entry=">, 
Group, Flags<[CC1Option]>,
+  HelpText<"Generate N NOPs at function entry">;

Names of options with an equals sign end in `_EQ`.



Comment at: clang/test/Driver/fpatchable-function-entry.c:11
+// RUN: not %clang -target i386 -fsyntax-only %s 
-fpatchable-function-entry=1,1 2>&1 | FileCheck --check-prefix=NONZERO %s
+// NONZERO: error: the clang compiler does not support '1'
+

I think we should have a more descriptive message here, maybe something like:

  error: the second parameter of '-fpatchable-function-entry=' must be '0' or 
omitted


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,M]

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon question-circle color=gray} Unit tests: unknown.

{icon question-circle color=gray} clang-tidy: unknown.

{icon question-circle color=gray} clang-format: unknown.

Build artifacts 
: 
diff.json 
,
 console-log.txt 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,M]

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61253 tests passed, 0 failed 
and 736 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,M]

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 236222.
MaskRay added a comment.

Forgot to add test/Driver/fpatchable-function-entry.c


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/XRayArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/patchable-function-entry.c
  clang/test/Driver/fpatchable-function-entry.c

Index: clang/test/Driver/fpatchable-function-entry.c
===
--- /dev/null
+++ clang/test/Driver/fpatchable-function-entry.c
@@ -0,0 +1,17 @@
+// RUN: %clang -target i386 %s -fpatchable-function-entry=1 -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64 %s -fpatchable-function-entry=1 -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target aarch64 %s -fpatchable-function-entry=1 -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target aarch64 %s -fpatchable-function-entry=1,0 -c -### 2>&1 | FileCheck %s
+// CHECK: "-fpatchable-function-entry=1"
+
+// RUN: not %clang -target ppc64 -fsyntax-only %s -fpatchable-function-entry=1 2>&1 | FileCheck --check-prefix=TARGET %s
+// TARGET: error: unsupported option '-fpatchable-function-entry=1' for target 'ppc64'
+
+// RUN: not %clang -target i386 -fsyntax-only %s -fpatchable-function-entry=1,1 2>&1 | FileCheck --check-prefix=NONZERO %s
+// NONZERO: error: the clang compiler does not support '1'
+
+// RUN: not %clang -target x86_64 -fsyntax-only %s -fpatchable-function-entry=1,0, 2>&1 | FileCheck --check-prefix=EXCESS %s
+// EXCESS: error: invalid argument '1,0,' to -fpatchable-function-entry=
+
+// RUN: not %clang -target aarch64-linux -fsyntax-only %s -fxray-instrument -fpatchable-function-entry=1 2>&1 | FileCheck --check-prefix=XRAY %s
+// XRAY: error: '-fxray-instrument' and '-fpatchable-function-entry=' cannot be used together
Index: clang/test/CodeGen/patchable-function-entry.c
===
--- clang/test/CodeGen/patchable-function-entry.c
+++ clang/test/CodeGen/patchable-function-entry.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple aarch64 -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64 -emit-llvm %s -fpatchable-function-entry=1 -o - | FileCheck --check-prefixes=CHECK,OPT %s
 
 // CHECK: define void @f0() #0
 __attribute__((patchable_function_entry(0))) void f0() {}
@@ -16,5 +17,9 @@
 __attribute__((patchable_function_entry(2,0))) void f20decl();
 __attribute__((noinline)) void f20decl() {}
 
+// OPT: define void @f() #2
+void f() {}
+
 // CHECK: attributes #0 = { {{.*}} "patchable-function-entry"="0,0"
 // CHECK: attributes #1 = { {{.*}} "patchable-function-entry"="2,0"
+// OPT:   attributes #2 = { {{.*}} "patchable-function-entry"="1,0"
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1101,6 +1101,8 @@
   parseXRayInstrumentationBundle("-fxray-instrumentation-bundle=", A, Args,
  Diags, Opts.XRayInstrumentationBundle);
 
+  Opts.PatchableFunctionEntrySize =
+  getLastArgIntValue(Args, OPT_fpatchable_function_entry, 0, Diags);
   Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
   Opts.CallFEntry = Args.hasArg(OPT_mfentry);
   Opts.MNopMCount = Args.hasArg(OPT_mnop_mcount);
Index: clang/lib/Driver/XRayArgs.cpp
===
--- clang/lib/Driver/XRayArgs.cpp
+++ clang/lib/Driver/XRayArgs.cpp
@@ -70,6 +70,13 @@
   D.Diag(diag::err_drv_clang_unsupported)
   << (std::string(XRayInstrumentOption) + " on " + Triple.str());
 }
+
+// Both XRay and -fpatchable-function-entry use
+// TargetOpcode::PATCHABLE_FUNCTION_ENTER.
+if (Arg *A = Args.getLastArg(options::OPT_fpatchable_function_entry))
+  D.Diag(diag::err_drv_incompatible_options)
+  << "-fxray-instrument" << A->getSpelling();
+
 XRayInstrument = true;
 if (const Arg *A =
 Args.getLastArg(options::OPT_fxray_instruction_threshold_,
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4998,6 +4998,24 @@
   const XRayArgs  = TC.getXRayArgs();
   XRay.addArgs(TC, Args, CmdArgs, InputType);
 
+  if (Arg *A = Args.getLastArg(options::OPT_fpatchable_function_entry)) {
+StringRef S0 = A->getValue(), S = S0;
+unsigned Size, Start = 0;
+if (!Triple.isAArch64() && Triple.getArch() != llvm::Triple::x86 &&
+Triple.getArch() != llvm::Triple::x86_64)
+  

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,M]

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: dberris, kristof.beyls, nickdesaulniers, rnk, rsmith, 
void.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
MaskRay added a parent revision: D72221: Support function attribute 
patchable_function_entry.
MaskRay added subscribers: uweigand, jonpa.

In the backend, this feature is implemented with the function attribute
"patchable-function-entry". Both the attribute and XRay use
TargetOpcode::PATCHABLE_FUNCTION_ENTER, so the two features are
incompatible.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D7

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/XRayArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/patchable-function-entry.c
  clang/test/Sema/patchable-function-entry-attr.cpp

Index: clang/test/Sema/patchable-function-entry-attr.cpp
===
--- clang/test/Sema/patchable-function-entry-attr.cpp
+++ clang/test/Sema/patchable-function-entry-attr.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple aarch64 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple i386 -fsyntax-only -verify=silence %s
 // RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify=silence %s
 // RUN: %clang_cc1 -triple ppc64le -fsyntax-only -verify %s
 
Index: clang/test/CodeGen/patchable-function-entry.c
===
--- clang/test/CodeGen/patchable-function-entry.c
+++ clang/test/CodeGen/patchable-function-entry.c
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -triple aarch64 -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64 -emit-llvm %s -fpatchable-function-entry=1 -o - | FileCheck --check-prefixes=CHECK,OPT %s
 
 // CHECK: define void @f0() #0
 __attribute__((patchable_function_entry(0))) void f0() {}
@@ -16,5 +17,9 @@
 __attribute__((patchable_function_entry(2,0))) void f20decl();
 __attribute__((noinline)) void f20decl() {}
 
+// OPT: define void @f() #2
+void f() {}
+
 // CHECK: attributes #0 = { {{.*}} "patchable-function-entry"="0,0"
 // CHECK: attributes #1 = { {{.*}} "patchable-function-entry"="2,0"
+// OPT:   attributes #2 = { {{.*}} "patchable-function-entry"="1,0"
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1101,6 +1101,8 @@
   parseXRayInstrumentationBundle("-fxray-instrumentation-bundle=", A, Args,
  Diags, Opts.XRayInstrumentationBundle);
 
+  Opts.PatchableFunctionEntrySize =
+  getLastArgIntValue(Args, OPT_fpatchable_function_entry, 0, Diags);
   Opts.InstrumentForProfiling = Args.hasArg(OPT_pg);
   Opts.CallFEntry = Args.hasArg(OPT_mfentry);
   Opts.MNopMCount = Args.hasArg(OPT_mnop_mcount);
Index: clang/lib/Driver/XRayArgs.cpp
===
--- clang/lib/Driver/XRayArgs.cpp
+++ clang/lib/Driver/XRayArgs.cpp
@@ -70,6 +70,13 @@
   D.Diag(diag::err_drv_clang_unsupported)
   << (std::string(XRayInstrumentOption) + " on " + Triple.str());
 }
+
+// Both XRay and -fpatchable-function-entry use
+// TargetOpcode::PATCHABLE_FUNCTION_ENTER.
+if (Arg *A = Args.getLastArg(options::OPT_fpatchable_function_entry))
+  D.Diag(diag::err_drv_incompatible_options)
+  << "-fxray-instrument" << A->getSpelling();
+
 XRayInstrument = true;
 if (const Arg *A =
 Args.getLastArg(options::OPT_fxray_instruction_threshold_,
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4998,6 +4998,24 @@
   const XRayArgs  = TC.getXRayArgs();
   XRay.addArgs(TC, Args, CmdArgs, InputType);
 
+  if (Arg *A = Args.getLastArg(options::OPT_fpatchable_function_entry)) {
+StringRef S0 = A->getValue(), S = S0;
+unsigned Size, Start = 0;
+if (!Triple.isAArch64() && Triple.getArch() != llvm::Triple::x86 &&
+Triple.getArch() != llvm::Triple::x86_64)
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+else if (S.consumeInteger(10, Size) ||
+ (!S.empty() && (!S.consume_front(",") ||
+ S.consumeInteger(10, Start) || !S.empty(
+  D.Diag(diag::err_drv_invalid_argument_to_option)
+  << S0 << A->getOption().getName();
+else if (Start)
+  D.Diag(diag::err_drv_clang_unsupported) << Start;
+else
+  CmdArgs.push_back(Args.MakeArgString(A->getSpelling() + Twine(Size)));
+  }
+
   if (TC.SupportsProfiling()) {