[PATCH] D75936: Add a Pass to X86 that builds a Condensed CFG for Load Value Injection (LVI) Gadgets [4/6]

[Zola Bridges via Phabricator via cfe-commits]

zbrid added a comment.

Thanks for putting this up! Here are a few comments.



================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:1
+//==========-- ImmutableGraph.h - A fast DAG implementation ---------=========//
+//
----------------
Might be useful if you add a comment about what makes this a fast DAG impl in case someone may want to use it later.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:254
+
+  LLVM_DEBUG(dbgs() << "***** " << getPassName() << " : " << MF.getName()
+                    << " *****\n");
----------------
I think this should go at the top of the function.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:271
+// Apply the mitigation to `MF`, return the number of fences inserted.
+// If `FixedLoads` is `true`, then the mitigation will be applied to both fixed
+// and non-fixed loads; otherwise, only non-fixed loads.
----------------
Am I misunderstanding this comment? It sounds like if FixedLoads is true then BOTH fixed loads and non-fixed loads will be mitigated. Since runOnMachineFunction would call hardenLoads twice for non-fixed loads, would that result in double mitigation for non-fixed loads in the case where we also harden fixed loads? Unfortunately I'm having trouble reasoning through this myself, so I'd appreciate some clarification.


================
Comment at: llvm/test/CodeGen/X86/O0-pipeline.ll:61
+; CHECK-NEXT:       Machine Dominance Frontier Construction
+; CHECK-NEXT:       X86 Load Value Injection (LVI) Load Hardening Pass
 ; CHECK-NEXT:       Lazy Machine Block Frequency Analysis
----------------
Remove pass from name since that's typically the convention.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75936/new/

https://reviews.llvm.org/D75936