[Clang][Driver] Add new flags to control machine instruction verification

[mizvekov]

Present shortcomings are that this only works for non-LTO builds, and that this new pass doesn't work quite the same as the IR verification flag, as it runs between every machine pass and is thus much more expensive.

Though I recently discussed this with @aeubanks during the US devmtg and we think this is worthwhile as a first step.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

Present shortcomings are that this only works for non-LTO builds, and that this new pass doesn't work quite the same as the IR verification flag, as it runs between every machine pass and is thus much more expensive.

Though I recently discussed this with @aeubanks during the US devmtg and we think this is worthwhile as a first step.

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+7)
• (modified) clang/include/clang/Driver/Options.td (+6)
• (modified) clang/lib/Driver/ToolChains/Clang.cpp (+8)
• (modified) clang/test/Driver/clang_f_opts.c (+5)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 42f20b9a9bb0410..80cebbe3210983f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -206,6 +206,13 @@ New Compiler Flags
   Since enabling the verifier adds a non-trivial cost of a few percent impact on
   build times, it's disabled by default, unless your LLVM distribution itself is
   compiled with runtime checks enabled.
+* ``-fverify-machine-code`` and its complement ``-fno-verify-machine-code``.
+  Enable or disable the verification of the generated machine code.
+  Users can pass this to turn on extra verification to catch certain types of
+  compiler bugs at the cost of extra compile time.
+  This verifier adds a huge overhead to compile time, it's expected that build times
+  can double, so this is disabled by default.
+  This flag is currently limited to non-LTO builds.
 * ``-fkeep-system-includes`` modifies the behavior of the ``-E`` option,
   preserving ``#include`` directives for "system" headers instead of copying
   the preprocessed text to the output. This can greatly reduce the size of the
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index c6b1903a32a0621..c190af69d1e1145 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1926,6 +1926,12 @@ def fverify_intermediate_code : Flag<["-"], "fverify-intermediate-code">,
 def fno_verify_intermediate_code : Flag<["-"], "fno-verify-intermediate-code">,
   Group<f_clang_Group>, Visibility<[ClangOption, CLOption, DXCOption]>,
   HelpText<"Disable verification of LLVM IR">, Flags<[NoXarchOption]>;
+def fverify_machine_code : Flag<["-"], "fverify-machine-code">,
+  Group<f_clang_Group>, Visibility<[ClangOption, CLOption, DXCOption]>,
+  HelpText<"Enable verification of generated machine code">, Flags<[NoXarchOption]>;
+def fno_verify_machine_code : Flag<["-"], "fno-verify-machine-code">,
+  Group<f_clang_Group>, Visibility<[ClangOption, CLOption, DXCOption]>,
+  HelpText<"Disable verification of generated machine code">, Flags<[NoXarchOption]>;
 def fdiscard_value_names : Flag<["-"], "fdiscard-value-names">,
   Group<f_clang_Group>, Visibility<[ClangOption, DXCOption]>,
   HelpText<"Discard value names in LLVM IR">, Flags<[NoXarchOption]>;
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 601bbfb927746fc..6b7ae896863e4da 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5175,6 +5175,14 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
     CmdArgs.push_back("-disable-llvm-verifier");
   }
 
+  // Enable the machine code verification pass. Note that this is force enabled
+  // elsewhere with LLVM_ENABLE_EXPENSIVE_CHECKS.
+  if (Args.hasFlag(options::OPT_fverify_machine_code,
+                   options::OPT_fno_verify_machine_code, false)) {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back("-verify-machineinstrs");
+  }
+
   // Discard value names in assert builds unless otherwise specified.
   if (Args.hasFlag(options::OPT_fdiscard_value_names,
                    options::OPT_fno_discard_value_names, !IsAssertBuild)) {
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index ebe8a0520bf0fca..e6bb6f80f00368b 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -525,6 +525,11 @@
 // CHECK-VERIFY-INTERMEDIATE-CODE-NOT: "-disable-llvm-verifier"
 // CHECK-NO-VERIFY-INTERMEDIATE-CODE: "-disable-llvm-verifier"
 
+// RUN: %clang -### -S -fverify-machine-code %s 2>&1 | FileCheck -check-prefix=CHECK-VERIFY-MACHINE-CODE %s
+// RUN: %clang -### -S -fno-verify-machine-code %s 2>&1 | FileCheck -check-prefix=CHECK-NO-VERIFY-MACHINE-CODE %s
+// CHECK-VERIFY-MACHINE-CODE: "-mllvm" "-verify-machineinstrs"
+// CHECK-NO-VERIFY-MACHINE-CODE-NOT: "-mllvm" "-verify-machineinstrs"
+
 // RUN: %clang -### -S -fdiscard-value-names %s 2>&1 | FileCheck -check-prefix=CHECK-DISCARD-NAMES %s
 // RUN: %clang -### -S -fno-discard-value-names %s 2>&1 | FileCheck -check-prefix=CHECK-NO-DISCARD-NAMES %s
 // CHECK-DISCARD-NAMES: "-discard-value-names"

[MaskRay]

If you want to strengthen this RUN line, -fverify-machine-code -fno-verify-machine-code

[nikic]

I'm not convinced that adding this flag makes sense, given that you can already use -mllvm -verify-machineinstrs. This is a developer option, and I don't think it should be exposed by the clang driver (and if you expose it in cc1 only, then -Xclang -fverify-machine-code is really no better than -mllvm -verify-machineinstrs). We have a lot of different verification options for different things, and I don't think they should be exposed to end users.

[aeubanks]

agreed that -mllvm -verify-machineinstrs is good enough

[mizvekov]

I don't disagree that if we stop at this step, then the flag doesn't buy much. But it would be worthwhile if we ever implemented LTO integration or implemented a lightweight machine verification which runs the pass 1 or 2 times, which would work similarly to the IR verification flag.

So shall we put this MR to sleep until we have that, or otherwise abandon this effort?
Either way works for me.

[aeubanks]

We can still pass -mllvm for LTO builds.

If we have a more lightweight version, then I wonder if we just fold that under the previous verification flag.

Yeah I'd hold off on this until we have a more compelling reason to add it.