[PATCH] [libclang] getSizeOf/getAlignOf/getOffsetOf (was [PATCH] Expose AST Record layout attributes to libclang)
Argyrios Kyrtzidis
akyrtzi at gmail.com
Mon Mar 18 17:59:04 PDT 2013
Hi Loïc,
Thanks for your work! I have some comments:
> /**
> + * \brief List the possible error codes for getTypeSizeOf and getTypeAlignOf.
> + *
> + * A value of this enumeration type can be returned if the target type is not
> + * \c a valid argument to sizeof or alignof.
> + */
In the comments you should use the full function names with "\c" in front. You should also mention the getOffset-kind functions.
> + CXTypeLayoutError_InvalidFieldName = -5,
> + /**
> + * \brief One field is an incomplete Type.
> + */
> + CXTypeLayoutError_IncompleteField = -6,
> + /**
> + * \brief One field is a dependent Type.
> + */
> + CXTypeLayoutError_DependentField = -7,
> + /**
> + * \brief One field is not a constant size type.
> + */
> + CXTypeLayoutError_NotConstantSizeField = -8
> +};
This confused me about what is the difference between CXTypeLayoutError_Incomplete and CXTypeLayoutError_IncompleteField. Via reading the code I assume these refer to the parent record that the field belongs to, right ?
A record cannot be "NotConstantSize", this is for variable-length (non-constant) arrays, so CXTypeLayoutError_NotConstantSizeField can be removed.
And how about adding "Parent" to make it more clear about what they describe:
CXTypeLayoutError_IncompleteFieldParent
CXTypeLayoutError_DependentFieldParent
> +CINDEX_LINKAGE long long clang_getAlignOf(CXType T);
...
> +CINDEX_LINKAGE long long clang_getSizeOf(CXType T);
...
Please "prefix" the function names accordingly:
clang_getAlignOf -> clang_Type_getAlignOf
clang_isBitField -> clang_Cursor_isBitField
<....>
> +/**
> + * \brief Return the offset of the field specified byt the cursor as it would be
> + * returned by __offsetof__
This should mention that offset is returned in bits.
> +long long getOffsetOfFieldDecl(const FieldDecl * FD) {
This should be static.
> +long long clang_getOffsetOf(CXType PT, const char* S) {
> + // get the parent record type declaration
> + CXCursor PC = clang_getTypeDeclaration(PT);
> + if (clang_isInvalid(PC.kind))
> + return CXTypeLayoutError_Invalid;
> + const RecordDecl *RD =
> + dyn_cast_or_null<RecordDecl>(cxcursor::getCursorDecl(PC));
> + if (!RD)
> + return CXTypeLayoutError_Invalid;
> + RD = RD->getDefinition();
> + if (!RD)
> + return CXTypeLayoutError_Incomplete;
Should this return CXTypeLayoutError_IncompleteFieldParent ?
Could you also check if the record is dependent before proceeding ?
> + // iterate the fields to get the matching name
> + StringRef fieldname = StringRef(clang_getCString(cxstring::createRef(S)));
Just do:
StringRef fieldname = StringRef(S);
> + for (RecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();
> + I != E; ++I) {
> + if ( fieldname == (*I)->getName())
> + return getOffsetOfFieldDecl((*I));
> + }
> + return CXTypeLayoutError_InvalidFieldName; // FieldDecl is Null
> +}
Does this handle anonymous structs/unions ? For example:
struct Test {
struct {
union {
int foo;
};
};
};
If I pass "struct Test", "foo" will it work ? If yes, could you add a related test ?
-Argyrios
On Mar 15, 2013, at 12:08 AM, Loïc Jaquemet <loic.jaquemet at gmail.com> wrote:
> Thanks for your time Dmitri. I'm sorry its taking so long for me to
> get that right.
>
>
>>> Should sizeOf return -1 when the type is a bitfield ? [expr.sizeof] p1]
>>
>> Yes.
>
> Well, given the current signature of the function (CXType), we will
> never be evaluating a field ( CXCursor). Only types.
> So my guess, there is no specific case here after all.
>
>>> + // Exceptions by GCC extension - see ASTContext.cpp:1313 getTypeInfoImpl
>>> + // if (QT->isFunctionType()) return 4;
>>>
>>> I could not find the gcc extension reference document.
>>
>> http://gcc.gnu.org/onlinedocs/gcc/C-Extensions.html
>>
>> But apparently it does not cover this extension.
>
> Do I need to change the current situation for alignof? And to what ?
> I expect the #15511 bug fix to fix it automatically.
> Current output:
> CXXMethod=foo:212:16 (Definition) (virtual) [type=void ()]
> [typekind=FunctionProto] [sizeof=1] [alignof=4]
>
>
>>> + // [expr.sizeof] p1: return -1 on: func, incomplete, bitfield,
>>> incomplete enumeration
>>> + // [expr.sizeof] p3: pointer ok, function not ok.
>>> + // [gcc extension] lib/AST/ExprConstant.cpp:1372 HandleSizeof : vla == error
>>> + if (QT->isIncompleteType() || QT->isDependentType() ||
>>> !QT->isConstantSizeType())
>>> + return -2;
>>>
>>> I have to test for incompleteType and dependentType ( and constant
>>> size type in sizeof) otherwise it asserts on templates.
>>
>> Could you use different error codes for these situations? A dumb
>> client can check return value >= 0, while a smart one can get more
>> information.
>
> Done.
>
>>
>> -#define CINDEX_VERSION_MINOR 12
>> +#define CINDEX_VERSION_MINOR 13
>>
>> Please refresh your patches to apply to trunk (we are at version 15 now :)
>>
>
> Done
>
>
>> /**
>> + * \brief Return the alignment of a type in bytes as per
>> C++[expr.alignof] standard.
>>
>> 80 cols.
>>
> [.. and bunch of others ]
>
> done
>
>> + */
>> +CINDEX_LINKAGE long long clang_getRecordFieldOffsetInBits(CXCursor C);
>>
>> It would help to add an enum with error codes.
>
> Done
>
>>
>> But, there's other concern: what you need is to expose the record
>> layout. While a general sizeof() is useful, it will not work for you,
>> exactly because of all these exceptions. For example,
>> sizeof(reference type) is sizeof(type), but you need to know the
>> number of bytes the *reference itself* takes.
>
> Yes. I though about that, and for what I want to my achieve, I feel
> that do not need an extra libclang function after all.
> I will always query the type of the field, and have specific code for
> reference and pointers anyway.
> Other exceptions do not apply in the context.
> So no need to clutter libclang with that.
>
> I dropped the getRecordXXX functions in favor of sizeof/alignof/offsetof
> I did put in two function for offsetof, because it can be queried both
> with the Cursor and with the standard parent + fieldname combination.
> And I do feel its an added value.
>
>
>>
>> +long long clang_getRecordFieldOffsetInBits(CXCursor C) {
>> + if (!clang_isDeclaration(C.kind))
>> + return -1;
>> + const FieldDecl *FD =
>> dyn_cast_or_null<FieldDecl>(cxcursor::getCursorDecl(C));
>> + if (!FD)
>> + return -1;
>> + QualType QT = GetQualType(clang_getCursorType(C));
>> + if (QT->isIncompleteType() || QT->isDependentType() ||
>> !QT->isConstantSizeType())
>>
>> I don't see a test for isConstantSizeType case.
>
> Sadly I have no idea of an example for a non ConstantSizeType.
> Any hint ?
>
>>
>> +static enum CXChildVisitResult PrintTypeSize(CXCursor cursor, CXCursor p,
>> + CXClientData d) {
>>
>> Please align 'CXClientData' to 'CXCursor'.
>
> done
>
>>
>> +static enum CXChildVisitResult PrintTypeSize(CXCursor cursor, CXCursor p,
>> + CXClientData d) {
>> + if (!clang_isInvalid(clang_getCursorKind(cursor))) {
>>
>> Reverse the condition and de-nest:
>
> done
>
> Added tests for bitfields too.
>
>
>>
>> The tests could be reduced or reorganized to make them more targeted
>> (to check a specific thing). There's a lot of stuff that actually
>> checks MS struct layout algorithms.
>
> Cleaned up tests to be only composed of useful tests. Got rid of old
> pasted tests.
>
>>
>> Python bindings look good.
>
> added bitfields tests and offsetof
>
>>
>> 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>*/
>
>
>
> --
> Loïc Jaquemet
> <sizeof-alignof-offsetof-001><sizeof-alignof-offsetof-002-tests><sizeof-alignof-offsetof-003-python-bindings><sizeof-alignof-offsetof-004-python-bindings-tests>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130318/40f60bb3/attachment.html>
More information about the cfe-commits
mailing list