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

[Evgeniy Stepanov]

On Tue, Feb 5, 2013 at 10:37 AM, Alexey Samsonov <samsonov at google.com> wrote:
>
> 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))?

We still want to do best-effort shadow propagation in such functions,
just omit the checks.

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