On Wed, 29 May 2013 13:12:18 -0500 mdroth <mdr...@linux.vnet.ibm.com> wrote:
> On Wed, May 29, 2013 at 01:32:52PM -0400, Luiz Capitulino wrote: > > On Sun, 26 May 2013 22:20:58 -0500 > > Michael Roth <mdr...@linux.vnet.ibm.com> wrote: > > > > > With the introduction of native list types, we now have types such as > > > int64List where the 'value' field is not a pointer, but the actual > > > 64-bit value. > > > > > > On 32-bit architectures, this can lead to situations where 'next' field > > > offset in GenericList does not correspond to the 'next' field in the > > > types that we cast to GenericList when using the visit_next_list() > > > interface, causing issues when we attempt to traverse linked list > > > structures of these types. > > > > > > To fix this, pad the 'value' field of GenericList and other > > > schema-defined/native *List types out to 64-bits. > > > > > > This is less memory-efficient for 32-bit architectures, but allows us to > > > continue to rely on list-handling interfaces that target GenericList to > > > simply visitor implementations. > > > > > > In the future we can improve efficiency by defaulting to using native C > > > array backends to handle list of non-pointer types, which would be more > > > memory efficient in itself and allow us to roll back this change. > > > > I'm also concerned with the small complexity this change is adding. > > How hard would it be to do the proper solution with arrays instead? > > It's not *too* bad, we'd need patches 9-11 from here: > > http://lists.gnu.org/archive/html/qemu-devel/2012-10/threads.html#05755 > > Along with code generation bits, and then all the unit test stuff. > > I think we should be able to get it in for 1.6, but I'd rather not leave > 32-bit busted in the meantime. Ok, I've applied this patch to the QMP branch. > > > > > > > > > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com> > > > --- > > > include/qapi/visitor.h | 5 ++++- > > > scripts/qapi-types.py | 10 ++++++++-- > > > tests/test-qmp-output-visitor.c | 5 ++++- > > > 3 files changed, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > > > index 1fef18c..28c21d8 100644 > > > --- a/include/qapi/visitor.h > > > +++ b/include/qapi/visitor.h > > > @@ -18,7 +18,10 @@ > > > > > > typedef struct GenericList > > > { > > > - void *value; > > > + union { > > > + void *value; > > > + uint64_t padding; > > > + }; > > > struct GenericList *next; > > > } GenericList; > > > > > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > > > index fd42d71..ddcfed9 100644 > > > --- a/scripts/qapi-types.py > > > +++ b/scripts/qapi-types.py > > > @@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, > > > builtin_type=False): > > > > > > typedef struct %(name)sList > > > { > > > - %(type)s value; > > > + union { > > > + %(type)s value; > > > + uint64_t padding; > > > + }; > > > struct %(name)sList *next; > > > } %(name)sList; > > > ''', > > > @@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s; > > > > > > typedef struct %(name)sList > > > { > > > - %(name)s *value; > > > + union { > > > + %(name)s *value; > > > + uint64_t padding; > > > + }; > > > struct %(name)sList *next; > > > } %(name)sList; > > > ''', > > > diff --git a/tests/test-qmp-output-visitor.c > > > b/tests/test-qmp-output-visitor.c > > > index 0942a41..b2fa9a7 100644 > > > --- a/tests/test-qmp-output-visitor.c > > > +++ b/tests/test-qmp-output-visitor.c > > > @@ -295,7 +295,10 @@ static void > > > test_visitor_out_struct_errors(TestOutputVisitorData *data, > > > > > > typedef struct TestStructList > > > { > > > - TestStruct *value; > > > + union { > > > + TestStruct *value; > > > + uint64_t padding; > > > + }; > > > struct TestStructList *next; > > > } TestStructList; > > > > > > > >
