Re: [PATCH] Fix X server on PowerPC when using GCC 4.7.x

[Martin Jansa <martin.jansa@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9458 bytes --]

On Wed, May 16, 2012 at 07:30:40AM -0600, Gary Thomas wrote:
> On 2012-05-16 07:23, Martin Jansa wrote:
> > On Wed, May 16, 2012 at 3:09 PM, Gary Thomas<gary@mlbassoc.com>  wrote:
> >> The function XaceHook() was trying to do something like this:
> >>    void *ptr;
> >>    switch(XX) {
> >>       case a:
> >>          define_some_structure A;
> >>          ptr =&A;
> >>          break;
> >>       case b:
> >>          define_some_structure B;
> >>          ptr =&B;
> >>          break;
> >>    }
> >>    call_some_function(ptr);
> >>
> >> Clearly this is not even legal - the scope of the variables A&  B
> >> is not well defined outside of the switch cases.  This code pattern
> >> stopped working on PowerPC with GCC>= 4.7.1 (it has worked forever
> >> up to&  including 4.6.3).  Replace this sequence with legal code:
> >>    switch(XX) {
> >>       case a:
> >>          define_some_structure A;
> >>          call_some_function(&A);
> >>          break;
> >>       case b:
> >>          define_some_structure B;
> >>          call_some_function(&B);
> >>          break;
> >>    }
> >> ---
> >>   .../fix-bogus-stack-variables.patch                |  130 ++++++++++++++++++++
> >>   .../xorg-xserver/xserver-kdrive_1.7.99.2.bb        |    3 +-
> >>   2 files changed, 132 insertions(+), 1 deletions(-)
> >>   create mode 100644 meta/recipes-graphics/xorg-xserver/xserver-kdrive-1.7.99.2/fix-bogus-stack-variables.patch
> >>
> >> diff --git a/meta/recipes-graphics/xorg-xserver/xserver-kdrive-1.7.99.2/fix-bogus-stack-variables.patch b/meta/recipes-graphics/xorg-xserver/xserver-kdrive-1.7.99.2/fix-bogus-stack-variables.patch
> >> new file mode 100644
> >> index 0000000..0946dfb
> >> --- /dev/null
> >> +++ b/meta/recipes-graphics/xorg-xserver/xserver-kdrive-1.7.99.2/fix-bogus-stack-variables.patch
> >> @@ -0,0 +1,130 @@
> >> +Index: xorg-server-1.7.99.2/Xext/xace.c
> >> +===================================================================
> >> +--- xorg-server-1.7.99.2.orig/Xext/xace.c
> >> ++++ xorg-server-1.7.99.2/Xext/xace.c
> >> +@@ -87,8 +87,7 @@ void XaceHookAuditEnd(ClientPtr ptr, int
> >> +  */
> >> + int XaceHook(int hook, ...)
> >> + {
> >> +-    pointer calldata; /* data passed to callback */
> >> +-    int *prv = NULL;  /* points to return value from callback */
> >> ++    int res = Success;
> >> +     va_list ap;               /* argument list */
> >> +     va_start(ap, hook);
> >> +
> >> +@@ -109,8 +108,8 @@ int XaceHook(int hook, ...)
> >> +           rec.parent = va_arg(ap, pointer);
> >> +           rec.access_mode = va_arg(ap, Mask);
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_DEVICE_ACCESS: {
> >> +@@ -119,8 +118,8 @@ int XaceHook(int hook, ...)
> >> +           rec.dev = va_arg(ap, DeviceIntPtr);
> >> +           rec.access_mode = va_arg(ap, Mask);
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_SEND_ACCESS: {
> >> +@@ -131,8 +130,8 @@ int XaceHook(int hook, ...)
> >> +           rec.events = va_arg(ap, xEventPtr);
> >> +           rec.count = va_arg(ap, int);
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_RECEIVE_ACCESS: {
> >> +@@ -142,8 +141,8 @@ int XaceHook(int hook, ...)
> >> +           rec.events = va_arg(ap, xEventPtr);
> >> +           rec.count = va_arg(ap, int);
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_CLIENT_ACCESS: {
> >> +@@ -152,8 +151,8 @@ int XaceHook(int hook, ...)
> >> +           rec.target = va_arg(ap, ClientPtr);
> >> +           rec.access_mode = va_arg(ap, Mask);
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_EXT_ACCESS: {
> >> +@@ -162,8 +161,8 @@ int XaceHook(int hook, ...)
> >> +           rec.ext = va_arg(ap, ExtensionEntry*);
> >> +           rec.access_mode = DixGetAttrAccess;
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_SERVER_ACCESS: {
> >> +@@ -171,8 +170,8 @@ int XaceHook(int hook, ...)
> >> +           rec.client = va_arg(ap, ClientPtr);
> >> +           rec.access_mode = va_arg(ap, Mask);
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_SCREEN_ACCESS:
> >> +@@ -182,15 +181,15 @@ int XaceHook(int hook, ...)
> >> +           rec.screen = va_arg(ap, ScreenPtr);
> >> +           rec.access_mode = va_arg(ap, Mask);
> >> +           rec.status = Success; /* default allow */
> >> +-          calldata =&rec;
> >> +-          prv =&rec.status;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> ++          res = rec.status;
> >> +           break;
> >> +       }
> >> +       case XACE_AUTH_AVAIL: {
> >> +           XaceAuthAvailRec rec;
> >> +           rec.client = va_arg(ap, ClientPtr);
> >> +           rec.authId = va_arg(ap, XID);
> >> +-          calldata =&rec;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> +           break;
> >> +       }
> >> +       case XACE_KEY_AVAIL: {
> >> +@@ -198,7 +197,7 @@ int XaceHook(int hook, ...)
> >> +           rec.event = va_arg(ap, xEventPtr);
> >> +           rec.keybd = va_arg(ap, DeviceIntPtr);
> >> +           rec.count = va_arg(ap, int);
> >> +-          calldata =&rec;
> >> ++          CallCallbacks(&XaceHooks[hook],&rec);
> >> +           break;
> >> +       }
> >> +       default: {
> >> +@@ -208,9 +207,7 @@ int XaceHook(int hook, ...)
> >> +     }
> >> +     va_end(ap);
> >> +
> >> +-    /* call callbacks and return result, if any. */
> >> +-    CallCallbacks(&XaceHooks[hook], calldata);
> >> +-    return prv ? *prv : Success;
> >> ++    return res;
> >> + }
> >> +
> >> + /* XaceCensorImage
> >> diff --git a/meta/recipes-graphics/xorg-xserver/xserver-kdrive_1.7.99.2.bb b/meta/recipes-graphics/xorg-xserver/xserver-kdrive_1.7.99.2.bb
> >> index 360a0f3..dc17e2b 100644
> >> --- a/meta/recipes-graphics/xorg-xserver/xserver-kdrive_1.7.99.2.bb
> >> +++ b/meta/recipes-graphics/xorg-xserver/xserver-kdrive_1.7.99.2.bb
> >> @@ -7,7 +7,7 @@ RDEPENDS_${PN} += "xkeyboard-config"
> >>   EXTRA_OECONF += "--disable-glx"
> >>
> >>   PE = "1"
> >> -PR = "r29"
> >> +PR = "r30"
> >>
> >>   SRC_URI = "${XORG_MIRROR}/individual/xserver/xorg-server-${PV}.tar.bz2 \
> >>         file://extra-kmodes.patch \
> >> @@ -21,6 +21,7 @@ SRC_URI = "${XORG_MIRROR}/individual/xserver/xorg-server-${PV}.tar.bz2 \
> >>         file://crosscompile.patch \
> >>         file://error-address-work-around.patch \
> >>         file://nodolt.patch"
> >> +        file://fix-bogus-stack-variables.patch \
> >>   #      file://kdrive-evdev.patch
> >>   #      file://kdrive-use-evdev.patch
> >>   #      file://enable-builtin-fonts.patch
> >> --
> >> 1.7.7.6
> >>
> >>
> >> _______________________________________________
> >> Openembedded-core mailing list
> >> Openembedded-core@lists.openembedded.org
> >> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
> >
> > Isn't this better fix?
> > http://cgit.freedesktop.org/xorg/xserver/commit/Xext/xace.c?id=6dae7f3792611aace1df0cca63bf50c50d93de43
> 
> Either way - they are pretty much the same (I had actually done it the
> freedesktop.org way before but I thought it obfuscated what was happening).
> I'm sure the person that wrote this was being clever, trying to have only
> one function  call, but a smart compiler will unwind this anyway and writing
> it out explicitly is easier to read...
> 
> All I care is that it gets fixed and my X server starts working again :-)

I don't care about very old xserver-kdrive recipes, but from maintenance
POV it's imho much better to backport patch from newer version in upstream 
then introduce something different.. at least for someone who will maybe
upgrade xserver-kdrive later to see that your patch is no longer needed
for newer versions...

Cheers,

-- 
Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]