This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH][RFC] Remove volatile from data members in libstdc++


> From: Richard Guenther [mailto:rguenther@suse.de]
> 
> I'm sure it doesn't.  As long as you use proper atomic 
> increment/decrement operations (proper as in let the compiler know the

> value is clobbered and let the CPU carry out the operation 
> atomically), no spilling or caching or whatever else optimization can 
> violate the invariants you set for reference counting, which are 
> (assuming zero is the unused case)
> 
>  - if the value (somehow) reads non-zero, any other means of reading
>    the value will produce non-zero unless this thread decrements the
>    value
>  - if the value (somehow) reads zero, this thread had better just
>    decremented the value, and any other further means of reading the
>    value will produce zero, too.
> 
> you don't need volatile to ensure proper atomic counter behavior, you 
> don't need an "atomic read" thing, you only need atomic 
> increment/decrement.
> 
> Richard.
> 
If this were a simple reference counting implementation where the only
accesses are through atomics, you might be right.  (Though I suspect it
depends on how you test for zero counts.) The problem is that rope

a) Should/will use atomic increments and decrements for updating, and I
think zero tests for deallocation.
b) Sometimes asynchronously reads the reference count to see whether a
particular representation is unshared.  If so some update operations can
be performed in-place instead of by copying pieces of the rope.

The accesses in (b) are intentional data races.  The value may change
while it's being read.  But if it was ever one, it is presumed to stay
one.  (Violating that would require updates to the same logical rope,
which is against the rules.)  But it can certainly change from 2 to 1
while it's being read.  Thus in places where the rope implementation
does a copy to a local:
	
	size_t __count = __r->_M_ref_count;

(as it in fact does do), count may subsequently change from 2 to 1 if
the compiler decides to reload it from the underlying global.  This is
certainly unexpected by the code.  In this particular case, it doesn't
look to me like it introduces a bug.  But we are violating an assumption
that the compiler is entitled to make, namely that __r->_M_ref_count
doesn't change.  Hence there may be other cleverer optimizations that
the backend is performing (or will perform in the future) that will
break the code.  I certainly don't have enough visibility into current
and future gcc optimizations to have any confidence that this is safe.

Here is another kind of transformation that the compiler is allowed to
perform on non-volatile variables:

Assume rc is non-volatile, and we have

switch(...) {
  case ...:
	...; rc = 1; break;
  case ...:
	...; rc = 1; break;
  case foo:
	...; rc = 2; break;
  case ...:
	...; rc = 1; break;
  case ...:
	...; rc = 1; break;
  default:
	...; rc = 1; break;
}

Where the ellipses are known to not include any synchronizatiobn calls,
and do not touch rc.

This can safely (even under the proposed C++ memory model) be
transformed to:

rc = 1;
switch(...) {
  case ...:
	...; break;
  case ...:
	...; break;
  case foo:
	...; rc = 2; break;
  case ...:
	...; break;
  case ...:
	...; break;
  default:
	...; break;
}

which clearly reduces code size.  (This is safe under the new C++ memory
model since rc is touched on all paths.  Correct multithreaded code
can't tell the difference without introducing a data race, which would
in turn introduce undefined behavior.)

Again this kind of transformation could be unsafe if rc where a rope
reference count.  I don't think there is a switch statement like this in
the rope implementation.  But I really don't think we want to reason
about all possible future gcc transformations and all possible future
changes to the rope package in this way.

Long term, letting rope reference counts be accessed with ordinary field
references is clearly broken.

Hans


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]