[PATCH] D123958: [randstruct] Randomize all elements of a record

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think you'll need a more targeted approach than assuming the only kinds of declarations in a struct are field-like in C.

It seems that the issue you've got is with anonymous objects in a structure where the inner fields are available for lookup in the outer structure. One question I have is: what's the expectation for the user? There's two ways to look at this. 1) The anonymous object is a single field; that its members can be found in the outer object is not relevant. 2) The fields of the anonymous object should also be randomized. The same is true for any inner structure, not just anonymous ones.

I had assumed that any structure not marked for randomization would not be randomized. Based on that, I don't think inner structure objects (anonymous or otherwise) should automatically randomize their fields. WDYT?



================
Comment at: clang/lib/Sema/SemaDecl.cpp:18049
         !Record->isRandomized()) {
-      SmallVector<Decl *, 32> OrigFieldOrdering(Record->fields());
+      SmallVector<Decl *, 32> OrigFieldOrdering(Record->decls());
       SmallVector<Decl *, 32> NewFieldOrdering;
----------------
This (and everything else about fields) is now misnamed.


================
Comment at: clang/lib/Sema/SemaInit.cpp:2126
   RecordDecl::field_iterator FieldEnd = RD->field_end();
-  size_t NumRecordFields = std::distance(RD->field_begin(), RD->field_end());
+  size_t NumRecordFields = std::distance(RD->decls_begin(), RD->decls_end());
   bool CheckForMissingFields =
----------------
This is now misnamed.


================
Comment at: clang/lib/Sema/SemaInit.cpp:2189
       if (IList->getNumInits() == 1) {
         if (NumRecordFields == 1)
           return true;
----------------
This is no longer testing the correct thing. e.g.,
```
struct S {
  int f;
  _Static_assert(sizeof(int) == 4, "oh no!");
};
```
This has two decls but only one field:
```
TranslationUnitDecl
`-RecordDecl <line:1:1, line:4:1> line:1:8 struct S definition
  |-FieldDecl <line:2:3, col:7> col:7 f 'int'
  `-StaticAssertDecl <line:3:3, col:44> col:3
    |-ImplicitCastExpr <col:18, col:33> '_Bool' <IntegralToBoolean>
    | `-BinaryOperator <col:18, col:33> 'int' '=='
    |   |-UnaryExprOrTypeTraitExpr <col:18, col:28> 'unsigned long' sizeof 'int'
    |   `-ImplicitCastExpr <col:33> 'unsigned long' <IntegralCast>
    |     `-IntegerLiteral <col:33> 'int' 4
    `-StringLiteral <col:36> 'char[7]' lvalue "oh no!"
```
Note, when you randomize that structure, we drop the `_Static_assert` declaration, so there is definitely a bug to be fixed here: https://godbolt.org/z/n9YE63rno


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123958