[X86] Disable LVI load hardening pass at O0

[nikic]

Summary

D75936 caused a compile-time regression for O0 builds (1.5% for sqlite), because the LVI load hardening pass requires a number of additional analysis passes. While load hardening is only performed if a function attribute is specified, the analysis passes will always be executed.

As it doesn't seem to be possible to only run the analysis passes if necessary, this change disables the LVI load hardening pass entirely at O0, on the assumption that this pass is only relevant for production binaries anyway.

[sconstab]

I can accept this change if the driver can also emit a warning when LVI hardening is enabled with -O0. We already have a similar warning when LVI CFI protections are enabled with retpoline. The warning is emitted in clang/lib/Driver/ToolChains/Arch/X86.cpp.

[jethrogb]

In D80064#2040163, @sconstab wrote:

I can accept this change if the driver can also emit a warning when LVI hardening is enabled with -O0. We already have a similar warning when LVI CFI protections are enabled with retpoline. The warning is emitted in clang/lib/Driver/ToolChains/Arch/X86.cpp.

IIUC, this warning triggers only on particular command-line settings. However, the pass is activated by a function attribute, which may be enabled in different ways. I think there should be a warning if there is any function with the attribute but the pass not enabled.

[mattdr]

Of course that raises a fun question: where do we put the code to make sure we warn if the pass is disabled? It can't be in the pass itself.

I agree we should find or create a good place for this warning to avoid users shooting themselves in the foot, but my opinion is that shouldn't block this change from landing.

[sconstab]

In D80064#2041175, @jethrogb wrote:

In D80064#2040163, @sconstab wrote:

I can accept this change if the driver can also emit a warning when LVI hardening is enabled with -O0. We already have a similar warning when LVI CFI protections are enabled with retpoline. The warning is emitted in clang/lib/Driver/ToolChains/Arch/X86.cpp.

IIUC, this warning triggers only on particular command-line settings. However, the pass is activated by a function attribute, which may be enabled in different ways. I think there should be a warning if there is any function with the attribute but the pass not enabled.

Is there a way to do this in TargetMachine.cpp? The pass is inserted into the X86PassConfig. Is the module accessible at this point?

[craig.topper]

In D80064#2056188, @sconstab wrote:

In D80064#2041175, @jethrogb wrote:

In D80064#2040163, @sconstab wrote:

I can accept this change if the driver can also emit a warning when LVI hardening is enabled with -O0. We already have a similar warning when LVI CFI protections are enabled with retpoline. The warning is emitted in clang/lib/Driver/ToolChains/Arch/X86.cpp.

IIUC, this warning triggers only on particular command-line settings. However, the pass is activated by a function attribute, which may be enabled in different ways. I think there should be a warning if there is any function with the attribute but the pass not enabled.

Is there a way to do this in TargetMachine.cpp? The pass is inserted into the X86PassConfig. Is the module accessible at this point?

I don't think so. The only thought I have is to add an O0 version of the pass that just errors. Or maybe does something simple like fence every load.

[nikic]

It's possible to make the pass dependencies OptLevel dependent, so it would be possible to report an error directly from the pass. Do we have any error reporting mechanism that works this deep in the backend?

[nikic]

I'm not really convinced that erroring in that case makes sense though. I expect that a typical use of this functionality is something like https://github.com/rust-lang/rust/pull/72655/files#diff-23a2e218f0f24d0c63cc4347012d83ff, in which case making +lvi-load-hardening at optnone an error doesn't help anyone. It just means extra complexity to not set the target features at optnone, leading to exactly the same place as we would with this patch, just reimplemented at a higher level in the frontend (or the user of the frontend).

I think this should either be silently disabled at optnone (as this patch does), or as @craig.topper suggests, it should fence every load as a conservative approach.

[sconstab]

@nikic I have posted https://reviews.llvm.org/D80964 as an alternative to this patch.

[nikic]

Abandoning this one in favor of D80964, which makes more sense to me.

lib/Target/X86/X86TargetMachine.cpp

 }
 void X86PassConfig::addMachineSSAOptimization() {
   addPass(createX86DomainReassignmentPass());
   TargetPassConfig::addMachineSSAOptimization();
 }
 
 void X86PassConfig::addPostRegAlloc() {
   addPass(createX86FloatingPointStackifierPass());
+
+  // LVI load hardening requires a number of analysis passes that we do not
+  // want to run at optnone.
+  if (getOptLevel() != CodeGenOpt::None)
     addPass(createX86LoadValueInjectionLoadHardeningPass());
 }
 
 void X86PassConfig::addPreSched2() { addPass(createX86ExpandPseudoPass()); }
 
 void X86PassConfig::addPreEmitPass() {
   if (getOptLevel() != CodeGenOpt::None) {
     addPass(new X86ExecutionDomainFix());
     addPass(createBreakFalseDeps());

test/CodeGen/X86/O0-pipeline.ll

 ; CHECK-NEXT:       MachineDominator Tree Construction
 ; CHECK-NEXT:       X86 EFLAGS copy lowering
 ; CHECK-NEXT:       X86 WinAlloca Expander
 ; CHECK-NEXT:       Eliminate PHI nodes for register allocation
 ; CHECK-NEXT:       Two-Address instruction pass
 ; CHECK-NEXT:       Fast Register Allocator
 ; CHECK-NEXT:       Bundle Machine CFG Edges
 ; CHECK-NEXT:       X86 FP Stackifier
-; CHECK-NEXT:       MachineDominator Tree Construction
-; CHECK-NEXT:       Machine Natural Loop Construction
-; CHECK-NEXT:       Machine Dominance Frontier Construction
-; CHECK-NEXT:       X86 Load Value Injection (LVI) Load Hardening
 ; CHECK-NEXT:       Fixup Statepoint Caller Saved
 ; CHECK-NEXT:       Lazy Machine Block Frequency Analysis
 ; CHECK-NEXT:       Machine Optimization Remark Emitter
 ; CHECK-NEXT:       Prologue/Epilogue Insertion & Frame Finalization
 ; CHECK-NEXT:       Post-RA pseudo instruction expansion pass
 ; CHECK-NEXT:       X86 pseudo instruction expansion pass
 ; CHECK-NEXT:       Analyze Machine Code For Garbage Collection
 ; CHECK-NEXT:       Insert fentry calls