Page MenuHomePhabricator

[X86] Disable LVI load hardening pass at O0
AbandonedPublic

Authored by nikic on May 16 2020, 9:06 AM.

Details

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.

Diff Detail

Event Timeline

nikic created this revision.May 16 2020, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2020, 9:06 AM

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.

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.

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.

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 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.

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?

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.

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

nikic abandoned this revision.Jun 5 2020, 1:18 PM

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