[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Bill Wendling via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG31ea4798ad09: [randstruct] Add test for 
-frandomize-layout-seed-file flag (authored by void).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123636

Files:
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- clang/unittests/AST/RandstructTest.cpp
+++ clang/unittests/AST/RandstructTest.cpp
@@ -27,6 +27,7 @@
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/ToolOutputFile.h"
 
 #include 
 
@@ -36,18 +37,7 @@
 
 using field_names = std::vector;
 
-static std::unique_ptr makeAST(const std::string ) {
-  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
-  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
-
-  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
-
-  return tooling::buildASTFromCodeWithArgs(
-  SourceCode, Args, "input.c", "clang-tool",
-  std::make_shared(),
-  tooling::getClangStripDependencyFileAdjuster(),
-  tooling::FileContentMappings(), );
-}
+constexpr const char Seed[] = "1234567890abcdef";
 
 static RecordDecl *getRecordDeclFromAST(const ASTContext ,
 const std::string ) {
@@ -85,10 +75,64 @@
   return IsSubseq;
 }
 
+static bool recordsEqual(const std::unique_ptr ,
+ const std::unique_ptr ,
+ const std::string ) {
+  const RecordDecl *LHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+  const RecordDecl *RHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+
+  return getFieldNamesFromRecord(LHSRD) == getFieldNamesFromRecord(RHSRD);
+}
+
+static std::unique_ptr
+makeAST(const std::string , bool ExpectError = false,
+std::vector RecordNames = std::vector()) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=" + std::string(Seed));
+
+  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
+
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  int SeedFileFD = -1;
+  llvm::SmallString<256> SeedFilename;
+  EXPECT_FALSE(llvm::sys::fs::createTemporaryFile("seed", "rng", SeedFileFD,
+  SeedFilename));
+  llvm::ToolOutputFile SeedFile(SeedFilename, SeedFileFD);
+  SeedFile.os() << Seed << "\n";
+
+  Args.clear();
+  Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed-file=" +
+ SeedFile.getFilename().str());
+
+  std::unique_ptr ASTFileSeed = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  if (!ExpectError) {
+if (RecordNames.empty())
+  RecordNames.push_back("test");
+
+for (std::string Name : RecordNames)
+  EXPECT_TRUE(recordsEqual(AST, ASTFileSeed, Name));
+  }
+
+  return AST;
+}
+
 namespace clang {
 namespace ast_matchers {
 
-#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+#define RANDSTRUCT_TEST_SUITE_TEST RecordLayoutRandomizationTestSuiteTest
 
 TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
   const field_names Seq = {"a", "b", "c", "d"};
@@ -100,10 +144,10 @@
   EXPECT_FALSE(isSubsequence(Seq, {"a", "d"}));
 }
 
-#define RANDSTRUCT_TEST StructureLayoutRandomization
+#define RANDSTRUCT_TEST RecordLayoutRandomization
 
 TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -121,7 +165,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -139,7 +183,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MarkedRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -157,7 +201,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MismatchedAttrsDeclVsDef) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test __attribute__((randomize_layout));
 struct test {
 int bacon;
@@ -165,11 +209,12 @@
 long long tomato;
 float mayonnaise;
 } __attribute__((no_randomize_layout));
-  )c");
+  )c",
+  

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 422976.
void added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123636

Files:
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- clang/unittests/AST/RandstructTest.cpp
+++ clang/unittests/AST/RandstructTest.cpp
@@ -27,6 +27,7 @@
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/ToolOutputFile.h"
 
 #include 
 
@@ -36,18 +37,7 @@
 
 using field_names = std::vector;
 
-static std::unique_ptr makeAST(const std::string ) {
-  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
-  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
-
-  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
-
-  return tooling::buildASTFromCodeWithArgs(
-  SourceCode, Args, "input.c", "clang-tool",
-  std::make_shared(),
-  tooling::getClangStripDependencyFileAdjuster(),
-  tooling::FileContentMappings(), );
-}
+constexpr const char Seed[] = "1234567890abcdef";
 
 static RecordDecl *getRecordDeclFromAST(const ASTContext ,
 const std::string ) {
@@ -85,10 +75,64 @@
   return IsSubseq;
 }
 
+static bool recordsEqual(const std::unique_ptr ,
+ const std::unique_ptr ,
+ const std::string ) {
+  const RecordDecl *LHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+  const RecordDecl *RHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+
+  return getFieldNamesFromRecord(LHSRD) == getFieldNamesFromRecord(RHSRD);
+}
+
+static std::unique_ptr
+makeAST(const std::string , bool ExpectError = false,
+std::vector RecordNames = std::vector()) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=" + std::string(Seed));
+
+  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
+
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  int SeedFileFD = -1;
+  llvm::SmallString<256> SeedFilename;
+  EXPECT_FALSE(llvm::sys::fs::createTemporaryFile("seed", "rng", SeedFileFD,
+  SeedFilename));
+  llvm::ToolOutputFile SeedFile(SeedFilename, SeedFileFD);
+  SeedFile.os() << Seed << "\n";
+
+  Args.clear();
+  Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed-file=" +
+ SeedFile.getFilename().str());
+
+  std::unique_ptr ASTFileSeed = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  if (!ExpectError) {
+if (RecordNames.empty())
+  RecordNames.push_back("test");
+
+for (std::string Name : RecordNames)
+  EXPECT_TRUE(recordsEqual(AST, ASTFileSeed, Name));
+  }
+
+  return AST;
+}
+
 namespace clang {
 namespace ast_matchers {
 
-#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+#define RANDSTRUCT_TEST_SUITE_TEST RecordLayoutRandomizationTestSuiteTest
 
 TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
   const field_names Seq = {"a", "b", "c", "d"};
@@ -100,10 +144,10 @@
   EXPECT_FALSE(isSubsequence(Seq, {"a", "d"}));
 }
 
-#define RANDSTRUCT_TEST StructureLayoutRandomization
+#define RANDSTRUCT_TEST RecordLayoutRandomization
 
 TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -121,7 +165,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -139,7 +183,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MarkedRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -157,7 +201,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MismatchedAttrsDeclVsDef) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test __attribute__((randomize_layout));
 struct test {
 int bacon;
@@ -165,11 +209,12 @@
 long long tomato;
 float mayonnaise;
 } __attribute__((no_randomize_layout));
-  )c");
+  )c",
+ true);
 
   EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
 
-  DiagnosticsEngine  = AST->getDiagnostics();
+  const 

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:72
+
+  return std::tuple(AST.release(), 
ASTFileSeed.release());
+};

MaskRay wrote:
> void wrote:
> > MaskRay wrote:
> > > aaron.ballman wrote:
> > > > void wrote:
> > > > > aaron.ballman wrote:
> > > > > > Why not keep these as unique pointers and move them into the tuple? 
> > > > > > Then the callers don't have to call delete manually.
> > > > > The d'tors for the `unique_ptr`s is called if I place them in the 
> > > > > tuple. I think it's because I can't do something like this:
> > > > > 
> > > > > ```
> > > > > const unique_ptr std::tie(AST, ASTFileSeed) = makeAST(...);
> > > > > ```
> > > > > 
> > > > > in the test functions. When I assign it as a non-initializer, it 
> > > > > apparently calls the d'tor. So, kinda stumped on what to do.
> > > > > 
> > > > > And the `EXPECT_FALSE` above is used because the `ASSERT_FALSE` adds 
> > > > > an extra return point, which messes with the lambda.
> > > > Okay, thank you for the explanations!
> > > I think std::unique_ptr should and can be used here. See comment above.
> > Yeah, it's just that it doesn't work. I actually tried it a lot of 
> > different ways before, of course, and it just doesn't work. Sorry about 
> > that.
> > 
> > ```
> > /usr/local/google/home/morbo/llvm/llvm-project/clang/unittests/AST/RandstructTest.cpp:73:10:
> >  error: no matching constructor for initialization of 
> > 'std::pair, std::unique_ptr>'
> >   return std::pair, std::unique_ptr>(AST, 
> > ASTFileSeed);
> >  ^ 
> > 
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:266:17:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_ConstructiblePair()' was not 
> > satisfied [with _U1 = std::unique_ptr, _U2 = 
> > std::unique_ptr]
> >   constexpr pair(const _T1& __a, const _T2& __b)
> > ^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:276:26:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_ConstructiblePair()' was not 
> > satisfied [with _U1 = std::unique_ptr, _U2 = 
> > std::unique_ptr]
> >   explicit constexpr pair(const _T1& __a, const _T2& __b)
> >  ^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:322:18:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_MoveCopyPair()' was not satisfied 
> > [with _U1 = std::unique_ptr &]
> >constexpr pair(_U1&& __x, const _T2& __y)
> >  ^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:329:27:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_MoveCopyPair()' was not satisfied 
> > [with _U1 = std::unique_ptr &]
> >explicit constexpr pair(_U1&& __x, const _T2& __y)
> >   ^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:336:18:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_CopyMovePair()' was not satisfied 
> > [with _U2 = std::unique_ptr &]
> >constexpr pair(const _T1& __x, _U2&& __y)
> >  ^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:343:17:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_CopyMovePair()' was not satisfied 
> > [with _U2 = std::unique_ptr &]
> >explicit pair(const _T1& __x, _U2&& __y)
> > ^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:352:12:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_MoveConstructiblePair()' was not 
> > satisfied [with _U1 = std::unique_ptr &, _U2 = 
> > std::unique_ptr &]
> > constexpr pair(_U1&& __x, _U2&& __y)
> >   ^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:361:21:
> >  note: candidate template ignored: requirement '_PCC > std::unique_ptr>, 
> > std::unique_ptr > std::default_delete>>::_MoveConstructiblePair()' was not 
> > satisfied [with _U1 = std::unique_ptr &, _U2 = 
> > std::unique_ptr &]
> > explicit constexpr pair(_U1&& __x, _U2&& __y)
> >^
> > /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:300:19:
> >  note: candidate constructor template not viable: requires single argument 
> > '__p', but 2 arguments were provided
> > constexpr pair(const pair<_U1, _U2>& __p)
> >

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 422965.
void added a comment.

Use "unique_ptr" when possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123636

Files:
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- clang/unittests/AST/RandstructTest.cpp
+++ clang/unittests/AST/RandstructTest.cpp
@@ -27,6 +27,7 @@
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/ToolOutputFile.h"
 
 #include 
 
@@ -36,18 +37,7 @@
 
 using field_names = std::vector;
 
-static std::unique_ptr makeAST(const std::string ) {
-  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
-  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
-
-  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
-
-  return tooling::buildASTFromCodeWithArgs(
-  SourceCode, Args, "input.c", "clang-tool",
-  std::make_shared(),
-  tooling::getClangStripDependencyFileAdjuster(),
-  tooling::FileContentMappings(), );
-}
+constexpr const char Seed[] = "1234567890abcdef";
 
 static RecordDecl *getRecordDeclFromAST(const ASTContext ,
 const std::string ) {
@@ -85,10 +75,64 @@
   return IsSubseq;
 }
 
+static bool recordsEqual(const std::unique_ptr ,
+ const std::unique_ptr ,
+ const std::string ) {
+  const RecordDecl *LHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+  const RecordDecl *RHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+
+  return getFieldNamesFromRecord(LHSRD) == getFieldNamesFromRecord(RHSRD);
+}
+
+static std::unique_ptr
+makeAST(const std::string , bool ExpectError = false,
+std::vector RecordNames = std::vector()) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=" + std::string(Seed));
+
+  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
+
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  int SeedFileFD = -1;
+  llvm::SmallString<256> SeedFilename;
+  EXPECT_FALSE(llvm::sys::fs::createTemporaryFile("seed", "rng", SeedFileFD,
+  SeedFilename));
+  llvm::ToolOutputFile SeedFile(SeedFilename, SeedFileFD);
+  SeedFile.os() << Seed << "\n";
+
+  Args.clear();
+  Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed-file=" +
+ SeedFile.getFilename().str());
+
+  std::unique_ptr ASTFileSeed = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  if (!ExpectError) {
+if (RecordNames.empty())
+  RecordNames.push_back("test");
+
+for (std::string Name : RecordNames)
+  EXPECT_TRUE(recordsEqual(AST, ASTFileSeed, Name));
+  }
+
+  return AST;
+}
+
 namespace clang {
 namespace ast_matchers {
 
-#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+#define RANDSTRUCT_TEST_SUITE_TEST RecordLayoutRandomizationTestSuiteTest
 
 TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
   const field_names Seq = {"a", "b", "c", "d"};
@@ -100,10 +144,10 @@
   EXPECT_FALSE(isSubsequence(Seq, {"a", "d"}));
 }
 
-#define RANDSTRUCT_TEST StructureLayoutRandomization
+#define RANDSTRUCT_TEST RecordLayoutRandomization
 
 TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -121,7 +165,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -139,7 +183,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MarkedRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -157,7 +201,7 @@
 }
 
 TEST(RANDSTRUCT_TEST, MismatchedAttrsDeclVsDef) {
-  const std::unique_ptr AST = makeAST(R"c(
+  std::unique_ptr AST = makeAST(R"c(
 struct test __attribute__((randomize_layout));
 struct test {
 int bacon;
@@ -165,11 +209,12 @@
 long long tomato;
 float mayonnaise;
 } __attribute__((no_randomize_layout));
-  )c");
+  )c",
+ true);
 
   EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
 
-  DiagnosticsEngine  = 

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:72
+
+  return std::tuple(AST.release(), 
ASTFileSeed.release());
+};

void wrote:
> MaskRay wrote:
> > aaron.ballman wrote:
> > > void wrote:
> > > > aaron.ballman wrote:
> > > > > Why not keep these as unique pointers and move them into the tuple? 
> > > > > Then the callers don't have to call delete manually.
> > > > The d'tors for the `unique_ptr`s is called if I place them in the 
> > > > tuple. I think it's because I can't do something like this:
> > > > 
> > > > ```
> > > > const unique_ptr std::tie(AST, ASTFileSeed) = makeAST(...);
> > > > ```
> > > > 
> > > > in the test functions. When I assign it as a non-initializer, it 
> > > > apparently calls the d'tor. So, kinda stumped on what to do.
> > > > 
> > > > And the `EXPECT_FALSE` above is used because the `ASSERT_FALSE` adds an 
> > > > extra return point, which messes with the lambda.
> > > Okay, thank you for the explanations!
> > I think std::unique_ptr should and can be used here. See comment above.
> Yeah, it's just that it doesn't work. I actually tried it a lot of different 
> ways before, of course, and it just doesn't work. Sorry about that.
> 
> ```
> /usr/local/google/home/morbo/llvm/llvm-project/clang/unittests/AST/RandstructTest.cpp:73:10:
>  error: no matching constructor for initialization of 
> 'std::pair, std::unique_ptr>'
>   return std::pair, std::unique_ptr>(AST, 
> ASTFileSeed);
>  ^ 
> 
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:266:17:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_ConstructiblePair()' was not 
> satisfied [with _U1 = std::unique_ptr, _U2 = 
> std::unique_ptr]
>   constexpr pair(const _T1& __a, const _T2& __b)
> ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:276:26:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_ConstructiblePair()' was not 
> satisfied [with _U1 = std::unique_ptr, _U2 = 
> std::unique_ptr]
>   explicit constexpr pair(const _T1& __a, const _T2& __b)
>  ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:322:18:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_MoveCopyPair()' was not satisfied 
> [with _U1 = std::unique_ptr &]
>constexpr pair(_U1&& __x, const _T2& __y)
>  ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:329:27:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_MoveCopyPair()' was not satisfied 
> [with _U1 = std::unique_ptr &]
>explicit constexpr pair(_U1&& __x, const _T2& __y)
>   ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:336:18:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_CopyMovePair()' was not satisfied 
> [with _U2 = std::unique_ptr &]
>constexpr pair(const _T1& __x, _U2&& __y)
>  ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:343:17:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_CopyMovePair()' was not satisfied 
> [with _U2 = std::unique_ptr &]
>explicit pair(const _T1& __x, _U2&& __y)
> ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:352:12:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_MoveConstructiblePair()' was not 
> satisfied [with _U1 = std::unique_ptr &, _U2 = 
> std::unique_ptr &]
> constexpr pair(_U1&& __x, _U2&& __y)
>   ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:361:21:
>  note: candidate template ignored: requirement '_PCC std::unique_ptr>, 
> std::unique_ptr std::default_delete>>::_MoveConstructiblePair()' was not 
> satisfied [with _U1 = std::unique_ptr &, _U2 = 
> std::unique_ptr &]
> explicit constexpr pair(_U1&& __x, _U2&& __y)
>^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:300:19:
>  note: candidate constructor template not viable: requires single argument 
> '__p', but 2 arguments were provided
> constexpr pair(const pair<_U1, _U2>& __p)
>   ^
> /usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:309:21:
>  note: candidate constructor template not viable: requires single argument 
> '__p', but 2 arguments were provided
>  

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Bill Wendling via Phabricator via cfe-commits]

void marked an inline comment as done.
void added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:72
+
+  return std::tuple(AST.release(), 
ASTFileSeed.release());
+};

MaskRay wrote:
> aaron.ballman wrote:
> > void wrote:
> > > aaron.ballman wrote:
> > > > Why not keep these as unique pointers and move them into the tuple? 
> > > > Then the callers don't have to call delete manually.
> > > The d'tors for the `unique_ptr`s is called if I place them in the tuple. 
> > > I think it's because I can't do something like this:
> > > 
> > > ```
> > > const unique_ptr std::tie(AST, ASTFileSeed) = makeAST(...);
> > > ```
> > > 
> > > in the test functions. When I assign it as a non-initializer, it 
> > > apparently calls the d'tor. So, kinda stumped on what to do.
> > > 
> > > And the `EXPECT_FALSE` above is used because the `ASSERT_FALSE` adds an 
> > > extra return point, which messes with the lambda.
> > Okay, thank you for the explanations!
> I think std::unique_ptr should and can be used here. See comment above.
Yeah, it's just that it doesn't work. I actually tried it a lot of different 
ways before, of course, and it just doesn't work. Sorry about that.

```
/usr/local/google/home/morbo/llvm/llvm-project/clang/unittests/AST/RandstructTest.cpp:73:10:
 error: no matching constructor for initialization of 
'std::pair, std::unique_ptr>'
  return std::pair, std::unique_ptr>(AST, 
ASTFileSeed);
 ^ 

/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:266:17:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_ConstructiblePair()' was not satisfied 
[with _U1 = std::unique_ptr, _U2 = 
std::unique_ptr]
  constexpr pair(const _T1& __a, const _T2& __b)
^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:276:26:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_ConstructiblePair()' was not satisfied 
[with _U1 = std::unique_ptr, _U2 = 
std::unique_ptr]
  explicit constexpr pair(const _T1& __a, const _T2& __b)
 ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:322:18:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_MoveCopyPair()' was not satisfied [with 
_U1 = std::unique_ptr &]
   constexpr pair(_U1&& __x, const _T2& __y)
 ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:329:27:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_MoveCopyPair()' was not satisfied [with 
_U1 = std::unique_ptr &]
   explicit constexpr pair(_U1&& __x, const _T2& __y)
  ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:336:18:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_CopyMovePair()' was not satisfied [with 
_U2 = std::unique_ptr &]
   constexpr pair(const _T1& __x, _U2&& __y)
 ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:343:17:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_CopyMovePair()' was not satisfied [with 
_U2 = std::unique_ptr &]
   explicit pair(const _T1& __x, _U2&& __y)
^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:352:12:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_MoveConstructiblePair()' was not 
satisfied [with _U1 = std::unique_ptr &, _U2 = 
std::unique_ptr &]
constexpr pair(_U1&& __x, _U2&& __y)
  ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:361:21:
 note: candidate template ignored: requirement '_PCC>, 
std::unique_ptr>>::_MoveConstructiblePair()' was not 
satisfied [with _U1 = std::unique_ptr &, _U2 = 
std::unique_ptr &]
explicit constexpr pair(_U1&& __x, _U2&& __y)
   ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:300:19:
 note: candidate constructor template not viable: requires single argument 
'__p', but 2 arguments were provided
constexpr pair(const pair<_U1, _U2>& __p)
  ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:309:21:
 note: candidate constructor template not viable: requires single argument 
'__p', but 2 arguments were provided
explicit constexpr pair(const pair<_U1, _U2>& __p)
   ^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:314:17:
 note: candidate constructor not viable: requires 1 argument, but 2 were 
provided
  constexpr pair(const pair&) = default;///< Copy constructor
^
/usr/lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_pair.h:315:17:
 note: candidate 

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:40
 
-static std::unique_ptr makeAST(const std::string ) {
+static std::string Seed = "1234567890abcdef";
+

`constexpr llvm::StringLiteral Seed = ...` or `constexpr char Seed[]`



Comment at: clang/unittests/AST/RandstructTest.cpp:42
+
+static auto makeAST = [](const std::string ) {
   std::vector Args = getCommandLineArgsForTesting(Lang_C99);

Change this to a normal function

```
static std::pair, std::unique_ptr>
makeAST(llvm::StringRef SourceCode) {
```



Comment at: clang/unittests/AST/RandstructTest.cpp:72
+
+  return std::tuple(AST.release(), 
ASTFileSeed.release());
+};

aaron.ballman wrote:
> void wrote:
> > aaron.ballman wrote:
> > > Why not keep these as unique pointers and move them into the tuple? Then 
> > > the callers don't have to call delete manually.
> > The d'tors for the `unique_ptr`s is called if I place them in the tuple. I 
> > think it's because I can't do something like this:
> > 
> > ```
> > const unique_ptr std::tie(AST, ASTFileSeed) = makeAST(...);
> > ```
> > 
> > in the test functions. When I assign it as a non-initializer, it apparently 
> > calls the d'tor. So, kinda stumped on what to do.
> > 
> > And the `EXPECT_FALSE` above is used because the `ASSERT_FALSE` adds an 
> > extra return point, which messes with the lambda.
> Okay, thank you for the explanations!
I think std::unique_ptr should and can be used here. See comment above.



Comment at: clang/unittests/AST/RandstructTest.cpp:118
+
+  std::vector LHSFields = getFieldNamesFromRecord(LHSRD);
+  std::vector RHSFields = getFieldNamesFromRecord(RHSRD);

`return getFieldNamesFromRecord(LHSRD) == getFieldNamesFromRecord(RHSRD);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123636

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM but please wait a bit in case @MaskRay has comments.




Comment at: clang/unittests/AST/RandstructTest.cpp:72
+
+  return std::tuple(AST.release(), 
ASTFileSeed.release());
+};

void wrote:
> aaron.ballman wrote:
> > Why not keep these as unique pointers and move them into the tuple? Then 
> > the callers don't have to call delete manually.
> The d'tors for the `unique_ptr`s is called if I place them in the tuple. I 
> think it's because I can't do something like this:
> 
> ```
> const unique_ptr std::tie(AST, ASTFileSeed) = makeAST(...);
> ```
> 
> in the test functions. When I assign it as a non-initializer, it apparently 
> calls the d'tor. So, kinda stumped on what to do.
> 
> And the `EXPECT_FALSE` above is used because the `ASSERT_FALSE` adds an extra 
> return point, which messes with the lambda.
Okay, thank you for the explanations!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123636

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:56
+  llvm::SmallString<256> SeedFilename;
+  EXPECT_FALSE(llvm::sys::fs::createTemporaryFile("seed", "rng", SeedFileFD,
+  SeedFilename));

aaron.ballman wrote:
> I think this is a case where you want to use `ASSERT_FALSE` because if this 
> fails, the rest of the test also fails.
There's an explanation below.



Comment at: clang/unittests/AST/RandstructTest.cpp:72
+
+  return std::tuple(AST.release(), 
ASTFileSeed.release());
+};

aaron.ballman wrote:
> Why not keep these as unique pointers and move them into the tuple? Then the 
> callers don't have to call delete manually.
The d'tors for the `unique_ptr`s is called if I place them in the tuple. I 
think it's because I can't do something like this:

```
const unique_ptr std::tie(AST, ASTFileSeed) = makeAST(...);
```

in the test functions. When I assign it as a non-initializer, it apparently 
calls the d'tor. So, kinda stumped on what to do.

And the `EXPECT_FALSE` above is used because the `ASSERT_FALSE` adds an extra 
return point, which messes with the lambda.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123636

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:56
+  llvm::SmallString<256> SeedFilename;
+  EXPECT_FALSE(llvm::sys::fs::createTemporaryFile("seed", "rng", SeedFileFD,
+  SeedFilename));

I think this is a case where you want to use `ASSERT_FALSE` because if this 
fails, the rest of the test also fails.



Comment at: clang/unittests/AST/RandstructTest.cpp:72
+
+  return std::tuple(AST.release(), 
ASTFileSeed.release());
+};

Why not keep these as unique pointers and move them into the tuple? Then the 
callers don't have to call delete manually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123636

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123636: [randstruct] Add test for "-frandomize-layout-seed-file" flag

[Bill Wendling via Phabricator via cfe-commits]

void created this revision.
void added reviewers: aaron.ballman, MaskRay.
Herald added a subscriber: StephenFan.
Herald added a project: All.
void requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This test makes sure that the "-frandomize-layout-seed" and
"-frandomize-layout-seed-file" flags generate the same layout for the
record.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123636

Files:
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- clang/unittests/AST/RandstructTest.cpp
+++ clang/unittests/AST/RandstructTest.cpp
@@ -27,6 +27,7 @@
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/ToolOutputFile.h"
 
 #include 
 
@@ -36,18 +37,40 @@
 
 using field_names = std::vector;
 
-static std::unique_ptr makeAST(const std::string ) {
+static std::string Seed = "1234567890abcdef";
+
+static auto makeAST = [](const std::string ) {
   std::vector Args = getCommandLineArgsForTesting(Lang_C99);
-  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+  Args.push_back("-frandomize-layout-seed=" + Seed);
 
   IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
 
-  return tooling::buildASTFromCodeWithArgs(
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
   SourceCode, Args, "input.c", "clang-tool",
   std::make_shared(),
   tooling::getClangStripDependencyFileAdjuster(),
   tooling::FileContentMappings(), );
-}
+
+  int SeedFileFD = -1;
+  llvm::SmallString<256> SeedFilename;
+  EXPECT_FALSE(llvm::sys::fs::createTemporaryFile("seed", "rng", SeedFileFD,
+  SeedFilename));
+  llvm::ToolOutputFile SeedFile(SeedFilename, SeedFileFD);
+  SeedFile.os() << Seed << "\n";
+
+  Args.clear();
+  Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed-file=" +
+ SeedFile.getFilename().str());
+
+  std::unique_ptr ASTFileSeed = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  return std::tuple(AST.release(), ASTFileSeed.release());
+};
 
 static RecordDecl *getRecordDeclFromAST(const ASTContext ,
 const std::string ) {
@@ -85,6 +108,19 @@
   return IsSubseq;
 }
 
+static bool recordsEqual(const ASTUnit *LHS, const ASTUnit *RHS,
+ std::string RecordName) {
+  const RecordDecl *LHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+  const RecordDecl *RHSRD =
+  getRecordDeclFromAST(LHS->getASTContext(), RecordName);
+
+  std::vector LHSFields = getFieldNamesFromRecord(LHSRD);
+  std::vector RHSFields = getFieldNamesFromRecord(RHSRD);
+
+  return LHSFields == RHSFields;
+}
+
 namespace clang {
 namespace ast_matchers {
 
@@ -103,7 +139,8 @@
 #define RANDSTRUCT_TEST StructureLayoutRandomization
 
 TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
-  const std::unique_ptr AST = makeAST(R"c(
+  const ASTUnit *AST, *ASTFileSeed;
+  std::tie(AST, ASTFileSeed) = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -118,10 +155,13 @@
 
   EXPECT_FALSE(RD->hasAttr());
   EXPECT_FALSE(RD->isRandomized());
+  delete AST;
+  delete ASTFileSeed;
 }
 
 TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  const ASTUnit *AST, *ASTFileSeed;
+  std::tie(AST, ASTFileSeed) = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -134,12 +174,16 @@
 
   const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
 
+  EXPECT_TRUE(recordsEqual(AST, ASTFileSeed, "test"));
   EXPECT_TRUE(RD->hasAttr());
   EXPECT_FALSE(RD->isRandomized());
+  delete AST;
+  delete ASTFileSeed;
 }
 
 TEST(RANDSTRUCT_TEST, MarkedRandomize) {
-  const std::unique_ptr AST = makeAST(R"c(
+  const ASTUnit *AST, *ASTFileSeed;
+  std::tie(AST, ASTFileSeed) = makeAST(R"c(
 struct test {
 int bacon;
 long lettuce;
@@ -152,12 +196,16 @@
 
   const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
 
+  EXPECT_TRUE(recordsEqual(AST, ASTFileSeed, "test"));
   EXPECT_TRUE(RD->hasAttr());
   EXPECT_TRUE(RD->isRandomized());
+  delete AST;
+  delete ASTFileSeed;
 }
 
 TEST(RANDSTRUCT_TEST, MismatchedAttrsDeclVsDef) {
-  const std::unique_ptr AST = makeAST(R"c(
+  const ASTUnit *AST, *ASTFileSeed;
+  std::tie(AST, ASTFileSeed) = makeAST(R"c(
 struct test __attribute__((randomize_layout));
 struct test {
 int bacon;
@@ -169,18 +217,21 @@
 
   EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
 
-  DiagnosticsEngine  = AST->getDiagnostics();
+  const DiagnosticsEngine  =