string methods thread safety

[Anton Pevtsov <An...@moscow.vdiweb.com>]

I updated the 21.string.push_back.mt test to use the test driver
features and extended it to exercise the append and replace methods in
the same way.
The test and required changes are here:
http://people.apache.org/~antonp/stdcxx07032006/

I found the tricky place in the rwthread.cpp, line 397:
	void* const next_arg = thr_arg ? thr_arg + i : (void*)(thr_id +
i);
I suspect here should be
	void* const next_arg = thr_arg ? *(thr_arg + i) : (void*)(thr_id
+ i);
The fix included.

Also I have several questions.
1. The test becomes slow due to a huge amount of asserts to be verified.
I think that we should exercise the multithreading on several test cases
per eaach method, but shall we use all our char and allocator types for
this purpose?

2. Martin, you have mentioned
(http://mail-archives.apache.org/mod_mbox/incubator-stdcxx-dev/200605.mb
ox/%3c44692962.6040103@roguewave.com%3e) that
"We need to write thread safety tests for other member functions as well
(not necessarily all of them but at least those that we know are used a
lot. The most important ones are those that call _C_get_ref(), i.e., the
copy ctor, assignment operator, begin(), append(const string&), and
replace()."
But as far as I see the begin method will fail the test if I didn't miss
something.

3. The test looks useless in the single-threaded environment. May be we
should provide a stub for this cases and not run the test (or run and do
nothing) for these cases?


Thanks,
Anton Pevtsov

Re: string methods thread safety

[Martin Sebor <se...@roguewave.com>]

Martin Sebor wrote:
[...]
> I have a semi-exhaustive thread safety test for sting stashed
> away somewhere. Let me dig it up and post it to give you an
> idea.

The test is attached. It's supposed to be portable across most
today's C++ implementations (which isn't our goal).

The thread safety tests I envision for our test suite would
exercise a subset (as small as a pair) of functions per each
run. The one constant (as in the same across all test runs)
function would be the copy ctor (or its equivalent such as
the assignment operator) and the other a modifying string
function. The copy ctor would be used to create a copy of
a shared string object for each thread and the modifying
function would then operate on the copy.

The test would serve as its own driver with the initial
invocation spawning a new process (a copy of self) for each
subset (or pair) of tested functions and reporting non-zero
exit status of the child process as an assertion failure.
That way the test would exhaustively exercise all subsets
of the tested functions even if one or more of them crashed
(which is the typical manifestation of thread-safety bugs)
and clearly report which one failed (which would be hard
to do otherwise).

This last part would be best implemented in the test driver
under some generic interface similar to rw_thread_create()
(e.g., rw_process_create()).

If this doesn't sound like something you'd like to tackle feel
free to move on to other types of string tests, or if we're
done with those, to the container tests.

Martin

Re: string methods thread safety

[Martin Sebor <se...@roguewave.com>]

Martin Sebor wrote:
> Anton Pevtsov wrote:
[...]
>> I found the tricky place in the rwthread.cpp, line 397:
>>     void* const next_arg = thr_arg ? thr_arg + i : (void*)(thr_id +
>> i);
>> I suspect here should be
>>     void* const next_arg = thr_arg ? *(thr_arg + i) : (void*)(thr_id
>> + i);
>> The fix included.
> 
> 
> Let me look into this tomorrow.

I put together a test to help me understand the problem (see
below) but I don't see anything wrong with its output. Do you?

(Btw., we should probably rename rwthread.h and rwthread.cpp
to rw_thread.h and thread.cpp for consistency with the rest of
the driver source files.)

Martin

$ cat v.cpp && nice make v && ./v
#include <rwthread.h>
#include <stdio.h>

void* thr_func (void *arg)
{
     void* const* const parg = (void**)arg;

     printf ("%p, %p\n", parg, *parg);

     return *parg;
}

void test (bool arg)
{
     rw_thread_t thr_id [4];
     static const unsigned nthrs = sizeof thr_id / sizeof *thr_id;

     void* thr_arg [nthrs] = { 0, 0, 0, 0 };
     if (arg) {
         thr_arg [0] = (void*)0xa;
         thr_arg [1] = (void*)0xb;
         thr_arg [2] = (void*)0xc;
         thr_arg [3] = (void*)0xd;
     };

     printf ("invoking rw_thread_pool() with a %snull thread argument\n",
             arg ? "non-" : "");

     rw_thread_pool (thr_id, nthrs, 0, thr_func, arg ? thr_arg : 0);

     void* thr_res [nthrs] = { 0, 0, 0, 0 };

     for (unsigned i = 0; i != nthrs; ++i) {
         rw_thread_join (thr_id [i], thr_res + i);
         printf ("thread %u result = %p\n", i, thr_res [i]);
     }
}

int main ()
{
     test (false);
     test (true);
}
gcc -c -I/build/sebor/dev/stdlib/include/ansi -D_RWSTDDEBUG   -pthreads 
-D_RWSTD_USE_CONFIG -I/build/sebor/dev/stdlib/include 
-I/build/sebor/gcc-4.1.0-15s/include -I/build/sebor/dev/stdlib/../rwtest 
-I/build/sebor/dev/stdlib/../rwtest/include 
-I/build/sebor/dev/stdlib/tests/include  -pedantic -nostdinc++ -g  -W 
-Wall -Wcast-qual -Winline -Wshadow -Wwrite-strings -Wno-long-long  v.cpp
v.cpp: In function 'void* thr_func(void*)':
v.cpp:8: warning: format '%p' expects type 'void*', but argument 2 has 
type 'void* const*'
gcc v.o -o v -L/build/sebor/gcc-4.1.0-15s/rwtest -lrwtest15s -pthreads 
-L/build/sebor/gcc-4.1.0-15s/lib -lstd15s  -lsupc++ -lm
invoking rw_thread_pool() with a null thread argument
ffbffa48, 0
ffbffa54, 1
ffbffa6c, 3
ffbffa60, 2
thread 0 result = 0
thread 1 result = 1
thread 2 result = 2
thread 3 result = 3
invoking rw_thread_pool() with a non-null thread argument
ffbffa88, a
ffbffa90, c
ffbffa8c, b
thread 0 result = a
thread 1 result = b
thread 2 result = c
ffbffa94, d
thread 3 result = d

Re: string methods thread safety

[Martin Sebor <se...@roguewave.com>]

Anton Pevtsov wrote:
> I updated the 21.string.push_back.mt test to use the test driver
> features and extended it to exercise the append and replace methods in
> the same way.
> The test and required changes are here:
> http://people.apache.org/~antonp/stdcxx07032006/

I've run out of time today. Let me look at it tomorrow and get
back to you. At a high level, though, I don't think we should
be using the test driver (i.e., the 21.strings.cpp framework)
in the thread safety tests. Also, keep in mind that the rest
of the driver (e.g., rw_assert()) is not thread-safe either.
I think the thread safety tests should only use the assert
macro.

I have a semi-exhaustive thread safety test for sting stashed
away somewhere. Let me dig it up and post it to give you an
idea.

> 
> I found the tricky place in the rwthread.cpp, line 397:
> 	void* const next_arg = thr_arg ? thr_arg + i : (void*)(thr_id +
> i);
> I suspect here should be
> 	void* const next_arg = thr_arg ? *(thr_arg + i) : (void*)(thr_id
> + i);
> The fix included.

Let me look into this tomorrow.

> 
> Also I have several questions.
> 1. The test becomes slow due to a huge amount of asserts to be verified.
> I think that we should exercise the multithreading on several test cases
> per eaach method, but shall we use all our char and allocator types for
> this purpose?

Yes, we should exercise only a small number of functions in each
run of the test. The test program itself can be written so as to
exercise a larger number of string functions but which ones are
actually executed should be controlled via a command line option.

> 
> 2. Martin, you have mentioned
> (http://mail-archives.apache.org/mod_mbox/incubator-stdcxx-dev/200605.mb
> ox/%3c44692962.6040103@roguewave.com%3e) that
> "We need to write thread safety tests for other member functions as well
> (not necessarily all of them but at least those that we know are used a
> lot. The most important ones are those that call _C_get_ref(), i.e., the
> copy ctor, assignment operator, begin(), append(const string&), and
> replace()."
> But as far as I see the begin method will fail the test if I didn't miss
> something.

You mean because the non-const version triggers COW?

> 
> 3. The test looks useless in the single-threaded environment. May be we
> should provide a stub for this cases and not run the test (or run and do
> nothing) for these cases?

Yes, definitely. The test should be a no-op in a single threaded
environment.

Martin