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

[Scott Constable via Phabricator via llvm-commits]

sconstab 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();
----------------
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)


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:540
+        ElimEdges.insert(&E);
+        for_each(Dest->edges(),
+                 [&ElimEdges](const MachineGadgetGraph::Edge &E) {
----------------
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::`?


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:566
+    std::function<void(const MachineGadgetGraph::Node *, bool)> TraverseDFS =
+        [&](const MachineGadgetGraph::Node *N, bool FirstNode) {
+          if (!FirstNode) {
----------------
Should this be `MachineGadgetGraph::NodeRef`?


================
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()) {
----------------
I think that `std::unique_ptr<T[]>` should work fine here...


================
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])
----------------
I think this `for` can lose the brackets.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:672
+      [&CutEdges, &AdditionalEdgesCut](const MachineGadgetGraph::Node *N) {
+        for (const MachineGadgetGraph::Edge &E : N->edges()) {
+          if (MachineGadgetGraph::isCFGEdge(E) && !CutEdges.contains(&E)) {
----------------
More places where `NodeRef` and `EdgeRef` might be clearer.


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

https://reviews.llvm.org/D75937