[Xcb] [PATCH libxcb 3/3] generator: optionally enable/disable implicit_padding

[Christian Linhart]

Hi Peter,

On 08/26/14 02:48, Peter Harris wrote:
> I was studying the results of this change, and it turns out that the
> implicit padding inserted by the generator is actually a no-op since
> http://cgit.freedesktop.org/xcb/libxcb/commit/?id=aa02096b8e7f94ad3c998a8d5af54963ee860b13
> changed xcb_align_to to be the alignment of the previous field.
> 
> The previous field was already naturally aligned[2], therefore the
> current position must already be aligned to the alignment of the
> previous field.
> 
> I suspect we only need the xcb_align_to variable iff
> self.contains_explicit_alignpads.
I also think so. In fact, that's what happens in my patch when implicit-padding=false.
> 
> If we stop generating the noop[1] implicit padding between lists, is the
> implicit-padding=false tag still necessary, or is there another issue
> that I missed seeing?

I think that stopping to generate the defacto-noop implicit padding will be a good idea.

This may make using the implicit-padding=false attribute unneeded in many cases.
But there may still be examples where it may be needed to force XCB_PACK for structs
in order to switch off implicit padding by the C-Compiler. ( example at the end of this email )

Essentially, I didn't dare to remove the implicit padding altogether because
of the huge testing/verification effort that may be needed.
I agree with you that it can probably be removed without causing problems,
but how can we be sure?

If we do not dare to remove implicit padding now, then the 
implicit-padding=false attribute may be a useful intermediate measure.

***

There are other related issues which may need another solution:

Currently, the align-padding inside <switch> 
computes the alignment relative to the start of the <switch>.

If the <switch> starts at an unaligned address 
(like for struct "InputState" in xinput.xml )
then all alignment inside the <switch> will be wrong. 

This applies to implicit-padding ( which will not be a noop then! )
and to explicit align-pads.

More specifically, in the current impl, with implicit padding enabled
we'd probably have an implicit pad of size 2-bytes at the end of case "Valuator" in struct "InputState".
This will cause problems because struct "InputState" is used in a list
and therefore all subsequent list-members will start at a wrong address.

There are basically two solutions:
* fix the align-padding for <switch>.
* make everything explicit, i,e,
	- switch off implicit padding
	- provide an "offset"-attribute for explicit align-pads 
	  which explicitly corrects for such things.
	  E.g.:
		<pad align="4" offset="2"/>

***

The same applies to structs which are used at a non-aligned position.

I don't know whether there are examples in any X11-extension,
but I wouldn't be surprised if there were structs which are meant
to be started at an unaligned position.

such as:

<!-- struct foo is to be used at address which is 4-byte-align + 3 bytes. -->
<struct name="Foo">
	<field type="CARD8" name="abc" />
	<field type="CARD32" name="cde" /><!-- with the current impl, "cde" will have adress &abc+4, but should have &abc+1 -->
	<field type="CARD32" name="efg" />
	<list type="CARD8" name="name" />
		<fieldref>cde</fieldref>
	</list>
	<pad align="4"/>
	<field type="CARD32" name="klm" />
</struct>


<request name="Bar" opcode="123">
	<field type="CARD16" name="xyz" />
	<pad bytes="1"/>
	<field type="Foo" name="foo" />
</request>

<request name="Baz" opcode="234">
	<field type="CARD8" name="x1" />
	<field type="CARD8" name="x2" />
	<field type="CARD8" name="x3" />
	<field type="Foo" name="foo" />
</request>

Does something like that exist somewhere?

If yes, then this cannot be solved implicitly,
because the parser/generator cannot know that 
struct Foo is supposed to start at an unaligned position.
( Well, it could determine it by global analysis,
potentially across multiple xml files, but we should
avoid that... )

So:
* struct Foo needs to be packed, to avoid implicit padding by the C-Compiler.
  This will need an attribute like "implicit-padding=false"
  because the parser/generator cannot know that, as stated above.

* The align-pad at end of struct "foo" needs to know that the struct
  starts at an unaligned position.:
	this can be done explicitely with an "offset"-attribute
	or automatically.
 

Chris

> 
> Thanks,
>  Peter Harris
> 
> [1] In my test branch, I dropped implicit padding and reviewed some of
> the generated code. I didn't manage to find a case where it made any
> difference. I also didn't manage to stay awake long enough to review the
> entire delta. If there is a case where implicit padding is necessary,
> let's change it to explicit padding now that we have <pad align> and
> drop implicitly padded lists anyway.
> [2] By definition. If it wasn't naturally aligned, it was already broken.