Re: [PATCH 1/3] kernel32/tests/fiber: Add tests for fiber-local storage.

[André Hentschel]

Am 02.07.2016 um 04:41 schrieb John Sheu:
> Signed-off-by: John Sheu <sheu(a)google.com>

Hi and welcome to Wine development!
May I ask if you could name an application which will benefit from your fixes?
Your tests currently fail on our testbot as you can see here: https://source.winehq.org/patches/
Some inline comments:
> static VOID WINAPI FiberMainProc(LPVOID lpFiberParameter)
>  {
>      BYTE *tparam = (BYTE *)lpFiberParameter;
> -    cbCount++;
> +    fiberCount++;
>      ok(*tparam == 185, "Parameterdata expected not to be changed\n");
> +    if (fls_index_to_set != FLS_OUT_OF_INDEXES)
> +    {
> +        if (!pFlsSetValue || !pFlsGetValue)
> +        {
> +            win_skip( "Fiber Local Storage not supported\n" );
> +        }
> +        else
> +        {
> +            PVOID pret;
> +            BOOL bret;
> +
> +            pret = FlsGetValue(fls_index_to_set);
> +            ok(pret == NULL, "FlsGetValue returned %p, expected NULL\n", pret);
> +
> +            /* Set the FLS value */
> +            SetLastError( 0xdeadbeef );
> +            bret = pFlsSetValue(fls_index_to_set, fls_value_to_set);
> +            ok(bret, "FlsSetValue failed with error %u\n", GetLastError());
> +
> +            /* Verify that FlsGetValue retrieves the value set by FlsSetValue */
> +            SetLastError( 0xdeadbeef );
> +            pret = FlsGetValue(fls_index_to_set);
> +            ok(pret == fls_value_to_set,
> +               "FlsGetValue returned %p, expected %p\n", pret, fls_value_to_set);
> +            ok(GetLastError() == ERROR_SUCCESS,
> +	       "FlsGetValue error %u\n", GetLastError());
> +        }
> +    }
>      pSwitchToFiber(fibers[0]);
>  }

You are mixing tabs and spaces in FiberMainProc, spaces are preferred. Further you can avoid some line brakes in ok() calls, only wrap at around 100
> +static void test_FiberLocalStorageWithFibers(PFLS_CALLBACK_FUNCTION cbfunc) {
> +    PVOID val1 = (PVOID) 0x314;
> +    PVOID val2 = (PVOID) 0x152;
> +    BOOL ret;
> +
> +    if (!pConvertThreadToFiber || !pSwitchToFiber || !pDeleteFiber)
> +    {
> +        win_skip( "Fibers not supported\n" );
> +        return;
> +    }
> +    if (!pFlsAlloc || !pFlsFree || !pFlsSetValue || !pFlsGetValue)
> +    {
> +        win_skip( "Fiber Local Storage not supported\n" );
> +        return;
> +    }
> +
> +
> +    fls_index_to_set = pFlsAlloc(cbfunc);
> +    ok(fls_index_to_set != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %d\n", GetLastError());
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fibers[0] = pConvertThreadToFiber(&testparam);

Why do you convert the current thread into a fiber? It seems to me that is causing the crash on w2k3.
You could try to run that test in a seperate thread
> +    fibers[1] = pCreateFiber(0,FiberMainProc,&testparam);
> +    fibers[2] = pCreateFiber(0,FiberMainProc,&testparam);
> +    ok(fibers[1] != 0, "CreateFiber failed with error %d\n", GetLastError());
> +    ok(fibers[2] != 0, "CreateFiber failed with error %d\n", GetLastError());
> +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val1;
> +    pSwitchToFiber(fibers[1]);
> +    ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount);
> +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val2;
> +    pSwitchToFiber(fibers[2]);
> +    ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount);
> +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> +
> +    fls_value_to_set = val2;
> +    ret = pFlsSetValue(fls_index_to_set, fls_value_to_set);
> +    ok(ret, "FlsSetValue failed\n");
> +    ok(val2 == pFlsGetValue(fls_index_to_set), "FlsGetValue failed\n");
> +
> +    fiberCount = 0;
> +    cbCount = 0;
> +    fls_value_to_set = val1;
> +    pDeleteFiber(fibers[1]);
> +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> +    todo_wine
> +    {
> +        ok(cbCount == 1, "Wrong callback count: %d\n", cbCount);
> +    }

You can make that a one liner:
todo_wine ok();




Hope that helps
Have fun!

[PATCH 1/3] kernel32/tests/fiber: Add tests for fiber-local storage.

[John Sheu]

Thanks!

I'm interested in using fiber-local storage, particularly for the
callback behavior (which is not implemented right now).  The Tls* API
in Windows does not support this.  This is for the purposes of better
thread-local storage for the Ion library: see
https://github.com/google/ion

On Sat, Jul 2, 2016 at 7:55 AM, André Hentschel <nerv(a)dawncrow.de> wrote:
> Am 02.07.2016 um 04:41 schrieb John Sheu:
> > Signed-off-by: John Sheu <sheu(a)google.com>
>
> Hi and welcome to Wine development!
> May I ask if you could name an application which will benefit from your fixes?
> Your tests currently fail on our testbot as you can see here: https://source.winehq.org/patches/
> Some inline comments:
> > static VOID WINAPI FiberMainProc(LPVOID lpFiberParameter)
> >  {
> >      BYTE *tparam = (BYTE *)lpFiberParameter;
> > -    cbCount++;
> > +    fiberCount++;
> >      ok(*tparam == 185, "Parameterdata expected not to be changed\n");
> > +    if (fls_index_to_set != FLS_OUT_OF_INDEXES)
> > +    {
> > +        if (!pFlsSetValue || !pFlsGetValue)
> > +        {
> > +            win_skip( "Fiber Local Storage not supported\n" );
> > +        }
> > +        else
> > +        {
> > +            PVOID pret;
> > +            BOOL bret;
> > +
> > +            pret = FlsGetValue(fls_index_to_set);
> > +            ok(pret == NULL, "FlsGetValue returned %p, expected NULL\n", pret);
> > +
> > +            /* Set the FLS value */
> > +            SetLastError( 0xdeadbeef );
> > +            bret = pFlsSetValue(fls_index_to_set, fls_value_to_set);
> > +            ok(bret, "FlsSetValue failed with error %u\n", GetLastError());
> > +
> > +            /* Verify that FlsGetValue retrieves the value set by FlsSetValue */
> > +            SetLastError( 0xdeadbeef );
> > +            pret = FlsGetValue(fls_index_to_set);
> > +            ok(pret == fls_value_to_set,
> > +               "FlsGetValue returned %p, expected %p\n", pret, fls_value_to_set);
> > +            ok(GetLastError() == ERROR_SUCCESS,
> > +            "FlsGetValue error %u\n", GetLastError());
> > +        }
> > +    }
> >      pSwitchToFiber(fibers[0]);
> >  }
>
> You are mixing tabs and spaces in FiberMainProc, spaces are preferred. Further you can avoid some line brakes in ok() calls, only wrap at around 100
> > +static void test_FiberLocalStorageWithFibers(PFLS_CALLBACK_FUNCTION cbfunc) {
> > +    PVOID val1 = (PVOID) 0x314;
> > +    PVOID val2 = (PVOID) 0x152;
> > +    BOOL ret;
> > +
> > +    if (!pConvertThreadToFiber || !pSwitchToFiber || !pDeleteFiber)
> > +    {
> > +        win_skip( "Fibers not supported\n" );
> > +        return;
> > +    }
> > +    if (!pFlsAlloc || !pFlsFree || !pFlsSetValue || !pFlsGetValue)
> > +    {
> > +        win_skip( "Fiber Local Storage not supported\n" );
> > +        return;
> > +    }
> > +
> > +
> > +    fls_index_to_set = pFlsAlloc(cbfunc);
> > +    ok(fls_index_to_set != FLS_OUT_OF_INDEXES, "FlsAlloc failed with error %d\n", GetLastError());
> > +
> > +    fiberCount = 0;
> > +    cbCount = 0;
> > +    fibers[0] = pConvertThreadToFiber(&testparam);
>
> Why do you convert the current thread into a fiber? It seems to me that is causing the crash on w2k3.
> You could try to run that test in a seperate thread
> > +    fibers[1] = pCreateFiber(0,FiberMainProc,&testparam);
> > +    fibers[2] = pCreateFiber(0,FiberMainProc,&testparam);
> > +    ok(fibers[1] != 0, "CreateFiber failed with error %d\n", GetLastError());
> > +    ok(fibers[2] != 0, "CreateFiber failed with error %d\n", GetLastError());
> > +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> > +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> > +
> > +    fiberCount = 0;
> > +    cbCount = 0;
> > +    fls_value_to_set = val1;
> > +    pSwitchToFiber(fibers[1]);
> > +    ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount);
> > +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> > +
> > +    fiberCount = 0;
> > +    cbCount = 0;
> > +    fls_value_to_set = val2;
> > +    pSwitchToFiber(fibers[2]);
> > +    ok(fiberCount == 1, "Wrong fiber count: %d\n", fiberCount);
> > +    ok(cbCount == 0, "Wrong callback count: %d\n", cbCount);
> > +
> > +    fls_value_to_set = val2;
> > +    ret = pFlsSetValue(fls_index_to_set, fls_value_to_set);
> > +    ok(ret, "FlsSetValue failed\n");
> > +    ok(val2 == pFlsGetValue(fls_index_to_set), "FlsGetValue failed\n");
> > +
> > +    fiberCount = 0;
> > +    cbCount = 0;
> > +    fls_value_to_set = val1;
> > +    pDeleteFiber(fibers[1]);
> > +    ok(fiberCount == 0, "Wrong fiber count: %d\n", fiberCount);
> > +    todo_wine
> > +    {
> > +        ok(cbCount == 1, "Wrong callback count: %d\n", cbCount);
> > +    }
>
> You can make that a one liner:
> todo_wine ok();
>
> Hope that helps
> Have fun!