[AST] Get field size in chars rather than bits in RecordLayoutBuilder.

[ebevhan]

Summary

In D79719, LayoutField was refactored to fetch the size of field
types in bits and then convert to chars, rather than fetching
them in chars directly. This is not ideal, since it makes the
calculations char size dependent, and breaks for sizes that
are not a multiple of the char size.

This patch changes it to use getTypeInfoInChars instead of
getTypeInfo.

[Xiangling_L]

clang/lib/AST/RecordLayoutBuilder.cpp

1841–1842

In most cases, getTypeInfoInChars invokes getTypeInfo underneath. So to make people be careful about this, I would suggest to leave a comment explaining/claiming we have to call getTypeInfoInChars here. And also maybe adding a testcase to guard the scenario you were talking about would be helpful to prevent someone to use getTypeInfo here in the future.

[ebevhan]

clang/lib/AST/RecordLayoutBuilder.cpp

1841–1842

I can do that.

I honestly don't think it would be a bad idea to add an assertion to toCharUnitsFromBits that checks for non-bytesize-multiple amounts. I wonder how much would fail if I did that, though.

[rsmith]

This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size.

How can we have a non-bitfield member whose size is not a multiple of the size of a char?

[ebevhan]

In D85191#2193645, @rsmith wrote:

This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size.

How can we have a non-bitfield member whose size is not a multiple of the size of a char?

Downstream, we have fixed-point types that are 24 bits large, but where the char size is 16. The type then takes up 2 chars, where 8 of the bits are padding. The only way in Clang to express that the width of the bit representation of a type should be smaller than the number of chars it takes up in memory -- and consequently, produce an i24 in IR -- is to return a non-charsize multiple from getTypeInfo.

We did it this way because it was possible. If the intent is for getTypeInfo to always return sizes that are multiples of the char size, then the design should be inverted and getTypeInfo should simply be calling getTypeInfoInChars and multiply that result by the char size. But that isn't how it works.

[ebevhan]

clang/lib/AST/RecordLayoutBuilder.cpp

1841–1842

Oh, I guess I only really replied to the first part about adding a comment here... I'm not sure I can make a test case for this, since I don't think there's anything that triggers this upstream.

[rsmith]

In D85191#2195923, @ebevhan wrote:

In D85191#2193645, @rsmith wrote:

This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size.

How can we have a non-bitfield member whose size is not a multiple of the size of a char?

Downstream, we have fixed-point types that are 24 bits large, but where the char size is 16. The type then takes up 2 chars, where 8 of the bits are padding. The only way in Clang to express that the width of the bit representation of a type should be smaller than the number of chars it takes up in memory -- and consequently, produce an i24 in IR -- is to return a non-charsize multiple from getTypeInfo.

This violates the C and C++ language rules, which require the size of every type to be a multiple of the size of char. A type with 24 value bits and 8 padding bits should report a type size of 32 bits, just like bool reports a size of CHAR_BIT bits despite having only 1 value bit, and x86_64 long double reports a type size of 128 bits despite having only 80 value bits.

[bjope]

In D85191#2196863, @rsmith wrote:

In D85191#2195923, @ebevhan wrote:

In D85191#2193645, @rsmith wrote:

This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size.

How can we have a non-bitfield member whose size is not a multiple of the size of a char?

Downstream, we have fixed-point types that are 24 bits large, but where the char size is 16. The type then takes up 2 chars, where 8 of the bits are padding. The only way in Clang to express that the width of the bit representation of a type should be smaller than the number of chars it takes up in memory -- and consequently, produce an i24 in IR -- is to return a non-charsize multiple from getTypeInfo.

This violates the C and C++ language rules, which require the size of every type to be a multiple of the size of char. A type with 24 value bits and 8 padding bits should report a type size of 32 bits, just like bool reports a size of CHAR_BIT bits despite having only 1 value bit, and x86_64 long double reports a type size of 128 bits despite having only 80 value bits.

I don't see that it breaks the language rules. The sizeof result for the 24 bit type should be 2 in the target described by @ebevhan (two 16-bit bytes). But I imagine that without this patch it is reported as 24/16=1, right?

So isn't the problem that toCharUnitsFromBits is rounding down when given a bitsize that isn't a multiple of CHAR_BIT? Would it perhaps make sense to let it round up instead?

[efriedma]

If the intent is for getTypeInfo to always return sizes that are multiples of the char size, then the design should be inverted and getTypeInfo should simply be calling getTypeInfoInChars and multiply that result by the char size. But that isn't how it works.

The reason it doesn't work this way is just that someone made the wrong choice a decade ago, and nobody has spent the time to rewrite it since. Patch welcome.

[ebevhan]

In D85191#2197550, @bjope wrote:

In D85191#2196863, @rsmith wrote:

In D85191#2195923, @ebevhan wrote:

In D85191#2193645, @rsmith wrote:

This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size.

How can we have a non-bitfield member whose size is not a multiple of the size of a char?

Downstream, we have fixed-point types that are 24 bits large, but where the char size is 16. The type then takes up 2 chars, where 8 of the bits are padding. The only way in Clang to express that the width of the bit representation of a type should be smaller than the number of chars it takes up in memory -- and consequently, produce an i24 in IR -- is to return a non-charsize multiple from getTypeInfo.

This violates the C and C++ language rules, which require the size of every type to be a multiple of the size of char. A type with 24 value bits and 8 padding bits should report a type size of 32 bits, just like bool reports a size of CHAR_BIT bits despite having only 1 value bit, and x86_64 long double reports a type size of 128 bits despite having only 80 value bits.

I don't see that it breaks the language rules. The sizeof result for the 24 bit type should be 2 in the target described by @ebevhan (two 16-bit bytes). But I imagine that without this patch it is reported as 24/16=1, right?

Yes, this is what's happening. The sizeof should be reported as 2 (32 bits), but isn't, because toCharUnitsFromBits always rounds down.

So isn't the problem that toCharUnitsFromBits is rounding down when given a bitsize that isn't a multiple of CHAR_BIT? Would it perhaps make sense to let it round up instead?

The issue with toCharUnitsFromBits is that it's an inherently dangerous API. There could be cases where you want to round down, and cases where you want to round up. The function cannot know.

It could be better if toCharUnitsFromBits took an extra parameter that explicitly specifies the rounding, and if that parameter is set to a default (for unspecified rounding) and the amount passed is not a multiple of the char size, it asserts. This would make a lot of tests fail until all of the uses are corrected, though.

In D85191#2196863, @rsmith wrote:

In D85191#2195923, @ebevhan wrote:

In D85191#2193645, @rsmith wrote:

This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size.

How can we have a non-bitfield member whose size is not a multiple of the size of a char?

Downstream, we have fixed-point types that are 24 bits large, but where the char size is 16. The type then takes up 2 chars, where 8 of the bits are padding. The only way in Clang to express that the width of the bit representation of a type should be smaller than the number of chars it takes up in memory -- and consequently, produce an i24 in IR -- is to return a non-charsize multiple from getTypeInfo.

This violates the C and C++ language rules, which require the size of every type to be a multiple of the size of char. A type with 24 value bits and 8 padding bits should report a type size of 32 bits, just like bool reports a size of CHAR_BIT bits despite having only 1 value bit, and x86_64 long double reports a type size of 128 bits despite having only 80 value bits.

But this is the crux of the matter; if you aren't allowed to return non-char-sizes from getTypeInfo, then there's no way to specify via TargetInfo/getTypeInfo that the number of value bits of a type is less than the size in chars. That is, that a type is padded. And as your examples show, C does not disallow that.

In D85191#2197663, @efriedma wrote:

If the intent is for getTypeInfo to always return sizes that are multiples of the char size, then the design should be inverted and getTypeInfo should simply be calling getTypeInfoInChars and multiply that result by the char size. But that isn't how it works.

The reason it doesn't work this way is just that someone made the wrong choice a decade ago, and nobody has spent the time to rewrite it since. Patch welcome.

This does sound like a good thing to do, but it would be problematic downstream since it would completely prohibit the design that we're trying to use.

[tschuett]

I don't know whether the name of your downstream target is a secret. Wouldn't it help you to add a fake 16bit per char target to clang and add units tests to prevent regressions?

[rsmith]

In D85191#2199574, @ebevhan wrote:

In D85191#2197663, @efriedma wrote:

If the intent is for getTypeInfo to always return sizes that are multiples of the char size, then the design should be inverted and getTypeInfo should simply be calling getTypeInfoInChars and multiply that result by the char size. But that isn't how it works.

The reason it doesn't work this way is just that someone made the wrong choice a decade ago, and nobody has spent the time to rewrite it since. Patch welcome.

This does sound like a good thing to do, but it would be problematic downstream since it would completely prohibit the design that we're trying to use.

That would be a good thing, as the design you're trying to use is not one that we intend to support. The size returned by getTypeInfo is intended to include padding.

[ebevhan]

It doesn't feel like this patch got a very positive reception, but I'd still like to try a bit more to get it in.

Even though it's difficult to test this particular change upstream, would it still be acceptable to take the patch since it reverts the behavior to what it was previously? If there are worries that things may break in the future due to other changes, we do catch these things in our downstream testing and are fairly diligent about reporting back about them.

[efriedma]

I'm still concerned your approach to the computation of getTypeSize() is a ticking time bomb, but I'll take the cleanup even if the underlying motivation doesn't really make sense.

clang/lib/AST/RecordLayoutBuilder.cpp

1847

Can we fix getTypeInfoInChars so it returns all the necessary info, so we don't look up the typeinfo twice?

[ebevhan]

clang/lib/AST/RecordLayoutBuilder.cpp

1847

That feels like a hefty change since it would require changing every caller of getTypeInfoInChars. Do you want me to do that in this patch or in a separate one?

[efriedma]

LGTM

clang/lib/AST/RecordLayoutBuilder.cpp

1847

Separate patch makes sense.

If you want to do it after this one, that's fine.

clang/lib/AST/RecordLayoutBuilder.cpp

void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
   CharUnits FieldSize;
   CharUnits FieldAlign;
   // The amount of this class's dsize occupied by the field.
   // This is equal to FieldSize unless we're permitted to pack
   // into the field's tail padding.
   CharUnits EffectiveFieldSize;
 
   auto setDeclInfo = [&](bool IsIncompleteArrayType) {
-    TypeInfo TI = Context.getTypeInfo(D->getType());
-    FieldAlign = Context.toCharUnitsFromBits(TI.Align);
+    auto TI = Context.getTypeInfoInChars(D->getType());
+    FieldAlign = TI.second;
     // Flexible array members don't have any size, but they have to be
     // aligned appropriately for their element type.
     EffectiveFieldSize = FieldSize =
-        IsIncompleteArrayType ? CharUnits::Zero()
-                              : Context.toCharUnitsFromBits(TI.Width);
-    AlignIsRequired = TI.AlignIsRequired;
+        IsIncompleteArrayType ? CharUnits::Zero() : TI.first;
+    AlignIsRequired = Context.getTypeInfo(D->getType()).AlignIsRequired;
   };
 
   if (D->getType()->isIncompleteArrayType()) {
     setDeclInfo(true /* IsIncompleteArrayType */);
   } else if (const ReferenceType *RT = D->getType()->getAs<ReferenceType>()) {
     unsigned AS = Context.getTargetAddressSpace(RT->getPointeeType());
     EffectiveFieldSize = FieldSize = Context.toCharUnitsFromBits(
         Context.getTargetInfo().getPointerWidth(AS));