[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 inline comments.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:318
+    }
+    auto *VertexArray = new Node[VertexSize + 1 /* terminator node */];
+    auto *EdgeArray = new Edge[EdgeSize];
----------------
sconstab wrote:
> mattdr wrote:
> > As a general rule `new` is a code-smell in modern C++. This should be a `vector`.
> @mattdr I do agree with the general rule. I also think that in this case where the structure is immutable, std::vector is wasteful because it needs to keep separate values for the current number of elements and the current capacity. At local scope within a function the unneeded value would likely be optimized away, but then there would be an awkward handoff to transfer the data from the vector to the array members.
> 
> I would not want to see the array members changed to vectors, unless LLVM provides an encapsulated array structure that does not need to grow and shrink.
So, first: I'm glad you removed the unnecessary use of `new[]` here and the corresponding (and error-prone!) use of `delete[]` later. That removes a memory leak LLVM won't have to debug.

You suggest here that something other than `std::vector` would be more efficient. If so, would `std::array` suffice? If not, can you explain why static allocation is impossible but dynamic allocation would be too expensive?


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:13
+/// The advantages to this implementation are two-fold:
+/// 1. Iteration and traversal operations should experience terrific caching
+///    performance.
----------------
erm, "terrific"? If there's a substantive argument w.r.t. cache locality etc., please make it explicit.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:16
+/// 2. Set representations and operations on nodes and edges become
+///    extraordinarily efficient. For instance, a set of edges is implemented as
+///    a bit vector, wherein each bit corresponds to one edge in the edge
----------------
"extraordinarily" is, again, not a useful engineering categorization. Please restrict comments to describing quantifiable claims of complexity.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:40
+
+template <typename NodeValueT, typename EdgeValueT, typename SizeT = int>
+class ImmutableGraph {
----------------
Every template argument for a class represents combinatorial addition of complexity for the resulting code. Why do each of these template arguments need to exist? in particular, why does SizeT need to exist?


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

https://reviews.llvm.org/D75936