llvm dev - Mar 2012 - [LLVMdev] recognizing DTORs and vptr updates in LLVM.

On Mon, Mar 19, 2012 at 4:30 PM, Chris Lattner <clattner at apple.com>
wrote:
>
> On Mar 19, 2012, at 2:52 PM, Kostya Serebryany wrote:
>
> Hello,
>
> While instrumenting LLVM IR in ThreadSanitizer (race detector), I need
> to distinguish between a store to vtable pointer (vptr) and any other
> regular store.
> This special treatment should be limited to class DTORs, so I should also
> know when a function is a DTOR.
> Rationale: need to distinguish benign and harmful races on vptr
>
(http://code.google.com/p/data-race-test/wiki/PopularDataRaces#Data_race_on_vptr).
>
> Currently, I can figure out when a function is a DTOR and when a store
> touches vptr by analyzing mangled names.
> _ZN1BD1Ev=="B::~B()"
> _ZTV1B=="vtable for B"
>
> define linkonce_odr void @_ZN1BD1Ev(%struct.B* %this) unnamed_addr nounwind
> uwtable align 2 {
> entry:
>   ....
>   store i32 (...)** bitcast (i8** getelementptr inbounds ([5 x i8*]*
> @_ZTV1B, i64 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 8
>
> However, this does not sound right.
> What would be the right way to pass this information from clang to LLVM?
> Will using metadata for this purpose be a right solution?
> (insn-level metadata for vptr store and module-level metadata for DTORs)
>
>
> Using instruction level metadata for this would be appropriate.  However, I
> also don't understand why a race on this is truly benign.
It isn't, really; calling it "benign" is deceptive.  It's just
that
storing a pointer which is equal to the existing pointer stored at a
given address almost always makes the optimizer/codegen generate code
which can't trigger the race in a way which visibly misbehaves.
Therefore, as a heuristic users apparently want ThreadSanitizer to
ignore (or list separately) such races.

Given that, I'm not sure I really see the issue with just
special-casing any store where the value stored is a pointer to a
global... but it could be argued either way, I guess.

-Eli

On Mon, Mar 19, 2012 at 4:46 PM, Eli Friedman <eli.friedman at
gmail.com>wrote:
> Given that, I'm not sure I really see the issue with just
> special-casing any store where the value stored is a pointer to a
> global... but it could be argued either way, I guess.
>
I users expect this to "just work", why not extend the language and
make it
just work?

We could, as an implementation, decide to emit these as relaxed atomic
stores, making the code well defined without changing the semantics (or
optimization) in any meaningful way, right?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120319/b0bc391b/attachment.html>

On Mon, Mar 19, 2012 at 4:46 PM, Eli Friedman <eli.friedman at
gmail.com>wrote:
> On Mon, Mar 19, 2012 at 4:30 PM, Chris Lattner <clattner at
apple.com> wrote:
> >
> > On Mar 19, 2012, at 2:52 PM, Kostya Serebryany wrote:
> >
> > Hello,
> >
> > While instrumenting LLVM IR in ThreadSanitizer (race detector), I need
> > to distinguish between a store to vtable pointer (vptr) and any other
> > regular store.
> > This special treatment should be limited to class DTORs, so I should
also
> > know when a function is a DTOR.
> > Rationale: need to distinguish benign and harmful races on vptr
> > (
>
http://code.google.com/p/data-race-test/wiki/PopularDataRaces#Data_race_on_vptr
> ).
> >
> > Currently, I can figure out when a function is a DTOR and when a store
> > touches vptr by analyzing mangled names.
> > _ZN1BD1Ev=="B::~B()"
> > _ZTV1B=="vtable for B"
> >
> > define linkonce_odr void @_ZN1BD1Ev(%struct.B* %this) unnamed_addr
> nounwind
> > uwtable align 2 {
> > entry:
> >   ....
> >   store i32 (...)** bitcast (i8** getelementptr inbounds ([5 x i8*]*
> > @_ZTV1B, i64 0, i64 2) to i32 (...)**), i32 (...)*** %0, align 8
> >
> > However, this does not sound right.
> > What would be the right way to pass this information from clang to
LLVM?
> > Will using metadata for this purpose be a right solution?
> > (insn-level metadata for vptr store and module-level metadata for
DTORs)
> >
> >
> > Using instruction level metadata for this would be appropriate.
>  However, I
> > also don't understand why a race on this is truly benign.
>
> It isn't, really; calling it "benign" is deceptive.

Well, yes. Generally, I agree with you here.
But then there are tsan users who have all that legacy code and want to
find races that will harm them for sure and don't want to see
"noise".
These vptr races are hard to suppress w/o risking to hide some other races.

>  It's just that
> storing a pointer which is equal to the existing pointer stored at a
> given address almost always makes the optimizer/codegen generate code
> which can't trigger the race in a way which visibly misbehaves.
> Therefore, as a heuristic users apparently want ThreadSanitizer to
> ignore (or list separately) such races.
>
Yep.

>
> Given that, I'm not sure I really see the issue with just
> special-casing any store where the value stored is a pointer to a
> global... but it could be argued either way, I guess.
>
That will hide too many real races, I afraid.
Including those "harmful" vptr races.

>  I'm also concerned that you're adding even more knobs to clang and
IR for
> special case situations.  How many more special cases like this are you
> going to require?


I don't remember more special cases off the top of my head. valgrind-based
variant has this special case and nothing else, I believe.
We've run our race detector unit tests (
http://code.google.com/p/data-race-test/source/browse/trunk/unittest/racecheck_unittest.cc
)
under the current LLVM-TSAN and this is the only thing we found so far.
But we did not run anything heavy under LLVM-TSAN yet, so something else
may be hiding from us.

--kcc
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120319/b53259f5/attachment.html>

On Mon, Mar 19, 2012 at 4:52 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> On Mon, Mar 19, 2012 at 4:46 PM, Eli Friedman <eli.friedman at
gmail.com>
> wrote:
>>
>> Given that, I'm not sure I really see the issue with just
>> special-casing any store where the value stored is a pointer to a
>> global... but it could be argued either way, I guess.
>
>
> I users expect this to "just work", why not extend the language
and make it
> just work?
I'm not sure anyone really expects this to "just work", just that
they
did it by accident.  Making cross-thread unsynchronized virtual calls
on an object which is being destroyed strikes me as a construct nobody
would intentionally write.
> We could, as an implementation, decide to emit these as relaxed atomic
> stores, making the code well defined without changing the semantics (or
> optimization) in any meaningful way, right?
Making all vptr loads and stores atomic would block some optimizations
(specifically, we can't perform certain optimizations involving
memcpy, and IIRC some optimizers have incomplete atomics handling).
Not sure if it would have much practical impact, though.

Specifically just making vptr stores in destructors "unordered", and
making unordered stores which don't change the stored value
effectively no-ops in the memory model, could work too; the potential
impact on optimization is much less, and I don't think the model
changes would lead to any optimizer changes.

-Eli

>> Using instruction level metadata for this would be appropriate. 
However, I
>> also don't understand why a race on this is truly benign.
>
> It isn't, really; calling it "benign" is deceptive.  It's
just that
> storing a pointer which is equal to the existing pointer stored at a
> given address almost always makes the optimizer/codegen generate code
> which can't trigger the race in a way which visibly misbehaves.
> Therefore, as a heuristic users apparently want ThreadSanitizer to
> ignore (or list separately) such races.
The gcc Ada front-end does this too, in quite a range of situations.  For
example multiple threads racily initialize a pointer variable, but they all
initialize to the same value.  The various valgrind based race detection
tools all complain about this, which makes them much less useful than they
might be for Ada.

Ciao, Duncan.

On Mar 20, 2012, at 12:51 AM, Duncan Sands wrote:
>>> Using instruction level metadata for this would be appropriate. 
However, I
>>> also don't understand why a race on this is truly benign.
>> 
>> It isn't, really; calling it "benign" is deceptive. 
It's just that
>> storing a pointer which is equal to the existing pointer stored at a
>> given address almost always makes the optimizer/codegen generate code
>> which can't trigger the race in a way which visibly misbehaves.
>> Therefore, as a heuristic users apparently want ThreadSanitizer to
>> ignore (or list separately) such races.
> 
> The gcc Ada front-end does this too, in quite a range of situations.  For
> example multiple threads racily initialize a pointer variable, but they all
> initialize to the same value.  The various valgrind based race detection
> tools all complain about this, which makes them much less useful than they
> might be for Ada.
FWIW, after thinking about this for awhile, I realize that we already have the
tools to handle this: TBAA.

It would be general goodness for clang to emit VTable loads and stores in their
with their own TBAA type class (one that does not even alias "char*").
This would give us improved code quality, is straight-forward to reason about,
is not "another knob" and would be a really easy for ASAN to use.

One issue is that TBAA is disabled in -O0 builds: I'd just make vtable TBAA
information be produced when the optimizer is enabled or if ASAN is enabled.

-Chris