This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by void on Feb 3 2023, 3:22 PM.

Details

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

Diff Detail

Event Timeline

void created this revision.Feb 3 2023, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 3:22 PM
void requested review of this revision.Feb 3 2023, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2023, 3:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

MaskRay added inline comments.Feb 3 2023, 3:45 PM
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 updated this revision to Diff 494772.Feb 3 2023, 4:29 PM

Move test and improve checks.

void marked an inline comment as done.Feb 3 2023, 4:29 PM

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

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 added a comment.Feb 6 2023, 9:53 AM

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 updated this revision to Diff 495194.Feb 6 2023, 9:58 AM

Fix test and add check for EnumDecl.

void added inline comments.Feb 6 2023, 10:00 AM
clang/lib/Sema/SemaDecl.cpp
18890

Would testing for a TagDecl be better here?

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.

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 updated this revision to Diff 495204.Feb 6 2023, 10:17 AM
void marked an inline comment as done.

Use TagDecl.

MaskRay accepted this revision.Feb 6 2023, 10:19 AM
MaskRay added inline comments.
clang/test/CodeGen/init-randomized-struct-fwd-decl.c
24

@t1(

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

This revision is now accepted and ready to land.Feb 6 2023, 10:19 AM
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 added inline comments.Feb 6 2023, 11:35 AM
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 added inline comments.Feb 6 2023, 11:57 AM
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 updated this revision to Diff 495237.Feb 6 2023, 12:06 PM

Remove extra line.

nickdesaulniers accepted this revision.Feb 6 2023, 12:47 PM

Thanks for the patch!

This revision was landed with ongoing or failed builds.Feb 6 2023, 2:26 PM
This revision was automatically updated to reflect the committed changes.
haowei added a subscriber: haowei.EditedFeb 6 2023, 4:24 PM

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 <morbo@google.com>
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 added a comment.Feb 6 2023, 10:22 PM

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 <morbo@google.com>
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. :-(