[libclang] Allow field offset lookups in types with incomplete arrays.

[JornVernee]

Summary

Currently, looking up the offset of a field in a type with an incomplete array, using clang_Type_getOffsetOf always returns CXTypeLayoutError_Incomplete. e.g. for the following:

struct Foo {
    int size;
    void* data[]; // incomplete array
};

This returns CXTypeLayoutError_Incomplete when looking up the offset of either the 'size' or the 'data' field.

But, since an incomplete array always appears at the end of a record, looking up the field offsets should be fine. Note that this also works fine using the offsetof macro.

The fix for this is to ignore incomplete array fields when doing the recursive type validation.

[JornVernee]

Fixed the similar problem with clang_Type_getAlignOf since the patch is pretty small any ways.

[JornVernee]

@yvvan Mind taking a look here as well?

[aaron.ballman]

test/Index/print-type-size.c

23

This should be the first line of the file. I don't have strong opinions on where the CHECK lines go, but I usually prefer seeing them near the construct being checked.

32

Please add a newline to the end of the file.

tools/libclang/CXType.cpp

902

I don't think IAT is a common enough acronym to use; I'd probably just drop the comment as it adds little value.

964

Same here.

964

I sort of wonder whether we want a helper function like isTypeIncompleteForLayout() or something, and then using that helper directly.

[JornVernee]

• Restructured test source file to put CHECK comments inline
• Removed IAT comments.

• holding of on adding helper method until hearing back.

[JornVernee]

Add forgotten newline

[aaron.ballman]

In D61239#1485994, @JornVernee wrote:

• holding of on adding helper method until hearing back.

My rationale for wanting a helper method is because we already have two places were incompleteness is important but has special cases. It's easier to maintain that with something that's a named function to explain what the predicate is actually doing. My concern is that we add another isIncompleteType() and not think about this issue.

Do we need this in validateFieldParentType()?

[JornVernee]

In D61239#1486005, @aaron.ballman wrote:

In D61239#1485994, @JornVernee wrote:

• holding of on adding helper method until hearing back.

My rationale for wanting a helper method is because we already have two places were incompleteness is important but has special cases. It's easier to maintain that with something that's a named function to explain what the predicate is actually doing. My concern is that we add another isIncompleteType() and not think about this issue.

Do we need this in validateFieldParentType()?

Thanks for the response, I misunderstood.

I usually go with naming my predicates in that way as well, but I misunderstood where you wanted to use it. I think adding the helper method for validateFieldParentType() is good. But, the check in clang_Cursor_getAlignOf is semantically different, since we basically want to check if the type has an alignment. (I actually realized we also need to check the element type for completeness in the case of incomplete arrays. Figuring that out now).

test/Index/print-type-size.c

23

This style was taken from print-type-size.cpp, which also puts everything at the end. I'll put it at the front/inline instead (makes sense to me as well). I guess it's a legacy thing we're trying to move away from?

tools/libclang/CXType.cpp

964

Note that clang_Type_getSizeOf will (and AFAIK should) still return CXTypeLayoutError_Incomplete for incomplete arrays. I'd expect a potential isTypeIncompleteForLayout() to be used by all three functions (OffsetOf, SizeOf, AlignOf). So, I'm in favour of doing without such a helper method for the special OffsetOf and AlignOf cases.

It's a pretty unique case (but it appears a bunch of times in Windows API headers so we'd like to have support for it :) ).

[JornVernee]

• Replaced plane predicates with helper methods describing their function.
• Fixed test file indentation

Changing the logic for clang_Type_getAlignOf was not needed after all, since the use of incomplete type as array element type was already causing an error earlier on (so I think we can assume at that point it's safe?)

[aaron.ballman]

test/Index/print-type-size.c

7

Can you run this through clang-format so it matches our usual formatting style rules?

tools/libclang/CXType.cpp

888

No need to use a reference here, just pass QualType (non-const).

957

Similar here.

[JornVernee]

• Removed const& QualType and passing by-value instead
• Ran print-type-size.c through clang-format

[JornVernee]

Removed spurious newline

[aaron.ballman]

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

[JornVernee]

In D61239#1486634, @aaron.ballman wrote:

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

Yeah, my mistake. It was there after all. The output from clang-format looked the same as what I had before, but I think I just didn't catch the newline when copy-pasting the diff the first time :)

Also, I don't have committer access. Would you mind committing this?

[aaron.ballman]

In D61239#1486692, @JornVernee wrote:

In D61239#1486634, @aaron.ballman wrote:

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

Yeah, my mistake. It was there after all. The output from clang-format looked the same as what I had before, but I think I just didn't catch the newline when copy-pasting the diff the first time :)

Ah that makes more sense, phew!

Also, I don't have committer access. Would you mind committing this?

Happy to do so -- I'll try to get it in this week if I have the time, but it may not happen until early next week.

[JornVernee]

In D61239#1487406, @aaron.ballman wrote:

In D61239#1486692, @JornVernee wrote:

Also, I don't have committer access. Would you mind committing this?

Happy to do so -- I'll try to get it in this week if I have the time, but it may not happen until early next week.

Take your time. Thanks for al the help!

[aaron.ballman]

I've commit in r360147, thank you for the patch!

test/Index/print-type-size.c

+// RUN: c-index-test -test-print-type-size %s | FileCheck %s
+
+struct Foo {
+  int size;
+  // CHECK: FieldDecl=size:4:7 (Definition) [type=int] [typekind=Int] [sizeof=4] [alignof=4] [offsetof=0]
+  void *data[];
+  // CHECK: FieldDecl=data:6:9 (Definition) [type=void *[]] [typekind=IncompleteArray] [sizeof=-2] [alignof=8] [offsetof=64]
+};
+
+struct Bar {
+  int size;
+  // CHECK: FieldDecl=size:11:7 (Definition) [type=int] [typekind=Int] [sizeof=4] [alignof=4] [offsetof=0]
+  struct {
+    int dummy;
+    // CHECK: FieldDecl=dummy:14:9 (Definition) [type=int] [typekind=Int] [sizeof=4] [alignof=4] [offsetof=64/0]
+    void *data[];
+    // CHECK: FieldDecl=data:16:11 (Definition) [type=void *[]] [typekind=IncompleteArray] [sizeof=-2] [alignof=8] [offsetof=128/64]
+  };
+};
+
+struct Baz {
+  int size;
+  // CHECK: FieldDecl=size:22:7 (Definition) [type=int] [typekind=Int] [sizeof=4] [alignof=4] [offsetof=0]
+  union {
+    void *data1[];
+    // CHECK: FieldDecl=data1:25:11 (Definition) [type=void *[]] [typekind=IncompleteArray] [sizeof=-2] [alignof=8] [offsetof=64/0]
+    void *data2[];
+    // CHECK: FieldDecl=data2:27:11 (Definition) [type=void *[]] [typekind=IncompleteArray] [sizeof=-2] [alignof=8] [offsetof=64/0]
+  };
+};
+

tools/libclang/CXType.cpp

case Type::ConstantArray:
       break;
     default:
       break;
     }
   }
   return result;
 }
 
+static bool isIncompleteTypeWithAlignment(QualType QT) {
+  return QT->isIncompleteArrayType() || !QT->isIncompleteType();
+}
+
 long long clang_Type_getAlignOf(CXType T) {
   if (T.kind == CXType_Invalid)
     return CXTypeLayoutError_Invalid;
   ASTContext &Ctx = cxtu::getASTUnit(GetTU(T))->getASTContext();
   QualType QT = GetQualType(T);
   // [expr.alignof] p1: return size_t value for complete object type, reference
   //                    or array.
   // [expr.alignof] p3: if reference type, return size of referenced type
   if (QT->isReferenceType())
     QT = QT.getNonReferenceType();
-  if (QT->isIncompleteType())
+  if (!isIncompleteTypeWithAlignment(QT))
     return CXTypeLayoutError_Incomplete;
   if (QT->isDependentType())
     return CXTypeLayoutError_Dependent;
   if (const auto *Deduced = dyn_cast<DeducedType>(QT))
     if (Deduced->getDeducedType().isNull())
       return CXTypeLayoutError_Undeduced;
   // Exceptions by GCC extension - see ASTContext.cpp:1313 getTypeInfoImpl
   // if (QT->isFunctionType()) return 4; // Bug #15511 - should be 1
long long clang_Type_getSizeOf(CXType T) {
   // [gcc extension] lib/AST/ExprConstant.cpp:1372
   //                 HandleSizeof : {voidtype,functype} == 1
   // not handled by ASTContext.cpp:1313 getTypeInfoImpl
   if (QT->isVoidType() || QT->isFunctionType())
     return 1;
   return Ctx.getTypeSizeInChars(QT).getQuantity();
 }
 
+static bool isTypeIncompleteForLayout(QualType QT) {
+  return QT->isIncompleteType() && !QT->isIncompleteArrayType();
+}
+
 static long long visitRecordForValidation(const RecordDecl *RD) {
   for (const auto *I : RD->fields()){
     QualType FQT = I->getType();
-    if (FQT->isIncompleteType())
+    if (isTypeIncompleteForLayout(FQT))
       return CXTypeLayoutError_Incomplete;
     if (FQT->isDependentType())
       return CXTypeLayoutError_Dependent;
     // recurse
     if (const RecordType *ChildType = I->getType()->getAs<RecordType>()) {
       if (const RecordDecl *Child = ChildType->getDecl()) {
         long long ret = visitRecordForValidation(Child);
         if (ret < 0)