[Xcb] Alignment problems in XKB GetGeometry

[Daniel Martin]

On Mon, Aug 05, 2013 at 10:45:09AM +0300, Ran Benita wrote:
> On Mon, Aug 05, 2013 at 01:21:10AM +0200, Daniel Martin wrote:
> > On Sun, Aug 04, 2013 at 11:57:10PM +0300, Ran Benita wrote:
> > > Now if I try to change xkb.xml to read simply:
> > > 
> > > 	<struct name="Property">
> > > 		<field name="name" type="CountedString16" />
> > > 		<field name="value" type="CountedString16" />
> > > 	</struct>
> > > 
> > > Then struct xcb_xkb_property_t is empty, which is undefined behavior
> > > (AFAIK) and doesn't work anyway (the padding calculations becomes
> > > confused). If I try to inline the CountedString16 definition into
> > > Property, it doesn't work either.
> > > 
> > > Are there any suggestions on how I might proceed?
> > 
> > The generated struct is empty, because variadic length fields will be
> > omitted. But, having an empty struct at the end is suboptimal. Sure.
> > 
> > Did you saw the new functions, that've been generated due to the change
> > in the struct?:
> >   xcb_xkb_counted_string_16_t *xcb_xkb_property_name (const xcb_xkb_property_t *R  /**< */)
> > and
> >   xcb_xkb_counted_string_16_t *xcb_xkb_property_value (const xcb_xkb_property_t *R  /**< */).
> 
> Yes, I've tried. But the padding calculation becomes confused. Let me try
> to explain what happens. As above, there is a "labelFont"
> CountedString16 followed by a list of properties (key-value pairs).
> Looking at the first property only, this is correct:
> 
> labelFont = "-*-helvetica-medium-r-normal--*-120-*-*-*-*-iso8859-1"
> name[0]   = "description"
> value[0]  = "Generic 104"
> 
> With current (misaligned) xkb.xml what happens is:
> 
> labelFont = "-*-helvetica-medium-r-normal--*-120-*-*-*-*-iso8859-1"
> name[0]   = "description"
> value[0]  = "" (length is read incorrectly as 0)
> 
> With changing Property to two CountedString16's:
> 
> labelFont = "-*-helvetica-medium-r-normal--*-120-*-*-*-*-iso8859-1"
> name[0]   = "-*-helvetica-medium-r-normal--*-120-*-*-*-*-iso8859-1"
> value[0]  = "description"
> 
> The problem occurs in xcb_xkb_get_geometry_properties_iterator(). Here's
> the generated code:
> 
> xcb_xkb_property_iterator_t
> xcb_xkb_get_geometry_properties_iterator(const xcb_xkb_get_geometry_reply_t *R)
> {
>     xcb_xkb_property_iterator_t i;
>     /* This returns a correct value, if I understand the purpose of the
>      * 'index' field correctly.. */
>     xcb_generic_iterator_t prev = xcb_xkb_counted_string_16_alignment_pad_end(xcb_xkb_get_geometry_label_font(R));
> 
>     /* XCB_TYPE_PAD comes out -56, which brings prev.data right back to
>      * the beginning of the previous CountedString16. */
>     i.data = (xcb_xkb_property_t *) ((char *) prev.data + XCB_TYPE_PAD(xcb_xkb_property_t, prev.index));
> 
>     i.rem = R->nProperties;
>     i.index = (char *) i.data - (char *) R;
>     return i;
> }
> 
> So something's wrong here.

It's XCB_TYPE_PAD(). In this case (sizeof(xcb_xkb_property_t)==0) it
negates prev.index. I've attached a patch which changes XCB_TYPE_PAD to
return 0 here. At least it makes your test programm (having Property
with 2 CountedString16) to run without raising an assertion.

I didn't made it a real git patch. Because, I think we've to add an
XCB_ALIGN_PAD(), which would align based upon prev.data + prev.index and
change the generated code to check the size of the type and use either
XCB_TYPE_PAD or XCB_ALIGN_PAD. But, I'm not sure atm. if that's the
correct solution. (Could be that prev.data + prev.index is already
aligned.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xcb_type_pad.patch
Type: text/x-diff
Size: 455 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/xcb/attachments/20130806/b92a30c3/attachment.patch>