[PATCH 1/1] Implementation / tests for pthread_cond_clockwait()
Matthew Joyce
mfjoyce2004 at gmail.com
Thu Aug 19 13:10:10 UTC 2021
On Wed, Aug 18, 2021 at 8:44 PM Joel Sherrill <joel at rtems.org> wrote:
>
> On Wed, Aug 18, 2021 at 6:17 AM Matt Joyce <mfjoyce2004 at gmail.com> wrote:
> >
> > Added implementation of the pthread_cond_clockwait()
> > method to cpukit/posix/src/condclockwait.c. Additional
> > logic added to condwaitsupp.c to implement new method.
> > pthread_cond_clockwait() has been added to the Issue 8
> > POSIX Standard.
> >
> > psx17 test added to testsuites/psxtests to test the
> > newly added method.
> > ---
> > cpukit/include/rtems/posix/condimpl.h | 1 +
> > cpukit/posix/src/condclockwait.c | 69 +++
> > cpukit/posix/src/condtimedwait.c | 1 +
> > cpukit/posix/src/condwait.c | 1 +
> > cpukit/posix/src/condwaitsupp.c | 64 ++-
> > spec/build/testsuites/psxtests/grp.yml | 2 +
> > spec/build/testsuites/psxtests/psx17.yml | 20 +
> > testsuites/psxtests/Makefile.am | 8 +
> > testsuites/psxtests/configure.ac | 1 +
> > testsuites/psxtests/psx17/init.c | 425 ++++++++++++++++++
> > testsuites/psxtests/psx17/psx17.doc | 45 ++
> > testsuites/psxtests/psx17/psx17.scn | 82 ++++
> > testsuites/psxtests/psx17/system.h | 57 +++
> > .../psxhdrs/pthread/pthread_cond_clockwait.c | 2 +-
> > 14 files changed, 761 insertions(+), 17 deletions(-)
> > create mode 100644 cpukit/posix/src/condclockwait.c
> > create mode 100644 spec/build/testsuites/psxtests/psx17.yml
> > create mode 100644 testsuites/psxtests/psx17/init.c
> > create mode 100644 testsuites/psxtests/psx17/psx17.doc
> > create mode 100644 testsuites/psxtests/psx17/psx17.scn
> > create mode 100644 testsuites/psxtests/psx17/system.h
> >
> > diff --git a/cpukit/include/rtems/posix/condimpl.h b/cpukit/include/rtems/posix/condimpl.h
> > index 66e09bf6d8..6efbc3af70 100644
> > --- a/cpukit/include/rtems/posix/condimpl.h
> > +++ b/cpukit/include/rtems/posix/condimpl.h
> > @@ -152,6 +152,7 @@ int _POSIX_Condition_variables_Signal_support(
> > int _POSIX_Condition_variables_Wait_support(
> > pthread_cond_t *cond,
> > pthread_mutex_t *mutex,
> > + clockid_t clock_id,
> > const struct timespec *abstime
>
> The clock_id parameter should be aligned with the rest.
I'm having a little trouble with this and the one below: It looks
aligned in both vscode and in vim...even when I cat the patch. But
here it's clearly misaligned.
I went back in and re-aligned all the variables, but I'm still getting
this misaligned output. Could you please advise?
>
> > );
> >
> > diff --git a/cpukit/posix/src/condclockwait.c b/cpukit/posix/src/condclockwait.c
> > new file mode 100644
> > index 0000000000..4104235706
> > --- /dev/null
> > +++ b/cpukit/posix/src/condclockwait.c
> > @@ -0,0 +1,69 @@
> > +/**
> > + * @file
> > + *
> > + * @ingroup POSIXAPI
> > + *
> > + * @brief Waiting on a Condition
> > + */
> > +
> > +/*
> > +* Copyright (C) 2021 Matthew Joyce
>
> General question. Do we have a master list Matthew needs to be added to?
> Or is that just documentation?
>
> > +*
> > +* Redistribution and use in source and binary forms, with or without
> > +* modification, are permitted provided that the following conditions
> > +* are met:
> > +* 1. Redistributions of source code must retain the above copyright
> > +* notice, this list of conditions and the following disclaimer.
> > +* 2. Redistributions in binary form must reproduce the above copyright
> > +* notice, this list of conditions and the following disclaimer in the
> > +* documentation and/or other materials provided with the distribution.
> > +*
> > +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> > +* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > +* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > +* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> > +* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > +* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > +* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > +* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > +* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > +* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> > +* POSSIBILITY OF SUCH DAMAGE.
> > +*/
> > +
> > +/* Defining to have access to function prototype in libc/include/pthread.h */
> > +#define _GNU_SOURCE
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include "config.h"
> > +#endif
> > +
> > +#include <rtems/posix/condimpl.h>
> > +#include <rtems/score/todimpl.h>
> > +
> > +/*
> > + * pthread_cond_clockwait() appears in the Issue 8 POSIX Standard
> > + */
> > +
> > +int pthread_cond_clockwait(
> > + pthread_cond_t *restrict cond,
> > + pthread_mutex_t *restrict mutex,
> > + clockid_t clock_id,
>
> Indentation again.
>
> > + const struct timespec *restrict abstime
> > +)
> > +{
> > + if ( abstime == NULL ) {
> > + return EINVAL; /* not specified */
> > + }
>
> I checked Issue 8 (draft) and NULL is not specified. Please add that as
> a comment above the if. More like "POSIX Issue 8 does not specify that
> EINVAL is returned when abstime equals NULL"
>
> I think it is implied that we are protecting ourselves from a NULL access.
>
> FWIW glibc and cygwin tend to avoid NULL checks and just let the process
> fail with an address access exception. We don't have that luxury to be lazy
> and avoid the check.
>
> > +
> > + if ( clock_id != CLOCK_REALTIME && clock_id != CLOCK_MONOTONIC ) {
> > + return EINVAL; /* not specified */
> > + }
>
>
> This is specified at line 51714 in the PDF. Comment is wrong.
>
> Please rewrite so coverage testing and MCDC analysis is easier. Use simple
> expressions with a single condition when possible. This should be equivalent in
> this case:
>
> if (clock_id != CLOCK_REALTIME) {
> if (clock_id != CLOCK_MONOTONIC) {
> return EINVALl
> }
> }
>
> MCDC analysis is used for safety critical code and requires you to exercise
> a sufficient set of true/false values to exercise compound expressions. If you
> can write simple single condition expressions, then there is a logical operation
> followed by a conditional branch. If you know the branch was both taken and
> not taken, then you are done. In this case, a test case with CLOCK_REALTIME,
> one with CLOCK_MONOTONIC, and one bad clock_id covers this.
>
> > +
> > + return _POSIX_Condition_variables_Wait_support(
> > + cond,
> > + mutex,
> > + clock_id,
> > + abstime
> > + );
> > +}
> > diff --git a/cpukit/posix/src/condtimedwait.c b/cpukit/posix/src/condtimedwait.c
> > index 0bc8bfc18e..53fd88efc6 100644
> > --- a/cpukit/posix/src/condtimedwait.c
> > +++ b/cpukit/posix/src/condtimedwait.c
> > @@ -38,6 +38,7 @@ int pthread_cond_timedwait(
> > return _POSIX_Condition_variables_Wait_support(
> > cond,
> > mutex,
> > + NULL,
> > abstime
> > );
> > }
> > diff --git a/cpukit/posix/src/condwait.c b/cpukit/posix/src/condwait.c
> > index 09431e216d..6e9ca4854f 100644
> > --- a/cpukit/posix/src/condwait.c
> > +++ b/cpukit/posix/src/condwait.c
> > @@ -54,6 +54,7 @@ int pthread_cond_wait(
> > return _POSIX_Condition_variables_Wait_support(
> > cond,
> > mutex,
> > + NULL,
> > NULL
> > );
> > }
> > diff --git a/cpukit/posix/src/condwaitsupp.c b/cpukit/posix/src/condwaitsupp.c
> > index ee2f8a0787..93d6e1c9d1 100644
> > --- a/cpukit/posix/src/condwaitsupp.c
> > +++ b/cpukit/posix/src/condwaitsupp.c
> > @@ -26,6 +26,7 @@
> > #include <rtems/score/status.h>
> > #include <rtems/score/threaddispatch.h>
> >
> > +
>
> Added blank line. Remove.
>
> > static void _POSIX_Condition_variables_Mutex_unlock(
> > Thread_queue_Queue *queue,
> > Thread_Control *the_thread,
> > @@ -94,6 +95,7 @@ static void _POSIX_Condition_variables_Enqueue_with_timeout_realtime(
> > int _POSIX_Condition_variables_Wait_support(
> > pthread_cond_t *cond,
> > pthread_mutex_t *mutex,
> > + clockid_t clock_id,
> > const struct timespec *abstime
>
> Line it up.
>
> > )
> > {
> > @@ -107,28 +109,58 @@ int _POSIX_Condition_variables_Wait_support(
> > POSIX_CONDITION_VARIABLES_VALIDATE_OBJECT( the_cond, flags );
> >
> > _Thread_queue_Context_initialize( &queue_context );
> > -
> > - if ( abstime != NULL ) {
> > - _Thread_queue_Context_set_timeout_argument( &queue_context, abstime, true );
> > -
> > - if ( _POSIX_Condition_variables_Get_clock( flags ) == CLOCK_MONOTONIC ) {
> > +
> > + /* If there is a clock_id parameter, this is a call to
> > + * pthread_cond_clockwait. Set the clock according to
> > + * this parameter. */
> This can be pulled up to two lines I think and stay under 80 columns.
>
> > + if ( clock_id ) {
> > + _Thread_queue_Context_set_timeout_argument( &queue_context,
> > + abstime, true );
>
> Is this really too long? If it is, this isn't the pattern for breaking the line.
>
> > +
> > + /* We have already validated supported clocks in condclockwait.c.
> > + * if clock_id is not clock_monotonic, then it is clock_realtime. */
>
> Pull the closing */ down to its own line.
>
> Capitalize CLOCK_MONOTONIC and CLOCK_REALTIME since you are
> using their proper name.
>
> > + if ( clock_id == CLOCK_MONOTONIC ) {
> > _Thread_queue_Context_set_enqueue_callout(
> > &queue_context,
> > _POSIX_Condition_variables_Enqueue_with_timeout_monotonic
> > );
> > - } else {
> > - _Thread_queue_Context_set_enqueue_callout(
> > + }
> > +
> > + else {
> > + _Thread_queue_Context_set_enqueue_callout(
> > &queue_context,
> > _POSIX_Condition_variables_Enqueue_with_timeout_realtime
> > );
> > }
> > - } else {
> > - _Thread_queue_Context_set_enqueue_callout(
> > - &queue_context,
> > - _POSIX_Condition_variables_Enqueue_no_timeout
> > - );
> > }
> >
> > + /* If there is no clock_id parameter, this is either a call to
> > + * cond_timedwait or cond_wait. */
> > + else {
>
> Pull the */ down if this has to be on multiple lines.
>
> Go ahead and put {} around the true condition that we only see the
> else { for. This looks like a case where that would add clarity.
Sorry, I'm not quite sure what you mean here!
>
> NOTE: It is likely we should be better about having {} more frequently.
>
> > +
> > + /* If there is an abstime parameter, this is a call to cond_timedwait. */
> > + if ( abstime != NULL ) {
> > + _Thread_queue_Context_set_timeout_argument( &queue_context, abstime, true );
> > +
> > + if ( _POSIX_Condition_variables_Get_clock( flags ) == CLOCK_MONOTONIC ) {
> > + _Thread_queue_Context_set_enqueue_callout(
> > + &queue_context,
> > + _POSIX_Condition_variables_Enqueue_with_timeout_monotonic
> > + );
> > + } else {
> > + _Thread_queue_Context_set_enqueue_callout(
> > + &queue_context,
> > + _POSIX_Condition_variables_Enqueue_with_timeout_realtime
> > + );
> > + }
> > + /* If there is no abstime parameter, this is a call to cond_wait. */
> > + } else {
> > + _Thread_queue_Context_set_enqueue_callout(
> > + &queue_context,
> > + _POSIX_Condition_variables_Enqueue_no_timeout
> > + );
> > + }
> > + }
>
> Did this just get reformatted as part of adding the support? It seems
> to be the same.
>
> > executing = _POSIX_Condition_variables_Acquire( the_cond, &queue_context );
> >
> > if (
> > @@ -138,9 +170,8 @@ int _POSIX_Condition_variables_Wait_support(
> > _POSIX_Condition_variables_Release( the_cond, &queue_context );
> > return EINVAL;
> > }
> > -
> > +
>
> What happened here? Was there a space before the end of line or did you
> add a space?
>
> > the_cond->mutex = mutex;
> > -
> > _Thread_queue_Context_set_thread_state(
> > &queue_context,
> > STATES_WAITING_FOR_CONDITION_VARIABLE
> > @@ -151,8 +182,9 @@ int _POSIX_Condition_variables_Wait_support(
> > executing,
> > &queue_context
> > );
> > +
>
> I suspect this blank line is OK since it probably makes the code
> look better.
>
> > error = _POSIX_Get_error_after_wait( executing );
> > -
> > +
>
> Again with odd change.
Sorry, I didn't consciously make this change...I'll take it out!
>
> > /*
> > * If the thread is interrupted, while in the thread queue, by
> > * a POSIX signal, then pthread_cond_wait returns spuriously,
> > @@ -177,7 +209,7 @@ int _POSIX_Condition_variables_Wait_support(
> > _Assert( mutex_error == EINVAL );
> > error = EINVAL;
> > }
> > - }
> >
> > + }
>
>
> What is this change?
>
I'll remove...
> > return error;
> > }
> > diff --git a/spec/build/testsuites/psxtests/grp.yml b/spec/build/testsuites/psxtests/grp.yml
> > index fb7ce465ae..fe945fe7b8 100644
> > --- a/spec/build/testsuites/psxtests/grp.yml
> > +++ b/spec/build/testsuites/psxtests/grp.yml
> > @@ -47,6 +47,8 @@ links:
> > uid: psx15
> > - role: build-dependency
> > uid: psx16
> > +- role: build-dependency
> > + uid: psx17
> > - role: build-dependency
> > uid: psxaio01
> > - role: build-dependency
> > diff --git a/spec/build/testsuites/psxtests/psx17.yml b/spec/build/testsuites/psxtests/psx17.yml
> > new file mode 100644
> > index 0000000000..94f7567fc6
> > --- /dev/null
> > +++ b/spec/build/testsuites/psxtests/psx17.yml
> > @@ -0,0 +1,20 @@
> > +SPDX-License-Identifier: CC-BY-SA-4.0 OR BSD-2-Clause
> > +build-type: test-program
> > +cflags: []
> > +copyrights:
> > +- Copyright (C) 2020 embedded brains GmbH (http://www.embedded-brains.de)
> > +cppflags: []
> > +cxxflags: []
> > +enabled-by: true
> > +features: c cprogram
> > +includes: []
> > +ldflags: []
> > +links: []
> > +source:
> > +- testsuites/psxtests/psx17/init.c
> > +- cpukit/posix/src/condclockwait.c
>
> there is already psxcond01 and 02. I would make this psxcond03 so it
> has a name that more clearly shows its area.
>
> > +stlib: []
> > +target: testsuites/psxtests/psx17.exe
> > +type: build
> > +use-after: []
> > +use-before: []
> > diff --git a/testsuites/psxtests/Makefile.am b/testsuites/psxtests/Makefile.am
> > index a35f00b665..a8d2351e49 100755
> > --- a/testsuites/psxtests/Makefile.am
> > +++ b/testsuites/psxtests/Makefile.am
> > @@ -162,6 +162,14 @@ psx16_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_FLAGS_psx16) \
> > $(support_includes) -I$(top_srcdir)/include
> > endif
> >
> > +if TEST_psx17
> > +psx_tests += psx17
> > +psx_screens += psx17/psx17.scn
> > +psx17_SOURCES = psx17/init.c psx17/system.h include/pmacros.h
> > +psx17_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_FLAGS_psx17) \
> > + $(support_includes) -I$(top_srcdir)/include
> > +endif
> > +
> > if HAS_POSIX
> > if TEST_psxaio01
> > psx_tests += psxaio01
> > diff --git a/testsuites/psxtests/configure.ac b/testsuites/psxtests/configure.ac
> > index 3f95010cd3..cdfa349ae4 100644
> > --- a/testsuites/psxtests/configure.ac
> > +++ b/testsuites/psxtests/configure.ac
> > @@ -59,6 +59,7 @@ RTEMS_TEST_CHECK([psx13])
> > RTEMS_TEST_CHECK([psx14])
> > RTEMS_TEST_CHECK([psx15])
> > RTEMS_TEST_CHECK([psx16])
> > +RTEMS_TEST_CHECK([psx17])
> > RTEMS_TEST_CHECK([psxaio01])
> > RTEMS_TEST_CHECK([psxaio02])
> > RTEMS_TEST_CHECK([psxaio03])
> > diff --git a/testsuites/psxtests/psx17/init.c b/testsuites/psxtests/psx17/init.c
> > new file mode 100644
> > index 0000000000..bc284af94d
> > --- /dev/null
> > +++ b/testsuites/psxtests/psx17/init.c
> > @@ -0,0 +1,425 @@
> > +/*
> > +* Copyright (C) 2021 Matthew Joyce
> > +*
> > +* Redistribution and use in source and binary forms, with or without
> > +* modification, are permitted provided that the following conditions
> > +* are met:
> > +* 1. Redistributions of source code must retain the above copyright
> > +* notice, this list of conditions and the following disclaimer.
> > +* 2. Redistributions in binary form must reproduce the above copyright
> > +* notice, this list of conditions and the following disclaimer in the
> > +* documentation and/or other materials provided with the distribution.
> > +*
> > +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> > +* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > +* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > +* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> > +* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > +* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > +* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > +* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > +* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > +* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> > +* POSSIBILITY OF SUCH DAMAGE.
> > +*/
> > +
> > +/* Defining in order to access pthread_cond_clockwait in pthread.h */
> > +#define _GNU_SOURCE
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include "config.h"
> > +#endif
> > +
> > +#define CONFIGURE_INIT
> > +#include <pthread.h>
> > +#include <sched.h>
> > +#include <errno.h>
> > +#include <rtems.h>
> > +#include <tmacros.h>
> > +#include "system.h"
> > +
> > +pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> > +pthread_cond_t bad_cond;
> > +pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> > +pthread_mutex_t bad_mutex;
> > +pthread_mutex_t new_mutex = PTHREAD_MUTEX_INITIALIZER;
> > +clockid_t clock_id1 = CLOCK_MONOTONIC;
> > +clockid_t clock_id2 = CLOCK_REALTIME;
> > +clockid_t clock_id3 = CLOCK_MONOTONIC_RAW;
> > +int count;
> > +int eno;
> > +int status;
> > +const char rtems_test_name[] = "PSX 17";
> > +
> > +void *POSIX_Init(
> > + void *argument
> > +)
> > +{
> > + TEST_BEGIN();
> > + empty_line();
> > + pthread_t t1;
> > + struct timespec abstime;
> > +
> > + /* Expected pass: Clock Monotonic with sufficient abstime */
> > + printf("1st iteration Begin (Clock Monotonic)\n");
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock(&mutex);
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &mutex, clock_id1, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == 0 );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "1st iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Clock Monotonic with insufficient abstime */
> > + printf( "2nd iteration Begin (Clock Monotonic)\n" );
> > + abstime.tv_sec = 0;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &mutex, clock_id1, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == ETIMEDOUT );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "2nd iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected pass: Clock Realtime with sufficient abstime */
> > + printf( "3rd iteration Begin (Clock Realtime)\n" );
> > + status = clock_gettime( clock_id2, &abstime );
> > + rtems_test_assert( !status );
> > + abstime.tv_sec += 4;
> > + abstime.tv_nsec = 1;
> > +
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &mutex, clock_id2, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == 0 );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "3rd iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Clock Realtime with insufficient abstime */
> > + printf( "4th iteration Begin (Clock Realtime)\n" );
> > + status = clock_gettime( clock_id2, &abstime );
> > + rtems_test_assert( !status );
> > + abstime.tv_sec += 0;
> > + abstime.tv_nsec = 1;
> > +
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &mutex, clock_id2, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == ETIMEDOUT );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "4th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Unsupported Clock */
> > + printf( "5th iteration Begin (Unsupported Clock)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &mutex, clock_id3, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == EINVAL );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "5th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Invalid Clock */
> > + printf( "6th iteration Begin (Invalid Clock)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &mutex, (int)NULL, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == EINVAL );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "6th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Invalid Clock */
> > + printf( "7th iteration Begin (Invalid Abstime)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &mutex, clock_id1, NULL );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == EINVAL );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "7th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Invalid Clock */
> > + printf( "8th iteration Begin (Invalid Cond)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( NULL, &mutex, clock_id1, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno == EINVAL );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "8th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Invalid Mutex */
> > + printf( "9th iteration Begin (Invalid Mutex)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, NULL, clock_id1, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno != 0 );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "9th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Uninitialized condition variable */
> > + printf( "10th iteration Begin (Uninitialized Condition Variable)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &bad_cond, &mutex, clock_id1, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno != 0 );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "10th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Uninitialized condition variable */
> > + printf( "11th iteration Begin (Uninitialized Mutex)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &bad_mutex, clock_id1, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno != 0 );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "11th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected fail: Uninitialized condition variable */
> > + printf( "10th iteration Begin (Uninitialized Condition Variable)\n" );
> > + abstime.tv_sec = 2;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &bad_cond, &mutex, clock_id1, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + rtems_test_assert( eno != 0 );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "10th iteration END\n" );
> > + empty_line();
> > +
> > + /* Expected pass: Binding new mutex to condition variable */
> > + printf( "12th iteration Begin (New Mutex)\n" );
> > + status = clock_gettime( clock_id2, &abstime );
> > + rtems_test_assert( !status );
> > + abstime.tv_sec += 4;
> > + abstime.tv_nsec = 1;
> > + status = pthread_create( &t1, NULL, thread_func2, NULL );
> > + if ( status != 0 ) {
> > + perror( "pthread_create" );
> > + }
> > + pthread_mutex_lock( &new_mutex );
> > + while ( count == 0 ){
> > + eno = pthread_cond_clockwait( &cond, &new_mutex, clock_id2, &abstime );
> > + if ( eno != 0 ) {
> > + break;
> > + }
> > + }
> > +
> > + count -= 1;
> > + printf( "Minus called. Count is %d\n", count );
> > + pthread_mutex_unlock( &new_mutex );
> > + rtems_test_assert( eno == 0 );
> > + printf( "Return value from cond_clockwait is %d\n", eno );
> > + pthread_join( t1, NULL );
> > + printf( "12th iteration END\n" );
> > + empty_line();
> > +
> > + TEST_END();
> > + rtems_test_exit( 0 );
> > + return NULL;
> > +}
> > +
> > +void *thread_func( void *arg )
> > +{
> > + pthread_mutex_lock( &mutex );
> > + if (count == 0) {
> > + pthread_cond_signal( &cond );
> > + }
> > +
> > + count +=1;
> > + printf( "Add called. Count is %d\n", count );
> > + pthread_mutex_unlock( &mutex );
> > + return NULL;
> > +}
> > +
> > +void *thread_func2( void *arg )
> > +{
> > + pthread_mutex_lock( &new_mutex );
> > + if ( count == 0 ) {
> > + pthread_cond_signal( &cond );
> > + }
> > +
> > + count +=1;
> > + printf( "Add called. Count is %d\n", count );
> > + pthread_mutex_unlock( &new_mutex );
> > + return NULL;
> > +}
>
> Is this a copy of an existing test with changes to the cond wait call? If so,
> it would be better to conditionally compile the differences. There are other
> tests which use the same code with a minor change to test two similar
> APIs.
>
> If this is a new test, it needs to self check and print less. I see
> places where it
> does not check return codes. It should use test assertion macros on all
> return codes. The test automation does not compare the screen output
> with the actual run. The scn is a guide for developers on what to expect.
> But all checks for behavior as expected re internal to the test.
>
Ok, got it, thanks! It's a new test. I'll make it quieter and with more checks.
>
> > diff --git a/testsuites/psxtests/psx17/psx17.doc b/testsuites/psxtests/psx17/psx17.doc
> > new file mode 100644
> > index 0000000000..d1206982c9
> > --- /dev/null
> > +++ b/testsuites/psxtests/psx17/psx17.doc
> > @@ -0,0 +1,45 @@
> > +/*
> > +* Copyright (C) 2021 Matthew Joyce
> > +*
> > +* Redistribution and use in source and binary forms, with or without
> > +* modification, are permitted provided that the following conditions
> > +* are met:
> > +* 1. Redistributions of source code must retain the above copyright
> > +* notice, this list of conditions and the following disclaimer.
> > +* 2. Redistributions in binary form must reproduce the above copyright
> > +* notice, this list of conditions and the following disclaimer in the
> > +* documentation and/or other materials provided with the distribution.
> > +*
> > +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> > +* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > +* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > +* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> > +* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > +* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > +* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > +* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > +* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > +* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> > +* POSSIBILITY OF SUCH DAMAGE.
> > +*/
> > +
> > +This file describes the directives and concepts tested by this test set.
> > +
> > +test set name: psx17
> > +
> > +directives:
> > +
> > + pthread_cond_clockwait
> > + pthread_create
> > + pthread_mutex_lock
> > + pthread_mutex_unlock
> > + pthread_cond_signal
> > + pthread_join
> > +
> > +concepts:
> > +
> > ++ Tests the newly-added, Issue 8 POSIX Standard pthread_cond_clockwait method.
> > + Tests include valid/supported and invalid/unsupported clocks, sufficient and
> > + insufficient timeouts, and invalid or uninitialized condition/mutex
> > + parameters.
> > +
> > \ No newline at end of file
> > diff --git a/testsuites/psxtests/psx17/psx17.scn b/testsuites/psxtests/psx17/psx17.scn
> > new file mode 100644
> > index 0000000000..d3e7fd024f
> > --- /dev/null
> > +++ b/testsuites/psxtests/psx17/psx17.scn
> > @@ -0,0 +1,82 @@
> > +*** BEGIN OF TEST PSX 17 ***
> > +
> > +1st iteration Begin (Clock Monotonic)
> > +Add called. Count is 1
> > +Minus called. Count is 0
> > +Return value from cond_clockwait is 0
> > +1st iteration END
> > +
> > +2nd iteration Begin (Clock Monotonic)
> > +Add called. Count is 1
> > +Minus called. Count is 0
> > +Return value from cond_clockwait is 116
> > +2nd iteration END
> > +
> > +3rd iteration Begin (Clock Realtime)
> > +Add called. Count is 1
> > +Minus called. Count is 0
> > +Return value from cond_clockwait is 0
> > +3rd iteration END
> > +
> > +4th iteration Begin (Clock Realtime)
> > +Add called. Count is 1
> > +Minus called. Count is 0
> > +Return value from cond_clockwait is 116
> > +4th iteration END
> > +
> > +5th iteration Begin (Unsupported Clock)
> > +Minus called. Count is -1
> > +Return value from cond_clockwait is 22
> > +Add called. Count is 0
> > +5th iteration END
> > +
> > +6th iteration Begin (Invalid Clock)
> > +Minus called. Count is -1
> > +Return value from cond_clockwait is 22
> > +Add called. Count is 0
> > +6th iteration END
> > +
> > +7th iteration Begin (Invalid Abstime)
> > +Minus called. Count is -1
> > +Return value from cond_clockwait is 22
> > +Add called. Count is 0
> > +7th iteration END
> > +
> > +8th iteration Begin (Invalid Cond)
> > +Minus called. Count is -1
> > +Return value from cond_clockwait is 22
> > +Add called. Count is 0
> > +8th iteration END
> > +
> > +9th iteration Begin (Invalid Mutex)
> > +Minus called. Count is -1
> > +Return value from cond_clockwait is 1
> > +Add called. Count is 0
> > +9th iteration END
> > +
> > +10th iteration Begin (Uninitialized Condition Variable)
> > +Add called. Count is 1
> > +Minus called. Count is 0
> > +Return value from cond_clockwait is 116
> > +10th iteration END
> > +
> > +11th iteration Begin (Uninitialized Mutex)
> > +Minus called. Count is -1
> > +Return value from cond_clockwait is 116
> > +Add called. Count is 0
> > +11th iteration END
> > +
> > +10th iteration Begin (Uninitialized Condition Variable)
> > +Add called. Count is 1
> > +Minus called. Count is 0
> > +Return value from cond_clockwait is 116
> > +10th iteration END
> > +
> > +12th iteration Begin (New Mutex)
> > +Add called. Count is 1
> > +Minus called. Count is 0
> > +Return value from cond_clockwait is 0
> > +12th iteration END
> > +
> > +
> > +*** END OF TEST PSX 17 ***
> > diff --git a/testsuites/psxtests/psx17/system.h b/testsuites/psxtests/psx17/system.h
> > new file mode 100644
> > index 0000000000..7da50cca7f
> > --- /dev/null
> > +++ b/testsuites/psxtests/psx17/system.h
> > @@ -0,0 +1,57 @@
> > +/*
> > +* Copyright (C) 2021 Matthew Joyce
> > +*
> > +* Redistribution and use in source and binary forms, with or without
> > +* modification, are permitted provided that the following conditions
> > +* are met:
> > +* 1. Redistributions of source code must retain the above copyright
> > +* notice, this list of conditions and the following disclaimer.
> > +* 2. Redistributions in binary form must reproduce the above copyright
> > +* notice, this list of conditions and the following disclaimer in the
> > +* documentation and/or other materials provided with the distribution.
> > +*
> > +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> > +* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > +* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> > +* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> > +* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > +* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > +* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > +* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > +* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > +* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> > +* POSSIBILITY OF SUCH DAMAGE.
> > +*/
> > +
> > +/* functions */
> > +
> > +#include <pmacros.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +
> > +void *POSIX_Init(
> > + void *argument
> > +);
> > +
> > +void *thread_func(
> > + void *argument
> > +);
> > +
> > +void *thread_func2(
> > + void *argument
> > +);
> > +
> > +/* configuration information */
> > +
> > +#define CONFIGURE_APPLICATION_NEEDS_SIMPLE_CONSOLE_DRIVER
> > +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER
> > +
> > +#define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
> > +
> > +#define CONFIGURE_MAXIMUM_POSIX_THREADS 4
> > +
> > +#define CONFIGURE_POSIX_INIT_THREAD_TABLE
> > +
> > +#include <rtems/confdefs.h>
> > +
> > +/* end of include file */
> > diff --git a/testsuites/psxtests/psxhdrs/pthread/pthread_cond_clockwait.c b/testsuites/psxtests/psxhdrs/pthread/pthread_cond_clockwait.c
> > index 15485eb587..fe91b7954f 100644
> > --- a/testsuites/psxtests/psxhdrs/pthread/pthread_cond_clockwait.c
> > +++ b/testsuites/psxtests/psxhdrs/pthread/pthread_cond_clockwait.c
> > @@ -49,7 +49,7 @@ int test( void )
> > pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> > clockid_t clock_id = CLOCK_REALTIME;
> > struct timespec abstime;
> > - abstime.tv_sec = 2;
> > + abstime.tv_sec = 1;
>
> Why this change?
No good reason, sorry. I was just experimenting with the API after I
had already submitted the compile tests in the previous patch.
>
> > int result;
> >
> > /* This method appeared in the Issue 8 POSIX Standard */
> > --
> > 2.31.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list