[XRay] Support AArch64 in Clang

[rSerge]

Summary

This patch adds XRay support in Clang for AArch64 target.
This patch is one of a series:

• LLVM: https://reviews.llvm.org/D26412
• compiler-rt: https://reviews.llvm.org/D26413

[dberris]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77239)

I'm wondering whether it's worth turning this into a switch statement now that we have more than two supported architectures?

[rSerge]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77239)

I think that would lead to more awkward code: there wouldn't be a single decision outcome point (like the current else block), so to avoid duplicating the code which currently prints the message, a bool variable would be needed. I think it's more neat to just enumerate all the OS&CPU combinations in the if condition for now.
This place is not performance-critical and the compiler should convert appropriate ifs into switches anyway.

[dberris]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77239)

This is an issue of making it more readable, something like:

if (Triple.getOS() != llvm::Triple::Linux)
  D.Diag(...) << ...; // Unsupported OS.
switch (Triple.getArch()) {
  case llvm::Triple::arm:
  case llvm::Triple::x86_64:
  case llvm::Tripe::aarch64:
    // Supported.
    break;
  default:
    D.Diag(...) << ...;
}

This way any new architectures become supported, they just get added to the list of cases that short-circuit.

[rSerge]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77239)

We can't say that an OS is supported or unsupported unless all CPU architectures for this OS support or don't support XRay, and this is not going to happen in the near future. So it is more accurate to say about the triple: some triples are supported and some are not. So in coding it is natural to check for the triple with || and &&.

[dberris]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77239)

We can't say that an OS is supported or unsupported unless all CPU architectures for this OS support or don't support XRay, and this is not going to happen in the near future.

I don't get it. Right now, as written, it doesn't matter what OS it is -- any OS other than Linux wouldn't be supported anyway. Maybe I mis-wrote, but:

if (Triple.getOS() != llvm::Triple::Linux)
  D.Diag(...) << ...;
else switch(Triple.getArch()) {
  ...
  default:
    D.Diag(...) << ...;
}

Is a direct translation that's more readable than the current complex if statement conditional.

So it is more accurate to say about the triple: some triples are supported and some are not. So in coding it is natural to check for the triple with || and &&.

Sure, but conditional is already unwieldy with just three supported platforms.

[rSerge]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77239)

Changed.

[dberris]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77537)

Did you need to put an else before the switch? I don't know what happens when you're on Windows non-supported platform -- will this cause two diagnostics to be printed?

[rSerge]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77537)

I thought that the statement after D.Diag(diag::err_drv_clang_unsupported) will not be executed. Let me check...

[rSerge]

lib/Driver/Tools.cpp

4903–4906 ↗

(On Diff #77537)

Indeed, it was printing 2 diagnostics. I've added else.

[rSerge]

Rebased to the latest version of LLVM sources.

cfe/trunk/lib/Driver/Tools.cpp

if (!Args.hasFlag(options::OPT_funique_section_names,
                     options::OPT_fno_unique_section_names, true))
     CmdArgs.push_back("-fno-unique-section-names");
 
   Args.AddAllArgs(CmdArgs, options::OPT_finstrument_functions);
 
   if (Args.hasFlag(options::OPT_fxray_instrument,
                    options::OPT_fnoxray_instrument, false)) {
     const char *const XRayInstrumentOption = "-fxray-instrument";
-    if (Triple.getOS() == llvm::Triple::Linux &&
-        (Triple.getArch() == llvm::Triple::arm ||
-         Triple.getArch() == llvm::Triple::x86_64)) {
+    if (Triple.getOS() == llvm::Triple::Linux)
+      switch (Triple.getArch()) {
+      case llvm::Triple::x86_64:
+      case llvm::Triple::arm:
+      case llvm::Triple::aarch64:
         // Supported.
-    } else {
+        break;
+      default:
         D.Diag(diag::err_drv_clang_unsupported)
             << (std::string(XRayInstrumentOption) + " on " + Triple.str());
       }
+    else
+      D.Diag(diag::err_drv_clang_unsupported)
+          << (std::string(XRayInstrumentOption) + " on non-Linux target OS");
     CmdArgs.push_back(XRayInstrumentOption);
     if (const Arg *A =
             Args.getLastArg(options::OPT_fxray_instruction_threshold_,
                             options::OPT_fxray_instruction_threshold_EQ)) {
       CmdArgs.push_back("-fxray-instruction-threshold");
       CmdArgs.push_back(A->getValue());
     }
   }

cfe/trunk/test/Driver/XRay/xray-instrument-cpu.c

 // RUN: not %clang -o /dev/null -v -fxray-instrument -c %s
-// XFAIL: amd64-, x86_64-, x86_64h-, arm
+// XFAIL: amd64-, x86_64-, x86_64h-, arm, aarch64, arm64
 // REQUIRES: linux
 typedef int a;

cfe/trunk/test/Driver/XRay/xray-instrument-os.c

 // RUN: not %clang -o /dev/null -v -fxray-instrument -c %s
 // XFAIL: -linux-
-// REQUIRES-ANY: amd64, x86_64, x86_64h, arm
+// REQUIRES-ANY: amd64, x86_64, x86_64h, arm, aarch64, arm64
 typedef int a;