Happy to welcome Tim, agree with @emaste 's comment

Many thanks for getting this spelled out.

This puts the "how to report" instructions front and center, which is by far the most important thing in the doc. Thank you!

I want to avoid being the first one to approve, given our mutual affiliation, but at least wanted to confirm the story: @george.burgess.iv will be a valuable contact given his existing LLVM affiliation and deep involvement with the deployment of LLVM toolchains across several Google products.

Accepted and ready to land. Could I impose on someone with commit access? Thanks.

<note to self: procedure is "five business days", so next action here is Monday October 17>

Thanks for writing this down. I think this presents a consistent state on our way to something more permanent. LGTM.

Blast from the past. It seems like there's still benefit in checking in this documentation, appropriately updated to reflect the fact that SESES is now actually in the tree.

And now his watch is ended. Thank you for getting this started!

Glad that we could make this simpler without making the resulting code much slower!

Ack, thanks for setting me straight on that. Yes, I was talking about the mapping of assembly instructions back to sourceloc that could be displayed in GDB.

LGTM, one question about source locations

Thank you for the analysis and for the example, they were really helpful. Agree the approaches are different. They feel similar enough I wish we could find one that satisfied all relevant requirements, but I can't say with any certainty that e.g. SESES' performance is on par with the mitigation proposed here.

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

Took a quick look and seems sane -- will look after Craig's comment is addressed and build is passing

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.

Not sure we want to move this into the Parser -- SLH is a property of code generation, and I think it's possible (through LTO?) to mismatch the flags between parse and codegen.

I'm no expert in the pass manager, but given the very targeted applicability of LVI it definitely seems like our goal should be 0% impact for the vast majority of compilations that don't concern themselves with it.

The extra comments and the new variable name are all helpful. Thanks again.

In D76158#2026209, @craig.topper wrote:

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

In D76158#2004182, @sconstab wrote:

The coverage of the LVI mitigations for LLVM should match the coverage of the mitigations implemented for binutils. Both should adhere to the guidance outlined in this document: https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection.

I mean, yes, sure, I want that too, but that doc just says the Intel patches to GNU binutils work by "by inserting an LFENCE instruction after each instruction that performs a load". And the binutils implementation does seem to be trying to make that true?

Calling special attention to the comment at line 341, since I think it affects the correctness of the pass.

Still some comments to address, but I think this is substantially close to where it needs to be. Thanks so much for your work making the code more straightforward and readable!

Whew. I still think putting this in the X86 assembly parser (so it doesn't round-trip) is crazypants, and I wish we were relying more on instruction metadata rather than hardcoded lists, but if I take those decisions as given then this implementation seems fine.

Reviewed the algorithm, have to confess I skipped the tests.

Accepting modulo some comments to be addressed. Most of my review effort was spent on the data structure and algorithm employed as well as code style and readability.

@craig.topper can you please update/rebase this stack to remove the early LVI CFI work that ended up landed in https://reviews.llvm.org/D76812 instead?

I accidentally spent time reviewing this, only to cross-reference something in the LLVM code and find another diff (https://reviews.llvm.org/D76812) was written to answer Zola's request and has already been submitted.

LGTM

I'll wait for current comments to be addressed before doing my next round here.

Some more comments. FWIW, I'm doing rounds of review as I can in some evening quiet or during my son's nap. This is a huge change and it's really hard to get any part of it into my head at once in a reasonable amount of time.

Adding a few early style notes for the next round, but overall echo @chandlerc that this seems significantly outside of normal LLVM code.

First, want to apologize for the high latency -- thanks to (gestures vaguely in the air) I've been mostly offline during the day and trying to catch up at night.

This seems like a much more robust approach for mitigating inline asm. Thanks for that!