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

[Alexey Samsonov]

On Mon, Feb 4, 2013 at 6:00 PM, Kostya Serebryany <kcc at google.com> wrote:

> [resurrecting an old thread... again]
>
> Chris, Bill,
>
> Is the new Attribute implementation good enough to support two more
> function attributes?
> We need two attributes do enable/disable tsan and msan with function
> granularity.
> Just like with asan, where we have AddressSafety attribute mapped to
> __attribute__((no_address_safety_analysis)),
> I propose to add ThreadSafety attribute (will map to the existing
> __attribute__((no_thread_safety_analysis)))
> and UninitializedChecks attribute (will map to a new
> __attribute__((no_uninitialized_checks)))
>
(naming is hard, I'd love to hear better suggestions).
>

Why not __attribute__((no_uninitialized_memory_analysis))?


>
> Thoughts?
>
> --kcc
>
>
> 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
>>>>
>>>
>>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130205/1dedbb51/attachment.html>