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

[Peter Hutterer]

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.

Cheers,
  Peter