[Driver] Add options to control workaround for Cortex-A53 Erratum 843419

[bryanpkc]

Implement the -mfix-cortex-a53-843419 and -mno-fix-cortex-a53-843419 options, which have been introduced to GCC to allow the user to control the workaround for the erratum. If the option is enabled (which is the default, unchanged by this patch), Clang passes --fix-cortex-a53-843419 to the linker when it cannot ensure that the target is not a Cortex A53, otherwise it doesn't.

See https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html#index-mfix-cortex-a53-843419 for information on the GCC options.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Bryan Chan (bryanpkc)

Changes

Implement the -mfix-cortex-a53-843419 and -mno-fix-cortex-a53-843419 options, which have been introduced to GCC to allow the user to control the workaround for the erratum. If the option is enabled (which is the default, unchanged by this patch), Clang passes --fix-cortex-a53-843419 to the linker when it cannot assure that the target is not a Cortex A53, otherwise it doesn't.

See https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html#index-mfix-cortex-a53-843419 for information on the GCC options.

---

Full diff: https://github.com/llvm/llvm-project/pull/143915.diff

4 Files Affected:

• (modified) clang/include/clang/Driver/Options.td (+8-2)
• (modified) clang/lib/Driver/ToolChains/Fuchsia.cpp (+3-1)
• (modified) clang/lib/Driver/ToolChains/Gnu.cpp (+3-1)
• (modified) clang/test/Driver/android-link.cpp (+12)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 152df89118a6a..af17287aafd51 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5146,10 +5146,16 @@ def mno_fix_cortex_a72_aes_1655431 : Flag<["-"], "mno-fix-cortex-a72-aes-1655431
   Alias<mno_fix_cortex_a57_aes_1742098>;
 def mfix_cortex_a53_835769 : Flag<["-"], "mfix-cortex-a53-835769">,
   Group<m_aarch64_Features_Group>,
-  HelpText<"Workaround Cortex-A53 erratum 835769 (AArch64 only)">;
+  HelpText<"Work around Cortex-A53 erratum 835769 (AArch64 only)">;
 def mno_fix_cortex_a53_835769 : Flag<["-"], "mno-fix-cortex-a53-835769">,
   Group<m_aarch64_Features_Group>,
-  HelpText<"Don't workaround Cortex-A53 erratum 835769 (AArch64 only)">;
+  HelpText<"Don't work around Cortex-A53 erratum 835769 (AArch64 only)">;
+def mfix_cortex_a53_843419 : Flag<["-"], "mfix-cortex-a53-843419">,
+  Group<m_aarch64_Features_Group>,
+  HelpText<"Work around Cortex-A53 erratum 843419 (AArch64 only)">;
+def mno_fix_cortex_a53_843419 : Flag<["-"], "mno-fix-cortex-a53-843419">,
+  Group<m_aarch64_Features_Group>,
+  HelpText<"Don't work around Cortex-A53 erratum 843419 (AArch64 only)">;
 def mmark_bti_property : Flag<["-"], "mmark-bti-property">,
   Group<m_aarch64_Features_Group>,
   HelpText<"Add .note.gnu.property with BTI to assembly files (AArch64 only)">;
diff --git a/clang/lib/Driver/ToolChains/Fuchsia.cpp b/clang/lib/Driver/ToolChains/Fuchsia.cpp
index 1c165bbfe84f5..146dc8bbd5313 100644
--- a/clang/lib/Driver/ToolChains/Fuchsia.cpp
+++ b/clang/lib/Driver/ToolChains/Fuchsia.cpp
@@ -91,7 +91,9 @@ void fuchsia::Linker::ConstructJob(Compilation &C, const JobAction &JA,
     CmdArgs.push_back("--execute-only");
 
     std::string CPU = getCPUName(D, Args, Triple);
-    if (CPU.empty() || CPU == "generic" || CPU == "cortex-a53")
+    if (Args.hasFlag(options::OPT_mfix_cortex_a53_843419,
+                     options::OPT_mno_fix_cortex_a53_843419, true) &&
+        (CPU.empty() || CPU == "generic" || CPU == "cortex-a53"))
       CmdArgs.push_back("--fix-cortex-a53-843419");
   }
 
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp
index 9c68c5c6de2b2..9203bbc91b0bb 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -402,7 +402,9 @@ void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   // Most Android ARM64 targets should enable the linker fix for erratum
   // 843419. Only non-Cortex-A53 devices are allowed to skip this flag.
-  if (Arch == llvm::Triple::aarch64 && (isAndroid || isOHOSFamily)) {
+  if (Arch == llvm::Triple::aarch64 && (isAndroid || isOHOSFamily) &&
+      Args.hasFlag(options::OPT_mfix_cortex_a53_843419,
+                   options::OPT_mno_fix_cortex_a53_843419, true)) {
     std::string CPU = getCPUName(D, Args, Triple);
     if (CPU.empty() || CPU == "generic" || CPU == "cortex-a53")
       CmdArgs.push_back("--fix-cortex-a53-843419");
diff --git a/clang/test/Driver/android-link.cpp b/clang/test/Driver/android-link.cpp
index ab7dae5405587..b103263cdd3f0 100644
--- a/clang/test/Driver/android-link.cpp
+++ b/clang/test/Driver/android-link.cpp
@@ -16,6 +16,16 @@
 // RUN: FileCheck -check-prefix=CORTEX-A57 < %t %s
 
 // RUN: %clang --target=aarch64-none-linux-android \
+// RUN:   -mno-fix-cortex-a53-843419 \
+// RUN:   -### -v %s 2> %t
+// RUN: FileCheck -check-prefix=OVERRIDDEN < %t %s
+//
+// RUN: %clang -target aarch64-none-linux-android \
+// RUN:   -mno-fix-cortex-a53-843419 -mfix-cortex-a53-843419 \
+// RUN:   -### -v %s 2> %t
+// RUN: FileCheck -check-prefix=OVERRIDDEN2 < %t %s
+//
+// RUN: %clang -target aarch64-none-linux-android \
 // RUN:   -### -v %s 2> %t
 // RUN: FileCheck -check-prefix=MAX-PAGE-SIZE-16KB < %t %s
 
@@ -31,6 +41,8 @@
 // GENERIC-ARM: --fix-cortex-a53-843419
 // CORTEX-A53: --fix-cortex-a53-843419
 // CORTEX-A57-NOT: --fix-cortex-a53-843419
+// OVERRIDDEN-NOT: --fix-cortex-a53-843419
+// OVERRIDDEN2: --fix-cortex-a53-843419
 // MAX-PAGE-SIZE-4KB: "-z" "max-page-size=4096"
 // MAX-PAGE-SIZE-16KB: "-z" "max-page-size=16384"
 // NO-MAX-PAGE-SIZE-16KB-NOT: "-z" "max-page-size=16384"

[davemgreen]

I think this LGTM. I left a comment for maybe having a "IsPossiblyACortexA53" check that would could reuse in a few places, but that is probably best extended as a separate issue.

[davemgreen]

I was wondering if the option should always be honoured if it was present (with any CPU). If it isn't present then we enable it with a generic or empty or a53 CPU. But that doesn't look like how the GCC option is specified in the docs.

It should ideally be automatically disabled if the archecture is armv8.1-a or later, as that code cannot run on cortex-a53. That is probably a separate issue though, and would equally apply to mfix_cortex_a53_835769 too.

[bryanpkc]

The GCC docs say that these options are "ignored if an architecture or cpu is specified on the command line which does not need the workaround". So I think what we have in this PR mostly matches that description.

I can submit a new PR to add a condition to check whether the architecture is armv8.1-a or later.

[smithp35]

EDIT: please ignore this comment I, thought this changed the default behaviour for all targets, but this is just restricted to the existing targets that enabled it.

The linker workaround for 843419 is not particularly pleasant. I'm not sure how much it triggers in real examples, but it will add to link time, particularly for large programs.

For the amount of affected hardware out there (this erratum is 10 years old), which is likely concentrated on Android devices (the workaround is enabled by default there (https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Gnu.cpp#L405), I'm not sure that defaulting to on for such a wide variety of devices is the right one. For example almost every linux distribution is going to compile with -march=armv8-a and will get this applied.

Personally I'd default to off, particularly after having clang default this to off for the last 10 years. However data may show that this isn't as big of a deal in practice that I think it could be.

[davemgreen]

Personally I'd default to off, particularly after having clang default this to off for the last 10 years.

I didn't believe this changed the defaults (I may be mistaken, let me know if I am!). It was already enabled on Android + Fuchsia, and I believe this patch keeps that the same. It just adds an options to control it. Linux still does not enable it, only isAndroid || isOHOSFamily or Fuchsia through the other ToolChain.

[smithp35]

Personally I'd default to off, particularly after having clang default this to off for the last 10 years.

I didn't believe this changed the defaults (I may be mistaken, let me know if I am!). It was already enabled on Android + Fuchsia, and I believe this patch keeps that the same. It just adds an options to control it. Linux still does not enable it, only isAndroid || isOHOSFamily or Fuchsia through the other ToolChain.

My apologies, false alarm. I think I read too much into the description and less into the code. I've no objections. I'll update my comment with a please ignore.

[bryanpkc]

Thanks for the reviews @davemgreen and @smithp35.