[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 llvm-commits]

sconstab 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];
----------------
mattdr wrote:
> 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?
A statically sized array (e.g., std::array) is insufficient because the size in this case is not compiler determinable; a dynamically sized and dynamically resizable array (e.g., std::vector) is sufficient but overly costly; a dynamically sized and dynamically //unresizable// array is sufficient and has minimal cost.


================
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.
----------------
mattdr wrote:
> erm, "terrific"? If there's a substantive argument w.r.t. cache locality etc., please make it explicit.
This is valid. I will reword.


================
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
----------------
mattdr wrote:
> "extraordinarily" is, again, not a useful engineering categorization. Please restrict comments to describing quantifiable claims of complexity.
AFAIK there is not a precise engineering term for "tiny O(1)." Nonetheless I will reword.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:40
+
+template <typename NodeValueT, typename EdgeValueT, typename SizeT = int>
+class ImmutableGraph {
----------------
mattdr wrote:
> 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?
I suspect that there may be more uses for this data structure and that eventually it may migrate to ADT. I have SizeT as a template argument because I found it plenty sufficient to have `int` as the size parameter for the array bounds, but I suspect other uses may require `size_t`.


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

https://reviews.llvm.org/D75936