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

[Matthew Riley via Phabricator via llvm-commits]

mattdr added a comment.

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.



================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:73
+    // The end of this Node's edges is the beginning of the next node's edges.
+    const Edge *edges_end() const { return (this + 1)->Edges; }
+    ArrayRef<Edge> edges() const {
----------------
sconstab wrote:
> mattdr wrote:
> > Seems like you also want to add a comment here that we know we will never be asked for `edges_end` for the last stored node -- that is, we know that `this + 1` always refers to a valid Node (which is presumably a dummy/sentinel)
> Not sure I agree. I cannot think of a conventional use of this interface that would perform an operation on the sentinel.
> ```
> G->nodes_end().edges_end(); // invalid use of any end iterator
> SomeNode.edges_end(); // invalid if SomeNode == G->nodes_end()
> ```
> That is, the way that we "know" that we will never be asked for `edges_end()` for the last stored node is that the ask itself would already violate C++ conventions.
I believe any operation on the last `Node` in the array will end up accessing the sentinel:

```
Node* LastNode = G->nodes_begin() + (G->nodes_size() - 1);  // valid reference to the last node
LastNode->edges_end();  // uses `this+1`, accessing the sentinel value in the Nodes array
```



================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:79
+
+protected:
+  ImmutableGraph(std::unique_ptr<Node[]> Nodes, std::unique_ptr<Edge[]> Edges,
----------------
sconstab wrote:
> mattdr wrote:
> > Why "protected" rather than "private"? Usually seeing "protected" makes me think subclassing is expected, but that doesn't seem to be the case here.
> The `MachineGadgetGraph` class actually does subclass `ImmutableGraph` to add some contextual information. I did not want the constructors for `ImmutableGraph` to be public, because the intent is to use the builder. So `protected` seemed like the best option.
Ah, I missed that. I searched through the file for `public ImmutableGraph` and didn't find it because `MachineGadgetGraph` uses the default inheritance specifier.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:83-85
+#define ARG_NODE nullptr
+#define GADGET_EDGE ((int)(-1))
+#define WEIGHT(EdgeValue) ((double)(2 * (EdgeValue) + 1))
----------------
Please replace these with constants or functions.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:113
+          NumFences(NumFences), NumGadgets(NumGadgets) {}
+    MachineFunction &getMF() { // FIXME: This function should be cleaner
+      for (const Node &N : nodes())
----------------
sconstab wrote:
> mattdr wrote:
> > Cleaner how?
> Maybe by keeping a member reference to the associated `MachineFunction`?
Let's put that in the comment instead.


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

https://reviews.llvm.org/D75936