Re: Missing swapl() in Xext/saver.c?

[Keith Packard]

Alan Coopersmith  writes:

> On 03/10/18 01:31 AM, Mihai Moldovan wrote:
>> int everywhere and includes the (ignored) B32 width specifier
>
> We could just delete those now, as they're just noise for the pre-processor
> to strip ever since we dropped support for Cray builds with "everything is
> a 64-bit int, using bitfields to emulate smaller sizes" back in 2013:

Sure, would just require a simple sed script run over the xorgproto repo
at this point.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Missing swapl() in Xext/saver.c?

[Keith Packard]

Mihai Moldovan  writes:

> Sounds sane. Reverting to the initially intended BOOL type would cause 
> breakage,
> but using CARD32 + swapl() would leave the struct size consistent and fix the
> issue in newer versions.

I've posted patches for xcb/proto, xorgproto and the X server. For the X
server, I've also added a hack to keep old XCB clients on LSB machines
working by only using the low byte of the 4-byte value. There's not much
we can do for old XCB clients on MSB machines; the true value will be in
the high byte, whereas Xlib clients using the same version will be in
the low byte.

Of course, this changes the API for xcb, which used to use uint8_t and
now uses uint32_t. That doesn't change the ABI on any 32-bit int or
larger machine as the uint8_t would be passed in the same size object as
the uint32_t, and the X server will be ignoring the high three bytes.

Review is encouraged.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Missing swapl() in Xext/saver.c?

[Mihai Moldovan]

* On 03/12/2018 05:13 AM, Keith Packard wrote:
> Oh. This is pretty bad. 'Bool' is not a valid type in protocol headers,
> it should have been BOOL. And, BOOL is a single byte. But, it is Bool,
> and I guess that is generally 4 bytes on most architectures.
>
> The XCB headers define this field as a byte and have three pad bytes
> afterwards, so we actually have incompatible protocol definitions for
> xcb and xlib/xserver.

I guess I'm about 7 years late to the party, but what has been done has been 
done.

Yep, xcb-proto files confirm this.

On the bright side, this likely hasn't caused actual issues on most platforms
with CPUs that use LE. CPUs in BE mode are quite rare.


> I'm not entirely sure what we should do at this point; I suspect that
> treating it as a 32-bit value, fixing the protocol to use CARD32
> everywhere instead of Bool and BOOL, and then adding swapl would cause
> the least fuss.

Sounds sane. Reverting to the initially intended BOOL type would cause breakage,
but using CARD32 + swapl() would leave the struct size consistent and fix the
issue in newer versions.

Not particularly clean, but least invasive.

A second option might be to create another request type with the initially
intended interface and stub out the original one, returning BadRequest. Sounds
like overkill, though...



Mihai



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: Missing swapl() in Xext/saver.c?

[Keith Packard]

Mihai Moldovan  writes:

> Hi
>
>
> In
> https://cgit.freedesktop.org/xorg/xserver/commit/?id=9edcae78c46286baff42e74bfe26f6ae4d00fe01
> the swapl() call for stuff->suspend was removed. That sounds like a bad idea 
> to me?
>
> xScreenSaverSuspendReq to this day defines it as a Bool, which is typedef'd to
> int everywhere and includes the (ignored) B32 width specifier, so using 
> swapl()
> on the member sounds like a good idea. Definitely isn't a CARD8 object, 
> despite
> its name.

Oh. This is pretty bad. 'Bool' is not a valid type in protocol headers,
it should have been BOOL. And, BOOL is a single byte. But, it is Bool,
and I guess that is generally 4 bytes on most architectures.

The XCB headers define this field as a byte and have three pad bytes
afterwards, so we actually have incompatible protocol definitions for
xcb and xlib/xserver.

I'm not entirely sure what we should do at this point; I suspect that
treating it as a 32-bit value, fixing the protocol to use CARD32
everywhere instead of Bool and BOOL, and then adding swapl would cause
the least fuss.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Missing swapl() in Xext/saver.c?

[Mihai Moldovan]

Hi


In
https://cgit.freedesktop.org/xorg/xserver/commit/?id=9edcae78c46286baff42e74bfe26f6ae4d00fe01
the swapl() call for stuff->suspend was removed. That sounds like a bad idea to 
me?

xScreenSaverSuspendReq to this day defines it as a Bool, which is typedef'd to
int everywhere and includes the (ignored) B32 width specifier, so using swapl()
on the member sounds like a good idea. Definitely isn't a CARD8 object, despite
its name.



Mihai



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel