[PATCH] Expose AST Record layout attributes to libclang

[Dmitri Gribenko]

On Wed, Feb 13, 2013 at 7:08 AM, Loïc Jaquemet <loic.jaquemet at gmail.com> wrote:
>> After thinking a little more about it, I have a general question about
>> the approach here: wouldn't it be better to implement
>> clang_getTypeSize/clang_getTypeAlignment that work for all types?
>> These functions should work just like sizeof/alignof.  This should be
>> easy enough to implement: ASTContext::getTypeSize()/getTypeAlign() + a
>> few special cases, which are:
>
> partially done

+long long clang_getTypeAlign(CXType T) {
+  CXCursor C = clang_getTypeDeclaration(T);
+  if (!clang_isDeclaration(C.kind))
+    return -1;

Do we need this check?  (Same in clang_getTypeSize)

+  ASTContext &Ctx = cxcursor::getCursorContext(C);
+  QualType QT = GetQualType(T);
+  if (QT->isIncompleteType())·
+    return -1;

If we need the check above, please use different error codes.  (Same
in clang_getTypeSize)

+  return Ctx.getTypeAlign(QT);
+}

+long long clang_getRecordFieldOffset(CXCursor C) {
+  if (!clang_isDeclaration(C.kind))
+    return -1;
+  const Decl *D = cxcursor::getCursorDecl(C);
+  const FieldDecl *FD = dyn_cast<FieldDecl>(D);
+  if (!FD)
+    return -1;
+  QualType QT = GetQualType(clang_getCursorType(C));
+  if (QT->isIncompleteType())·
+    return -1;

I don't think we need a check for incomplete types here -- they are
not allowed.  Or do we?  (To handle incorrect source code?)  We could
check for "isInvalidDecl()" on FD->getParent().

+// RUN: c-index-test -test-print-typekind %s -target i386-linux-gnu |
FileCheck -check-prefix=CHECK1 %s
+// RUN: c-index-test -test-print-typekind %s -target
nvptx64-unknown-unknown | FileCheck -check-prefix=CHECK2 %s
+// RUN: c-index-test -test-print-typekind %s -target i386-pc-win32 |
FileCheck -check-prefix=CHECK3 %s
+// RUN: c-index-test -test-print-typekind %s -target msp430-none-none
| FileCheck -check-prefix=CHECK4 %s

Please extract these tests to a separate file.  It only helps to have
a single file when the check patterns are the same for the whole file.
 Actually, I don't think we need this -- you can just reuse the
CHECK32/CHECK64 lines from above.  We are not trying to test record
layout algorithms (should be testeded elsewhere), but just check that
these APIs return something sensible and don't regress.

+    tries=[(['-target','i386-linux-gnu'],(32,128,0,32,35,64)),(['-target','nvptx64-unknown-unknown'],(64,192,0,64,67,128)),
+            (['-target','i386-pc-win32'],(64,128,0,32,35,64)),
(['-target','msp430-none-none'],(16,112,0,32,35,48))]

Please add line breaks to have four lines here.

>> * sizeof(void), __alignof__(void), sizeof(function) = 1 as a gcc
>> extension (lib/AST/ExprConstant.cpp:1372)
>>
>> * C++ [expr.sizeof]p2: "When applied to a reference or a reference
>> type, the result is the size of the referenced type."
>> (lib/AST/ExprConstant.cpp:5228).
>>
>> * incomplete types -- just return an error for these.
>>
>> Sorry for not thinking of this earlier!
>
> I'm not sure I understand what you want me to achieve with these exceptions.

It would be nice to return the same results as sizeof().  It is a
well-known abstraction.

> should I hardcode
> 'typekind=FunctionProto' to return size:1
> and
> typekind=LValueReference to return size:sizeof(MyType)
>
> clang seems to disagree with the size of a reference in a record.
>
> for example, in the record-layout.cpp :
> TypeRef=class Test3::C:182:7 typekind=Record [size=704] [alignment=64] [isPOD=0]
> FieldDecl=rg:201:8 (Definition) typekind=LValueReference [offset=832] [isPOD=0]
> TypeRef=class Test3::C:182:7 typekind=Record [size=704] [alignment=64] [isPOD=0]
> FieldDecl=x:202:9 (Definition) typekind=Int [offset=896] [isPOD=1]
>
> If I had to return a size value for typekind=LValueReference, it
> should be the target word size, no ?
> I am not trying to implement a sizeof(). I'm trying to expose memory
> mapping of records.
> The size of the pointee is already given by the Typeref child entity.

I see -- we really need a different API here rather than imitating
'sizeof()'.  Please document these differences, and feel free to
revert to the previous approach of
clang_getRecordSize/clang_getRecordAlignment, because inventing an API
to "return size of a type, but not quite sizeof() in corner cases"
seems awkward.

>>> PS:
>>> I get a assertion error if record-layout-virtual.cpp is renamed as
>>> record-layout-virtual.c.
>>> c-index-test: /home/jal/compil/llvm/llvm/tools/clang/lib/AST/RecordLayoutBuilder.cpp:2470:
>>> const clang::ASTRecordLayout&
>>> clang::ASTContext::getASTRecordLayout(const clang::RecordDecl*) const:
>>> Assertion `D && "Cannot get layout of forward declarations!"' failed.
>>>
>>> I am not sure that error ( provoked by c++ in a .c file ) is actually
>>> part of my code or not.
>>> I have no knowledge of forward declaration or solution to that problem.
>>
>> Right, one can not ask the size of an incomplete type.  We need to
>> check for these, and we should probably have a separate error code for
>> incomplete types.
>>
>> QualType QT = GetQualType(T);
>> if (QT->isIncompleteType()) ...
>
> fix with return -1

OK, but see the comment above about different error codes.

> I didn't find a way to get a Sema from the ASTContext.

There's a way to get Sema from ASTUnit.  (ASTUnit::getSema() if
ASTUnit::hasSema()).  To get ASTUnit, you first get a
CXTranslationUnit from CXType (see tools/libclang/CXType.cpp:124 --
GetTU()), and then get ASTUnit from the TU (cxtu::getASTUnit()).

> Practically, from the tests I assessed that the only cases I got a
> forward incomplete type is for incorrect source code.

(1) Nevertheless, we should not crash on that.

(2) Please add a test for sizeof a pointer to incomplete type:

class A;
class B {
  A* a;
};

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/