[Driver][xray] Allow XRay on Apple Silicon

[ilammy]

Summary

Codegen can handle XRay for AArch64, tell the driver to allow it.

[MaskRay]

clang/test/Driver/XRay/xray-instrument-macos.c

1 ↗

(On Diff #504369)

--target=

Avoid -target (legacy) for new tests.

[ilammy]

Addressing feedback by @MaskRay:

• Moved // REQUIRES: directive to the top of the test file
• Replaced legacy -target option with proper --target in tests
• Also rebased on updated trunk

[MaskRay]

This is a kinda odd case. Normally we should enable the option in the driver when the feature actually works.
However, Triple.isMacOSX() is allowed before the feature actually works and compiler-rt/test/xray/lit.cfg.py tests it.

clang/test/Driver/XRay/ is broken and does not follow the practice testing driver changes.
clang/test/Driver should not test code generation but XRay/ tests do.

The targets aren't really enabled since REQUIRES: aarch64 || x86_64 || x86_64h lines specified lit features aren't available.

clang/lib/Driver/XRayArgs.cpp

63

The file was written in a non-conventional way. I just cleaned up the file a bit.

[ilammy]

In D145849#4412286, @MaskRay wrote:

However, Triple.isMacOSX() is allowed before the feature actually works and compiler-rt/test/xray/lit.cfg.py tests it.

I have some patches for compiler-rt cooking, fixing the build for AArch64. Should those go in first, before clang is ultimately allowed to use -fxray-instrument when targeting AArch64, so that everything works out of the box?

For me, Clang is able to compile binaries with XRay sections (with this change), but I have compiler-rt disabled and not linked in, since it's not possible to build it for AArch64 yet.

I thought it could be done separately. I was not aware of those tests in compiler-rt, checking all archs.

I guess the build should be fixed first, then this change should simultaneously allow clang to target AArch64 and update compiler-rt's expectations of what works.

How does that sound to you?

[MaskRay]

In D145849#4430870, @ilammy wrote:

In D145849#4412286, @MaskRay wrote:

However, Triple.isMacOSX() is allowed before the feature actually works and compiler-rt/test/xray/lit.cfg.py tests it.

I have some patches for compiler-rt cooking, fixing the build for AArch64. Should those go in first, before clang is ultimately allowed to use -fxray-instrument when targeting AArch64, so that everything works out of the box?

For me, Clang is able to compile binaries with XRay sections (with this change), but I have compiler-rt disabled and not linked in, since it's not possible to build it for AArch64 yet.

I thought it could be done separately. I was not aware of those tests in compiler-rt, checking all archs.

I guess the build should be fixed first, then this change should simultaneously allow clang to target AArch64 and update compiler-rt's expectations of what works.

How does that sound to you?

Since Mach-O porting is supposed to be done soon, I think landing this change is fine. This patch should make fixing the underlying issue easier as we don't need to locally patch XRayArgs.cpp...

I think D152661 is very much desired and ideally landed before other fixes.

[MaskRay]

In D145849#4436623, @MaskRay wrote:

In D145849#4430870, @ilammy wrote:

In D145849#4412286, @MaskRay wrote:

However, Triple.isMacOSX() is allowed before the feature actually works and compiler-rt/test/xray/lit.cfg.py tests it.

I have some patches for compiler-rt cooking, fixing the build for AArch64. Should those go in first, before clang is ultimately allowed to use -fxray-instrument when targeting AArch64, so that everything works out of the box?

For me, Clang is able to compile binaries with XRay sections (with this change), but I have compiler-rt disabled and not linked in, since it's not possible to build it for AArch64 yet.

I thought it could be done separately. I was not aware of those tests in compiler-rt, checking all archs.

I guess the build should be fixed first, then this change should simultaneously allow clang to target AArch64 and update compiler-rt's expectations of what works.

How does that sound to you?

Since Mach-O porting is supposed to be done soon, I think landing this change is fine. This patch should make fixing the underlying issue easier as we don't need to locally patch XRayArgs.cpp...

I think D152661 is very much desired and ideally landed before other fixes.

Needs to be rebased first. Hold on. I'll improve the xray-instrument* tests soon and you may want to git pull:)

clang/lib/Driver/XRayArgs.cpp

 XRayArgs::XRayArgs(const ToolChain &TC, const ArgList &Args) {
   const Driver &D = TC.getDriver();
   const llvm::Triple &Triple = TC.getTriple();
   if (!Args.hasFlag(options::OPT_fxray_instrument,
                     options::OPT_fno_xray_instrument, false))
     return;
   XRayInstrument = Args.getLastArg(options::OPT_fxray_instrument);
   if (Triple.isMacOSX()) {
-    if (Triple.getArch() != llvm::Triple::x86_64) {
+    switch (Triple.getArch()) {
+    case llvm::Triple::aarch64:
+    case llvm::Triple::x86_64:
+      break;
+    default:
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << XRayInstrument->getSpelling() << Triple.str();
+      break;
     }
   } else if (Triple.isOSBinFormatELF()) {
     switch (Triple.getArch()) {
     case llvm::Triple::x86_64:
     case llvm::Triple::arm:
     case llvm::Triple::aarch64:
     case llvm::Triple::hexagon:
     case llvm::Triple::ppc64le:
     case llvm::Triple::mips:
     case llvm::Triple::mipsel:
     case llvm::Triple::mips64:
     case llvm::Triple::mips64el:
       break;
     default:
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << XRayInstrument->getSpelling() << Triple.str();
     }
   } else {
     D.Diag(diag::err_drv_unsupported_opt_for_target)
         << XRayInstrument->getSpelling() << Triple.str();
   }
 
   // Both XRay and -fpatchable-function-entry use
   // TargetOpcode::PATCHABLE_FUNCTION_ENTER.
   if (Arg *A = Args.getLastArg(options::OPT_fpatchable_function_entry_EQ))
     D.Diag(diag::err_drv_argument_not_allowed_with)
         << XRayInstrument->getSpelling() << A->getSpelling();
 
   if (!Args.hasFlag(options::OPT_fxray_link_deps,

clang/test/Driver/XRay/xray-instrument.c

 // RUN: %clang -### --target=aarch64-pc-freebsd -fxray-instrument -c %s -o /dev/null 2>&1 | FileCheck %s
+// RUN: %clang -### --target=arm64-apple-macos -fxray-instrument -c %s -o /dev/null 2>&1 | FileCheck %s
 // RUN: %clang -### --target=x86_64-apple-darwin -fxray-instrument -c %s -o /dev/null 2>&1 | FileCheck %s
 // RUN: %clang -### --target=x86_64-pc-windows -fxray-instrument -c %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR
 
 // CHECK: "-cc1" {{.*}}"-fxray-instrument"
 // ERR:   error: unsupported option '-fxray-instrument' for target
 
 typedef int a;