-frandomize-layout-seed inconsistently randomizes all-function-pointers structs

[ebiggers]

I found that if the Linux kernel v6.2-rc5 is built with Clang (latest git, commit 898b5c9f5e77) with CONFIG_RANDSTRUCT=y included in the kernel configuration, there is a miscompilation in drivers/gpu/drm/amd/display/dc/dcn303/dcn303_init.c.

The problem is that when clang's -frandomize-layout-seed option automatically randomizes the layout of structs that contain only function pointers, it incorrectly skips structs that contain implicit forward declarations. For example, consider the following struct:

struct foo {
	struct bar *(*f)(void);
	struct bar *(*g)(void);
};

If struct bar is already declared, then clang sees that struct foo contains
only function pointers and it randomizes the layout of struct foo.

However, if struct bar is not yet declared, then clang inserts an implicit forward declaration of struct bar into struct foo. Because of that, clang then incorrectly considers struct foo to contain fields other than function pointers, so it doesn't randomize the layout of struct foo.

This also can happen with enums:

struct foo {
	enum bar (*f)(void);
	enum bar (*g)(void);
};

In either case, the real trouble happens when bar has an explicit declaration in some translation units but not others. That makes struct foo be randomized in some translation units but not others.

Changing this line of code in SemaDecl.cpp to return true instead of false fixes the bug.

[ebiggers]

The relevant code was added in commit 6f79700 by @isanbard.

Also Cc'ing @kees.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[kees]

cc @bwendling Can you verify and land this fix?

[bwendling]

That would solve the immediate issue, but would allow for others later on. What we need to do is explicitly test for a forward declaration of a struct / enum and return "true" if it is. This is somewhat misleading, because it's not a function pointer, but it'll allow the logic to work.

[ebiggers]

That would solve the immediate issue, but would allow for others later on. What we need to do is explicitly test for a forward declaration of a struct / enum and return "true" if it is. This is somewhat misleading, because it's not a function pointer, but it'll allow the logic to work.

IsFunctionPointer could be renamed to something like IsEligibleForAutoRandstruct.

By the way, a unit test will probably be needed as well, which is partly why I didn't just send a patch myself. I'm hoping someone more familiar with LLVM / clang development will know what to do for that.

[bwendling]

See https://reviews.llvm.org/D143300

[nickdesaulniers]

/cherry-pick f85a9a6

[llvmbot]

/branch llvm/llvm-project-release-prs/issue60349

[llvmbot]

/pull-request llvm/llvm-project-release-prs#276