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

Chris Lattner clattner at apple.com
Fri Mar 16 16:58:49 PDT 2012


On Mar 15, 2012, at 2:19 PM, Kostya Serebryany wrote:
>> Clang already has the attribute "__attribute__((no_thread_safety_analysis))" which is used to disable static thread-safety analysis. 
>> http://clang.llvm.org/docs/LanguageExtensions.html#threadsafety
>> In a separate change we will teach clang to unset ThreadSafety when no_thread_safety_analysis is present. 
>> (exactly as we do for no_address_safety_analysis and AddressSafety)
> 
> Hi Kostya,
> 
> I'm skeptical of adding LLVM IR function attributes for each instrumentation pass.  Have you investigated using module flags metadata to track this information?  
> 
> I didn't, thanks for the link. 
> It looks like this would work too, although an attribute still sounds more compelling to me.
> It's just one bit (so it is free, as long as you have spare bits).
> Do you dislike it because there are just too few Attribute bits available (31 left), or for other reasons? 

I dislike it, because as a formal part of the IR, it should be standardized, documented, and well understood.  For example, how does inlining, code specialization, dead argument elimination, cloning, etc affect the attribute?  This isn't specified.

Also, it's not really free.  "Just one more bit" keeps adding up to a lot of clutter over time.

> My (perhaps incorrect) understanding of the metadata system is that if the metadata is removed, the correctness of the code is not affected.
> In this case, loosing metadata will mean introducing incorrect instrumentation.  

This is the case for instruction-level metadata, but module flags doesn't necessarily follow those rules.  Attributes aren't necessarily preserved by inter-procedural passes either.

> 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! :)

> 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.

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.  

What happens if an "asan" function is inlined into a non-asan function?

-Chris


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120316/c9639ebe/attachment.html>


More information about the llvm-commits mailing list