[PATCH] Expose AST Record layout attributes to libclang

[Dmitri Gribenko]

On Tue, Feb 12, 2013 at 3:04 AM, Loïc Jaquemet <loic.jaquemet at gmail.com> wrote:
> Hello,
>
> This patch is 3 functions in libclang to get more Record Layout
> information from the AST when using libclang.
>
> I am trying to implement the python ctypeslib record generator by
> replacing gccxml with libclang.
>
> Following your comments, I corrected my submission.

This looks better now, but still not there yet.

+ * \brief Return the alignment value for a record, in bytes.

"Return the alignment of a record (struct or class), in bytes" ?

+ * \brief Return the offset of a field in a record in bits.

What does this return for bitfields?  Is it possible that return value
is not a multiple of 8?

+  return Layout.getAlignment().getQuantity();·

Trailing whitespace.

+  return Layout.getFieldOffset(FieldNo);·

Trailing whitespace.

+long long clang_getRecordSize(CXType T) {
...
+long long clang_getRecordAlignment(CXType T) {
...
+long long clang_getRecordFieldOffset(CXCursor C) {

Nit: please reorder according to the order in Index.h (and put it
after clang_getArraySize consistently).

+  const FieldDecl *F = dyn_cast<FieldDecl>(D);

'FD' is more in line with Clang style, rather than a plain 'F'.

+struct s{
+  int a;
+  char b;
+  long long c;
+};
+union u{
+  int u1;
+  long long u2;
+  struct s s1;
+};

Nit: space before '{'.

Important stuff: the actual checking is missing.  Since this is going
to be a target-specific test, it is better to put it into a separate
file rather than to convert an existing target-independent test.  In
order to make the test pass on all hosts, just explicitly specify the
triple.  (Grep for 'triple' in tests/ for examples that run clang that
way.)

More comments: please add a test with bitfields, and tests with C++
records (don't forget about virtual functions, inheritance and virtual
inheritance).  Since the scope for these functions was not limited in
the documentation, we assume they should work for all cases.  It is
perfectly reasonable to limit ourselves only to POD, but we need to
document that (& rename functions to include 'POD').

+      long long size = clang_getRecordSize(T);

Variable names start with a capital letter.  Although c-index-test is
written in a weird mix of styles, please follow the style guidelines.

+    import ctypes····

Trailing whitespace.

     assert all(x.translation_unit is not None for x in fields)
+····
+    align = teststruct.type.get_record_alignment()

Trailing whitespace.

assert fields[2].get_record_field_offset() == 8*(2*ctypes.sizeof(ctypes.c_int))

Sorry, but this (and other lines) is wrong (there might be padding).
The proposed functions are being added for precisely that reason -- we
can not compute member offsets *simply by adding sizes of members*.
This test should specify a triple, too, and check for hardcoded
values.

> Yet I have no idea how to correctly implement the test of a record
> size, given it is dependent of the tester's arch.
> is there some lit.py magic for that ?

You should to specify a '-triple' to c-index-test.  See above.

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>*/