Race aggregation

[SumedhArani]

Race aggregation based on the memory operation.

Example for in the case of a data race happening at a specific location is shown but it may not show in the case of an array as different locations come in picture.

[simoatze]

@SumedhArani Great job! I'll take a look this week and I'll try to give it a try!
Also, let's move the conversation and issue discussions in here so I'll close or the issure in the archer repository. Thanks!

[simoatze]

also I think we should just keep the race-aggregation branch as a development branch, no need for PR now, once everything works than we can do a PR.

[dongahn]

@simoatze: typically having an experimental PR helps ppl to review code with diffing support, inline commenting etc. We won't certainly merge it right away. With this, could you reopen this?

[simoatze]

and if you can keep it updated with the llvm-mirror/compiler-rt. Basically we update both master and race-aggregation branches so they are always allined to the llvm-mirror.

[simoatze]

@dongahn Got it, it makes more sense :)

[SumedhArani]

@simoatze Thank you! :)
I'll move the conversation here from next time onwards and when necessary.
Take your own time to review it cause anyways I'm busy with my college internal exams this week.

@dongahn
I'll post in an experimental PR whenever required.

Thank you,
Sumedh.

[dongahn]

Nice work @SumedhArani!

A couple of thoughts. Is there a way in which we can actually intercept PrintReport. E.g., using LD_PRELOAD trick of some sort. If we can, perhaps we can keep this logic into Archer itself without having to modify the compiler runtime. This way, we may be able to make this logic work on any Clang/LLVM we install on a system?

Two things that caught my eyes.

1. There is a linear search in searching for previously seen races. This can become expensive if there is larger number of races. Hash container (e.g., map in C++ STL) is provided, this can be a better data structure to use which allow you to do this operation in O (1).

2. Doing a reduction on an absolute address will work for reducing races within a single process but won't work across difference processes of an MPI application. Even if a same variable is used, it can be resolved to different addresses across different MPI processes. Further, this certainly won't work on addresses from multiple programs.

Is a more relative concept like module+offset or source file+line number also available through ReportSpec? That might be a bit better criteria to use?

[SumedhArani]

@dongahn, I know that better algorithms could be put to use but I came out with a prototype. My first thoughts were to use a map but tsan has very less documentation and they have written their own data structures (Vector instead of vector) so I found it a lot buggy to get started itself and it shows up with error when I tried to include the STL containers.

So for now I've done a linear search with the vector data structure that they have provided.

One of the plus points for map are that it if targeted properly, we can even output which races were reduced on which factor.

As for point two, I did understand the inhibitions of this type of reduction.

Yeah it's possible to do source file + line number.

I posted this prototype to verify the design of my code with your vision. If so, I'll have the remaining two scenarios as told in the issue comments up and running with necessary tweaks and also optimisations where possible.

And I don't know about LD_PRELOAD! I need to Google it!

Thanks,
Sumedh.

[SumedhArani]

And also @dongahn, thanks for the appreciation!!:)

It strikes me now that I can probably make use of ADT plugins provided by the llvm framework and make use of a dense map. Let's see how does it come out.

I'll keep you updated and will try that denseMap after my today's exam gets over.

[dongahn]

@SumedhArani: did you look at LD_PRELOAD to see if we can replace PrintReport even without having to make any modifications to the compiler runtime source file? I don't recall all the details of LD_PRELOAD but I think it's worth looking into it.

[SumedhArani]

@dongahn , No. Sorry I was a bit busy with my internal exams happening in my college. I'll look at it and get back to you after three days ? Had a few personal commitments over this weekend.

I'll get back to you on Monday with the same.

Thanks,
Sumedh.

[dongahn]

perfectly fine! Take as much time as you need.

[SumedhArani]

Thanks! I'll surely look into it once I'm back to college!! Thanks for being so amiable!!:)

Cheers,
Sumedh

[simoatze]

@dongahn I was thinking that LD_PRELOAD wouldn't work because tsan is a static library. I think a solution would be to have PrintReport declared as weak symbol and then define a new PrintReport during the instrumentation pass inside the module where the main function is, like I did for the suppressions. I was talking to @SumedhArani and he'll looking into it.

[dongahn]

I see. Yeah it seems the weak symbol trick is the second best then.

To support existing clang/llvm installation, though, is there a way to change the attribute of and existing symbol to become weak? (binary patch...) Let me look.

If TSan accepts the small patch to change the attribute, this won't be neccessary, of course.

[simoatze]

Yeah after @SumedhArani does the feasibility study, if everything works I can talk to Dmitry and see if they would accept the patch.

[dongahn]

sounds good. In the meantime, objcopy has --weaken-symbol option which may be able to rewrite the attrubute.

https://linux.die.net/man/1/objcopy

If you nm on this TSan runtime library, does this PrintReport function at least shows up as an externally visible symbol though?

[simoatze]

Yes it shows up. For the test Sumedh can also just set the function as weak in the source.

[dongahn]

Yes I agree.

[SumedhArani]

After many long discussions with @simoatze as to how to go about to implement the above suggested technique, I feel that it is not so feasible to get the workaround using weak attributes.

Reasons as to why I feel so,

1. __tsan_on_report_(the function marked as a weak symbol) does have a pointer to an object of class DescRep which internally has many members of different objects.
2. Because of the above point, there will be a significant amount of dependent header files(in the tsan library) and their corresponding implementation to be imported (copied) into archer.
3. This so because, the tsan implementation involves their own data structures and have not made use of data structures available in STL library.
4. Even if decided to be implemented, we would have to update every now and then
5. Also we would have to copy all of PrintReport as there is a call to it before the __tsan_on_report is called. (__tsan_on_report and PrintReport are called for every race detected) As found out by @simoatze we can set the log path to /dev/null of PrintReport call instead of the stderr to nullify the PrintReport call that happens before __tsan_on_report
6. Hence, will it not become almost equivalent to shipping our own tsan library ? and defeat the purpose ?

Let me know as to what to proceed with next?

Thanks,
Sumedh.

[simoatze]

@dongahn I was suggesting @SumedhArani to import the entire "compiler-rt" as a submodule of archer, in this way we can just redefine the __tsan_on_report function and for compiling purposes we will have all the header file we need. The function __tsan_on_report has a (void *) ptr to the RepDesc structure that has all the info for the report reduction, in this way we can just do everything in our shared library and don't need for any edit on the compiler-rt. Does it make sense?

[dongahn]

Nice suggestion! Are the API exposed from compiler-rt pretty stable? Just make sure the new definition of __tsan_on_report is written to only stable APIs which don't change much between different versions of compiler-rt. This way, you don't want to suffer from slight API/ABI mismatches between the imported compiler-rt and the compiler-rt used in a installed version. Also if there are ways to do API "version" checks, make sure you folks use these mechanisms.