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

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)


Thanks,

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

On Mon, Mar 19, 2012 at 2:52 PM, Kostya Serebryany <kcc at google.com>
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)
It's worth pointing out the according to the abstract LLVM IR model,
your "benign" races are in fact undefined behavior.  The only reason
it appears to work is that in practice non-atomic loads and stores
usually result in the same generated code as "relaxed" atomic loads
and stores.  If we are in fact supposed to guarantee some sort of
behavior here, we should generate an atomic store.  If we aren't, I'm
not sure why AddressSanitizer needs to distinguish between "usually
appears to work" and "almost always appears to work".

-Eli

On Mon, Mar 19, 2012 at 3:15 PM, Eli Friedman <eli.friedman at
gmail.com>wrote:
> On Mon, Mar 19, 2012 at 2:52 PM, Kostya Serebryany <kcc at
google.com> 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)
>
> It's worth pointing out the according to the abstract LLVM IR model,
> your "benign" races are in fact undefined behavior.

Oh yes. According to C++11 too, I believe.
But C++98 did not define threads, so we are in the grey area here.

> The only reason
> it appears to work is that in practice non-atomic loads and stores
> usually result in the same generated code as "relaxed" atomic
loads
> and stores.  If we are in fact supposed to guarantee some sort of
> behavior here, we should generate an atomic store.  If we aren't,
I'm
> not sure why AddressSanitizer

s/AddressSanitizer/ThreadSanitizer/

> needs to distinguish between "usually
> appears to work" and "almost always appears to work".
>
This is more like "almost always works in practice" and
"certainly broken".
We run ThreadSanitizer (the old valgrind-based one) on millions lines of
legacy code (not always our own code) and we see a lot of those
"benign"
vptr races.
Ignoring those (while still detecting really harmful ones) has been #1
feature request from our users until we implemented it in
valgrind-based ThreadSanitizer.

--kcc


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

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.  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?

-Chris

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

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: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.  I'm
also
> concerned that you're adding even more knobs to clang and IR for
special
> case situations.
>
As for "more knobs", Chandler mentioned to me recently that being able
to
identify vptr accesses will help
virtual function call inlining, so we may still need this knob for other
purposes.

--kcc

>  How many more special cases like this are you going to require?
>
> -Chris
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120319/e3049778/attachment.html>