[PATCH] D75937: Add Support to X86 for Load Hardening to Mitigate Load Value Injection (LVI) [5/6]

[Craig Topper via Phabricator via llvm-commits]

craig.topper marked 5 inline comments as done.
craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:535
+  if (Graph->NumFences > 0) { // eliminate fences
+    for (const auto &E : Graph->edges()) {
+      const MachineGadgetGraph::Node *Dest = E.getDest();
----------------
sconstab wrote:
> Would `EdgeRef E : Graph->edges()` be clearer here? Ditto for many other `for` loops.
> 
> (not sure if the LLVM conventions dictate something specific for these loops)
Personally, I'd prefer not to hide the & or *. And EdgeRef/NodeRef only exist in the Traits class not the main class. It's also confusing that NodeRef is a pointer and not a reference so I'd like to not leak that outside the Traits class where its easier to see.

LLVM is pretty conservative about the use of auto, but I figured in this case it wouldn't be unreasonable for a reader to understand that edges() returns Edge objects. But I'm happing to change it to MachineGadgetGraph::Edge.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:540
+        ElimEdges.insert(&E);
+        for_each(Dest->edges(),
+                 [&ElimEdges](const MachineGadgetGraph::Edge &E) {
----------------
sconstab wrote:
> This caught me off guard the first time I saw it because I didn't realize that LLVM had its own implementation that takes a range. For readability, would it be better to prefix with `llvm::`?
Agreed. I'll change.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:619
+    std::vector<int> EdgeCuts(G.edges_size());
+    std::vector<int> EdgeValues(G.edges_size());
+    for (const auto &N : G.nodes()) {
----------------
sconstab wrote:
> I think that `std::unique_ptr<T[]>` should work fine here...
Agreed, but std::vector is far more common in the codebase and since it lives on the stack the extra capacity pointer shouldn't be a big deal.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:622
+      Nodes[std::distance(G.nodes_begin(), &N)] =
+          std::distance(G.edges_begin(), N.edges_begin());
+    }
----------------
I'm thinking about adding a method to Graph to get a node/edge's index so we can hide all these std::distance calls.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:632
+                EdgeCuts.data(), G.edges_size());
+    for (int I = 0; I < G.edges_size(); ++I) {
+      if (EdgeCuts[I])
----------------
sconstab wrote:
> I think this `for` can lose the brackets.
Will do.


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

https://reviews.llvm.org/D75937