Replace TSan API declarations in atomicops_internals_tsan.h with a header inclusion

[GoogleCodeExporter]

The attached patch replaces the TSan API declarations with an inclusion of the 
<sanitizer/tsan_interface_atomic.h> header provided by Clang.
This fixes the issues caused by including several atomicops implementations 
from different projects (Chromium, v8, protobuf) and is in fact a backport of a 
Chromium change https://codereview.chromium.org/169463003/

Original issue reported on code.google.com by gli...@google.com on 18 Feb 2014 at 1:06

Attachments:

• protobuf.patch

[GoogleCodeExporter]

Where does this header file come from? Does it mean that users will need to 
install these header files before they can compile protobuf (while previously 
they don't need to)?

Original comment by xiaof...@google.com on 18 Feb 2014 at 6:12

[GoogleCodeExporter]

This header comes from the compiler that supports ThreadSanitizer (that's Clang 
3.3+ or GCC 4.7+). The corresponding functions (which definitions are being 
replaced in this CL) are only provided by these compilers and are useless 
otherwise.

Original comment by gli...@chromium.org on 19 Feb 2014 at 9:03

[GoogleCodeExporter]

Ping?

Original comment by gli...@chromium.org on 24 Mar 2014 at 11:16

[GoogleCodeExporter]

Committed as r521

Original comment by xiaof...@google.com on 24 Mar 2014 at 6:41

• Changed state: Fixed

[GoogleCodeExporter]

Can you revert this patch please? I don't have the header file 
sanitizer/tsan_interface_atomic.h on my Ubuntu 12.04 LTS nor my Ubuntu 14.04 
LTS installs. I installed clang from Launchpad PPA for 12.04 whereas it's from 
the main repos for 14.04, they obviously don't ship this header on Ubuntu 
clangs.

Niall

Original comment by nialldouglas14 on 7 Jul 2014 at 2:06

[GoogleCodeExporter]

Hi Niall,
Do you mean the Clang version you installed supports ThreadSanitizer but 
without header file sanitizer/tsan_interface_atomic.h? You shouldn't need this 
header file unless you compile protobuf with ThreadSanitizer enabled.

Original comment by xiaof...@google.com on 7 Jul 2014 at 5:41

[GoogleCodeExporter]

Correct, Ubuntu's clang supports ThreadSanitiser but without the header file 
mentioned. You can see the CI failure when latest protobuf was merged at 
https://ci.nedprod.com/job/Maidsafe%20Sanitise%20Linux64%20clang%203.4/45/. I 
have since reverted this patch in our copy of protobuf, all unit tests are back 
to passing.

That particular CI slave is running Ubuntu 14.04 LTS. Stock clang from standard 
repos.

Niall

Original comment by nialldouglas14 on 7 Jul 2014 at 9:40

[GoogleCodeExporter]

Niall, which Clang version are you using? Clang 3.5 must have this header (in 
fact we've just checked that), Clang 3.4 indeed does not.

Feng, do you think we need some check for Clang version around this header?
Perhaps something along the lines of:
(__clang_major__ > 3) || (__clang_major__ == 3 && __clang_minor__ > 4)

Original comment by gli...@chromium.org on 13 Oct 2014 at 2:45

[GoogleCodeExporter]

I would assume at the time of report I would be running 3.4 as 3.5 wasn't out 
yet. Actually, until that change allowing multiple clang versions to coexist 
hits the main apt repos, I still can't install clang 3.5 :(

Niall

Original comment by nialldouglas14 on 13 Oct 2014 at 3:48

[GoogleCodeExporter]

Yes, I think we need to make the header work on Clang 3.4 as well. Something 
like:
#if (__clang_major__ > 3) || (__clang_major__ == 3 && __clang_minor__ > 4)
#include <sanitizer/tsan_interface_atomic.h>
#else
... atomicops declarations for 3.4 and older...
#endif

Could you help make this change? We moved to github and you can send us a pull 
request here: https://github.com/google/protobuf/pulls

Original comment by xiaof...@google.com on 13 Oct 2014 at 5:26

[GoogleCodeExporter]

I can verify someone else's work after they have tested it themselves. I don't 
believe I am authorised by my current client to do the work myself. Niall

Original comment by nialldouglas14 on 13 Oct 2014 at 7:49