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

[Kostya Serebryany]

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/20120321/d2324c29/attachment.html>