[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:
> 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.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:335
+    VertexArray[VI].__edges = EdgeArray + EdgeSize; // terminator node
+    return new GraphT{VertexArray, VertexSize, EdgeArray, EdgeSize,
+                      std::forward<ArgT>(Args)...};
----------------
mattdr wrote:
> this should return a `unique_ptr` to signal ownership transfer
Yes, agree.


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

https://reviews.llvm.org/D75936