[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

18891

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

18891

Would testing for a TagDecl be better here?

[nickdesaulniers]

clang/lib/Sema/SemaDecl.cpp

18891

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

18891

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

18891

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. :-(

clang/lib/Sema/SemaDecl.cpp

if (Record) {
 
     if (!Completed)
       Record->completeDefinition();
 
     // Handle attributes before checking the layout.
     ProcessDeclAttributeList(S, Record, Attrs);
 
     // Check to see if a FieldDecl is a pointer to a function.
-    auto IsFunctionPointer = [&](const Decl *D) {
+    auto IsFunctionPointerOrForwardDecl = [&](const Decl *D) {
       const FieldDecl *FD = dyn_cast<FieldDecl>(D);
-      if (!FD)
+      if (!FD) {
+        // Check whether this is a forward declaration that was inserted by
+        // Clang. This happens when a non-forward declared / defined type is
+        // used, e.g.:
+        //
+        //   struct foo {
+        //     struct bar *(*f)();
+        //     struct bar *(*g)();
+        //   };
+        //
+        // "struct bar" shows up in the decl AST as a "RecordDecl" with an
+        // incomplete definition.
+        if (const auto *RD = dyn_cast<RecordDecl>(D))
+          return !RD->isCompleteDefinition();
         return false;
+      }
       QualType FieldType = FD->getType().getDesugaredType(Context);
       if (isa<PointerType>(FieldType)) {
         QualType PointeeType = cast<PointerType>(FieldType)->getPointeeType();
         return PointeeType.getDesugaredType(Context)->isFunctionType();
       }
       return false;
     };
 
     // Maybe randomize the record's decls. We automatically randomize a record
     // of function pointers, unless it has the "no_randomize_layout" attribute.
     if (!getLangOpts().CPlusPlus &&
         (Record->hasAttr<RandomizeLayoutAttr>() ||
          (!Record->hasAttr<NoRandomizeLayoutAttr>() &&
-          llvm::all_of(Record->decls(), IsFunctionPointer))) &&
+          llvm::all_of(Record->decls(), IsFunctionPointerOrForwardDecl))) &&
         !Record->isUnion() && !getLangOpts().RandstructSeed.empty() &&
         !Record->isRandomized()) {
       SmallVector<Decl *, 32> NewDeclOrdering;
       if (randstruct::randomizeStructureLayout(Context, Record,
                                                NewDeclOrdering))
         Record->reorderDecls(NewDeclOrdering);
     }
 

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

+// RUN: %clang_cc1 -triple=x86_64-unknown-linux -emit-llvm -frandomize-layout-seed=1234567890abcdef < %s | FileCheck %s
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux -emit-llvm -frandomize-layout-seed=1234567890abcdef -DFORWARD_DECL < %s | FileCheck -check-prefix=FWD-DECL %s
+// PR60349
+
+// Clang will add a forward declaration of "struct bar" and "enum qux" to the
+// structures. This shouldn't prevent these structures from being randomized.
+// So the 'f' element shouldn't be at the start of the structure anymore.
+
+#ifdef FORWARD_DECL
+struct bar;
+enum qux;
+#endif
+
+struct foo {
+  struct bar *(*f)(void);
+  struct bar *(*g)(void);
+  struct bar *(*h)(void);
+  struct bar *(*i)(void);
+  struct bar *(*j)(void);
+  struct bar *(*k)(void);
+} __attribute__((randomize_layout));
+
+// CHECK-LABEL: define {{.*}}@t1
+// CHECK: getelementptr inbounds %struct.foo, ptr %0, i32 0, i32 2
+
+// FWD-DECL-LABEL: define {{.*}}@t1
+// FWD-DECL: getelementptr inbounds %struct.foo, ptr %0, i32 0, i32 2
+struct bar *t1(struct foo *z) {
+  return z->f();
+}
+
+struct baz {
+  enum qux *(*f)(void);
+  enum qux *(*g)(void);
+  enum qux *(*h)(void);
+  enum qux *(*i)(void);
+  enum qux *(*j)(void);
+  enum qux *(*k)(void);
+} __attribute__((randomize_layout));
+
+// CHECK-LABEL: define {{.*}}@t2
+// CHECK: getelementptr inbounds %struct.baz, ptr %0, i32 0, i32 4
+
+// FWD-DECL-LABEL: define {{.*}}@t2
+// FWD-DECL: getelementptr inbounds %struct.baz, ptr %0, i32 0, i32 4
+enum qux *t2(struct baz *z) {
+  return z->f();
+}