[PATCH] D121556: [randstruct] Add randomize structure layout support

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/AST/Randstruct.cpp:156
+  // Produce the new ordering of the elements from the Buckets.
+  SmallVector<FieldDecl *, 16> FinalOrder;
+  for (const std::unique_ptr<Bucket> &B : Buckets) {
----------------



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5912
+    CmdArgs.push_back(
+        Args.MakeArgString("-frandomize-layout-seed=" + Twine(A->getValue(0))));
+
----------------
`A->render(Args, CmdArgs);`


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5915
+  if (Arg *A = Args.getLastArg(options::OPT_frandomize_layout_seed_file_EQ))
+    CmdArgs.push_back(Args.MakeArgString("-frandomize-layout-seed-file=" +
+                                         Twine(A->getValue(0))));
----------------
`A->render(Args, CmdArgs);`


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4244
 
+  if (const Arg *A = Args.getLastArg(OPT_frandomize_layout_seed_file_EQ)) {
+    std::ifstream SeedFile(A->getValue(0));
----------------
Why is -frandomize-layout-seed-file= needed? Can't the user use something like -frandomize-layout-seed=$(<file) ? Or backquotes for POSIX sh compatibility?

The impl uses the very uncommon header <fstream>.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17852
+        !Record->isRandomized()) {
+      SmallVector<Decl *, 32> OrigFieldOrdering(Record->fields());
+      SmallVector<Decl *, 32> NewFieldOrdering;
----------------
32 may be too large. Consider the default inlined number of elements (fit the vector in 64 bytes).


================
Comment at: clang/unittests/AST/RandstructTest.cpp:41
+
+std::unique_ptr<ASTUnit> makeAST(const std::string &SourceCode,
+                                 bool ExpectErr = false) {
----------------
Use static. See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: clang/unittests/AST/RandstructTest.cpp:62
+
+RecordDecl *getRecordDeclFromAST(const ASTContext &C, const std::string &Name) {
+  RecordDecl *RD = FirstDeclMatcher<RecordDecl>().match(
----------------
Use static. See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556