Why is perl_RnW1_mutex_t.readers_count unsigned?

We have a relatively new struct in perl.h that looks like this:

typedef struct {
  perl_mutex lock;
  perl_cond wakeup;
  Size_t readers_count;
} perl_RnW1_mutex_t;

Meanwhile, over in thread.h, we have a number of macros that do things
like the following to make sure the unsigned readers_count does not go
negative:

            if ((mutex)->readers_count <= 0) {                      \
                assert((mutex)->readers_count == 0);                \
                (mutex)->readers_count = 0;                         \
                break;                                              \
            }                                                       \

That seems pretty pointless to me since an unsigned value can't go
negative. Should we make readers_count SSize_t rather than Size_t, or
should we get rid of the check for negative values?

I ask because the compiler I'm using complains as follows in a couple
dozen places.

    GETENV_UNLOCK;
....^
%CC-I-QUESTCOMPARE, In this statement, the unsigned expression
"(&PL_env_mutex)->readers_count" is being compared with a relational
operator to a constant whose value is not greater than zero.  This
might not be what you intended.
at line number 2802 in file SMOKE_BUILD_ROOT:[blead]inline.h;1

This recently broke the build until I relaxed Time::HiRes's
configuration code and still causes at least one test failure
(porting/extrefs.t).  If the intent was to limit the number of readers
on a mutex, there must be some more reasonable way to specify the
range than depending on integer wraparound to let us know there are
too many. I think we would run into resource exhaustion long before
getting anywhere near INT_MAX readers.
0
craig
12/30/2020 9:55:56 PM
perl.perl5.porters 48290 articles. 1 followers. Follow

2 Replies
15 Views

Similar Articles

[PageSpeed] 36

On 12/30/20 2:55 PM, Craig A. Berry wrote:
> We have a relatively new struct in perl.h that looks like this:
> 
> typedef struct {
>    perl_mutex lock;
>    perl_cond wakeup;
>    Size_t readers_count;
> } perl_RnW1_mutex_t;
> 
> Meanwhile, over in thread.h, we have a number of macros that do things
> like the following to make sure the unsigned readers_count does not go
> negative:
> 
>              if ((mutex)->readers_count <= 0) {                      \
>                  assert((mutex)->readers_count == 0);                \
>                  (mutex)->readers_count = 0;                         \
>                  break;                                              \
>              }                                                       \
> 
> That seems pretty pointless to me since an unsigned value can't go
> negative. Should we make readers_count SSize_t rather than Size_t, or
> should we get rid of the check for negative values?
> 
> I ask because the compiler I'm using complains as follows in a couple
> dozen places.
> 
>      GETENV_UNLOCK;
> ...^
> %CC-I-QUESTCOMPARE, In this statement, the unsigned expression
> "(&PL_env_mutex)->readers_count" is being compared with a relational
> operator to a constant whose value is not greater than zero.  This
> might not be what you intended.
> at line number 2802 in file SMOKE_BUILD_ROOT:[blead]inline.h;1
> 
> This recently broke the build until I relaxed Time::HiRes's
> configuration code and still causes at least one test failure
> (porting/extrefs.t).  If the intent was to limit the number of readers
> on a mutex, there must be some more reasonable way to specify the
> range than depending on integer wraparound to let us know there are
> too many. I think we would run into resource exhaustion long before
> getting anywhere near INT_MAX readers.
> 

Like most things, it was not thinking it through.  Changed by 
41b10d901d383944620dc9fe58a82531022cc937
0
public
12/31/2020 4:13:21 PM
On Thu, Dec 31, 2020 at 10:13 AM Karl Williamson
<public@khwilliamson.com> wrote:
>
> On 12/30/20 2:55 PM, Craig A. Berry wrote:
> > Should we make readers_count SSize_t rather than Size_t, or
> > should we get rid of the check for negative values?

> Like most things, it was not thinking it through.  Changed by
> 41b10d901d383944620dc9fe58a82531022cc937

Thanks very much.  That helps a lot.
0
craig
1/1/2021 10:36:09 PM
Reply: