On 11 September 2014 20:28, Stefan Fuhrmann
<stefan.fuhrmann_at_wandisco.com> wrote:
> On Wed, Sep 10, 2014 at 5:35 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>
>> I've accidentally ran Subversion test suite under elevated local
>> administrator account on Windows and got failures in named-atomics
>> tests:
>> [[[
>> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:520:
>> (apr_err=SVN_ERR_TEST_FAILED)
>> svn_tests: E200006: assertion 'value == 42' failed at
>> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:520
>> FAIL: named_atomic-test.exe 1: basic r/w access to a single atomic
>> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:581:
>> (apr_err=SVN_ERR_TEST_FAILED)
>> svn_tests: E200006: assertion 'value == 42 * HUGE_VALUE' failed at
>> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:581
>> FAIL: named_atomic-test.exe 2: atomics must be 64 bits
>> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:643:
>> (apr_err=SVN_ERR_TEST_FAILED)
>> svn_tests: E200006: assertion 'value1 == 46 * HUGE_VALUE' failed at
>> ..\..\..\subversion\tests\libsvn_subr\named_atomic-test.c:643
>> FAIL: named_atomic-test.exe 3: basic r/w access to multiple atomics
>> ]]]
>>
>> The investigation revealed the following:
>> 1. The named_atomics tests are skipped unless executed under elevated
>> local administrator on Windows.
>
>
> What worries me more is that our Windows build bots don't report
> those tests as "SKIP" - although the common init_test_shm() function
> in named_atomics-test.c will return SVN_ERR_TEST_SKIPPED.
> Are they reported as skipped in your setup when you run the test
> without local admin rights?
>
I'm getting SKIP notifications for named_atomics tests when run them
without local admin rights. It seems that buildbot executes these tests,
but Windows buildbot uses VS2010 which doesn't have intrinsics for
"efficient" atomics, so it uses different code.
>>
>> 2. The named_atomics are never properly worked on Windows.
>
>
> Yes, that is probably the case. The only good bit about the whole
> thing is that the tests themselves were looking to the right kind
> of problem.
>
Agree.
>>
>> A little bit more detailed explanation:
>> Originally "named atomics" framework was using shared memory for
>> syncronization.
>> Creating shared memory on Windows requires administrative permissions, so
>> svn_named_atomic__is_supported() had check if application has necessary
>> permissions to create shared memory.
>> In r1404112 [1] "named atomics" framework was rewritten to use apr_mmap
>> instead
>> of apr_shm_*, but code in svn_named_atomic__is_supported() was not updated
>> to
>> reflect this change.
>>
>> The bug itself introduced in r1327458 [2] where call to InterlockedAdd64()
>> was
>> replaced with call to InterlockedExchangeAdd64(). But functions have
>> different
>> return values: InterlockedAdd64() returns result of operation, while
>> InterlockedExchangeAdd64() returns original value.
>
>
> That analysis is correct.
>
>> I see two ways how resolve these test failures:
>> 1. Fix the code somehow.
>
>
> Which would not be too hard doing something like this:
>
[...]
>
> but ...
>
Yes, the patch like above should fix the problem. But using macro
argument twice may lead problems if non-constant argument will
be passed. So proper solution is use inline function.
>> 2. Remove it since "named atomics" framework is only used for currently
>> disabled revprop caching.
>
>
> ... I'm +1 on getting rid of the SHM code altogether (we are using MMAP to
> get shared memory). It turned out to be a poor choice as all sorts of
> Windows
> platform specifics make it hard caused us to deviate further and further
> from
> the initial APR-based design. Some of the Windows-specific issues, e.g. the
> file retry races in the init code, should have been reported before the
> 1.8.0
> release.
>
>>
>> Personally I don't see reason to spend resources fixing unused code,
>> especially
>> that even code on 'revprop-caching-ng' branch removes it also. Any
>> other opinions?
>
>
> No, I agree.
>
> The revpro-caching-ng branch pretty much exactly removes the SHM
> code.
I think that the revprop-caching-ng branch should not be merged. So
may be worth to remove named-atomics and SHM code from trunk to turn
the trunk back to the normal state (without unused and not working code)
[...]
--
Ivan Zhakov
Received on 2014-09-18 15:01:39 CEST