[llvm-commits] Please review: adding Function attribute "ThreadSafety"

[Kostya Serebryany]

Any comments?

On Thu, May 3, 2012 at 10:51 AM, Kostya Serebryany <kcc at google.com> wrote:

> [resurrecting an old thread...]
>
> Chris,
>
> today we use a function attribute (AddressSafety) to mark a function for
> instrumentation with AddressSanitizer.
> We also need a similar thing for ThreadSanitizer.
> Currently these attributes are checked in the instrumentation passes
> (asan, tsan) and in GVN, where we need to disable {asan,tsan}-hostile
> transformation.
>
> You suggested to use module flags metadata instead of function
> attributes.
> Is this what you mean (named metadata node that contains a list of
> functions)?
>    !llvm.address_safety_analysis = !{!0, !1, !2}
>    !0 = metadata !{i32 (i16*)* @Foo}
>    !1 = metadata !{i32 (i8*)* @Bar}
>    !2 = metadata !{i32 (i32*)* @Buz}
>
> With this approach the attribute lookup will be a bit more expensive
> (either linear search, or metadata sort + bsearch, or an auxiliary hash
> table).
> Not a great deal though.
> One way is to add sort(), isSorted() and binarySearch() methods
> to NamedMDNode.
>
> Thanks,
>
> --kcc
>
>
>
>
>
>
> On Wed, Mar 21, 2012 at 10:59 PM, Kostya Serebryany <kcc at google.com>wrote:
>
>> I wonder if we can use a function-level metadata instead of module-level
>> metadata (and instead of function attributes).
>> (I don't see such, so we may need to add and document the notion
>> of function-level metadata, similar to module-level).
>> WDYT?
>>
>> OTOH, we may add one more attribute and reconsider the problem later.
>> Neither ASAN nor TSAN require backward compatibility with LLVM IR, so
>> these two LLVM function attributes (AddressSafety, ThreadSafety)
>> can be easily replaced with function-level metadata once we have it.
>>
>> --kcc
>>
>>
>>
>> On Fri, Mar 16, 2012 at 5:42 PM, Kostya Serebryany <kcc at google.com>wrote:
>>
>>>  Also, if I use metadata for tsan and an attribute for asan, it will
>>>> look weird.
>>>> Would you suggest to use metadata for asan as well (instead of
>>>> AddressSafety attribute)?
>>>>
>>>>
>>>> Yes please! :)
>>>>
>>>>
>>> Ok (once we find out what's the right way).
>>>
>>>
>>>>  The AddressSafety attribute is checked at least in one place outside
>>>> of asan code (lib/Analysis/MemoryDependenceAnalysis.cpp).
>>>> The ThreadSafety attribute (or equivalent metadata) will need to be
>>>> checked in the same place too:
>>>> struct X {
>>>>    uint64_t for_alignment;
>>>>    char a, b, c, d;
>>>> };
>>>>
>>>> Thread1(X *x) {
>>>>    ... = X.a + X.d
>>>> }
>>>>
>>>> Thread2(X *x) {
>>>>    X.b = ...
>>>> }
>>>>
>>>> If GVN widens X.a + X.d, tsan will start reporting a non-existing race.
>>>>
>>>>
>>>> I never agreed with this patch, but let it go in to avoid blocking
>>>> forward progress.
>>>>
>>>
>>> Thanks!
>>> I agree that the patch is not perfect (all problems that you mentioned,
>>> especially with inlining).
>>>
>>> <shameless plug>
>>> BTW, this particular patch unblocked the progress for mozilla (e.g.
>>> http://www.mozilla.org/security/announce/2012/mfsa2012-14.html)
>>> and clang+asan bootstrap (found ~5 clang/llvm bugs already, more are in
>>> my queue)
>>> </shameless plug>
>>>
>>>
>>>>
>>>> However, now that you ask :), GVN should never have checked for a
>>>> "asan" attribute.  If you want an attribute to disable specific
>>>> transformations, we should design this properly, and it should be a
>>>> property of the load or store, not a property of the function.
>>>>
>>>
>>> I don't see how to attach the property to loads/stores.
>>> If we use instruction-level metadata, it may be lost (as you just
>>> explained) above.
>>> If we add an instruction-level attribute (are there such?), what value
>>> should be used if some phase creates a new load/store?
>>> Again, what should be done when some code is copied (inlined, unrolled,
>>> etc)?
>>>
>>>
>>> --kcc
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120511/a2a8d039/attachment.html>