[randstruct] Enforce using a designated init for a randomized struct

[void]

Summary

A randomized structure needs to use a designated or default initializer.
Using a non-designated initializer will result in values being assigned
to the wrong fields.

[aaron.ballman]

Thank you for this!

Because we're not checking anything about the randomized output or the AST nodes directly, this should be tested through the usual lit tests instead of using a unit test.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11591–11596

clang/unittests/AST/RandstructTest.cpp

484

Other things to test:

struct test t1 = {}; // This should be fine per WG14 N2900 (in C23) + our extension handling of it in earlier modes
struct test t2 = {0}; // This should also be fine per C99 6.7.8p19
struct test t3 = { .a = foo, bar, baz }; // Error

struct other_test {
  func_ptr a;
  func_ptr b[3];
  func_ptr c;
};

struct other_test t4 = { .a = foo, .b[0] = foo }; // Ok
struct other_test t5 = { .a = foo, .b[0] = foo, bar, baz }; // Ok
struct other_test t6 = { .a = foo, .b[0] = foo, bar, baz, gaz }; // Error
struct other_test t7 = { .a = foo, .b = { foo, bar, baz } }; // Ok

[void]

Accept initialzers of the forms:

struct foo f = {};
struct foo g = {0};

Move tests to lit tester.
Improve error message.

[void]

Add some smoke tests.

[void]

Updated with those changes.

[MaskRay]

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

30 ↗

(On Diff #422936)

Perhaps test an empty struct with randomize_layout and a struct with one field with randomize_layout?

[aaron.ballman]

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

30 ↗

(On Diff #422936)

Good point -- there's really no reason to reject something like:

struct degenerate {
  func_ptr a;
};

struct degenerate d= { foo };

even though it's not doing a zero initialization.

[void]

Add tests for empty and single element structs.

[aaron.ballman]

LGTM, thank you for adding some guard rails for this new feature!

[MaskRay]

LGTM, too.

It may be worthwhile adding a file-level comment at the header of the test.

[stuij]

clang/lib/Sema/SemaInit.cpp

2174

@void Unfortunately this end of fields check will break the non-designated initializer check below.

I GDB'ed through a failure of the below test, and if I'm understanding this correctly, the CheckDesignatedInitializer invocation above will move Field to the next available field in the struct. If there is none, we will break out of the loop and never reach the code below (On an AArch64 Linux host the field was placed last in the struct).

Instead I get a different error:

error: 'error' diagnostics expected but not seen:
  File /Users/zeno/code/llvm/clean/clang/test/Sema/init-randomized-struct.c Line 46: a randomized struct can only be initialized with a designated initializer
error: 'error' diagnostics seen but not expected:
  File /Users/zeno/code/llvm/clean/clang/test/Sema/init-randomized-struct.c Line 46: excess elements in struct initializer
2 errors generated.

You can replicate this on other build setups by varying the value of -frandomoze-layout-seed. On x86_64 Linux and on Aarch64 OSX this worked for me (in seed value of lit test, change f to d):

-frandomize-layout-seed=1234567890abcded

Also, I know this was talked about before, and I know a fix is planned, but just to add my two cents: yes, it would be great if the std::shuffle could be changed to llvm::shuffle, also because we're expecting to produced the same code across different platforms for safety (compliance) reasons.

[void]

clang/lib/Sema/SemaInit.cpp

2174

Oh! Good catch. Yeah, it's getting a bit messed up when the field that's initialized by a designated initializer is moved to the bottom of the RecordDecl. It looks like I can move it above the Field == FieldEnd check and it works (and bonus, I think it's the correct fix).

Thanks!

[sbc100]

This new test has been failing on the emscripten builders.. seemingly ever since it landed:

https://ci.chromium.org/ui/p/emscripten-releases/builders/ci/linux-test-suites/b8815286583388131569/overview

It also fails for me locally:

../llvm-build/bin/llvm-lit clang/test/Sema/init-randomized-struct.c -v -a
ninja: Entering directory `/usr/local/google/home/sbc/dev/wasm/llvm-build'
ninja: no work to do.
llvm-lit: /usr/local/google/home/sbc/dev/wasm/llvm-project/llvm/utils/lit/lit/llvm/config.py:438: note: using clang: /usr/local/google/home/sbc/dev/wasm/llvm-build/bin/clang
-- Testing: 1 tests, 1 workers --
FAIL: Clang :: Sema/init-randomized-struct.c (1 of 1)
******************** TEST 'Clang :: Sema/init-randomized-struct.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /usr/local/google/home/sbc/dev/wasm/llvm-build/bin/clang -cc1 -internal-isystem /usr/local/google/home/sbc/dev/wasm/llvm-build/lib/clang/15.0.0/include -nostdsysteminc -triple=x86_64-unknown-linux -frandomize-layout-seed=1234567890abcdef   -verify -fsyntax-only -Werror /usr/local/google/home/sbc/dev/wasm/llvm-project/clang/test/Sema/init-randomized-struct.c
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen: 
  File /usr/local/google/home/sbc/dev/wasm/llvm-project/clang/test/Sema/init-randomized-struct.c Line 34: a randomized struct can only be initialized with a designated initializer
  File /usr/local/google/home/sbc/dev/wasm/llvm-project/clang/test/Sema/init-randomized-struct.c Line 46: a randomized struct can only be initialized with a designated initializer
error: 'error' diagnostics seen but not expected: 
  File /usr/local/google/home/sbc/dev/wasm/llvm-project/clang/test/Sema/init-randomized-struct.c Line 34: excess elements in struct initializer
  File /usr/local/google/home/sbc/dev/wasm/llvm-project/clang/test/Sema/init-randomized-struct.c Line 46: excess elements in struct initializer
4 errors generated.

--

********************
********************
Failed Tests (1):
  Clang :: Sema/init-randomized-struct.c


Testing Time: 0.08s
  Failed: 1

Any ideas? I wonder why this is not failing for others?

Could it be something about how I'm configuring llvm: cmake -G Ninja -DCMAKE_C_COMPILER=/usr/local/google/home/sbc/dev/chromium/src/third_party/llvm-build/Release+Asserts/bin/clang -DCMAKE_CXX_COMPILER=/usr/local/google/home/sbc/dev/chromium/src/third_party/llvm-build/Release+Asserts/bin/clang++ -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/usr/local/google/home/sbc/dev/wasm/waterfall/src/work/wasm-install -DCMAKE_SHARED_LINKER_FLAGS=-fuse-ld=gold -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON -DLLVM_BUILD_LLVM_DYLIB=OFF -DLLVM_LINK_LLVM_DYLIB=OFF -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_SPHINX=ON -DLLVM_ENABLE_PROJECTS=lld;clang;libcxx;libcxxabi -DLLVM_INSTALL_BINUTILS_SYMLINKS=ON -DLLVM_INSTALL_TOOLCHAIN_ONLY=ON -DLLVM_ENABLE_TERMINFO=OFF -DCMAKE_SYSROOT=/usr/local/google/home/sbc/dev/wasm/waterfall/src/work/sysroot_debian_stretch_amd64/ -DCMAKE_C_COMPILER_LAUNCHER=/usr/local/google/home/sbc/bin/depot_tools/.cipd_bin/gomacc -DCMAKE_CXX_COMPILER_LAUNCHER=/usr/local/google/home/sbc/bin/depot_tools/.cipd_bin/gomacc -DLLVM_TARGETS_TO_BUILD=X86;WebAssembly ../llvm-project/llvm ?

[void]

In D123763#3485836, @sbc100 wrote:

This new test has been failing on the emscripten builders.. seemingly ever since it landed:

https://ci.chromium.org/ui/p/emscripten-releases/builders/ci/linux-test-suites/b8815286583388131569/overview

[snip]

Any ideas? I wonder why this is not failing for others?

It's probably because the bloody randomization code isn't the same on all platforms. :-( There's a fix in review right now.

[dschuff]

In D123763#3485836, @sbc100 wrote:

This new test has been failing on the emscripten builders.. seemingly ever since it landed:

This uses a fairly old sysroot (gcc 6.3). I tried with a new version of libcxx, and it seems to be passing. So maybe this is depending on some new C++ that our old sysroot doesn't support?

clang/include/clang/Basic/DiagnosticSemaKinds.td

 
 // HLSL Diagnostics
 def err_hlsl_attr_unsupported_in_stage : Error<"attribute %0 is unsupported in %select{Pixel|Vertex|Geometry|Hull|Domain|Compute|Library|RayGeneration|Intersection|AnyHit|ClosestHit|Miss|Callable|Mesh|Amplification|Invalid}1 shaders, requires %2">;
 
 def err_hlsl_numthreads_argument_oor : Error<"argument '%select{X|Y|Z}0' to numthreads attribute cannot exceed %1">;
 def err_hlsl_numthreads_invalid : Error<"total number of threads cannot exceed %0">;
 def err_hlsl_attribute_param_mismatch : Error<"%0 attribute parameters do not match the previous declaration">;
 
-// Layout randomization warning.
+// Layout randomization diagnostics.
+def err_non_designated_init_used : Error<
+  "non-designated initializers cannot be used on a randomized struct">;
 def err_cast_from_randomized_struct : Error<
   "casting from randomized structure pointer type %0 to %1">;
+
 } // end of sema component.

clang/lib/Sema/SemaInit.cpp

if (DesignatedInitExpr *DIE = dyn_cast<DesignatedInitExpr>(Init)) {
       InitializedSomething = true;
 
       // Disable check for missing fields when designators are used.
       // This matches gcc behaviour.
       CheckForMissingFields = false;
       continue;
     }
 
     if (Field == FieldEnd) {
       // We've run out of fields. We're done.
       break;
     }
 
+    // Don't allow non-designated initializers on randomized structures.
+    if (RD->isRandomized()) {
+      if (!VerifyOnly)
+        SemaRef.Diag(InitLoc, diag::err_non_designated_init_used);
+      hadError = true;
+      break;
+    }
+
     // We've already initialized a member of a union. We're done.
     if (InitializedSomething && DeclType->isUnionType())
       break;
 
     // If we've hit the flexible array member at the end, we're done.
     if (Field->getType()->isIncompleteArrayType())
       break;
 

clang/unittests/AST/RandstructTest.cpp

TEST(RANDSTRUCT_TEST, MarkedRandomize) {
   EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
 
   const RecordDecl *RD = getRecordDeclFromAST(AST->getASTContext(), "test");
 
   EXPECT_TRUE(RD->hasAttr<RandomizeLayoutAttr>());
   EXPECT_TRUE(RD->isRandomized());
 }
 
-TEST(RANDSTRUCT_TEST, MismatchedAttrsDeclVsDef) {
-  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
-    struct test __attribute__((randomize_layout));
-    struct test {
-        int bacon;
-        long lettuce;
-        long long tomato;
-        float mayonnaise;
-    } __attribute__((no_randomize_layout));
-  )c");
-
-  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
-
-  DiagnosticsEngine &Diags = AST->getDiagnostics();
-
-  EXPECT_FALSE(Diags.hasFatalErrorOccurred());
-  EXPECT_FALSE(Diags.hasUncompilableErrorOccurred());
-  EXPECT_FALSE(Diags.hasUnrecoverableErrorOccurred());
-  EXPECT_EQ(Diags.getNumWarnings(), 1u);
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-}
-
-TEST(RANDSTRUCT_TEST, MismatchedAttrsRandomizeVsNoRandomize) {
-  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
-    struct test2 {
-        int bacon;
-        long lettuce;
-        long long tomato;
-        float mayonnaise;
-    } __attribute__((randomize_layout)) __attribute__((no_randomize_layout));
-  )c");
-
-  EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
-
-  DiagnosticsEngine &Diags = AST->getDiagnostics();
-
-  EXPECT_TRUE(Diags.hasUncompilableErrorOccurred());
-  EXPECT_TRUE(Diags.hasUnrecoverableErrorOccurred());
-  EXPECT_EQ(Diags.getNumWarnings(), 0u);
-  EXPECT_EQ(Diags.getNumErrors(), 1u);
-}
-
-TEST(RANDSTRUCT_TEST, MismatchedAttrsNoRandomizeVsRandomize) {
-  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
-    struct test3 {
-        int bacon;
-        long lettuce;
-        long long tomato;
-        float mayonnaise;
-    } __attribute__((no_randomize_layout)) __attribute__((randomize_layout));
-  )c");
-
-  EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
-
-  DiagnosticsEngine &Diags = AST->getDiagnostics();
-
-  EXPECT_TRUE(Diags.hasUncompilableErrorOccurred());
-  EXPECT_TRUE(Diags.hasUnrecoverableErrorOccurred());
-  EXPECT_EQ(Diags.getNumWarnings(), 0u);
-  EXPECT_EQ(Diags.getNumErrors(), 1u);
-}
-
 TEST(RANDSTRUCT_TEST, CheckAdjacentBitfieldsRemainAdjacentAfterRandomization) {
   const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
     struct test {
         int a;
         int b;
         int x : 1;
         int y : 1;
         int z : 1;
if (const FieldDecl *FD = dyn_cast<FieldDecl>(D)) {
         }
       }
     }
 
   EXPECT_TRUE(AnonStructTested);
   EXPECT_TRUE(AnonUnionTested);
 }
 
+TEST(RANDSTRUCT_TEST, MismatchedAttrsDeclVsDef) {
+  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
+    struct test __attribute__((randomize_layout));
+    struct test {
+        int bacon;
+        long lettuce;
+        long long tomato;
+        float mayonnaise;
+    } __attribute__((no_randomize_layout));
+  )c");
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  DiagnosticsEngine &Diags = AST->getDiagnostics();
+
+  EXPECT_FALSE(Diags.hasFatalErrorOccurred());
+  EXPECT_FALSE(Diags.hasUncompilableErrorOccurred());
+  EXPECT_FALSE(Diags.hasUnrecoverableErrorOccurred());
+  EXPECT_EQ(Diags.getNumWarnings(), 1u);
+  EXPECT_EQ(Diags.getNumErrors(), 0u);
+}
+
+TEST(RANDSTRUCT_TEST, MismatchedAttrsRandomizeVsNoRandomize) {
+  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
+    struct test2 {
+        int bacon;
+        long lettuce;
+        long long tomato;
+        float mayonnaise;
+    } __attribute__((randomize_layout)) __attribute__((no_randomize_layout));
+  )c");
+
+  EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
+
+  DiagnosticsEngine &Diags = AST->getDiagnostics();
+
+  EXPECT_TRUE(Diags.hasUncompilableErrorOccurred());
+  EXPECT_TRUE(Diags.hasUnrecoverableErrorOccurred());
+  EXPECT_EQ(Diags.getNumWarnings(), 0u);
+  EXPECT_EQ(Diags.getNumErrors(), 1u);
+}
+
+TEST(RANDSTRUCT_TEST, MismatchedAttrsNoRandomizeVsRandomize) {
+  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
+    struct test3 {
+        int bacon;
+        long lettuce;
+        long long tomato;
+        float mayonnaise;
+    } __attribute__((no_randomize_layout)) __attribute__((randomize_layout));
+  )c");
+
+  EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
+
+  DiagnosticsEngine &Diags = AST->getDiagnostics();
+
+  EXPECT_TRUE(Diags.hasUncompilableErrorOccurred());
+  EXPECT_TRUE(Diags.hasUnrecoverableErrorOccurred());
+  EXPECT_EQ(Diags.getNumWarnings(), 0u);
+  EXPECT_EQ(Diags.getNumErrors(), 1u);
+}
+
+TEST(RANDSTRUCT_TEST, RandomizedStructWithDesignatedInit) {
+  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
+    typedef void (*func_ptr)();
+
+    void foo(void);
+    void bar(void);
+    void baz(void);
+    void gaz(void);
+
+    struct test {
+        func_ptr a;
+        func_ptr b;
+        func_ptr c;
+        func_ptr d;
+        func_ptr e;
+        func_ptr f;
+        func_ptr g;
+    } __attribute__((randomize_layout)) inst = {
+      .f = baz, .b = bar, .g = gaz, .a = foo
+    };
+  )c");
+
+  EXPECT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  DiagnosticsEngine &Diags = AST->getDiagnostics();
+
+  EXPECT_FALSE(Diags.hasFatalErrorOccurred());
+  EXPECT_FALSE(Diags.hasUncompilableErrorOccurred());
+  EXPECT_FALSE(Diags.hasUnrecoverableErrorOccurred());
+  EXPECT_EQ(Diags.getNumWarnings(), 0u);
+  EXPECT_EQ(Diags.getNumErrors(), 0u);
+}
+
+TEST(RANDSTRUCT_TEST, RandomizedStructWithNonDesignatedInit) {
+  const std::unique_ptr<ASTUnit> AST = makeAST(R"c(
+    typedef void (*func_ptr)();
+
+    void foo(void);
+    void bar(void);
+    void baz(void);
+    void gaz(void);
+
+    struct test {
+        func_ptr a;
+        func_ptr b;
+        func_ptr c;
+        func_ptr d;
+        func_ptr e;
+        func_ptr f;
+        func_ptr g;
+    } __attribute__((randomize_layout)) inst = {
+      baz, bar, gaz, foo
+    };
+  )c");
+
+  EXPECT_TRUE(AST->getDiagnostics().hasErrorOccurred());
+
+  DiagnosticsEngine &Diags = AST->getDiagnostics();
+
+  EXPECT_FALSE(Diags.hasFatalErrorOccurred());
+  EXPECT_TRUE(Diags.hasUncompilableErrorOccurred());
+  EXPECT_TRUE(Diags.hasUnrecoverableErrorOccurred());
+  EXPECT_EQ(Diags.getNumWarnings(), 0u);
+  EXPECT_EQ(Diags.getNumErrors(), 1u);
+}
+
 } // namespace ast_matchers
 } // namespace clang