Repository: numba/numba
Pull request: Fix bug in alignment computation of `Record.make_c_struct`
<!--

Thanks for wanting to contribute to Numba :)

First, if you need some help or want to chat to the core developers, please
visit https://gitter.im/numba/numba for real time chat or post to the Numba
mailing list https://groups.google.com/a/continuum.io/forum/#!forum/numba-users.

Here's some guidelines to help the review process go smoothly.

0. Please write a description in this text box of the changes that are being
   made.

1. Please ensure that you have written units tests for the changes made/features
   added.

2. If you are closing an issue please use one of the automatic closing words as
   noted here: https://help.github.com/articles/closing-issues-using-keywords/

3. If your pull request is not ready for review but you want to make use of the
   continuous integration testing facilities please label it with `[WIP]` and
   then remove the label when you'd like it to be reviewed.

4. Once review has taken place please do not add features or make changes out of
   the scope of those requested by the reviewer (doing this just add delays as
   already reviewed code ends up having to be re-reviewed/it is hard to tell
   what is new etc!). Further, please do not rebase your branch on master/force
   push/rewrite history, doing any of these causes the context of any comments
   made by reviewers to be lost. If conflicts occur against master they should
   be resolved by merging master into the branch used for making the pull
   request.

Many thanks in advance for your cooperation!

-->


Fixes #3949

Problem statement:
weird behavior of Records.make_c_struct.

OK so the idea I want to implement is make a `cfunc` and pass that function into some C++ code which calls the function pointer as shown in this python script. Here `test_callback` is some cython module which compiles a C function which calls the function pointer.
```
from test_callback import callback_C
from numba import cfunc,carray,types

wrapper = types.Record.make_c_struct([
   ('m', types.intc),
   ('n', types.complex128),
])

sig = types.void(types.CPointer(wrapper))

@cfunc(sig)
def C_caller(argptr):
	arg = carray(argptr,1)
	arg[0].m = 1
	arg[0].n = -1j
	return 

print wrapper
callback_C(C_caller)
```

note that the make_c_struct does not align the memory properly,
the result of `print wrapper` gives: `Record(m[type=int32;offset=0],n[type=complex128;offset=16];24;True)`
but the offset for `n` should be 4 bytes not 16. 
I can reverse the order and it works fine but in the current order will not work.

Hints from issue discussion:
Thanks for the report. I think what you are seeing is due to the following. The `intc` is:
```
size: 4
align: 4
misalign: 0
offset: 0
```
then the `complex128` ends up as:
```
size: 16
align: 8
misalign: 4
offset: 16
```
i.e. it would be a misaligned load if packed against the 4 byte alignment of the `intc` so gets padded to the 16 byte offset. I think it might be possible to adjust the offset computation to be the `offset += align - misalign`, but probably not the suggested 4 byte offset as it would be misaligned for the type. @sklam thoughts?
@stuartarchibald OK I think maybe I don't quite understand how the data is actually packed into the struct.

however the outstanding issue is that the code I wrote does not work with the integer first, if you want I Can provide the source code for the C++/cython code which fails when you change the order in the struct. 
Looks like there's a bug in the alignment of `n`.

<details>

```python
from numba import cfunc,carray,types
from cffi import FFI
from pprint import pprint


wrapper = types.Record.make_c_struct([
    ('m', types.intc),
    ('n', types.complex128),
])

print(wrapper)


ffi = FFI()
ffi.cdef("""
struct complex128 { double real, imag; };
typedef struct _Rec{
    int m;
    struct complex128 n;
} Rec;
""")

c_rec_type = ffi.typeof('Rec')
pprint(dir(c_rec_type))
print(c_rec_type)
for k, fd in c_rec_type.fields:
    print(k, fd.offset, fd.type)
```
</details>

Prints:

```
Record(m[type=int32;offset=0],n[type=complex128;offset=16];24;True)
['cname', 'fields', 'kind']
<ctype 'Rec'>
m 0 <ctype 'int'>
n 8 <ctype 'struct complex128'>
```

`n` should have 8-byte offset but it's getting a 16-byte offset from `Record.make_c_struct`.
the `offset += align - misalign` suggestion seems to have fixed it.  i'll be opening a patch soon.
Oh, there's some trouble: 

```
In [9]: np.dtype([('m', np.intc), ('n', np.complex128)], align=True)
Out[9]: dtype({'names':['m','n'], 'formats':['<i4','<c16'], 'offsets':[0,16], 'itemsize':32}, align=True)
```

Numpy thinks `np.complex128` has 16-byte alignment but LLVM thinks it has 8-byte alignment.
According to https://github.com/numpy/numpy/issues/3768, the 16-byte alignment was due to sparc64.
so, this is fixed since numpy1.16.

Commit message: Add runtime checks for alignment compatibility between NumPy and LLVM.

Patch to review:
diff --git a/numba/numpy_support.py b/numba/numpy_support.py
index d758e04ac80..f17c5b68eb9 100644
--- a/numba/numpy_support.py
+++ b/numba/numpy_support.py
@@ -160,8 +160,24 @@ def as_struct_dtype(rec):
         'offsets': [rec.offset(k) for k in names],
         'itemsize': rec.size,
     }
+    _check_struct_alignment(rec, fields)
     return np.dtype(fields, align=rec.aligned)
 
+
+def _check_struct_alignment(rec, fields):
+    """Check alignment compatibility with Numpy"""
+    if rec.aligned:
+        for k, dt in zip(fields['names'], fields['formats']):
+            llvm_align = rec.alignof(k)
+            npy_align = dt.alignment
+            if llvm_align is not None and npy_align != llvm_align:
+                msg = (
+                    'NumPy is using a different alignment ({}) '
+                    'than Numba/LLVM ({}) for {}. '
+                    'This is likely a numpy bug.'
+                )
+                raise ValueError(msg.format(npy_align, llvm_align, dt))
+
 def is_arrayscalar(val):
     return np.dtype(type(val)) in FROM_DTYPE
 
diff --git a/numba/tests/test_record_dtype.py b/numba/tests/test_record_dtype.py
index 3e4f9adb7ec..0aedc6cb66c 100644
--- a/numba/tests/test_record_dtype.py
+++ b/numba/tests/test_record_dtype.py
@@ -9,6 +9,7 @@
 from numba.compiler import compile_isolated
 from numba.itanium_mangler import mangle_type
 from numba.utils import IS_PY3
+from numba.numpy_support import version as numpy_version
 from .support import tag
 
 
@@ -293,6 +294,44 @@ class Ref(ctypes.Structure):
         dtype = ty.dtype
         self.assertTrue(dtype.isalignedstruct)
 
+    def test_complex_struct(self):
+        class Complex(ctypes.Structure):
+            _fields_ = [
+                ('real', ctypes.c_double),
+                ('imag', ctypes.c_double),
+            ]
+
+        class Ref(ctypes.Structure):
+            _fields_ = [
+                ('apple', ctypes.c_int32),
+                ('mango', Complex),
+            ]
+
+        ty = types.Record.make_c_struct([
+            ('apple', types.intc),
+            ('mango', types.complex128),
+        ])
+        # Correct offsets
+        self.assertEqual(len(ty), 2)
+        self.assertEqual(ty.offset('apple'), Ref.apple.offset)
+        self.assertEqual(ty.offset('mango'), Ref.mango.offset)
+        # Correct size
+        self.assertEqual(ty.size, ctypes.sizeof(Ref))
+        # Is aligned?
+        # NumPy version < 1.16 misalign complex-128 types to 16bytes,
+        if numpy_version >= (1, 16):
+            dtype = ty.dtype
+            self.assertTrue(dtype.isalignedstruct)
+        else:
+            with self.assertRaises(ValueError) as raises:
+                dtype = ty.dtype
+            self.assertIn(
+                ("NumPy is using a different alignment (16) "
+                 "than Numba/LLVM (8) for complex128. "
+                 "This is likely a numpy bug."),
+                str(raises.exception),
+            )
+
 
 class TestRecordDtype(unittest.TestCase):
 
diff --git a/numba/types/npytypes.py b/numba/types/npytypes.py
index 805fe455d6a..e6fa295bd1d 100644
--- a/numba/types/npytypes.py
+++ b/numba/types/npytypes.py
@@ -44,7 +44,7 @@ def key(self):
         return self.count
 
 
-_RecordField = collections.namedtuple('_RecordField', 'type,offset')
+_RecordField = collections.namedtuple('_RecordField', 'type,offset,alignment')
 
 
 class Record(Type):
@@ -84,8 +84,10 @@ def make_c_struct(cls, name_types):
             # align
             misaligned = offset % align
             if misaligned:
-                offset += size - misaligned
-            fields.append((k, {'type': ty, 'offset': offset}))
+                offset += align - misaligned
+            fields.append((k, {
+                'type': ty, 'offset': offset, 'alignment': align,
+            }))
             offset += size
         # Adjust sizeof structure
         abi_size = ctx.get_abi_sizeof(ir.LiteralStructType(lltypes))
@@ -99,9 +101,13 @@ def __init__(self, fields, size, aligned):
 
         # Create description
         descbuf = []
-        fmt = "{}[type={};offset={}]"
+        fmt = "{}[type={};offset={}{}]"
         for k, infos in fields:
-            descbuf.append(fmt.format(k, infos.type, infos.offset))
+            if infos.alignment is not None:
+                extra = ';alignment={}'.format(infos.alignment)
+            else:
+                extra = ''
+            descbuf.append(fmt.format(k, infos.type, infos.offset, extra))
 
         desc = ','.join(descbuf)
         name = 'Record({};{};{})'.format(desc, self.size, self.aligned)
@@ -111,11 +117,15 @@ def __init__(self, fields, size, aligned):
     def _normalize_fields(cls, fields):
         """
         fields:
-            [name: str, { type: Type, offset: int }]
+            [name: str, { type: Type, offset: int, [ alignment: int ] }]
         """
         res = []
         for name, infos in sorted(fields, key=lambda x: (x[1]['offset'], x[0])):
-            fd = _RecordField(type=infos['type'], offset=infos['offset'])
+            fd = _RecordField(
+                type=infos['type'],
+                offset=infos['offset'],
+                alignment=infos.get('alignment'),
+            )
             res.append((name, fd))
         return res
 
@@ -144,6 +154,13 @@ def typeof(self, key):
         """
         return self.fields[key].type
 
+    def alignof(self, key):
+        """Get the specified alignment of the field.
+
+        Since field aligment is optional, This may return None.
+        """
+        return self.fields[key].alignment
+
     @property
     def members(self):
         """An ordered list of (name, type) for the fields.


Code review comments:
diff_hunk: @@ -293,6 +294,44 @@ class Ref(ctypes.Structure):
         dtype = ty.dtype
         self.assertTrue(dtype.isalignedstruct)
 
+    def test_complex_struct(self):
+        class Complex(ctypes.Structure):
+            _fields_ = [
+                ('real', ctypes.c_double),
+                ('imag', ctypes.c_double),
+            ]
+
+        class Ref(ctypes.Structure):
+            _fields_ = [
+                ('apple', ctypes.c_int32),
+                ('mango', Complex),
+            ]
+
+        ty = types.Record.make_c_struct([
+            ('apple', types.intc),
+            ('mango', types.complex128),
+        ])
+        # Correct offsets
+        self.assertEqual(len(ty), 2)
+        self.assertEqual(ty.offset('apple'), Ref.apple.offset)
+        self.assertEqual(ty.offset('mango'), Ref.mango.offset)
+        # Correct size
+        self.assertEqual(ty.size, ctypes.sizeof(Ref))
+        # Is aligned?
+        # NumPy version < 1.16 misalign complex-128 types to 16bytes,
+        if numpy_version >= (1, 16):
+            dtype = ty.dtype
+            self.assertTrue(dtype.isalignedstruct)
+        else:
+            with self.assertRaises(ValueError) as raises:
+                dtype = ty.dtype
+            self.assertIn(
+                ("NumPy is using a different alignment (16) "
+                 "than Numba/LLVM (8) for complex128. "
+                 "This is likely a numpy bug."),
path: numba/tests/test_record_dtype.py
text: @user1:
```suggestion
                 "This is likely a NumPy bug."),
```
also, is there a reasonable NumPy issue to point to where this got fixed? Perhaps we could suggest using > 116?