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

[Kostya Serebryany]

[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/20120503/365c0ba8/attachment.html>