Advice about modifying MicrosoftRecordLayoutBuilder

Clang Frontend
1

I’m trying to fix what seems like a bug to me regarding structure layout when msvc compatibility us enabled (issue #59486 on github).
The first question I have is, how can I confirm this is a bug rather than an odd requirement for MS ABI compatibility (i.e. who do I ask!)?
Secondly, I have a (very trivial) fix that I’d like to verify doesn’t break ABI compatibility. How would I do that?

2

Could you try
clang++ -std=c++17 -Xclang -fdump-record-layouts example.cpp
to show the record layout?

3

with my repro case:

typedef __attribute__((__ext_vector_type__(4),__aligned__(4))) float V;
static_assert(alignof(V)==4, "?");
struct S {
    alignas(4) V v;
};
  
static_assert(alignof(S)==4, "?");

I get
*** Dumping AST Record Layout
0 | struct S
0 | V v
| [sizeof=16, align=16,
| nvsize=16, nvalign=16]
…..\bug.cpp:7:1: error: static assertion failed due to requirement ‘alignof(S) == 4’: ?
static_assert(alignof(S)==4, “?”);
^ ~~~~~~~~~~~~~
…..\bug.cpp:7:25: note: expression evaluates to ‘16 == 4’
static_assert(alignof(S)==4, “?”);

without my change, and

*** Dumping AST Record Layout
0 | struct S
0 | V v
| [sizeof=16, align=4,
| nvsize=16, nvalign=4]

with my change.

But the bigger question is, what should I get (for ABI compatibility)?

4

If you get different layouts for Windows and Linux, then that is probably a feature of the different ABIs. If you get different layouts from clang and MSVC, then it is a real issue.

5

We have a -fclang-abi-compat flag that we sometimes use to apply ABI breaking changes.

This specific case is a bit funny - it involves a clang extension that doesn’t exist on MSVC. @rnk is the code owner for MSVC ABI in this case.

6

I’m trying to fix what seems like a bug to me regarding structure layout when msvc compatibility us enabled (issue #59486 on github).

Can you share more about the background here. Why do you want to lower the alignment of the vector?

But the bigger question is, what should I get (for ABI compatibility)?

It’s hard to say since the code uses Clang extensions and we can’t just see what MSVC would do with it. To find an equivalent test case, perhaps the vector could be replaced with __m128. IIUC __declspec(align(n)) and alignas(n) can only be used to increase alignment, not reduce it, but we can use #pragma pack(n):

#include <xmmintrin.h>

#pragma pack(4)
struct S {
    __m128 v;
};
static_assert(alignof(S)==4, "?");

MSVC will still trip on the static_assert: Compiler Explorer
Which perhaps suggests that Clang is doing the right thing compatibility wise.

Secondly, I have a (very trivial) fix that I’d like to verify doesn’t break ABI compatibility. How would I do that?

Can you show the diff? Maybe git blame on the code you’re modifying has some hints about the current behavior. Do the lit tests still pass with your patch?

7

You’re right that alignas can only increase alignment, but in this case it’s redundant and only serves to verify that V truly is (no more than) 4-byte aligned in that context. So we have a structure comprised exclusively of 4-byte aligned things that somehow requires 16 byte alignment.

__m128 is explicitly defined (in xmmintrin.h) to be 16 byte aligned, so a fairer msvc comparison would be to define V as something like:

typedef union __declspec(intrin_type) __declspec(align(4)) V {
     float               m128_f32[4];
 } V;

…and then MSVC also gives the S 4-byte alignment.

My use case has a vector in a base class which is inherited from in a nested situation something like this:

struct S1 : S { int i1; };
struct S2 : S1 { int i; };
struct S3 : S2 { int i2;};

the alignment causes 12 additional bytes padding at each depth.

8

This is the diff (I said it was trivial!)

diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 52bd3f20221f..f32a28ce2aaf 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2705,7 +2705,7 @@ MicrosoftRecordLayoutBuilder::getAdjustedElementInfo(
     Info.Alignment = std::min(Info.Alignment, MaxFieldAlignment);
   if (FD->hasAttr<PackedAttr>())
     Info.Alignment = CharUnits::One();
-  Info.Alignment = std::max(Info.Alignment, FieldRequiredAlignment);
+  Info.Alignment = FieldRequiredAlignment;
   return Info;
 }

The thing is that msvc-compatibility uses a separate codepath for structure layout, so it’s not obvious what is a deliberate ABI change and what is just an oversight in the re-written code; and this separate codepath (and this alignment issue) both date back to clang 3.6!

9

Hmm, well the patch makes clang assert a lot during tests, so in any case that doesn’t seem like the right fix.

So we have a structure comprised exclusively of 4-byte aligned things that somehow requires 16 byte alignment.

I’m not sure it’s a 4-byte aligned thing though. When targeting Windows it seems clang wants the vector type 16-byte aligned. It would be interesting to find where that decision is made, and if it seems deliberate or a bug.

Unfortunately I probably won’t have time to look more into this before the holidays.

10

It is a good idea that __m128 is 16 byte aligned. The CPU can use aligned loads and stores, which are more performant than unaligned loads. It is probably not a good idea to fiddle with the alignment of vectors.