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

[Scott Constable via Phabricator via cfe-commits]

sconstab added a comment.

Summary points for @craig.topper who has commandeered this diff:

- fix the typo that Matt pointed out
- `SizeT` should not be a template parameter, and `size_type` should be fixed to `int`.
- Maybe have a member reference in `MachineGadgetGraph` to the associated `MachineFunction`.
- Determine how this pass (and other X86 machine passes) should fail on unsupported X86 subtargets.



================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:41
+class ImmutableGraph {
+  using Traits = GraphTraits<ImmutableGraph<NodeValueT, EdgeValueT> *>;
+  template <typename> friend class ImmutableGraphBuilder;
----------------
mattdr wrote:
> I think this self-reference to `ImmutableGraph` dropped the `SizeT` parameter.
Yup. Good catch.


================
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 {
----------------
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.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:79
+
+protected:
+  ImmutableGraph(std::unique_ptr<Node[]> Nodes, std::unique_ptr<Edge[]> Edges,
----------------
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.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:117
+    NodeSet(const ImmutableGraph &G, bool ContainsAll = false)
+        : G{G}, V{static_cast<unsigned>(G.nodes_size()), ContainsAll} {}
+    bool insert(const Node &N) {
----------------
mattdr wrote:
> How do we know that a value of `size_type` (aka `SizeT`) can be cast to `unsigned` without truncation?
Ah. We do not know that. We could have a static assert here, but maybe the best thing to do would be to follow Matt's earlier advice and fix `size_type` to `int`, rather than have it as a template parameter. Anything larger would break the `BitVectors` and/or waste space.


================
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())
----------------
mattdr wrote:
> Cleaner how?
Maybe by keeping a member reference to the associated `MachineFunction`?


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:233
+  if (!STI->useLVILoadHardening() || !STI->is64Bit())
+    return false; // FIXME: support 32-bit
+
----------------
mattdr wrote:
> If the user requests hardening and we can't do it, it seems better to fail loudly so they don't accidentally deploy an unmitigated binary.
@craig.topper I think this is related to the discussion we were having about what would happen for SLH on unsupported subtargets. I'm not sure what the most appropriate solution would be.


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

https://reviews.llvm.org/D75936