From 9525530ea4ee4d70b55b5b376ad96ecdd587bd39 Mon Sep 17 00:00:00 2001 From: Chih-Hung Hsieh Date: Thu, 15 Nov 2018 16:07:02 -0800 Subject: Update asserts in mojo about int64_t's alignment The purpose of these asserts is to check that the options structs are aligned sufficiently that an int64_t (or similar) could be added to them. The asserts were added in https://codereview.chromium.org/295383012 However, the alignment of int64_t on 32-bit x86 is actually 4 bytes, not 8. So how did this ever work? Before Clang r345419, the alignof() operator (which is what MOJO_ALIGNOF maps to) would return the *preferred alignment* of a type, not the *ABI alignment*. That's not what the C and C++ standards specify, so the bug was fixed and the asserts started firing. (See also https://llvm.org/pr26547) To fix the build, change the asserts to check that int64_t requires 8 bytes alignment *or less*. Bug: 900406 Bug: 119634736 Test: make checkbuild Change-Id: Icf80cea80ac31082423faab8c192420d0b98d515 Reviewed-on: https://chromium-review.googlesource.com/c/1318971 Commit-Queue: Ken Rockot Reviewed-by: Ken Rockot Cr-Commit-Position: refs/heads/master@{#605699} --- libchrome_tools/patch/alignof_int64.patch | 88 +++++++++++++++++++++++++++++++ mojo/core/options_validation_unittest.cc | 2 +- mojo/public/c/system/buffer.h | 2 +- mojo/public/c/system/data_pipe.h | 2 +- mojo/public/c/system/message_pipe.h | 2 +- 5 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 libchrome_tools/patch/alignof_int64.patch diff --git a/libchrome_tools/patch/alignof_int64.patch b/libchrome_tools/patch/alignof_int64.patch new file mode 100644 index 0000000000..121def4d39 --- /dev/null +++ b/libchrome_tools/patch/alignof_int64.patch @@ -0,0 +1,88 @@ +From 5177b28400e03d8666dfb3e65c3016b5062ce0f7 Mon Sep 17 00:00:00 2001 +From: Chih-Hung Hsieh +Date: Thu, 15 Nov 2018 16:07:02 -0800 +Subject: [PATCH] Update asserts in mojo about int64_t's alignment + +The purpose of these asserts is to check that the options structs are aligned +sufficiently that an int64_t (or similar) could be added to them. The asserts +were added in https://codereview.chromium.org/295383012 + +However, the alignment of int64_t on 32-bit x86 is actually 4 bytes, not 8. So +how did this ever work? Before Clang r345419, the alignof() operator (which is what +MOJO_ALIGNOF maps to) would return the *preferred alignment* of a type, not +the *ABI alignment*. That's not what the C and C++ standards specify, so the +bug was fixed and the asserts started firing. (See also +https://llvm.org/pr26547) + +To fix the build, change the asserts to check that int64_t requires 8 bytes +alignment *or less*. + +Bug: 900406 +Bug: 119634736 +Change-Id: Icf80cea80ac31082423faab8c192420d0b98d515 +Reviewed-on: https://chromium-review.googlesource.com/c/1318971 +Commit-Queue: Ken Rockot +Reviewed-by: Ken Rockot +Cr-Commit-Position: refs/heads/master@{#605699} +--- + mojo/core/options_validation_unittest.cc | 2 +- + mojo/public/c/system/buffer.h | 2 +- + mojo/public/c/system/data_pipe.h | 2 +- + mojo/public/c/system/message_pipe.h | 2 +- + 4 files changed, 4 insertions(+), 4 deletions(-) + +diff --git a/mojo/core/options_validation_unittest.cc b/mojo/core/options_validation_unittest.cc +index 52e0032..b4b02dc 100644 +--- a/mojo/core/options_validation_unittest.cc ++++ b/mojo/core/options_validation_unittest.cc +@@ -18,7 +18,7 @@ namespace { + + using TestOptionsFlags = uint32_t; + +-static_assert(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); ++static_assert(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); + struct MOJO_ALIGNAS(8) TestOptions { + uint32_t struct_size; + TestOptionsFlags flags; +diff --git a/mojo/public/c/system/buffer.h b/mojo/public/c/system/buffer.h +index 2cc5427..83b198b 100644 +--- a/mojo/public/c/system/buffer.h ++++ b/mojo/public/c/system/buffer.h +@@ -30,7 +30,7 @@ struct MOJO_ALIGNAS(8) MojoCreateSharedBufferOptions { + // See |MojoCreateSharedBufferFlags|. + MojoCreateSharedBufferFlags flags; + }; +-MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); ++MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); + MOJO_STATIC_ASSERT(sizeof(MojoCreateSharedBufferOptions) == 8, + "MojoCreateSharedBufferOptions has wrong size"); + +diff --git a/mojo/public/c/system/data_pipe.h b/mojo/public/c/system/data_pipe.h +index 3702cdb..c72f553 100644 +--- a/mojo/public/c/system/data_pipe.h ++++ b/mojo/public/c/system/data_pipe.h +@@ -40,7 +40,7 @@ struct MOJO_ALIGNAS(8) MojoCreateDataPipeOptions { + // system-dependent capacity of at least one element in size. + uint32_t capacity_num_bytes; + }; +-MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); ++MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); + MOJO_STATIC_ASSERT(sizeof(MojoCreateDataPipeOptions) == 16, + "MojoCreateDataPipeOptions has wrong size"); + +diff --git a/mojo/public/c/system/message_pipe.h b/mojo/public/c/system/message_pipe.h +index 9f222f9..0f642dd 100644 +--- a/mojo/public/c/system/message_pipe.h ++++ b/mojo/public/c/system/message_pipe.h +@@ -35,7 +35,7 @@ struct MOJO_ALIGNAS(8) MojoCreateMessagePipeOptions { + // See |MojoCreateMessagePipeFlags|. + MojoCreateMessagePipeFlags flags; + }; +-MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); ++MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); + MOJO_STATIC_ASSERT(sizeof(MojoCreateMessagePipeOptions) == 8, + "MojoCreateMessagePipeOptions has wrong size"); + +-- +2.20.0.rc0.387.gc7a69e6b6c-goog + diff --git a/mojo/core/options_validation_unittest.cc b/mojo/core/options_validation_unittest.cc index 52e0032a89..b4b02dc735 100644 --- a/mojo/core/options_validation_unittest.cc +++ b/mojo/core/options_validation_unittest.cc @@ -18,7 +18,7 @@ namespace { using TestOptionsFlags = uint32_t; -static_assert(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); +static_assert(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); struct MOJO_ALIGNAS(8) TestOptions { uint32_t struct_size; TestOptionsFlags flags; diff --git a/mojo/public/c/system/buffer.h b/mojo/public/c/system/buffer.h index 2cc54270ad..83b198b2a8 100644 --- a/mojo/public/c/system/buffer.h +++ b/mojo/public/c/system/buffer.h @@ -30,7 +30,7 @@ struct MOJO_ALIGNAS(8) MojoCreateSharedBufferOptions { // See |MojoCreateSharedBufferFlags|. MojoCreateSharedBufferFlags flags; }; -MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); +MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); MOJO_STATIC_ASSERT(sizeof(MojoCreateSharedBufferOptions) == 8, "MojoCreateSharedBufferOptions has wrong size"); diff --git a/mojo/public/c/system/data_pipe.h b/mojo/public/c/system/data_pipe.h index 3702cdb624..c72f55387a 100644 --- a/mojo/public/c/system/data_pipe.h +++ b/mojo/public/c/system/data_pipe.h @@ -40,7 +40,7 @@ struct MOJO_ALIGNAS(8) MojoCreateDataPipeOptions { // system-dependent capacity of at least one element in size. uint32_t capacity_num_bytes; }; -MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); +MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); MOJO_STATIC_ASSERT(sizeof(MojoCreateDataPipeOptions) == 16, "MojoCreateDataPipeOptions has wrong size"); diff --git a/mojo/public/c/system/message_pipe.h b/mojo/public/c/system/message_pipe.h index 9f222f9aa8..0f642dd965 100644 --- a/mojo/public/c/system/message_pipe.h +++ b/mojo/public/c/system/message_pipe.h @@ -35,7 +35,7 @@ struct MOJO_ALIGNAS(8) MojoCreateMessagePipeOptions { // See |MojoCreateMessagePipeFlags|. MojoCreateMessagePipeFlags flags; }; -MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) == 8, "int64_t has weird alignment"); +MOJO_STATIC_ASSERT(MOJO_ALIGNOF(int64_t) <= 8, "int64_t has weird alignment"); MOJO_STATIC_ASSERT(sizeof(MojoCreateMessagePipeOptions) == 8, "MojoCreateMessagePipeOptions has wrong size"); -- cgit v1.2.3