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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D123958#3461714 <https://reviews.llvm.org/D123958#3461714>, @aaron.ballman wrote:

> In D123958#3461020 <https://reviews.llvm.org/D123958#3461020>, @void wrote:
>
>> In D123958#3459205 <https://reviews.llvm.org/D123958#3459205>, @aaron.ballman wrote:
>>
>>> 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?
>>
>> We don't randomize inner structures unless they have the `randomize_layout` flag. That's always been the case, and this patch doesn't change that.
>
> That's good, I misunderstood originally, so thanks!
>
>> The issue is that we were dropping the inner structures/unions because they aren't `FieldDecl`s, but `RecordDecl`s, with an `IndirectFieldDecl` if the inner structure is anonymous. The `IndirectFieldDecl` bits appear after the `RecordDecl` they're attached to, but doesn't seem like the ordering is important. The `IndirectFieldDecl` thing is there so that the anonymous fields are available in the outer structure.
>
> Agreed that we need to fix that behavior, but I think it's pretty fragile to assume every declaration in a struct can be reordered. I mentioned static assert decls above as one obvious example, but also consider things like:
>
>   struct S {
>     enum E {
>       One,
>     };
>   
>     enum E whatever;
>   } __attribute__((randomize_layout));

*sigh* Gotta love C.

> We currently drop the enumeration declaration entirely (oops), but if we rearrange all declaration orders, then the declaration of `whatever` may suddenly look to come BEFORE we know what the type of `enum E` will be.

Maybe the best way we can handle these things is to reorder only FieldDecls and RecordDecls. All of the other Decl types will be smooshed towards the top of the RecordDecl. Again, I'm making an assumption that the ordering of the IndirectFieldDecls and others aren't going to be an issue once the parsing is finished. Alternatively, we could move things like the IndirectFieldDecls and StaticAsserts to the end of the RecordDecl (but before the flexible array decl).

If my assumption about the IndirectFieldDecls is wrong, and the ordering is important, then first, "Wha?! It's such a fragile model!!" Second, I'll have to come up with a way to encapsulate the inner RecordDecl and associated IndirectFieldDecls so that they can be moved around as a single unit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123958