On 10/28/2012 17:44, Max TenEyck Woodbury wrote:
On 10/28/2012 02:40 AM, Nikolay Sivov wrote:
On 10/28/2012 04:59, m...@mtew.isa-geek.net wrote:
From: Max TenEyck Woodbury <m...@mtew.isa-geek.net>
I have been looking at the Microsoft COM and related documentations
and noticed that they emphatically recommend using the Interlocked...
functions when manipulating reference counts. I managed to set up a
search that showed where many of the reference count updates occur and
was somewhat surprised at how often this advice was not followed.
It doesn't mean it always has to be followed.
True in a limited sense, but there is a good reason behind their
recommendation. Unless there is a good reason not to do so, this
particular piece of advice should be followed.
COM objects in wine follow this recommendation in general, even object
itself is not thread safe.
This doesn't mean however that you need this every time you have some
kind of refcount of any sort.
While I have not converted every reference count update to use the
Interlocked... functions, this set of patches fixes a fair number of
them.
These are not associated with any particular bug report; they are simply
a general precaution against operations that are known to be associated
with race conditions.
This precaution doesn't work in general. It's not enough to atomically
update refcount to make an implementation thread safe. Also not everything
is supposed to be thread safe in a first place.
First, explain what does not have to be thread safe.
Anything really, COM objects in particular if you were talking about them.
I believe that
application may try to use multiple threads anywhere, so everything
that can be made thread safe, should be.
No.
Using interlocks on the
reference count updates is a necessary step for thread safety. You
are correct that it is not sufficient, but it is necessary.
Again, it depends.
Changes like this:
- for (i=0;i<howmuch;i++)
+ for (i=0;i<howmuch;++i)
TRACE("notify at %d to %p\n",
notify[i].dwOffset,notify[i].hEventNotify);
are not helpful at all.
The post increment and decrement operation are specified as saving
the original value for use in the evaluation of the expression they
are part of and modifying the underlying stored value. In expressions
like this, that saved value is then discarded. The optimization phase
of the compilation usually removes both the save and discard operations.
Sure, but I don't think it's enough to justify such changes all over the
place, in existing code.
Murphy's law suggests that this might cause problems at some point when
least desired.
Please don't do this.
Specifying the unnecessary use of a temporary store is a bad habit to
have. You should tell the compiler exactly what needs to be done, not
throw in extraneous operations.
So, the use of a post operator where a prefix operator is sufficient,
while almost certainly harmless, is still technically a mistake.
At the minimum, these changes provide examples of the correct way these
statements should be coded. So, I have to disagree, although very
mildly, that changes like that are not helpful. This kind of change
does not deserve a separate patch, but should be perfectly acceptable as
an adjunct to other patches.
