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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3445124 , @aaron.ballman 
wrote:

> In D121556#3444837 , @void wrote:
>
>> In D121556#334 , @MaskRay 
>> wrote:
>>
>>> In D121556#3444260 , @void wrote:
>>>
 In D121556#3444221 , @MaskRay 
 wrote:

> In D121556#3444131 , @void 
> wrote:
>
>> In D121556#3444021 , @MaskRay 
>> wrote:
>>
>>> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
>>>  
>>> still uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes 
>>> I did.
>>
>> You said it was still failing after the std::shuffle to llvm::shuffle 
>> change.
>
> By saying it still failed, I meant there were other Windows vs 
> non-Windows differences, not that std::shuffle=>llvm::shuffle was an 
> unintended change.

 So does it work or not? If I change it to `llvm::shuffle`, will the tests 
 fail or will they pass regardless of the platform?
>>>
>>> If you change `std::shuffle` to `llvm::shuffle` and  add back tests like 
>>> `EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));`, the test will likely 
>>> fail on Windows.
>>> I recall that @aaron.ballman uses Windows and may help you find the 
>>> differences.
>>
>> It was also failing on MacOS. And it failed a different way on another 
>> Windows version. That's why I removed the test for the deterministic shuffle.
>>
>>> I tend to agree with your `I think it's just a case where Windows' 
>>> algorithm for std::mt19937 is subtly different than the one for Linux.`
>>
>> The only other option would be to add the `EXPECT_*` stuff on one platform 
>> (like Linux). I suppose that would be better than nothing.
>
> Here's what has me confused... You're using `std::mt19937` which is a very 
> specific random number algorithm and you're giving it the same seed. I don't 
> think we *should* be getting different behavior from the random number 
> generator across platforms. The issue is the call to `std::shuffle` (see 
> https://en.cppreference.com/w/cpp/algorithm/random_shuffle#Notes), so I think 
> using `llvm::shuffle` will fix all the tests on all the platforms.

Okay. So I'll make this change and submit it during a time when there isn't a 
lot of activity so that I can revert it if it fails without annoying too many 
people.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D121556#3444837 , @void wrote:

> In D121556#334 , @MaskRay wrote:
>
>> In D121556#3444260 , @void wrote:
>>
>>> In D121556#3444221 , @MaskRay 
>>> wrote:
>>>
 In D121556#3444131 , @void wrote:

> In D121556#3444021 , @MaskRay 
> wrote:
>
>> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
>>  
>> still uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I 
>> did.
>
> You said it was still failing after the std::shuffle to llvm::shuffle 
> change.

 By saying it still failed, I meant there were other Windows vs non-Windows 
 differences, not that std::shuffle=>llvm::shuffle was an unintended change.
>>>
>>> So does it work or not? If I change it to `llvm::shuffle`, will the tests 
>>> fail or will they pass regardless of the platform?
>>
>> If you change `std::shuffle` to `llvm::shuffle` and  add back tests like 
>> `EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));`, the test will likely 
>> fail on Windows.
>> I recall that @aaron.ballman uses Windows and may help you find the 
>> differences.
>
> It was also failing on MacOS. And it failed a different way on another 
> Windows version. That's why I removed the test for the deterministic shuffle.
>
>> I tend to agree with your `I think it's just a case where Windows' algorithm 
>> for std::mt19937 is subtly different than the one for Linux.`
>
> The only other option would be to add the `EXPECT_*` stuff on one platform 
> (like Linux). I suppose that would be better than nothing.

Here's what has me confused... You're using `std::mt19937` which is a very 
specific random number algorithm and you're giving it the same seed. I don't 
think we *should* be getting different behavior from the random number 
generator across platforms. The issue is the call to `std::shuffle` (see 
https://en.cppreference.com/w/cpp/algorithm/random_shuffle#Notes), so I think 
using `llvm::shuffle` will fix all the tests on all the platforms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#334 , @MaskRay wrote:

> In D121556#3444260 , @void wrote:
>
>> In D121556#3444221 , @MaskRay 
>> wrote:
>>
>>> In D121556#3444131 , @void wrote:
>>>
 In D121556#3444021 , @MaskRay 
 wrote:

> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
>  
> still uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I 
> did.

 You said it was still failing after the std::shuffle to llvm::shuffle 
 change.
>>>
>>> By saying it still failed, I meant there were other Windows vs non-Windows 
>>> differences, not that std::shuffle=>llvm::shuffle was an unintended change.
>>
>> So does it work or not? If I change it to `llvm::shuffle`, will the tests 
>> fail or will they pass regardless of the platform?
>
> If you change `std::shuffle` to `llvm::shuffle` and  add back tests like 
> `EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));`, the test will likely 
> fail on Windows.
> I recall that @aaron.ballman uses Windows and may help you find the 
> differences.

It was also failing on MacOS. And it failed a different way on another Windows 
version. That's why I removed the test for the deterministic shuffle.

> I tend to agree with your `I think it's just a case where Windows' algorithm 
> for std::mt19937 is subtly different than the one for Linux.`

The only other option would be to add the `EXPECT_*` stuff on one platform 
(like Linux). I suppose that would be better than nothing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D121556#3444260 , @void wrote:

> In D121556#3444221 , @MaskRay wrote:
>
>> In D121556#3444131 , @void wrote:
>>
>>> In D121556#3444021 , @MaskRay 
>>> wrote:
>>>
 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
  
 still uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I 
 did.
>>>
>>> You said it was still failing after the std::shuffle to llvm::shuffle 
>>> change.
>>
>> By saying it still failed, I meant there were other Windows vs non-Windows 
>> differences, not that std::shuffle=>llvm::shuffle was an unintended change.
>
> So does it work or not? If I change it to `llvm::shuffle`, will the tests 
> fail or will they pass regardless of the platform?

If you change `std::shuffle` to `llvm::shuffle` and  add back tests like 
`EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));`, the test will likely fail 
on Windows.
I recall that @aaron.ballman uses Windows and may help you find the differences.
I tend to agree with your `I think it's just a case where Windows' algorithm 
for std::mt19937 is subtly different than the one for Linux.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3444221 , @MaskRay wrote:

> In D121556#3444131 , @void wrote:
>
>> In D121556#3444021 , @MaskRay 
>> wrote:
>>
>>> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
>>>  still 
>>> uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I did.
>>
>> You said it was still failing after the std::shuffle to llvm::shuffle change.
>
> By saying it still failed, I meant there were other Windows vs non-Windows 
> differences, not that std::shuffle=>llvm::shuffle was an unintended change.

So does it work or not? If I change it to `llvm::shuffle`, will the tests fail 
or will they pass regardless of the platform?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D121556#3444131 , @void wrote:

> In D121556#3444021 , @MaskRay wrote:
>
>> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
>>  still 
>> uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I did.
>
> You said it was still failing after the std::shuffle to llvm::shuffle change.

By saying it still failed, I meant there were other Windows vs non-Windows 
differences, not that std::shuffle=>llvm::shuffle was an unintended change.

I wondered why the test did not fail again when you re-landed it. Now I see: 
you simply removed all order checks like `EXPECT_EQ(Expected, 
getFieldNamesFromRecord(RD));`
The tests seem overly relaxing and no longer serve the original purposes to 
catch errors (if the algorithm is changed to not randomize at all, I suspect 
the tests will pass as well).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3444021 , @MaskRay wrote:

> 7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
>  still 
> uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I did.

You said it was still failing after the std::shuffle to llvm::shuffle change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3444026 , @MaskRay wrote:

> In D121556#3444015 , @void wrote:
>
>> In D121556#3444006 , @MaskRay 
>> wrote:
>>
>>> The relanded form still lacks the -f option testing I requested.
>>
>> The unit test tests the `-frandomize-layout-seed=` option already.
>
> OK, I see. Then test `-frandomize-layout-seed-file=` as well? I cannot find 
> its test.

Okay, I'll add a test for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D121556#3444015 , @void wrote:

> In D121556#3444006 , @MaskRay wrote:
>
>> The relanded form still lacks the -f option testing I requested.
>
> The unit test tests the `-frandomize-layout-seed=` option already.

OK, I see. Then test `-frandomize-layout-seed-file=` as well? I cannot find its 
test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 
 still 
uses `std::shuffle`, not incorporating the `llvm::shuffle` fixes I did.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3444006 , @MaskRay wrote:

> The relanded form still lacks the -f option testing I requested.

The unit test tests the `-frandomize-layout-seed=` option already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:154-158
+#ifdef _WIN32
+  const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"};
+#else
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
+#endif

jyknight wrote:
> aaron.ballman wrote:
> > void wrote:
> > > aaron.ballman wrote:
> > > > Any idea what's gone wrong here? (Do we have a bug to file because 
> > > > these come out reversed? If so, can you add a FIXME comment here that 
> > > > we expect this test to change someday?)
> > > I think it's just a case where Windows' algorithm for `std::mt19937` is 
> > > subtly different than the one for Linux. I'm not sure we should worry 
> > > about it too much, to be honest. As long as it produces a deterministic 
> > > output on the same platform we should be fine. I think it's expected that 
> > > the same compiler/environment is used during all compilation steps. 
> > > (I.e., one's not going to compile a module on Windows for a kernel build 
> > > on Linux.)
> > Okay, that's a great reason for this to be left alone.
> I do think that as an //ideal//, a compile run on one host platform ought to 
> produce the exact same output as a compile run on another (presuming a triple 
> and sysroot are provided, and mumblemumble file-paths).
> 
> But I have no idea how close or far we currently are from such an ideal.
I completely agree. Alternatively, we could use some other RNG-type algorithm 
that will be the same across platforms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

The relanded form still lacks the -f option testing I requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: clang/unittests/AST/RandstructTest.cpp:154-158
+#ifdef _WIN32
+  const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"};
+#else
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
+#endif

aaron.ballman wrote:
> void wrote:
> > aaron.ballman wrote:
> > > Any idea what's gone wrong here? (Do we have a bug to file because these 
> > > come out reversed? If so, can you add a FIXME comment here that we expect 
> > > this test to change someday?)
> > I think it's just a case where Windows' algorithm for `std::mt19937` is 
> > subtly different than the one for Linux. I'm not sure we should worry about 
> > it too much, to be honest. As long as it produces a deterministic output on 
> > the same platform we should be fine. I think it's expected that the same 
> > compiler/environment is used during all compilation steps. (I.e., one's not 
> > going to compile a module on Windows for a kernel build on Linux.)
> Okay, that's a great reason for this to be left alone.
I do think that as an //ideal//, a compile run on one host platform ought to 
produce the exact same output as a compile run on another (presuming a triple 
and sysroot are provided, and mumblemumble file-paths).

But I have no idea how close or far we currently are from such an ideal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7aa8c38a9e19: [randstruct] Add randomize structure layout 
support (authored by connorkuehl, committed by void).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,424 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+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(), );
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  EXPECT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  EXPECT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  EXPECT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  EXPECT_TRUE(isSubsequence(Seq, {"a"}));
+  EXPECT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+
+  

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



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));

aaron.ballman wrote:
> MaskRay wrote:
> > void wrote:
> > > MaskRay wrote:
> > > > Why is -frandomize-layout-seed-file= needed? Can't the user use 
> > > > something like -frandomize-layout-seed=$( > > > POSIX sh compatibility?
> > > > 
> > > > The impl uses the very uncommon header .
> > > That seems a bit clunky to me. If you don't like it, I can just remove 
> > > the option entirely. Wish you would have mentioned these concerns 
> > > earlier...like in the several weeks this has been in review.
> > > 
> > > The `fstream` header is used in other places. If there's a better 
> > > alternative, please suggest one.
> > > 
> > I was a subscriber only vaguely aware of this patch and mostly absent in 
> > the past 2 weeks on trips (which meant I spent really little time on 
> > reading patches) :)
> > 
> > I just hope that every option added is useful. A thing that is not so 
> > necessarily can be delayed until it is actually needed.
> > 
> > Just noticed that there is test coverage gap that the cc1 options are 
> > completely untested. There are unit tests, but no lit test.
> > I just hope that every option added is useful. A thing that is not so 
> > necessarily can be delayed until it is actually needed.
> 
> I think this option is useful. Windows' cmd.exe doesn't make it particularly 
> trivial to pipe contents to an argument in a command line, but also, IDEs 
> don't always make it obvious how you would pipe the seed content into a file 
> either. I don't see an issue with using `fstream` either; we use it when 
> necessary.
Ah, LG! 

Just need some tests for the two -f -cc1 options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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));

MaskRay wrote:
> void wrote:
> > MaskRay wrote:
> > > Why is -frandomize-layout-seed-file= needed? Can't the user use something 
> > > like -frandomize-layout-seed=$( > > compatibility?
> > > 
> > > The impl uses the very uncommon header .
> > That seems a bit clunky to me. If you don't like it, I can just remove the 
> > option entirely. Wish you would have mentioned these concerns 
> > earlier...like in the several weeks this has been in review.
> > 
> > The `fstream` header is used in other places. If there's a better 
> > alternative, please suggest one.
> > 
> I was a subscriber only vaguely aware of this patch and mostly absent in the 
> past 2 weeks on trips (which meant I spent really little time on reading 
> patches) :)
> 
> I just hope that every option added is useful. A thing that is not so 
> necessarily can be delayed until it is actually needed.
> 
> Just noticed that there is test coverage gap that the cc1 options are 
> completely untested. There are unit tests, but no lit test.
> I just hope that every option added is useful. A thing that is not so 
> necessarily can be delayed until it is actually needed.

I think this option is useful. Windows' cmd.exe doesn't make it particularly 
trivial to pipe contents to an argument in a command line, but also, IDEs don't 
always make it obvious how you would pipe the seed content into a file either. 
I don't see an issue with using `fstream` either; we use it when necessary.



Comment at: clang/unittests/AST/RandstructTest.cpp:41
+
+std::unique_ptr makeAST(const std::string ,
+ bool ExpectErr = false) {

void wrote:
> MaskRay wrote:
> > Use static. See 
> > https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
> Nah.
FWIW, I agree with this feedback -- please follow the coding standards unless 
there's strong incentive not to (which I don't think there is here). Sorry for 
not catching this before during review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



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));

void wrote:
> MaskRay wrote:
> > Why is -frandomize-layout-seed-file= needed? Can't the user use something 
> > like -frandomize-layout-seed=$( > compatibility?
> > 
> > The impl uses the very uncommon header .
> That seems a bit clunky to me. If you don't like it, I can just remove the 
> option entirely. Wish you would have mentioned these concerns earlier...like 
> in the several weeks this has been in review.
> 
> The `fstream` header is used in other places. If there's a better 
> alternative, please suggest one.
> 
I was a subscriber only vaguely aware of this patch and mostly absent in the 
past 2 weeks on trips (which meant I spent really little time on reading 
patches) :)

I just hope that every option added is useful. A thing that is not so 
necessarily can be delayed until it is actually needed.

Just noticed that there is test coverage gap that the cc1 options are 
completely untested. There are unit tests, but no lit test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

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

Fix test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,424 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+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(), );
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  EXPECT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  EXPECT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  EXPECT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  EXPECT_TRUE(isSubsequence(Seq, {"a"}));
+  EXPECT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+
+  EXPECT_FALSE(RD->hasAttr());
+  EXPECT_FALSE(RD->isRandomized());
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long 

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

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



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));

MaskRay wrote:
> Why is -frandomize-layout-seed-file= needed? Can't the user use something 
> like -frandomize-layout-seed=$( compatibility?
> 
> The impl uses the very uncommon header .
That seems a bit clunky to me. If you don't like it, I can just remove the 
option entirely. Wish you would have mentioned these concerns earlier...like in 
the several weeks this has been in review.

The `fstream` header is used in other places. If there's a better alternative, 
please suggest one.




Comment at: clang/lib/Sema/SemaDecl.cpp:17852
+!Record->isRandomized()) {
+  SmallVector OrigFieldOrdering(Record->fields());
+  SmallVector NewFieldOrdering;

MaskRay wrote:
> 32 may be too large. Consider the default inlined number of elements (fit the 
> vector in 64 bytes).
Linux has many structures with far more fields. In fact, it's probably not 
useful to randomize structures with a small number of fields.



Comment at: clang/unittests/AST/RandstructTest.cpp:41
+
+std::unique_ptr makeAST(const std::string ,
+ bool ExpectErr = false) {

MaskRay wrote:
> Use static. See 
> https://llvm.org/docs/CodingStandards.html#anonymous-namespaces
Nah.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3440383 , @MaskRay wrote:

> Windows test is still failing after the std::shuffle => llvm::shuffle change, 
> guess time for revert.
>
>   ../../clang/unittests/AST/RandstructTest.cpp(165): error: Expected equality 
> of these values:
> Expected
>   Which is: { "lettuce", "bacon", "mayonnaise", "tomato" }
> getFieldNamesFromRecord(RD)
>   Which is: { "bacon", "lettuce", "mayonnaise", "tomato" }
>   [  FAILED  ] StructureLayoutRandomization.MarkedRandomize (47 ms)
>   [--] 1 test from StructureLayoutRandomization (47 ms total)
>   
>   [--] Global test environment tear-down
>   [==] 1 test from 1 test suite ran. (47 ms total)
>   [  PASSED  ] 0 tests.
>   [  FAILED  ] 1 test, listed below:
>   [  FAILED  ] StructureLayoutRandomization.MarkedRandomize
>   
>1 FAILED TEST
>   
>   
>   Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
>   
>   Failed Tests (1):
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize

Did this fail after the test was removed? If not, then I don't appreciate you 
reverting my patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

[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 FinalOrder;
+  for (const std::unique_ptr  : 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=$(.



Comment at: clang/lib/Sema/SemaDecl.cpp:17852
+!Record->isRandomized()) {
+  SmallVector OrigFieldOrdering(Record->fields());
+  SmallVector 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 makeAST(const std::string ,
+ 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 , const std::string ) 
{
+  RecordDecl *RD = FirstDeclMatcher().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

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Windows test is still failing after the std::shuffle => llvm::shuffle change, 
guess time for revert.

  ../../clang/unittests/AST/RandstructTest.cpp(165): error: Expected equality 
of these values:
Expected
  Which is: { "lettuce", "bacon", "mayonnaise", "tomato" }
getFieldNamesFromRecord(RD)
  Which is: { "bacon", "lettuce", "mayonnaise", "tomato" }
  [  FAILED  ] StructureLayoutRandomization.MarkedRandomize (47 ms)
  [--] 1 test from StructureLayoutRandomization (47 ms total)
  
  [--] Global test environment tear-down
  [==] 1 test from 1 test suite ran. (47 ms total)
  [  PASSED  ] 0 tests.
  [  FAILED  ] 1 test, listed below:
  [  FAILED  ] StructureLayoutRandomization.MarkedRandomize
  
   1 FAILED TEST
  
  
  Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
  
  Failed Tests (1):
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

You can use llvm::shuffle to avoid STL impl difference




Comment at: clang/lib/AST/Randstruct.cpp:160
+if (!B->isBitfieldRun())
+  std::shuffle(std::begin(RandFields), std::end(RandFields), RNG);
+

`llvm::shuffle` to avoid 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Why? This is a fairly small CL. It should be easy to reland, and there 
shouldn't be any rebasing pain.

You should at least check with your reviewers that they're fine relanding this 
without test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3440308 , @thakis wrote:

> It's more important to remove it from the cmakelists file than to delete the 
> source file ;)
>
> Please revert the whole thing instead of just removing the test coverage 
> though. You can reland when the test is figured out, and that makes sure this 
> doesn't stay in without coverage.

I'd rather not re-land the whole thing. The test isn't that important, and 
quite frankly it might not be possible to test it with the unittest. Too many 
architectures seem to have a different randomization algorithm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

It's more important to remove it from the cmakelists file than to delete the 
source file ;)

Please revert the whole thing instead of just removing the test coverage 
though. You can reland when the test is figured out, and that makes sure this 
doesn't stay in without coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3440290 , @thakis wrote:

> Looks like this breaks tests on mac: http://45.33.8.238/macm1/32938/step_7.txt
>
> Please take a look, and revert for now if it takes a while to fix.

Ugh! This sucks. I just removed the test until I can get it to work on all 
platforms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Looks like this breaks tests on mac: http://45.33.8.238/macm1/32938/step_7.txt

Please take a look, and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3440089 , @dyung wrote:

> Hi, the test you added is failing on the PS4 Windows bot 
> https://lab.llvm.org/buildbot/#/builders/216/builds/2647.
>
> Can you take a look?

Crud! I thought I got all of the Windows issues. I'll fix soon. Sorry about 
that!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Douglas Yung via Phabricator via cfe-commits]

dyung added a comment.

Hi, the test you added is failing on the PS4 Windows bot 
https://lab.llvm.org/buildbot/#/builders/216/builds/2647.

Can you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[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 rG3f0587d0c668: [randstruct] Add randomize structure layout 
support (authored by connorkuehl, committed by void).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,458 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ,
+ bool ExpectErr = false) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
+
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  if (ExpectErr)
+EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
+  else
+EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3438722 , @aaron.ballman 
wrote:

> Precommit CI is failing on one test:
>
> Failed Tests (1):
>
>   Clang :: Misc/pragma-attribute-supported-attributes-list.test
>
> but with that test fixed, this LGTM! (Feel free to land with the test fixed 
> unless you want another round of review.) Thank you for the awesome new 
> security feature!

That'll teach me not to run tests. :-)

Thanks, Aaron!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

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

Fix simple test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,458 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ,
+ bool ExpectErr = false) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
+
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  if (ExpectErr)
+EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
+  else
+EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = 

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

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

Precommit CI is failing on one test:

Failed Tests (1):

  Clang :: Misc/pragma-attribute-supported-attributes-list.test

but with that test fixed, this LGTM! (Feel free to land with the test fixed 
unless you want another round of review.) Thank you for the awesome new 
security feature!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

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

Fix bad formatting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,458 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ,
+ bool ExpectErr = false) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
+
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  if (ExpectErr)
+EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
+  else
+EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = 

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

[Bill Wendling via Phabricator via cfe-commits]

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

Make the randomize_layout and no_randomize_layout attributes mutually 
exclutsive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,460 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ,
+ bool ExpectErr = false) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  IgnoringDiagConsumer IgnoringConsumer = IgnoringDiagConsumer();
+
+  std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(
+  SourceCode, Args, "input.c", "clang-tool",
+  std::make_shared(),
+  tooling::getClangStripDependencyFileAdjuster(),
+  tooling::FileContentMappings(), );
+
+  if (ExpectErr)
+EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
+  else
+EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Recap: aside from the function merging behavior and the possible question about 
assertions in tests, I think this is ready to go.




Comment at: clang/lib/Sema/SemaDecl.cpp:27
 #include "clang/AST/NonTrivialTypeVisitor.h"
+#include "clang/AST/Randstruct.h"
 #include "clang/AST/StmtCXX.h"

void wrote:
> aaron.ballman wrote:
> > Is this include necessary?
> Yes. There's a call to `randstruct::randomizeStructureLayout` below.
Oh derp, I missed that. :-)



Comment at: clang/unittests/AST/RandstructTest.cpp:154-158
+#ifdef _WIN32
+  const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"};
+#else
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
+#endif

void wrote:
> aaron.ballman wrote:
> > Any idea what's gone wrong here? (Do we have a bug to file because these 
> > come out reversed? If so, can you add a FIXME comment here that we expect 
> > this test to change someday?)
> I think it's just a case where Windows' algorithm for `std::mt19937` is 
> subtly different than the one for Linux. I'm not sure we should worry about 
> it too much, to be honest. As long as it produces a deterministic output on 
> the same platform we should be fine. I think it's expected that the same 
> compiler/environment is used during all compilation steps. (I.e., one's not 
> going to compile a module on Windows for a kernel build on Linux.)
Okay, that's a great reason for this to be left alone.



Comment at: clang/unittests/AST/RandstructTest.cpp:208-213
+// FIXME: Clang trips an assertion in the DiagnosticsEngine when the warning is
+// emitted while running under the test suite:
+// clang/lib/Frontend/TextDiagnosticPrinter.cpp:150: virtual void
+// 
clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level,
+// const clang::Diagnostic&): Assertion `TextDiag && "UnExpected diagnostic
+// outside source file processing"' failed.

void wrote:
> aaron.ballman wrote:
> > Is the assertion unrelated to the changes in your patch and you just happen 
> > to hit it with this test? (I get worried when tests trigger assertions.)
> To be honest, I haven't looked at these tests. I inherited them from the 
> previous code base. I'll revisit these and probably delete them if they're 
> not useful.
Okay, that sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:27
 #include "clang/AST/NonTrivialTypeVisitor.h"
+#include "clang/AST/Randstruct.h"
 #include "clang/AST/StmtCXX.h"

aaron.ballman wrote:
> Is this include necessary?
Yes. There's a call to `randstruct::randomizeStructureLayout` below.



Comment at: clang/unittests/AST/RandstructTest.cpp:154-158
+#ifdef _WIN32
+  const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"};
+#else
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
+#endif

aaron.ballman wrote:
> Any idea what's gone wrong here? (Do we have a bug to file because these come 
> out reversed? If so, can you add a FIXME comment here that we expect this 
> test to change someday?)
I think it's just a case where Windows' algorithm for `std::mt19937` is subtly 
different than the one for Linux. I'm not sure we should worry about it too 
much, to be honest. As long as it produces a deterministic output on the same 
platform we should be fine. I think it's expected that the same 
compiler/environment is used during all compilation steps. (I.e., one's not 
going to compile a module on Windows for a kernel build on Linux.)



Comment at: clang/unittests/AST/RandstructTest.cpp:208-213
+// FIXME: Clang trips an assertion in the DiagnosticsEngine when the warning is
+// emitted while running under the test suite:
+// clang/lib/Frontend/TextDiagnosticPrinter.cpp:150: virtual void
+// 
clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level,
+// const clang::Diagnostic&): Assertion `TextDiag && "UnExpected diagnostic
+// outside source file processing"' failed.

aaron.ballman wrote:
> Is the assertion unrelated to the changes in your patch and you just happen 
> to hit it with this test? (I get worried when tests trigger assertions.)
To be honest, I haven't looked at these tests. I inherited them from the 
previous code base. I'll revisit these and probably delete them if they're not 
useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 420972.
void marked 3 inline comments as done.
void added a comment.

Don't re-randomize a structure. Also some minor style changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,461 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  std::unique_ptr AST =
+  tooling::buildASTFromCodeWithArgs(SourceCode, Args);
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->hasAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+   

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/Randstruct.h:31-35
+using llvm::SmallVectorImpl;
+
+bool randomizeStructureLayout(const ASTContext , llvm::StringRef Name,
+  llvm::ArrayRef Fields,
+  SmallVectorImpl );

Might as well be consistent with the other type names.



Comment at: clang/include/clang/Driver/Options.td:2122-2133
+def frandomize_layout_seed_EQ
+: Joined<["-"], "frandomize-layout-seed=">,
+  MetaVarName<"">,
+  Group,
+  Flags<[CC1Option]>,
+  HelpText<"The seed used by the randomize structure layout feature">;
+def frandomize_layout_seed_file_EQ

You should condense these a bit to keep the style the same as the surrounding 
code.



Comment at: clang/lib/AST/Randstruct.cpp:71-75
+  std::unique_ptr CurrentBucket = nullptr;
+
+  // The current bucket containing the run of adjacent bitfields to ensure they
+  // remain adjacent.
+  std::unique_ptr CurrentBitfieldRun = nullptr;

Default ctor already does the right thing, so no need to do the initialization.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8550-8556
+  case ParsedAttr::AT_RandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_NoRandomizeLayout:
+// Drop the "randomize_layout" attribute if it's on the decl.
+D->dropAttr();
+break;

void wrote:
> aaron.ballman wrote:
> > void wrote:
> > > aaron.ballman wrote:
> > > > I don't think this is sufficient. Consider redeclaration merging cases:
> > > > ```
> > > > struct [[gnu::randomize_layout]] S;
> > > > struct [[gnu::no_randomize_layout]] S {};
> > > > 
> > > > struct [[gnu::no_randomize_layout]] T;
> > > > struct [[gnu::randomize_layout]] T {};
> > > > ```
> > > > I think if the user accidentally does something like this, it should be 
> > > > diagnosed. I would have assumed this would warrant an error diagnostic 
> > > > because the user is obviously confused as to what they want, but it 
> > > > seems GCC ignores the subsequent diagnostic with a warning: 
> > > > https://godbolt.org/z/1q8dazYPW.
> > > > 
> > > > There's also the "confused attribute list" case which GCC just... 
> > > > does... things... with: https://godbolt.org/z/rnfsn7hG1. I think we 
> > > > want to behave more consistently with how we treat these cases.
> > > > 
> > > > Either way, we shouldn't be silent.
> > > The GCC feature is done via a plugin in Linux. So godbolt probably won't 
> > > work in this case. I'll check to see how GCC responds to these attribute 
> > > situations.
> > Thoughts on this?
> Okay, finally tested it with the GCC plugin. It doesn't produce a diagnostic. 
> I'm not sure if that's the correct behavior, but at least it matches. How is 
> such a thing handled with similar attributes?
> Okay, finally tested it with the GCC plugin. It doesn't produce a diagnostic. 
> I'm not sure if that's the correct behavior, but at least it matches. How is 
> such a thing handled with similar attributes?

As you might be unsurprised to learn: inconsistently. :-D However, we typically 
try to make attributes mutually exclusive (e.g., `hot` and `cold`, `global` and 
`device`, etc).

Unfortunately, I gave you some advice earlier to combine into one semantic 
attribute and that may have been less-than-awesome advice. Attr.td supports the 
ability to trivially define to attributes as mutually exclusive (e.g., `def : 
MutualExclusions<[Hot, Cold]>;` but this works on the *attribute* level and not 
the *spelling* level.

It's probably easier for you to split the definition back into two attributes 
in Attr.td and just use the `MutualExclusions` support we already have. Sorry 
for that churn! If you wanted to (and I certainly don't insist), another option 
would be to modify ClangAttrEmitter.cpp and Attr.td to support mutual exclusion 
syntax on the spelling level, so you could do something like:
```
def : MutualExclusions<[
  RandomizeLayout>,
  RandomizeLayout>
]>;
```
where you specify the attribute and the spelling, then you could leave 
`RandomizeLayout` as a single semantic spelling. But again, I don't insist.



Comment at: clang/unittests/AST/RandstructTest.cpp:154-158
+#ifdef _WIN32
+  const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"};
+#else
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
+#endif

Any idea what's gone wrong here? (Do we have a bug to file because these come 
out reversed? If so, can you add a FIXME comment here that we expect this test 
to change someday?)



Comment at: clang/unittests/AST/RandstructTest.cpp:208-213
+// FIXME: Clang trips an assertion in the DiagnosticsEngine when the warning is
+// emitted while running under the test suite:
+// clang/lib/Frontend/TextDiagnosticPrinter.cpp:150: virtual 

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

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8550-8556
+  case ParsedAttr::AT_RandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_NoRandomizeLayout:
+// Drop the "randomize_layout" attribute if it's on the decl.
+D->dropAttr();
+break;

aaron.ballman wrote:
> void wrote:
> > aaron.ballman wrote:
> > > I don't think this is sufficient. Consider redeclaration merging cases:
> > > ```
> > > struct [[gnu::randomize_layout]] S;
> > > struct [[gnu::no_randomize_layout]] S {};
> > > 
> > > struct [[gnu::no_randomize_layout]] T;
> > > struct [[gnu::randomize_layout]] T {};
> > > ```
> > > I think if the user accidentally does something like this, it should be 
> > > diagnosed. I would have assumed this would warrant an error diagnostic 
> > > because the user is obviously confused as to what they want, but it seems 
> > > GCC ignores the subsequent diagnostic with a warning: 
> > > https://godbolt.org/z/1q8dazYPW.
> > > 
> > > There's also the "confused attribute list" case which GCC just... does... 
> > > things... with: https://godbolt.org/z/rnfsn7hG1. I think we want to 
> > > behave more consistently with how we treat these cases.
> > > 
> > > Either way, we shouldn't be silent.
> > The GCC feature is done via a plugin in Linux. So godbolt probably won't 
> > work in this case. I'll check to see how GCC responds to these attribute 
> > situations.
> Thoughts on this?
Okay, finally tested it with the GCC plugin. It doesn't produce a diagnostic. 
I'm not sure if that's the correct behavior, but at least it matches. How is 
such a thing handled with similar attributes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

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

Second attempt to fix Windows errors. Expecting one more iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

STAMPS
actor(@void) application(Differential) author(@void) herald(H157) herald(H311) 
herald(H337) herald(H343) herald(H423) herald(H477) herald(H576) herald(H617) 
herald(H638) herald(H649) herald(H678) herald(H688) herald(H864) 
monogram(D121556) object-type(DREV) phid(PHID-DREV-pdinu5qraknmmd3qzmtm) 
reviewer(@aaron.ballman) reviewer(@connorkuehl) reviewer(@jfb) 
reviewer(@rsmith) reviewer(@timpugh) revision-repository(rG) 
revision-status(needs-review) subscriber(@cfe-commits) subscriber(@connorkuehl) 
subscriber(@daloni) subscriber(@Dan) subscriber(@dang) subscriber(@dexonsmith) 
subscriber(@ebevhan) subscriber(@hintonda) subscriber(@jdoerfert) 
subscriber(@kees) subscriber(@MaskRay) subscriber(@mgorny) 
subscriber(@nickdesaulniers) subscriber(@pcc) subscriber(@riccibruno) 
subscriber(@shawnl) subscriber(@tschuett) subscriber(@vlad.tsyrklevich) 
subscriber(@xbolva00) tag(#all) tag(#clang) via(conduit)

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,461 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  std::unique_ptr AST =
+  tooling::buildASTFromCodeWithArgs(SourceCode, Args);
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", 

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

[Bill Wendling via Phabricator via cfe-commits]

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

First pass at fixing the Windows unit test errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,453 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  std::unique_ptr AST =
+  tooling::buildASTFromCodeWithArgs(SourceCode, Args);
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->hasAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+

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

[Bill Wendling via Phabricator via cfe-commits]

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

- Add an entry in the Release Notes.
- Change the command line flags to better match the attribute naming.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,425 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandomize-layout-seed=1234567890abcdef");
+
+  std::unique_ptr AST =
+  tooling::buildASTFromCodeWithArgs(SourceCode, Args);
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->hasAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test 

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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D121556#3427620 , @void wrote:

> In D121556#3419184 , @aaron.ballman 
> wrote:
>
>> Generally I think things are looking pretty good, but there's still an open 
>> question and Precommit CI is still failing on Windows:
>>
>>   Failed Tests (7):
>> Clang-Unit :: 
>> AST/./ASTTests.exe/StructureLayoutRandomization.AnonymousStructsAndUnionsRetainFieldOrder
>> Clang-Unit :: 
>> AST/./ASTTests.exe/StructureLayoutRandomization.CheckAdjacentBitfieldsRemainAdjacentAfterRandomization
>> Clang-Unit :: 
>> AST/./ASTTests.exe/StructureLayoutRandomization.CheckVariableLengthArrayMemberRemainsAtEndOfStructure
>> Clang-Unit :: 
>> AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize
>> Clang-Unit :: 
>> AST/./ASTTests.exe/StructureLayoutRandomization.MultipleAttrsRandomize
>> Clang-Unit :: 
>> AST/./ASTTests.exe/StructureLayoutRandomization.RandstructDoesNotOverrideThePackedAttr
>> Clang-Unit :: 
>> AST/./ASTTests.exe/StructureLayoutRandomization.ZeroWidthBitfieldsSeparateAllocationUnits
>
> Do you have a URL for this failure or the output message?

https://reviews.llvm.org/D121556
->
Harbormaster completed remote builds in B156477 
: Diff 418480. 
(https://reviews.llvm.org/harbormaster/build/233718/)
->
x64 windows failed 
(https://buildkite.com/llvm-project/premerge-checks/builds/86170#c5c61c34-004b-4e62-a3b4-4654fb8d1dc7)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3427604 , @xbolva00 wrote:

> Release note is missing

Sorry about that. I'll add one with the next update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3419184 , @aaron.ballman 
wrote:

> Generally I think things are looking pretty good, but there's still an open 
> question and Precommit CI is still failing on Windows:
>
>   Failed Tests (7):
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.AnonymousStructsAndUnionsRetainFieldOrder
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.CheckAdjacentBitfieldsRemainAdjacentAfterRandomization
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.CheckVariableLengthArrayMemberRemainsAtEndOfStructure
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.MultipleAttrsRandomize
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.RandstructDoesNotOverrideThePackedAttr
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.ZeroWidthBitfieldsSeparateAllocationUnits

Do you have a URL for this failure or the output message?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Release note is missing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 420294.
void marked an inline comment as done.
void added a comment.

Simplify the attributes by using an accessor instead of a separate 
"no_ranomize_layout" attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,425 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+
+  std::unique_ptr AST =
+  tooling::buildASTFromCodeWithArgs(SourceCode, Args);
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->hasAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+  

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121556#3419184 , @aaron.ballman 
wrote:

> Generally I think things are looking pretty good, but there's still an open 
> question and Precommit CI is still failing on Windows:
>
>   Failed Tests (7):
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.AnonymousStructsAndUnionsRetainFieldOrder
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.CheckAdjacentBitfieldsRemainAdjacentAfterRandomization
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.CheckVariableLengthArrayMemberRemainsAtEndOfStructure
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.MultipleAttrsRandomize
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.RandstructDoesNotOverrideThePackedAttr
> Clang-Unit :: 
> AST/./ASTTests.exe/StructureLayoutRandomization.ZeroWidthBitfieldsSeparateAllocationUnits

Strange. I wonder if Windows is using a different algorithm for the 
randomization function. I'll investigate and see what can be done here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Generally I think things are looking pretty good, but there's still an open 
question and Precommit CI is still failing on Windows:

  Failed Tests (7):
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.AnonymousStructsAndUnionsRetainFieldOrder
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.CheckAdjacentBitfieldsRemainAdjacentAfterRandomization
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.CheckVariableLengthArrayMemberRemainsAtEndOfStructure
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.MultipleAttrsRandomize
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.RandstructDoesNotOverrideThePackedAttr
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.ZeroWidthBitfieldsSeparateAllocationUnits




Comment at: clang/include/clang/Basic/Attr.td:3954-3968
+def RandomizeLayout : InheritableAttr {
+  let Spellings = [GCC<"randomize_layout">];
+  let Subjects = SubjectList<[Record]>;
+  let Documentation = [ClangRandstructDocs];
+  let LangOpts = [COnly];
+}
+

This gets rid of one of the AST nodes and uses an accessor instead. e.g., the 
user can still write `[[gnu::no_randomize_layout]]` on a struct declaration, 
we'll generate a `RandomizeLayoutAttr` for it, and you can call 
`->isDisabled()` on an object to tell whether it's the `no_` spelling or not.

I don't feel super strong about this change, so if it turns out to make code 
elsewhere significantly worse, feel free to ignore. My reasoning for combing is 
that the `no` variant doesn't carry much semantic weight but eats up an 
attribute kind value just the same; it seemed like a heavy use for an AST node.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8550-8556
+  case ParsedAttr::AT_RandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_NoRandomizeLayout:
+// Drop the "randomize_layout" attribute if it's on the decl.
+D->dropAttr();
+break;

void wrote:
> aaron.ballman wrote:
> > I don't think this is sufficient. Consider redeclaration merging cases:
> > ```
> > struct [[gnu::randomize_layout]] S;
> > struct [[gnu::no_randomize_layout]] S {};
> > 
> > struct [[gnu::no_randomize_layout]] T;
> > struct [[gnu::randomize_layout]] T {};
> > ```
> > I think if the user accidentally does something like this, it should be 
> > diagnosed. I would have assumed this would warrant an error diagnostic 
> > because the user is obviously confused as to what they want, but it seems 
> > GCC ignores the subsequent diagnostic with a warning: 
> > https://godbolt.org/z/1q8dazYPW.
> > 
> > There's also the "confused attribute list" case which GCC just... does... 
> > things... with: https://godbolt.org/z/rnfsn7hG1. I think we want to behave 
> > more consistently with how we treat these cases.
> > 
> > Either way, we shouldn't be silent.
> The GCC feature is done via a plugin in Linux. So godbolt probably won't work 
> in this case. I'll check to see how GCC responds to these attribute 
> situations.
Thoughts on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Friendly ping for review. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

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

Moved the randomization to a better spot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,418 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+
+  std::unique_ptr AST =
+  tooling::buildASTFromCodeWithArgs(SourceCode, Args);
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  RecordDecl *RD = FirstDeclMatcher().match(
+  C.getTranslationUnitDecl(), recordDecl(hasName(Name)));
+  return RD;
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->getAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+  

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

[Bill Wendling via Phabricator via cfe-commits]

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

- Make sure the command line seed is properly passed on to each front-end level.
- Some general cleanups.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,414 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+
+  std::unique_ptr AST =
+  tooling::buildASTFromCodeWithArgs(SourceCode, Args);
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  return AST;
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  return FirstDeclMatcher().match(C.getTranslationUnitDecl(),
+  recordDecl(hasName(Name)));
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->getAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int 

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

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 418356.
void marked 2 inline comments as done.
void added a comment.

Move casting check into the SemaCast where it belongs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,409 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+  return tooling::buildASTFromCodeWithArgs(SourceCode, Args, "input.c");
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  return FirstDeclMatcher().match(C.getTranslationUnitDecl(),
+  recordDecl(hasName(Name)));
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->getAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+} 

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

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: clang/include/clang/AST/Decl.h:2842
   mutable unsigned CachedFieldIndex : 30;
+  mutable unsigned OriginalFieldIndex : 30;
 

aaron.ballman wrote:
> It's unfortunate that every field node in the AST is now going to be 4 bytes 
> larger for this feature; I worry about the extra memory pressure from the 
> additional overhead, so if there's a way for us to save some space here, I 
> think it might be worth it. (I'm worried that our max template instantiation 
> depth will shrink because of this.)
Turns out this field wasn't used. I removed it.



Comment at: clang/include/clang/Basic/AttrDocs.td:6367
+program should be compiled with the same seed, but keep the seed safe
+otherwise.
+

aaron.ballman wrote:
> I think it would help to explain the effects of this attribute in conjunction 
> with structure packing techniques (the `packed` attribute and bit-fields) and 
> whether any attempt is made to keep the structure packed or those fields 
> together or not. I'd expect no such attempt is made, but people may want to 
> know "attempts to pack this structure" and "randomize this structure layout" 
> are not compatible. (We may want to diagnose in the case of seeing the 
> `packed` attribute, in fact.)
> 
> We should also be more clear that the seed specified does not have to be 
> numeric in nature (from the file or when directly on the command line).
Having randomization and the `packed` attribute probably isn't the best option. 
I'll modify things to prevent that from happening.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 418307.
void marked 5 inline comments as done.
void added a comment.

Add "err_" prefix to error message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,409 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+  return tooling::buildASTFromCodeWithArgs(SourceCode, Args, "input.c");
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  return FirstDeclMatcher().match(C.getTranslationUnitDecl(),
+  recordDecl(hasName(Name)));
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->getAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+

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

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 418305.
void added a comment.
Herald added a subscriber: MaskRay.

Fix how the command line flags are handled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,409 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+  return tooling::buildASTFromCodeWithArgs(SourceCode, Args, "input.c");
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  return FirstDeclMatcher().match(C.getTranslationUnitDecl(),
+  recordDecl(hasName(Name)));
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->getAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+   

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

[Bill Wendling via Phabricator via cfe-commits]

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

Move SEED into LangOpts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,409 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+  return tooling::buildASTFromCodeWithArgs(SourceCode, Args, "input.c");
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  return FirstDeclMatcher().match(C.getTranslationUnitDecl(),
+  recordDecl(hasName(Name)));
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->getAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+} 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/Randstruct.h:34
+
+extern std::string SEED;
+

void wrote:
> aaron.ballman wrote:
> > Doing this with a global variable is unfortunate; it could make things 
> > harder when we multithread the frontend. Can we solve this without a 
> > global? (And if not, does this global need to be a `ManagedStatic`?)
> Maybe this could be moved to `LangOpts`?
That's a really good idea, let's go with that approach.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11543
+// Layout randomization warning.
+def cast_from_randomized_struct : Error<
+  "casting from randomized structure pointer type %0 to %1">;





Comment at: clang/lib/AST/Randstruct.cpp:99
+
+if (FD->isBitField() && !FD->isZeroLengthBitField(Context)) {
+  // Start a bitfield run if this is the first bitfield we have found.

void wrote:
> aaron.ballman wrote:
> > Oh cool, we do bit-field runs!
> > 
> > How should we handle anonymous bit-fields (if anything special should be 
> > done for them at all)? People usually use them for padding between 
> > bit-fields, so it's not clear to me how to treat them when randomizing.
> Good question! I'm not sure either. On one hand, I'm rather concerned about 
> changing bitfield order in general, but it appears to be something that GCC's 
> plugin does, so...
> 
Yeah, I was really surprised about bit-fields as well. I'm especially concerned 
that bit-fields which were [un]carefully straddling allocation unit boundaries 
might be rearranged such that subtle atomic bugs and whatnot will be exposed.

If we find these sort of concerns are valid in real world code, we may want to 
add a second command line option (off-by-default, perhaps) for enabling 
rearranging bit-fields. But for now, I think it's fine to follow GCC's lead 
here.



Comment at: clang/lib/AST/Randstruct.cpp:179
+
+  SmallVector RandomizedFields;
+

void wrote:
> aaron.ballman wrote:
> > This one seems a bit large to me; any reason not to use `16` again?
> The code above looks to apply mostly to bitfield runs. This is for all fields 
> in a structure. I assumed (without proof) that this will always be larger 
> than the bitfield runs. :-)
I think that's a safe assumption and it's probably not worth worrying about 
overly much. It's more that 16 bit-fields seemed like it would be large enough 
to cover most bit-fields in a class while still being "small", but 64 fields 
seems likely to be way larger than most structures will need and isn't very 
small.

That said, I don't have a strong feeling here and I think the numbers are 
defensible unless we measure something that says otherwise.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2503
+
+  // FIXME: Any way to get a handle to a RecordDecl struct here?
+  clang::randstruct::checkForBadCasts(S, AC);

void wrote:
> aaron.ballman wrote:
> > No, this function is only called when popping a function scope, and so the 
> > only declaration it has access to is the `FunctionDecl`. Or do I 
> > misunderstand the question?
> Sounds good to me. Is this the best place for this check then?
I'm not convinced it is the best place for it -- analysis-based warnings are 
typically ones that require a CFG to be analyzed and so they are only run once 
we know the function body itself is valid (they also typically require the user 
to manually enable the checks -- your check runs always, even if the user 
hasn't enabled struct randomization, so it walks a lot of ASTs it doesn't need 
to). Your check doesn't require a CFG at all and it seems like it can be done 
purely when building the AST. I think a better place for this checking is in 
SemaCast.cpp in the `CastOperation` class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void marked 7 inline comments as done.
void added inline comments.



Comment at: clang/include/clang/AST/Decl.h:4067
+
+  void setIsRandomized(bool V) const { RecordDeclBits.IsRandomized = V; }
+

aaron.ballman wrote:
> A setter that is marked `const` does not spark joy for me... Can we use 
> friendship rather than mutability to solve this?
It was done to prevent a `const_cast`. It looks like the other setters are 
non-const. I'll see if I can get around that.



Comment at: clang/include/clang/AST/Randstruct.h:34
+
+extern std::string SEED;
+

aaron.ballman wrote:
> Doing this with a global variable is unfortunate; it could make things harder 
> when we multithread the frontend. Can we solve this without a global? (And if 
> not, does this global need to be a `ManagedStatic`?)
Maybe this could be moved to `LangOpts`?



Comment at: clang/include/clang/Basic/Attr.td:3938-3939
+def RandomizeLayout : InheritableAttr {
+  let Spellings = [GCC<"randomize_layout">, Declspec<"randomize_layout">,
+Keyword<"randomize_layout">];
+  let Subjects = SubjectList<[Record]>;

aaron.ballman wrote:
> Microsoft does not implement this `__declspec`, correct? If they don't, we 
> shouldn't either (even if GCC does) unless there's a *very* good reason to do 
> so. That's Microsoft's design space we're encroaching on.
> 
> I'd also like to understand the justification for adding a new keyword for 
> this.
Good point. I'll remove those.



Comment at: clang/lib/AST/DeclBase.cpp:28
 #include "clang/AST/ExternalASTSource.h"
+#include "clang/AST/Randstruct.h"
 #include "clang/AST/Stmt.h"

aaron.ballman wrote:
> Is this include necessary?
Hmm...that must have been from a previous change. Removed.



Comment at: clang/lib/AST/Randstruct.cpp:72
+  // All of the Buckets produced by best-effort cache-line algorithm.
+  // TODO: Figure out a better initial size.
+  SmallVector, 16> Buckets;

aaron.ballman wrote:
> This size seems as defensible as most others; do you plan to do more here, or 
> should this comment be removed?
It's probably not worth looking into unless it becomes an issue. I'll remove it.



Comment at: clang/lib/AST/Randstruct.cpp:99
+
+if (FD->isBitField() && !FD->isZeroLengthBitField(Context)) {
+  // Start a bitfield run if this is the first bitfield we have found.

aaron.ballman wrote:
> Oh cool, we do bit-field runs!
> 
> How should we handle anonymous bit-fields (if anything special should be done 
> for them at all)? People usually use them for padding between bit-fields, so 
> it's not clear to me how to treat them when randomizing.
Good question! I'm not sure either. On one hand, I'm rather concerned about 
changing bitfield order in general, but it appears to be something that GCC's 
plugin does, so...




Comment at: clang/lib/AST/Randstruct.cpp:179
+
+  SmallVector RandomizedFields;
+

aaron.ballman wrote:
> This one seems a bit large to me; any reason not to use `16` again?
The code above looks to apply mostly to bitfield runs. This is for all fields 
in a structure. I assumed (without proof) that this will always be larger than 
the bitfield runs. :-)



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2063-2069
+  if (RD->isRandomized()) {
+for (Decl *D : FinalOrdering)
+  RD->removeDecl(D);
+
+for (Decl *D : FinalOrdering)
+  RD->addDecl(D);
+  }

aaron.ballman wrote:
> I think this works okay for C, but I think if we ever attempted to provide 
> this functionality in C++, the calls to `addDecl()` would be rather expensive 
> because it eventually calls `addedMember()` which calculates information 
> about the structure (whether it's copy constructible, has a trivial 
> destructor, etc) and we don't need to redo any of that work.
> 
> However, for now, I think this is fine.
God help the person who does this for C++! :-)

Joking aside, supporting C++ will probably result in a almost total rewrite of 
this feature.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2503
+
+  // FIXME: Any way to get a handle to a RecordDecl struct here?
+  clang::randstruct::checkForBadCasts(S, AC);

aaron.ballman wrote:
> No, this function is only called when popping a function scope, and so the 
> only declaration it has access to is the `FunctionDecl`. Or do I 
> misunderstand the question?
Sounds good to me. Is this the best place for this check then?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8550-8556
+  case ParsedAttr::AT_RandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_NoRandomizeLayout:
+// Drop the "randomize_layout" attribute if it's on the decl.
+D->dropAttr();
+break;

aaron.ballman 

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

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 416354.
void marked 3 inline comments as done.
void added a comment.

General cleanup of code. Removing unneeded headers and comments. Removing a 
"const" from a setter. Not defining this for MS's declspecs or making the 
attributes keywords.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,409 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+  return tooling::buildASTFromCodeWithArgs(SourceCode, Args, "input.cc");
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  return FirstDeclMatcher().match(C.getTranslationUnitDecl(),
+  recordDecl(hasName(Name)));
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define RANDSTRUCT_TEST_SUITE_TEST StructureLayoutRandomizationTestSuiteTest
+
+TEST(RANDSTRUCT_TEST_SUITE_TEST, CanDetermineIfSubsequenceExists) {
+  const field_names Seq = {"a", "b", "c", "d"};
+
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a", "b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"b", "c", "d"}));
+  ASSERT_TRUE(isSubsequence(Seq, {"a"}));
+  ASSERT_FALSE(isSubsequence(Seq, {"a", "d"}));
+}
+
+#define RANDSTRUCT_TEST StructureLayoutRandomization
+
+TEST(RANDSTRUCT_TEST, UnmarkedStruct) {
+  const std::unique_ptr AST = makeAST(R"c(
+struct test {
+int bacon;
+long lettuce;
+long long tomato;
+float mayonnaise;
+};
+  )c");
+
+  const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
+  const field_names Expected = {"bacon", "lettuce", "tomato", "mayonnaise"};
+
+  ASSERT_FALSE(RD->getAttr());
+  ASSERT_FALSE(RD->isRandomized());
+  ASSERT_EQ(Expected, getFieldNamesFromRecord(RD));
+}
+
+TEST(RANDSTRUCT_TEST, MarkedNoRandomize) {
+  const std::unique_ptr AST = makeAST(R"c(
+

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this new security feature! I like it and I think it's 
heading in the right direction.

Can you please add a release note for the new functionality?

This is missing tests for the Driver diagnostics, the Sema diagnostics 
(including existing diagnostics like writing the attribute on the wrong subject 
or giving it args when it doesn't expect them, wrong language mode, etc), and 
the CodeGen behavior, so those should be added. Also, the Windows pre-commit CI 
is failing:

  Failed Tests (7):
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.AnonymousStructsAndUnionsRetainFieldOrder
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.CheckAdjacentBitfieldsRemainAdjacentAfterRandomization
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.CheckVariableLengthArrayMemberRemainsAtEndOfStructure
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.MultipleAttrsRandomize
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.RandstructDoesNotOverrideThePackedAttr
Clang-Unit :: 
AST/./ASTTests.exe/StructureLayoutRandomization.ZeroWidthBitfieldsSeparateAllocationUnits
   
  Testing Time: 676.49s
Skipped  : 4
Unsupported  :   202
Passed   : 29832
Expectedly Failed:31
Failed   : 7




Comment at: clang/include/clang/AST/Decl.h:2842
   mutable unsigned CachedFieldIndex : 30;
+  mutable unsigned OriginalFieldIndex : 30;
 

It's unfortunate that every field node in the AST is now going to be 4 bytes 
larger for this feature; I worry about the extra memory pressure from the 
additional overhead, so if there's a way for us to save some space here, I 
think it might be worth it. (I'm worried that our max template instantiation 
depth will shrink because of this.)



Comment at: clang/include/clang/AST/Decl.h:4067
+
+  void setIsRandomized(bool V) const { RecordDeclBits.IsRandomized = V; }
+

A setter that is marked `const` does not spark joy for me... Can we use 
friendship rather than mutability to solve this?



Comment at: clang/include/clang/AST/Randstruct.h:34
+
+extern std::string SEED;
+

Doing this with a global variable is unfortunate; it could make things harder 
when we multithread the frontend. Can we solve this without a global? (And if 
not, does this global need to be a `ManagedStatic`?)



Comment at: clang/include/clang/Basic/Attr.td:3938-3939
+def RandomizeLayout : InheritableAttr {
+  let Spellings = [GCC<"randomize_layout">, Declspec<"randomize_layout">,
+Keyword<"randomize_layout">];
+  let Subjects = SubjectList<[Record]>;

Microsoft does not implement this `__declspec`, correct? If they don't, we 
shouldn't either (even if GCC does) unless there's a *very* good reason to do 
so. That's Microsoft's design space we're encroaching on.

I'd also like to understand the justification for adding a new keyword for this.



Comment at: clang/include/clang/Basic/Attr.td:3948-3949
+def NoRandomizeLayout : InheritableAttr {
+  let Spellings = [GCC<"no_randomize_layout">, Declspec<"no_randomize_layout">,
+Keyword<"no_randomize_layout">];
+  let Subjects = SubjectList<[Record]>;

Same here.



Comment at: clang/include/clang/Basic/AttrDocs.td:6367
+program should be compiled with the same seed, but keep the seed safe
+otherwise.
+

I think it would help to explain the effects of this attribute in conjunction 
with structure packing techniques (the `packed` attribute and bit-fields) and 
whether any attempt is made to keep the structure packed or those fields 
together or not. I'd expect no such attempt is made, but people may want to 
know "attempts to pack this structure" and "randomize this structure layout" 
are not compatible. (We may want to diagnose in the case of seeing the `packed` 
attribute, in fact.)

We should also be more clear that the seed specified does not have to be 
numeric in nature (from the file or when directly on the command line).



Comment at: clang/lib/AST/DeclBase.cpp:28
 #include "clang/AST/ExternalASTSource.h"
+#include "clang/AST/Randstruct.h"
 #include "clang/AST/Stmt.h"

Is this include necessary?



Comment at: clang/lib/AST/Randstruct.cpp:72
+  // All of the Buckets produced by best-effort cache-line algorithm.
+  // TODO: Figure out a better initial size.
+  SmallVector, 16> Buckets;

This size seems as defensible as most others; do you plan to do more here, or 
should this comment be removed?



Comment at: clang/lib/AST/Randstruct.cpp:99
+
+if (FD->isBitField() && !FD->isZeroLengthBitField(Context)) {
+  // Start a 

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

[Bill Wendling via Phabricator via cfe-commits]

void added reviewers: jfb, connorkuehl, rsmith, timpugh.
void added subscribers: connorkuehl, Dan, xbolva00, shawnl, dexonsmith, daloni, 
hintonda, riccibruno, vlad.tsyrklevich, ebevhan, kees, pcc, nickdesaulniers.
void added a comment.

This is a soft reworking of @connorkuehl's work in 
https://reviews.llvm.org/D59254. Please take a look!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556

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

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

[Bill Wendling via Phabricator via cfe-commits]

void created this revision.
Herald added subscribers: dang, jdoerfert, mgorny.
Herald added a reviewer: aaron.ballman.
Herald added a project: All.
void requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The Randstruct feature is a compile-time hardening technique that
randomizes the field layout for designated structures of a code base.
Admittedly, this is mostly useful for closed-source releases of code,
since the randomization seed would need to be available for public and
open source applications.

Why implement it? This patch set enhances Clang’s feature parity with
that of GCC which already has the Randstruct feature. It's used by the
Linux kernel in certain structures to help thwart attacks that depend on
structure layouts in memory.

This patch set is a from-scratch reimplementation of the Randstruct
feature that was originally ported to GCC. The patches for the GCC
implementation can be found here:

  https://www.openwall.com/lists/kernel-hardening/2017/04/06/14

Link: https://lists.llvm.org/pipermail/cfe-dev/2019-March/061607.html
Co-authored-by: Cole Nixon 
Co-authored-by: Connor Kuehl 
Co-authored-by: James Foster 
Co-authored-by: Jeff Takahashi 
Co-authored-by: Jordan Cantrell 
Co-authored-by: Nikk Forbus 
Co-authored-by: Tim Pugh 
Co-authored-by: Bill Wendling 
Signed-off-by: Bill Wendling 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121556

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/Randstruct.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/Randstruct.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RandstructTest.cpp

Index: clang/unittests/AST/RandstructTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RandstructTest.cpp
@@ -0,0 +1,409 @@
+//===- unittest/AST/RandstructTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for Clang's structure field layout randomization.
+//
+//===--===//
+
+/*
+ * Build this test suite by running `make ASTTests` in the build folder.
+ *
+ * Run this test suite by running the following in the build folder:
+ * ` ./tools/clang/unittests/AST/ASTTests
+ * --gtest_filter=StructureLayoutRandomization*`
+ */
+
+#include "clang/AST/Randstruct.h"
+#include "gtest/gtest.h"
+
+#include "DeclMatcher.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Testing/CommandLineArgs.h"
+#include "clang/Tooling/Tooling.h"
+
+#include 
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::randstruct;
+
+using field_names = std::vector;
+
+namespace {
+
+std::unique_ptr makeAST(const std::string ) {
+  std::vector Args = getCommandLineArgsForTesting(Lang_C99);
+  Args.push_back("-frandstruct-seed=1234567890abcdef");
+  return tooling::buildASTFromCodeWithArgs(SourceCode, Args, "input.cc");
+}
+
+RecordDecl *getRecordDeclFromAST(const ASTContext , const std::string ) {
+  return FirstDeclMatcher().match(C.getTranslationUnitDecl(),
+  recordDecl(hasName(Name)));
+}
+
+std::vector getFieldNamesFromRecord(const RecordDecl *RD) {
+  std::vector Fields;
+
+  Fields.reserve(8);
+  for (auto *Field : RD->fields())
+Fields.push_back(Field->getNameAsString());
+
+  return Fields;
+}
+
+bool isSubsequence(const field_names , const field_names ) {
+  unsigned SeqLen = Seq.size();
+  unsigned SubLen = Subseq.size();
+
+  bool IsSubseq = false;
+  for (unsigned I = 0; I < SeqLen; ++I)
+if (Seq[I] == Subseq[0]) {
+  IsSubseq = true;
+  for (unsigned J = 0; J + I < SeqLen && J < SubLen; ++J) {
+if (Seq[J + I] != Subseq[J]) {
+  IsSubseq = false;
+  break;
+}
+  }
+}
+
+  return IsSubseq;
+}
+
+} // end anonymous namespace
+
+namespace clang {
+namespace ast_matchers {
+
+#define