Atomics implementation for compiler-based ThreadSanitizer (http://dev.chromium.org/developers/testi…

Atomics implementation for compiler-based ThreadSanitizer (http://dev.chromium.org/developers/testing/threadsanitizer-tsan-v2)

BUG=128314
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=159705

[Alexander Potapenko, 2012-09-19 11:33:20]

Lei, Brett, can you please take a look?
atomicops_internals_tsan.h is almost exact copy of the internal TSan atomics
implementation made by Dmitry (dvyukov@, CCed). It's intended to route all the
atomics to the TSan runtime to avoid false positives.

[Dmitry Vyukov, 2012-09-19 17:04:29]

LGTM

https://codereview.chromium.org/10948035/diff/4001/base/atomicops_internals_t...
File base/atomicops_internals_tsan.h (right):

https://codereview.chromium.org/10948035/diff/4001/base/atomicops_internals_t...
base/atomicops_internals_tsan.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
2012?

[brettw, 2012-09-19 20:55:38]

owners lgtm, I didn't check the code

[Lei Zhang, 2012-09-19 21:46:19]

lgtm

Please fix the lint errors.

https://codereview.chromium.org/10948035/diff/4001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/10948035/diff/4001/base/base.gypi#newcode58
base/base.gypi:58: 'atomicops_internals_tsan.h',
nit: alphabetical order

[Alexander Potapenko, 2012-09-26 09:33:45]

Sorry about the delay, was trying to figure out why these lint errors are
different from those I get locally.

I've fixed all the lint errors except the following ones:

(local and web)
base/atomicops_internals_tsan.h:116:  Declaration has space between
type name and * in __tsan_atomic8 *c  [whitespace/declaration] [3]
-- this is strange. IIUC there's no requirement about whitespaces between type
name and * other than that those should be consistent across the file (mine
are).

(local)
base/atomicops.h:137:  Extra space before ( in function call
[whitespace/parens] [4]
-- this is a whitespace between elif and (. I don't think I should remove it (I
can fix cpplint however).

(web)
https://codereview.chromium.org/lint_patch/issue10948035_10001_11001
Include "base/atomicops_internals_tsan.h" not in alphabetical order
-- I believe they should not be. The include order is dictated by the #ifdef
logic here.

Also, the lint errors from a local run are different from those on the web. I've
emailed erg@ about that.

https://codereview.chromium.org/10948035/diff/4001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/10948035/diff/4001/base/base.gypi#newcode58
base/base.gypi:58: 'atomicops_internals_tsan.h',
On 2012/09/19 21:46:19, Lei Zhang wrote:
> nit: alphabetical order

Done.

[Alexander Potapenko, 2012-09-27 11:25:26]

https://codereview.chromium.org/10948035/diff/4001/base/atomicops_internals_t...
File base/atomicops_internals_tsan.h (right):

https://codereview.chromium.org/10948035/diff/4001/base/atomicops_internals_t...
base/atomicops_internals_tsan.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2012/09/19 17:04:29, Dmitry Vyukov wrote:
> 2012?

Done.

[Alexander Potapenko, 2012-09-28 09:47:48]

Lei, what do you think about the remaining lint errors?

[Lei Zhang, 2012-09-28 18:46:42]

I think it's ok.

[Nico, 2014-06-17 19:37:10]

https://codereview.chromium.org/10948035/diff/17001/base/atomicops_internals_...
File base/atomicops_internals_tsan.h (right):

https://codereview.chromium.org/10948035/diff/17001/base/atomicops_internals_...
base/atomicops_internals_tsan.h:24: AtomicOps_Internalx86CPUFeatures;
Do you need this struct at all for the tsan-based implementation? I don't see
any references to this below.

[Dmitry Vyukov, 2014-06-18 03:42:36]

On 2014/06/17 19:37:10, Nico (away) wrote:
>
https://codereview.chromium.org/10948035/diff/17001/base/atomicops_internals_...
> File base/atomicops_internals_tsan.h (right):
> 
>
https://codereview.chromium.org/10948035/diff/17001/base/atomicops_internals_...
> base/atomicops_internals_tsan.h:24: AtomicOps_Internalx86CPUFeatures;
> Do you need this struct at all for the tsan-based implementation? I don't see
> any references to this below.

Most likely we don't need it in chromium. I've added it in a different codebase,
because there was code that accesses this data structure directly (breaking
abstraction boundaries). That was the only reason.

[Alexander Potapenko, 2014-06-18 09:46:21]

Nico, I've sent you https://codereview.chromium.org/339343002 that removes this
structure.