Incorrect result copying array-typed field of structured array in 0.55.0rc1

@HPLegion

This works as expected in plain python but is incorrect in 0.55.0rc1 on Windows 10 with python 3.7.1

Correct plain python result: (1.2, [1, 2, 3])
Incorrect njit result: (1.2, [0, 0, 0])

import numpy as np
from numba import njit

src_dtype = np.dtype([
    ("user", np.float64),
    ("array", np.int16, (3,))
], align=True)


dest_dtype = np.dtype([
    ("user1", np.float64),
    ("array1", np.int16, (3,))
], align=True)

@njit
def copy(index, src, dest):
    dest['user1'] = src[index]['user']
    dest['array1'] = src[index]['array']

source = np.empty(5, dtype=src_dtype)
dest = np.empty(5, dtype=dest_dtype)

source[0] = (1.2, [1, 2, 3])
copy(0, source, dest[0])
print(dest[0])

This was first discussed in discourse using 0.53.1 with a different manifestation. Shout-out to @HPLegion for suggesting trying the latest release candidate.

@luk-f-a

I can reproduce this - unfortunately we've gone from getting a lowering error in earlier versions:

numba.core.errors.LoweringError: Failed in nopython mode pipeline (step: nopython mode backend)
Can only insert i16 at [0] in [3 x i16]: got i8*

File "repro.py", line 18:
def copy(index, src, dest):
    <source elided>
    dest['user1'] = src[index]['user']
    dest['array1'] = src[index]['array']
    ^

During: lowering "dest['array1'] = $26binary_subscr.11" at repro.py (18)

to silently doing the wrong thing - I think of this as a regression, and we should have a look at fixing it for 0.55. I think this will probably have been enabled by #7359 (cc @luk-f-a in case he has any thoughts).

I think it was probably enabled by #7359. It's interesting that these work fine

def get(index, src, dest):
    return src[index]['array']

def set(index, src, dest):
    dest['array1'][:] = np.ones(3)

and this one throws a typing error

def set(index, src, dest):
    dest['array1'] = np.ones(3)

BUT

@njit
def set2(index, src, dest):
    dest['array1'] = src[index]['array']

does not throw an error. Probably related to the difference in typing between array and nestedarray.

I will have a look today.

small update: looking at the numba ir I didn't spot any obvious problems. there's a

$10binary_subscr.4 = static_getitem(value=$6binary_subscr.2, index=array, index_var=$const8.3, fn=<built-in function getitem>) 
dest['array1'] = $10binary_subscr.4

which looks fine. I'll have to look into what LLVM IR this generates.

small update: looking at the numba ir I didn't spot any obvious problems. there's a
$10binary_subscr.4 = static_getitem(value=$6binary_subscr.2, index=array, index_var=$const8.3, fn=<built-in function getitem>) 
dest['array1'] = $10binary_subscr.4
which looks fine. I'll have to look into what LLVM IR this generates.

Thanks for working on this,

Related issue I found whilst experimenting:

import numpy as np
from numba import njit

dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True)


@njit
def ex1(dest):
    tmp = (np.arange(3) + 1).astype(np.int16)
    dest['array1'] = tmp 


dest = np.empty(5, dtype=dest_dtype)
ex1(dest[0])

gives:

Traceback (most recent call last):
  File "/home/gmarkall/numbadev/issues/7693/ex1.py", line 14, in <module>
    ex1(dest[0])
  File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 487, in _compile_for_args
    raise e
  File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 420, in _compile_for_args
    return_val = self.compile(tuple(argtypes))
  File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 965, in compile
    cres = self._compiler.compile(args, return_type)
  File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 125, in compile
    status, retval = self._compile_cached(args, return_type)
  File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 139, in _compile_cached
    retval = self._compile_core(args, return_type)
  File "/home/gmarkall/numbadev/numba/numba/core/dispatcher.py", line 152, in _compile_core
    cres = compiler.compile_extra(self.targetdescr.typing_context,
  File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 693, in compile_extra
    return pipeline.compile_extra(func)
  File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 429, in compile_extra
    return self._compile_bytecode()
  File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 497, in _compile_bytecode
    return self._compile_core()
  File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 472, in _compile_core
    raise e
  File "/home/gmarkall/numbadev/numba/numba/core/compiler.py", line 463, in _compile_core
    pm.run(self.state)
  File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 349, in run
    raise e
  File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 341, in run
    self._runPass(idx, pass_inst, state)
  File "/home/gmarkall/numbadev/numba/numba/core/compiler_lock.py", line 35, in _acquire_compile_lock
    return func(*args, **kwargs)
  File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 296, in _runPass
    mutated |= check(pss.run_pass, internal_state)
  File "/home/gmarkall/numbadev/numba/numba/core/compiler_machinery.py", line 269, in check
    mangled = func(compiler_state)
  File "/home/gmarkall/numbadev/numba/numba/core/typed_passes.py", line 396, in run_pass
    lower.lower()
  File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 183, in lower
    self.lower_normal_function(self.fndesc)
  File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 237, in lower_normal_function
    entry_block_tail = self.lower_function_body()
  File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 266, in lower_function_body
    self.lower_block(block)
  File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 280, in lower_block
    self.lower_inst(inst)
  File "/home/gmarkall/numbadev/numba/numba/core/lowering.py", line 483, in lower_inst
    return impl(self.builder, (target, inst.index, value))
  File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 1201, in __call__
    res = self._imp(self._context, builder, self._sig, args, loc=loc)
  File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 1231, in wrapper
    return fn(*args, **kwargs)
  File "/home/gmarkall/numbadev/numba/numba/np/arrayobj.py", line 2640, in record_static_setitem_str
    return impl(builder, (rec, val))
  File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 641, in wrapped
    return impl(self, builder, sig, args, attr)
  File "/home/gmarkall/numbadev/numba/numba/core/imputils.py", line 166, in res
    return real_impl(context, builder, sig, args, attr)
  File "/home/gmarkall/numbadev/numba/numba/np/arrayobj.py", line 2604, in record_setattr
    val = context.cast(builder, val, valty, elemty)
  File "/home/gmarkall/numbadev/numba/numba/core/base.py", line 713, in cast
    return impl(self, builder, fromty, toty, val)
  File "/home/gmarkall/numbadev/numba/numba/np/arrayobj.py", line 5410, in array_to_array
    assert fromty.mutable != toty.mutable or toty.layout == 'A'
AssertionError

With:

diff --git a/numba/np/arrayobj.py b/numba/np/arrayobj.py
index a7046a404..ce3a347af 100644
--- a/numba/np/arrayobj.py
+++ b/numba/np/arrayobj.py
@@ -5407,7 +5407,8 @@ def array_argsort(context, builder, sig, args):
 @lower_cast(types.Array, types.Array)
 def array_to_array(context, builder, fromty, toty, val):
     # Type inference should have prevented illegal array casting.
-    assert fromty.mutable != toty.mutable or toty.layout == 'A'
+    layout_compatible = toty.layout == 'A' or fromty.layout == toty.layout
+    assert fromty.mutable != toty.mutable or layout_compatible
     return val

(which I think is a reasonable change) the AssertionError is no longer hit, but the answer is still not correct:

import numpy as np
from numba import njit

dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True)


@njit
def ex1(dest):
    tmp = (np.arange(3) + 1).astype(np.int16)
    dest['array1'] = tmp


dest_nb = np.empty(5, dtype=dest_dtype)
dest_py = dest_nb.copy()

ex1.py_func(dest_py[0])
ex1(dest_nb[0])

print('Expected:', dest_py[0])
print('Actual:  ', dest_nb[0])

gives:

Expected: ([1, 2, 3],)
Actual:   ([-25280,  16338,  21945],)

Modifying the example above so that the prior value of dest_nb is shown suggests that 4 bytes of garbage is written (as opposed to nothing being written and us seeing some old garbage values)

import numpy as np
from numba import njit

dest_dtype = np.dtype([("array1", np.int16, (3,))], align=True)


@njit
def ex1(dest):
    tmp = (np.arange(3) + 1).astype(np.int16)
    dest['array1'] = tmp


dest_nb = np.empty(5, dtype=dest_dtype)
dest_py = dest_nb.copy()

print('Original: ', dest_nb[0])
ex1.py_func(dest_py[0])
ex1(dest_nb[0])

print('Expected:', dest_py[0])
print('Actual:  ', dest_nb[0])

Original:  ([-15760, -18323,  22029],)
Expected: ([1, 2, 3],)
Actual:   ([-26096, -18253,  22029],)

Also this sometimes segfaults:

Original:  ([-32512,   1701,  21847],)
Expected: ([1, 2, 3],)
Actual:   ([11712,  1786, 21847],)
Segmentation fault (core dumped)

This is handy, as valgrind should help to pinpoint where we're doing some bad memory accesses.

@stuartarchibald

As suggested by @stuartarchibald, somewhere the data of the record is likely to be getting interpreted as an array struct.

Is this what's happening:

; Function Attrs: nofree norecurse nounwind writeonly
define i32 @_ZN8__main__7ex1_241B42c8tJTIeFCjyCbUFRqqOAK_2f6h0kCng1maAA_3d_3dE6RecordILi374EE5ArrayIsLi1E1C7mutable7alignedE(i8** noalias nocapture %retptr, { i8*, i32, i8* }** noalias nocapture readnone %excinfo, [6 x i8]* nocapture %arg.dest, i8* %arg.tmp.0, i8* %arg.tmp.1, i64 %arg.tmp.2, i64 %arg.tmp.3, i16* %arg.tmp.4, i64 %arg.tmp.5.0, i64 %arg.tmp.6.0) local_unnamed_addr #0 {
entry:
  ; meminfo
  %.18.repack = bitcast [6 x i8]* %arg.dest to i8**
  store i8* %arg.tmp.0, i8** %.18.repack, align 8

  ; parent
  %.18.repack1 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 1, i64 2
  %0 = bitcast i8* %.18.repack1 to i8**
  store i8* %arg.tmp.1, i8** %0, align 8

  ; nitems
  %.18.repack3 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 2, i64 4
  %1 = bitcast i8* %.18.repack3 to i64*
  store i64 %arg.tmp.2, i64* %1, align 8

  ; itemsize
  %.18.repack5 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 4
  %2 = bitcast [6 x i8]* %.18.repack5 to i64*
  store i64 %arg.tmp.3, i64* %2, align 8

  ; data
  %.18.repack7 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 5, i64 2
  %3 = bitcast i8* %.18.repack7 to i16**
  store i16* %arg.tmp.4, i16** %3, align 8

  ; shape
  %.18.repack9 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 6, i64 4
  %4 = bitcast i8* %.18.repack9 to i64*
  store i64 %arg.tmp.5.0, i64* %4, align 8

  ; strides
  %.18.repack11 = getelementptr inbounds [6 x i8], [6 x i8]* %arg.dest, i64 8
  %5 = bitcast [6 x i8]* %.18.repack11 to i64*
  store i64 %arg.tmp.6.0, i64* %5, align 8

  ; CC return
  store i8* null, i8** %retptr, align 8
  ret i32 0
}

i.e. record which ends up as a block of bytes being read as an array model?

@stuartarchibald

@stuartarchibald @gmarkall from I can see in the numba IR, the variables are correcly typed as nestedarray. The NestedArrayModel inherits from the ArrayModel so it makes sense that it's being used.

When I made the original changes I noticed that nestedarray were using the array model but in the examples I looked at, the members like nitems, shape, strides, etc were being inserted in hard-coded way like this store <2 x i64> <i64 3, i64 2>, <2 x i64>* %1, align 8, and only the data member was being copied from the nestedarray into the array. This happens in an example like this

def set1(dest):
   # dest is a record, not a record array, the field array1 is a nested array
    return dest['array1']

In this case I can see the data of dest being copied into the data of %retptr, while the other fields of %retptr are filled directly.

However, in a case like this

def set1(index, src, dest):
    dest['array1'] = src[index]['array']

the whole LLVM struct of src[index]['array'] (ie nitem, strides, data, etc, etc) is being copied intodest['array1']`. Could this be an issue in the lowering of staticsetitem? Maybe it needs to be modified to treat nestedarray different from array. I'll look into this later today.

As an aside to help me understand the code, do you know what the "fe" and "be" mean in fe_type and be_type? Like in the example below

@register_default(types.NestedArray)
class NestedArrayModel(ArrayModel):
    def __init__(self, dmm, fe_type):
        self._be_type = dmm.lookup(fe_type.dtype).get_data_type()
        super(NestedArrayModel, self).__init__(dmm, fe_type)

As an aside to help me understand the code, do you know what the "fe" and "be" mean in fe_type and be_type? Like in the example below

FE is frontend - the Numba types like numba.types.uint32, etc., and BE is backend - LLVM types like llvmlite.ir.types.IntType(32) - the data models provide the mapping between the frontend types and the backend types.

update: I've tracked the issue to this function

numba/numba/np/arrayobj.py

Lines 2589 to 2606 in bf480b9

    
           @lower_setattr_generic(types.Record) 
        
           def record_setattr(context, builder, sig, args, attr): 
        
               """ 
        
               Generic setattr() implementation for records: set the given 
        
               record member, i.e. a scalar. 
        
               """ 
        
               typ, valty = sig.args 
        
               target, val = args 
        
               context.sentry_record_alignment(typ, attr) 
        
               offset = typ.offset(attr) 
        
               elemty = typ.typeof(attr) 
        
               dptr = cgutils.get_record_member(builder, target, offset, 
        
                                                context.get_data_type(elemty)) 
        
               val = context.cast(builder, val, valty, elemty) 
        
               align = None if typ.aligned else 1 
        
               context.pack_value(builder, elemty, val, dptr, align=align)

The logic is fine for assignment of scalars into scalar fields, but with arrays into arrays this happens:
the dptr variable has value %".74" = bitcast i8* %".73" to {i8*, i8*, i64, i64, i16*, [1 x i64], [1 x i64]}*. This a problem because %".73" is defined as %".73" = getelementptr inbounds [16 x i8], [16 x i8]* %"arg.dest", i32 0, i32 8.
So %".73" is the pointer to the data of the nestedarray, but %".74" is the wrong bitcast, to the structure that represents the full data model, not just the data

I have a very ugly poc that extracts the data of the source and stores it in the record field. #7712

I'm pretty sure that the resulting LLVM IR is not good, and I probably didn't follow established patterns in numba of how to do this. For example, is it ok to call builder.store directly from record_setattr or should I have done that via context?

On the plus side it works, so at least I know it is a step in the right direction.

Add tests for Issue numba#7693

Navigation Menu

Repository navigation

Incorrect result copying array-typed field of structured array in 0.55.0rc1 #7693

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	@lower_setattr_generic(types.Record)
	def record_setattr(context, builder, sig, args, attr):
	"""
	Generic setattr() implementation for records: set the given
	record member, i.e. a scalar.
	"""
	typ, valty = sig.args
	target, val = args

	context.sentry_record_alignment(typ, attr)
	offset = typ.offset(attr)
	elemty = typ.typeof(attr)

	dptr = cgutils.get_record_member(builder, target, offset,
	context.get_data_type(elemty))
	val = context.cast(builder, val, valty, elemty)
	align = None if typ.aligned else 1
	context.pack_value(builder, elemty, val, dptr, align=align)

Incorrect result copying array-typed field of structured array in 0.55.0rc1 #7693

Description

Activity

gmarkall commented on Dec 29, 2021

luk-f-a commented on Jan 4, 2022

luk-f-a commented on Jan 4, 2022

nelson2005 commented on Jan 5, 2022

gmarkall commented on Jan 5, 2022

gmarkall commented on Jan 5, 2022

gmarkall commented on Jan 5, 2022

gmarkall commented on Jan 5, 2022

stuartarchibald commented on Jan 5, 2022

luk-f-a commented on Jan 7, 2022

gmarkall commented on Jan 7, 2022

luk-f-a commented on Jan 7, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions