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

mattdr added a comment.

Adding a few early style notes for the next round, but overall echo @chandlerc that this seems significantly outside of normal LLVM code.



================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:26
+
+#ifndef IMMUTABLEGRAPH_H
+#define IMMUTABLEGRAPH_H
----------------
It's sort of surprising that the LLVM style guide doesn't call this out explicitly, but `#include` guards are supposed to include the full file path. If they just used the filename, like, this, files with the same name in different paths would collide.

For an example of the expected style, see an adjacent header in this directory: https://github.com/llvm/llvm-project/blob/ba8b3052b59ebee4311d10bee5209dac8747acea/llvm/lib/Target/X86/X86AsmPrinter.h#L10



================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:318
+    }
+    auto *VertexArray = new Node[VertexSize + 1 /* terminator node */];
+    auto *EdgeArray = new Edge[EdgeSize];
----------------
As a general rule `new` is a code-smell in modern C++. This should be a `vector`.


================
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)...};
----------------
this should return a `unique_ptr` to signal ownership transfer


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

https://reviews.llvm.org/D75936