[PATCH xserver (v4) 03/10] Make Await SyncTrigger functions generic

[James Jones]

On Tuesday 07 December 2010 19:53:10 Peter Hutterer wrote:
> On Mon, Dec 06, 2010 at 02:53:17PM -0800, James Jones wrote:
> > Update all the functions dealing with Await
> > sync triggers handle generic sync objects
> > instead of just counters.  This will
> > facilitate code sharing between the counter
> > sync waits and the fence sync waits.
> > 
> > Signed-off-by: James Jones <jajones at nvidia.com>
> > ---
> > 
> >  Xext/sync.c    |  308
> >  ++++++++++++++++++++++++++++++++++++-------------------- Xext/syncsrv.h
> >  |    2 +-
> >  2 files changed, 198 insertions(+), 112 deletions(-)
> > 
> > diff --git a/Xext/sync.c b/Xext/sync.c
> > index d5187dd..1a2d55d 100644
> > --- a/Xext/sync.c
> > +++ b/Xext/sync.c
> > @@ -107,18 +107,19 @@ static void SyncInitIdleTime(void);
> > 
> >   *  delete and add triggers on this list.
> >   */
> >  
> >  static void
> > 
> > -SyncDeleteTriggerFromCounter(SyncTrigger *pTrigger)
> > +SyncDeleteTriggerFromSyncObject(SyncTrigger *pTrigger)
> > 
> >  {
> >  
> >      SyncTriggerList *pCur;
> >      SyncTriggerList *pPrev;
> > 
> > +    SyncCounter *pCounter;
> > 
> > -    /* pCounter needs to be stored in pTrigger before calling here. */
> > +    /* pSync needs to be stored in pTrigger before calling here. */
> > 
> > -    if (!pTrigger->pCounter)
> > +    if (!pTrigger->pSync)
> > 
> >  	return;
> >  	
> >      pPrev = NULL;
> > 
> > -    pCur = pTrigger->pCounter->sync.pTriglist;
> > +    pCur = pTrigger->pSync->pTriglist;
> > 
> >      while (pCur)
> >      {
> > 
> > @@ -127,7 +128,7 @@ SyncDeleteTriggerFromCounter(SyncTrigger *pTrigger)
> > 
> >  	    if (pPrev)
> >  		
> >  		pPrev->next = pCur->next;
> >  		
> >  	    else
> > 
> > -		pTrigger->pCounter->sync.pTriglist = pCur->next;
> > +		pTrigger->pSync->pTriglist = pCur->next;
> > 
> >  	    free(pCur);
> >  	    break;
> > 
> > @@ -137,21 +138,27 @@ SyncDeleteTriggerFromCounter(SyncTrigger *pTrigger)
> > 
> >  	pCur = pCur->next;
> >  	
> >      }
> > 
> > -    if (IsSystemCounter(pTrigger->pCounter))
> > -	SyncComputeBracketValues(pTrigger->pCounter);
> > +    if (SYNC_COUNTER == pTrigger->pSync->type)
> > +    {
> > +	pCounter = (SyncCounter *)pTrigger->pSync;
> > +
> > +	if (IsSystemCounter(pCounter))
> > +	    SyncComputeBracketValues(pCounter);
> > +    }
> > 
> >  }
> >  
> >  
> >  static int
> > 
> > -SyncAddTriggerToCounter(SyncTrigger *pTrigger)
> > +SyncAddTriggerToSyncObject(SyncTrigger *pTrigger)
> > 
> >  {
> >  
> >      SyncTriggerList *pCur;
> > 
> > +    SyncCounter *pCounter;
> > 
> > -    if (!pTrigger->pCounter)
> > +    if (!pTrigger->pSync)
> > 
> >  	return Success;
> >  	
> >      /* don't do anything if it's already there */
> > 
> > -    for (pCur = pTrigger->pCounter->sync.pTriglist; pCur; pCur =
> > pCur->next) +    for (pCur = pTrigger->pSync->pTriglist; pCur; pCur =
> > pCur->next)
> > 
> >      {
> >  	
> >  	if (pCur->pTrigger == pTrigger)
> >  	
> >  	    return Success;
> > 
> > @@ -161,11 +168,16 @@ SyncAddTriggerToCounter(SyncTrigger *pTrigger)
> > 
> >  	return BadAlloc;
> >  	
> >      pCur->pTrigger = pTrigger;
> > 
> > -    pCur->next = pTrigger->pCounter->sync.pTriglist;
> > -    pTrigger->pCounter->sync.pTriglist = pCur;
> > +    pCur->next = pTrigger->pSync->pTriglist;
> > +    pTrigger->pSync->pTriglist = pCur;
> > 
> > -    if (IsSystemCounter(pTrigger->pCounter))
> > -	SyncComputeBracketValues(pTrigger->pCounter);
> > +    if (SYNC_COUNTER == pTrigger->pSync->type)
> > +    {
> > +	pCounter = (SyncCounter *)pTrigger->pSync;
> > +
> > +	if (IsSystemCounter(pCounter))
> > +	    SyncComputeBracketValues(pCounter);
> > +    }
> > 
> >      return Success;
> >  
> >  }
> > 
> > @@ -188,69 +200,91 @@ SyncAddTriggerToCounter(SyncTrigger *pTrigger)
> > 
> >  static Bool
> >  SyncCheckTriggerPositiveComparison(SyncTrigger *pTrigger, CARD64 oldval)
> >  {
> > 
> > -    return (pTrigger->pCounter == NULL ||
> > -	    XSyncValueGreaterOrEqual(pTrigger->pCounter->value,
> > -				     pTrigger->test_value));
> > +    SyncCounter *pCounter;
> > +
> > +    assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
> > +    pCounter = (SyncCounter *)pTrigger->pSync;
> > +
> > +    return (pCounter == NULL ||
> > +	    XSyncValueGreaterOrEqual(pCounter->value, pTrigger-
>test_value));
> > 
> >  }
> >  
> >  static Bool
> >  SyncCheckTriggerNegativeComparison(SyncTrigger *pTrigger,  CARD64
> >  oldval) {
> > 
> > -    return (pTrigger->pCounter == NULL ||
> > -	    XSyncValueLessOrEqual(pTrigger->pCounter->value,
> > -				  pTrigger->test_value));
> > +    SyncCounter *pCounter;
> > +
> > +    assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
> > +    pCounter = (SyncCounter *)pTrigger->pSync;
> > +
> > +    return (pCounter == NULL ||
> > +	    XSyncValueLessOrEqual(pCounter->value, pTrigger->test_value));
> > 
> >  }
> >  
> >  static Bool
> >  SyncCheckTriggerPositiveTransition(SyncTrigger *pTrigger, CARD64 oldval)
> >  {
> > 
> > -    return (pTrigger->pCounter == NULL ||
> > +    SyncCounter *pCounter;
> > +
> > +    assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
> > +    pCounter = (SyncCounter *)pTrigger->pSync;
> > +
> > +    return (pCounter == NULL ||
> > 
> >  	    (XSyncValueLessThan(oldval, pTrigger->test_value) &&
> > 
> > -	     XSyncValueGreaterOrEqual(pTrigger->pCounter->value,
> > -				      pTrigger->test_value)));
> > +	     XSyncValueGreaterOrEqual(pCounter->value, pTrigger-
>test_value)));
> > 
> >  }
> >  
> >  static Bool
> >  SyncCheckTriggerNegativeTransition(SyncTrigger *pTrigger, CARD64 oldval)
> >  {
> > 
> > -    return (pTrigger->pCounter == NULL ||
> > +    SyncCounter *pCounter;
> > +
> > +    assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
> > +    pCounter = (SyncCounter *)pTrigger->pSync;
> > +
> > +    return (pCounter == NULL ||
> > 
> >  	    (XSyncValueGreaterThan(oldval, pTrigger->test_value) &&
> > 
> > -	     XSyncValueLessOrEqual(pTrigger->pCounter->value,
> > -				   pTrigger->test_value)));
> > +	     XSyncValueLessOrEqual(pCounter->value, pTrigger->test_value)));
> > 
> >  }
> >  
> >  static int
> > 
> > -SyncInitTrigger(ClientPtr client, SyncTrigger *pTrigger, XSyncCounter
> > counter, -		Mask changes)
> > +SyncInitTrigger(ClientPtr client, SyncTrigger *pTrigger, XID syncObject,
> > +		RESTYPE resType, Mask changes)
> > 
> >  {
> > 
> > -    SyncCounter *pCounter = pTrigger->pCounter;
> > +    SyncObject *pSync = pTrigger->pSync;
> > +    SyncCounter *pCounter = NULL;
> > 
> >      int		rc;
> > 
> > -    Bool	newcounter = FALSE;
> > +    Bool	newSyncObject = FALSE;
> > 
> >      if (changes & XSyncCACounter)
> >      {
> > 
> > -	if (counter == None)
> > -	    pCounter = NULL;
> > -	else if (Success != (rc = dixLookupResourceByType ((pointer
> > *)&pCounter, -				counter, RTCounter, client, 
DixReadAccess)))
> > +	if (syncObject == None)
> > +	    pSync = NULL;
> > +	else if (Success != (rc = dixLookupResourceByType ((pointer 
*)&pSync,
> > +				syncObject, resType, client, DixReadAccess)))
> > 
> >  	{
> > 
> > -	    client->errorValue = counter;
> > +	    client->errorValue = syncObject;
> > 
> >  	    return rc;
> >  	
> >  	}
> > 
> > -	if (pCounter != pTrigger->pCounter)
> > +	if (pSync != pTrigger->pSync)
> > 
> >  	{ /* new counter for trigger */
> > 
> > -	    SyncDeleteTriggerFromCounter(pTrigger);
> > -	    pTrigger->pCounter = pCounter;
> > -	    newcounter = TRUE;
> > +	    SyncDeleteTriggerFromSyncObject(pTrigger);
> > +	    pTrigger->pSync = pSync;
> > +	    newSyncObject = TRUE;
> > 
> >  	}
> >  	
> >      }
> >      
> >      /* if system counter, ask it what the current value is */
> > 
> > -    if (IsSystemCounter(pCounter))
> > +    if (SYNC_COUNTER == pSync->type)
> 
> I get a NULL-pointer dereference here. There's a path where pSync can be
> NULL:
> if (changes & XSyncCACounter)
>         if (syncObject == None)
>              pSync = NULL;
> ..
> if (SYNC_COUNTER == pSync->type) /* boom */
> 
> This is triggered on startup each time, I guess by gdm trying to do
> something with the sync triggers since I can see the gdm cursor for about a
> second or two before the crash.
> 
> Either way, this code path looks like it's missing something.

Right, Keith ran into the same and committed a fix right away I believe.  Very 
sorry about the breakage, but the latest bits should work.

Thanks,
-James

> Cheers,
>   Peter