[randstruct] Don't allow implicit forward decl to stop struct randomization

[void]

Summary

If a struct/enum type used in a record doesn't have a forward decl /
def, an implicit one is injected into the struct. This stops clang from
randomizing the structure in some situations---i.e. when the struct
contains only function pointers. So we accept forward decls so they
don't prevent randomization.

Fixes 60349

[MaskRay]

The test appears to belong to test/CodeGen instead of test/Sema ? Mixed 8-column and 2-column indentation probably should be unified.

[MaskRay]

clang/test/Sema/init-randomized-struct-fwd-decl.c

23 ↗

(On Diff #494757)

// CHECK-LABEL: define {{.*}} @t1( is less likely to cause a conflict with other stuff happening to contain t1 as a substring.

[void]

Move test and improve checks.

[MaskRay]

clang/test/CodeGen/init-randomized-struct-fwd-decl.c passes without this patch. Is it correct?

[nickdesaulniers]

In D143300#4104469, @MaskRay wrote:

clang/test/CodeGen/init-randomized-struct-fwd-decl.c passes without this patch. Is it correct?

Perhaps related to my comment about RecordDecl vs EnumDecl...

or the tests might require a specific randomization seed?

clang/lib/Sema/SemaDecl.cpp

18890

what about EnumDecls? I suspect the shared common base TagDecl might be better to use?

If it is, can you add a test? I'm guessing

struct foo {
  enum havent_seen_yet;
  enum havent_seen_yet2;
}

would be the test case.

[void]

In D143300#4107132, @nickdesaulniers wrote:

In D143300#4104469, @MaskRay wrote:

clang/test/CodeGen/init-randomized-struct-fwd-decl.c passes without this patch. Is it correct?

Perhaps related to my comment about RecordDecl vs EnumDecl...

or the tests might require a specific randomization seed?

It was a think-o on my part. The __attribute__((randomize_layout)) forces randomization. And yes I need to add a check for EnumDecl. Patch incoming.

[void]

Fix test and add check for EnumDecl.

[void]

clang/lib/Sema/SemaDecl.cpp

18890

Would testing for a TagDecl be better here?

[nickdesaulniers]

clang/lib/Sema/SemaDecl.cpp

18890

I think so; common shared base and same logic for both cases here. Unless there's something other than RecordDecl and EnumDecl where this shouldn't apply.

[nickdesaulniers]

clang/lib/Sema/SemaDecl.cpp

18890

Unless there's something other than RecordDecl and EnumDecl where this shouldn't apply.

The TagDecl constructor is only called from the RecordDecl and EnumDecl constructors. So I think it's simpler to just check TagDecl base type rather than each of the two derived types.

[void]

Use TagDecl.

[MaskRay]

clang/test/CodeGen/init-randomized-struct-fwd-decl.c

24

@t1(

(This helps distinguish @t10 from @t1, if we ever need t10 :) )

[nickdesaulniers]

clang/test/CodeGen/init-randomized-struct-fwd-decl.c

3

Is the second check prefix necessary? The check lines look the same to me, unless I'm missing something.

Removing it should let us also drop the redundant FWD-DECL lines below.

[void]

clang/lib/Sema/SemaDecl.cpp

18890

I like that better. Done. :)

clang/test/CodeGen/init-randomized-struct-fwd-decl.c

3

It's there to check that the randomization doesn't change if the forward decl is outside of the structure. However, Windows yet again uses a different algorithm than Linux and generates a different randomized ordering. It's annoying. I can remove this check.

[MaskRay]

clang/test/CodeGen/init-randomized-struct-fwd-decl.c

3

Instead of having two RUN lines, you may write both declarations (one using forard-decl, the other doesn't) in the source code. It decreases the number of clang invocations.

[void]

Remove extra line.

[nickdesaulniers]

Thanks for the patch!

[haowei]

FYI, this patch looks like was pushed into a new "Main" branch instead of the actual "main" branch.:

commit f85a9a6452e8f49f9768d66a86434a88a5891614 (origin/Main)
Author: Bill Wendling <[e-mail]>
Date:   Mon Feb 6 14:26:16 2023 -0800

    [randstruct] Don't allow implicit forward decl to stop struct randomization

It causes git checkout failures on Windows, see error message:

warning: encountered old-style '/ssl/certs/ca-bundle.crt' that should be '%(prefix)//ssl/certs/ca-bundle.crt'
From https://llvm.googlesource.com/a/llvm-project
 * [new branch]                Main       -> Main
error: cannot lock ref 'refs/heads/main': is at f85a9a6452e8f49f9768d66a86434a88a5891614 but expected abbd256a810a0b0c92dda88a3050fc85cb604a9c
 ! abbd256a810a..14ca2e68ff4c  main       -> main  (unable to update local ref)

Windows use case aware but insensitive filesystem and 2 "main" branches will not work out.
See explanation at https://stackoverflow.com/questions/10068640/git-error-on-git-pull-unable-to-update-local-ref/66832220#66832220
I will attempt deleting the "Main" branch to correct this error.

[void]

In D143300#4108500, @haowei wrote:

FYI, this patch looks like was pushed into a new "Main" branch instead of the actual "main" branch.:

commit f85a9a6452e8f49f9768d66a86434a88a5891614 (origin/Main)
Author: Bill Wendling <[e-mail]>
Date:   Mon Feb 6 14:26:16 2023 -0800

    [randstruct] Don't allow implicit forward decl to stop struct randomization

It causes git checkout failures on Windows, see error message:

warning: encountered old-style '/ssl/certs/ca-bundle.crt' that should be '%(prefix)//ssl/certs/ca-bundle.crt'
From https://llvm.googlesource.com/a/llvm-project
 * [new branch]                Main       -> Main
error: cannot lock ref 'refs/heads/main': is at f85a9a6452e8f49f9768d66a86434a88a5891614 but expected abbd256a810a0b0c92dda88a3050fc85cb604a9c
 ! abbd256a810a..14ca2e68ff4c  main       -> main  (unable to update local ref)

Windows use case aware but insensitive filesystem and 2 "main" branches will not work out.
See explanation at https://stackoverflow.com/questions/10068640/git-error-on-git-pull-unable-to-update-local-ref/66832220#66832220
I will attempt deleting the "Main" branch to correct this error.

Sorry about this. I thought I stopped the commit before it finished. :-(