LGTM

LGTM.

Update required by https://reviews.llvm.org/D88924.

Removed the Is64Bit parameter, which was redundant and unneeded.

@craig.topper Yes.

Addressed @craig.topper 's comments, and also changed some autos to Edge/Node wherever possible to improve readability.

Made one further optimization.

Addressed @mattdr 's lone comment on style.

Addressed comments by @mattdr

LGTM.

Other than the inline comments, LGTM.

That doesn't change my mind though because one could say the same wrt optimized LVI and SESES and just add that SESES has a more redundant LFENCEs than unoptimized LVI.

In D80964#2097227, @craig.topper wrote:

It may also be worth noting that this new unoptimized pass is equivalent to the behavior of the mitigation implemented for gcc through binutils. Given that I wonder if it makes sense to use this pass at O1 or O2 and save the mostly costly analysis for O3.

@zbrid From a practical perspective I think you are correct. SESES mitigates a superset of gadgets that this pass mitigates, and therefore for code reuse/maintainability reasons it would make sense to replace this pass with SESES.

Any progress on this patch? D75939 has been merged, but the SESES feature will not be secure until it has CFI protections.

Addressed suggestion by @craig.topper to consolidate calls to emitInstruction().

In D80964#2077614, @mattdr wrote:

Isn't this pass basically SESES? https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/X86/X86SpeculativeExecutionSideEffectSuppression.cpp

Perhaps there's an opportunity to unify the two.

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

I should add that I am submitting this patch as an alternative to D80064. That revision invisibly disables the mitigation at -O0, which may not be secure for some users.

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.

Changes have been merged.

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.

In D75936#2032027, @nikic wrote:

This change causes a 0.8% compile-time regression for unoptimized builds. Based on the pipeline test diffs, I expect this is because the new pass requests a bunch of analyses, which it most likely (LVI load hardening disabled) will not need. Would it be possible to compute the analyses only if LVI load hardening is actually enabled?

Addressed comments by @mattdr.

In D76158#2026316, @mattdr wrote:

In D76158#2026209, @craig.topper wrote:

Isn't that binutils patch adding IRET (opcode 0xcf)? LRET(opcodes 0xca and 0xcb) were already there.

I admit I don't know the opcodes offhand, but I agree with your reading of the code. From what I can see that commit does seem to be the first time lret appears in comments, and it appears to add a number of lret test cases.

Rebase onto master.

Rebase onto master.

Addressed the previously unaddressed comments, as pointed out by @craig.topper.

Eliminated the NoFixedLoads feature in D75936, which simplified this patch quite a bit.

Addressed comments from @mattdr.

Removed the -x86-lvi-no-fixed CLI flag. This change simplifies the code flow quite a bit.

I don't think that this feature will be secure unless it is also used with -mlvi-cfi. Specifically, it is not sufficient to mitigate a RET simply by placing an LFENCE before it. There must also be a read from RSP's pointee just prior to that LFENCE. Also, indirect calls/jumps from memory must be decomposed into discrete load and call/jump from register operations with an interposed LFENCE. The -mlvi-cfi enables an X86 target feature that performs both of these mitigations correctly.

Addressed comments from @craig.topper.

Addressed comment by @craig.topper about the position of the CLI argument.

Previous diff had an error.

Added a CLI option to enable the inline asm hardening feature, which is now disabled by default. There is also a disclaimer in the CLI description that this feature is experimental.

Superseded by D76810, D76811, and D76812.

@mattdr Thanks for the very scrupulous review!

Addressed feedback from @mattdr

@craig.topper @mattdr

There is now a more complete set of documentation for instructions that must be manually mitigated:

@craig.topper Anyone else I should add as a reviewer? Or can we just go ahead and merge?

Summary points for @craig.topper who has commandeered this diff:

• fix the typo that Matt pointed out
• SizeT should not be a template parameter, and size_type should be fixed to int.
• Maybe have a member reference in MachineGadgetGraph to the associated MachineFunction.
• Determine how this pass (and other X86 machine passes) should fail on unsupported X86 subtargets.

Overall, the restyling by @craig.topper looks much better than what I had committed before. I agree that std::unique_ptr<T *> is the right "container" in this circumstance. And the addition of ArrayRef<> accessors is also a nice touch. A few extra inline comments.

@craig.topper I think that removing spurious MBBs is not really necessary because the emitted machine code doesn't contain the spurious MBBs, from what I have observed. I added the check anyways, if only because others may look at this discrepancy and have the same question.

4x agree.

In D76811#1953287, @zbrid wrote:

In D76811#1952580, @sconstab wrote:

By the way, I had initially implemented this patch with a pure virtual base class and a retpoline thunk (and later LVI thunk) class that implements the interface. However, I could not for the life of me structure the classes in a manner that would allow the compiler to devirtualize. Using CRTP admittedly sacrifices some readability, but it does not prevent the compiler from inlining RetpolineThunkInserter's methods.

Asking to learn here. I've not heard of CRTP and don't quite understand your explanation.

Recommend the article here: https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern

• What do you mean by allowing the compiler to devirtualize?

Suppose you have

struct Base { virtual void foo() = 0; };
struct D1 : Base { void foo() { … }; };
struct D2 final : Base { void foo() { … }; };

Added a comment to the header of X86IndirectThunks.cpp to indicate support for LVI thunks.

By the way, I had initially implemented this patch with a pure virtual base class and a retpoline thunk (and later LVI thunk) class that implements the interface. However, I could not for the life of me structure the classes in a manner that would allow the compiler to devirtualize. Using CRTP admittedly sacrifices some readability, but it does not prevent the compiler from inlining RetpolineThunkInserter's methods.

Updated to address @zbrid and @craig.topper 's comments.

Due to performance/memory reasons, it is not a good idea to refactor this pass into a ModulePass.

In D76639#1937680, @craig.topper wrote:

This probably has a hidden side effect on the pass manager construction. Inserting a module pass into the middle of the pipeline effectively introduces a serialization point in the middle of the pipeline. This means all functions have to reach this point in the pipeline before any function can continue. I believe this will cause the Machine IR for all functions to become resident in memory at once. This will substantially increase the memory usage of the compiler.

One fix to correctly count the number of fences inserted.

Addressed Zola's comments.

Addressed Zola's comments.