[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

11605–11610

clang/unittests/AST/RandstructTest.cpp

484 ↗

(On Diff #422760)

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

31

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

31

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

2171

@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

2171

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

 def err_hlsl_attribute_param_mismatch : Error<"%0 attribute parameters do not match the previous declaration">;
 
 def err_hlsl_pointers_unsupported : Error<
   "%select{pointers|references}0 are unsupported in HLSL">;
 
 def err_hlsl_operator_unsupported : Error<
   "the '%select{&|*|->}0' operator is unsupported in HLSL">;
 
-// Layout randomization warning.
+// Layout randomization diagnostics.
+def err_non_designated_init_used : Error<
+  "a randomized struct can only be initialized with a designated initializer">;
 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

void InitListChecker::CheckStructUnionTypes(
   }
 
   // If structDecl is a forward declaration, this loop won't do
   // anything except look at designated initializers; That's okay,
   // because an error should get printed out elsewhere. It might be
   // worthwhile to skip over the rest of the initializer, though.
   RecordDecl *RD = DeclType->castAs<RecordType>()->getDecl();
   RecordDecl::field_iterator FieldEnd = RD->field_end();
+  size_t NumRecordFields = std::distance(RD->field_begin(), RD->field_end());
   bool CheckForMissingFields =
     !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts());
   bool HasDesignatedInit = false;
 
   while (Index < IList->getNumInits()) {
     Expr *Init = IList->getInit(Index);
     SourceLocation InitLoc = Init->getBeginLoc();
 
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;
     }
 
+    // Check if this is an initializer of forms:
+    //
+    //   struct foo f = {};
+    //   struct foo g = {0};
+    //
+    // These are okay for randomized structures. [C99 6.7.8p19]
+    //
+    // Also, if there is only one element in the structure, we allow something
+    // like this, because it's really not randomized in the tranditional sense.
+    //
+    //   struct foo h = {bar};
+    auto IsZeroInitializer = [&](const Expr *I) {
+      if (IList->getNumInits() == 1) {
+        if (NumRecordFields == 1)
+          return true;
+        if (const auto *IL = dyn_cast<IntegerLiteral>(I))
+          return IL->getValue().isZero();
+      }
+      return false;
+    };
+
+    // Don't allow non-designated initializers on randomized structures.
+    if (RD->isRandomized() && !IsZeroInitializer(Init)) {
+      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/test/Sema/init-randomized-struct.c

+// RUN: %clang_cc1 %s -verify -fsyntax-only -frandomize-layout-seed=1234567890abcdef
+
+// Initializing a randomized structure requires a designated initializer,
+// otherwise the element ordering will be off. The only exceptions to this rule
+// are:
+//
+//    - A structure with only one element, and
+//    - A structure initialized with "{0}".
+//
+// These are well-defined situations where the field ordering doesn't affect
+// the result.
+
+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));
+
+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 = { .f = baz, .b = bar, .g = gaz, .a = foo }; // Okay
+struct test t4 = { .a = foo, bar, baz }; // expected-error {{a randomized struct can only be initialized with a designated initializer}}
+
+struct other_test {
+  func_ptr a;
+  func_ptr b[3];
+  func_ptr c;
+} __attribute__((randomize_layout));
+
+struct other_test t5 = { .a = foo, .b[0] = foo }; // Okay
+struct other_test t6 = { .a = foo, .b[0] = foo, bar, baz }; // Okay
+struct other_test t7 = { .a = foo, .b = { foo, bar, baz } }; // Okay
+struct other_test t8 = { baz, bar, gaz, foo }; // expected-error {{a randomized struct can only be initialized with a designated initializer}}
+struct other_test t9 = { .a = foo, .b[0] = foo, bar, baz, gaz }; // expected-error {{a randomized struct can only be initialized with a designated initializer}}
+
+struct empty_test {
+} __attribute__((randomize_layout));
+
+struct empty_test t10 = {}; // Okay
+
+struct degen_test {
+  func_ptr a;
+} __attribute__((randomize_layout));
+
+struct degen_test t11 = { foo }; // Okay