Annotate ThreadData::PushToHeadOfList for MemorySanitizer.

Annotate ThreadData::PushToHeadOfList for MemorySanitizer.

BUG=178409
R=mark@chromium.org,darin@chromium.org,glider@chromium.org


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

[Alexander Potapenko, 2013-03-20 11:19:43]

https://codereview.chromium.org/12951003/diff/1/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/12951003/diff/1/base/compiler_specific.h#newc...
base/compiler_specific.h:169: #ifdef MEMORY_SANITIZER
Please add comments describing what shall into this section of the header, what
is shadow, and what are the methods below used for.

https://codereview.chromium.org/12951003/diff/1/base/tracked_objects.cc
File base/tracked_objects.cc (right):

https://codereview.chromium.org/12951003/diff/1/base/tracked_objects.cc#newco...
base/tracked_objects.cc:262: __msan_unpoison(&random_number_,
sizeof(random_number_));
As I've said before, it isn't obvious that __msan_unpoison() is a no-op if
MEMORY_SANITIZER is not defined.

I suggest to:
 - remove the leading underscores, since they denote that the identifier is
reserved by the compiler (this is ok under '#ifdef MEMORY_SANITIZER', but not in
the regular mode)
 - use uppercase only (to show this is a macro, not a function call)

[eugenis, 2013-03-20 11:32:27]

On 2013/03/20 11:19:43, Alexander Potapenko wrote:
> https://codereview.chromium.org/12951003/diff/1/base/compiler_specific.h
> File base/compiler_specific.h (right):
> 
>
https://codereview.chromium.org/12951003/diff/1/base/compiler_specific.h#newc...
> base/compiler_specific.h:169: #ifdef MEMORY_SANITIZER
> Please add comments describing what shall into this section of the header,
what
> is shadow, and what are the methods below used for.
> 
> https://codereview.chromium.org/12951003/diff/1/base/tracked_objects.cc
> File base/tracked_objects.cc (right):
> 
>
https://codereview.chromium.org/12951003/diff/1/base/tracked_objects.cc#newco...
> base/tracked_objects.cc:262: __msan_unpoison(&random_number_,
> sizeof(random_number_));
> As I've said before, it isn't obvious that __msan_unpoison() is a no-op if
> MEMORY_SANITIZER is not defined.
> 
> I suggest to:
>  - remove the leading underscores, since they denote that the identifier is
> reserved by the compiler (this is ok under '#ifdef MEMORY_SANITIZER', but not
in
> the regular mode)
>  - use uppercase only (to show this is a macro, not a function call)

Done, PTAL.

[eugenis, 2013-03-20 11:43:07]

On 2013/03/20 11:32:27, eugenis wrote:
> On 2013/03/20 11:19:43, Alexander Potapenko wrote:
> > https://codereview.chromium.org/12951003/diff/1/base/compiler_specific.h
> > File base/compiler_specific.h (right):
> > 
> >
>
https://codereview.chromium.org/12951003/diff/1/base/compiler_specific.h#newc...
> > base/compiler_specific.h:169: #ifdef MEMORY_SANITIZER
> > Please add comments describing what shall into this section of the header,
> what
> > is shadow, and what are the methods below used for.
> > 
> > https://codereview.chromium.org/12951003/diff/1/base/tracked_objects.cc
> > File base/tracked_objects.cc (right):
> > 
> >
>
https://codereview.chromium.org/12951003/diff/1/base/tracked_objects.cc#newco...
> > base/tracked_objects.cc:262: __msan_unpoison(&random_number_,
> > sizeof(random_number_));
> > As I've said before, it isn't obvious that __msan_unpoison() is a no-op if
> > MEMORY_SANITIZER is not defined.
> > 
> > I suggest to:
> >  - remove the leading underscores, since they denote that the identifier is
> > reserved by the compiler (this is ok under '#ifdef MEMORY_SANITIZER', but
not
> in
> > the regular mode)
> >  - use uppercase only (to show this is a macro, not a function call)
> 
> Done, PTAL.

Removed __msan_print_shadow, as it is not used yet.

[Alexander Potapenko, 2013-03-20 11:43:08]

https://codereview.chromium.org/12951003/diff/6001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/12951003/diff/6001/base/compiler_specific.h#n...
base/compiler_specific.h:174: void __msan_print_shadow(const void *p, unsigned
long s);
Please remove __msan_print_shadow since it's unlikely to be used in Chrome

[Alexander Potapenko, 2013-03-20 12:47:56]

LGTM, but let's see what others say.

[Mark Mentovai, 2013-03-20 13:34:58]

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:173: void __msan_unpoison(const void *p, unsigned long
s);
No indentation within extern "C". We treat it the same as a namespace.

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:174: }
}  // extern "C"

(again, like a namespace)

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:177: // Use this to annotate the code that deliberately
reads uninitialized data,
Remove the word “the.”

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:178: // for example a GC scavenging root set pointers
from stack.
And here, “stack” needs “the.”

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:180: #else // MEMORY_SANITIZER
At least two spaces between the last non-whitespace thing on a line and the
opening // of a line comment. Same on line 182.

[eugenis, 2013-03-20 13:40:29]

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:173: void __msan_unpoison(const void *p, unsigned long
s);
On 2013/03/20 13:34:58, Mark Mentovai wrote:
> No indentation within extern "C". We treat it the same as a namespace.

Done.

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:174: }
On 2013/03/20 13:34:58, Mark Mentovai wrote:
> }  // extern "C"
> 
> (again, like a namespace)

Done.

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:177: // Use this to annotate the code that deliberately
reads uninitialized data,
On 2013/03/20 13:34:58, Mark Mentovai wrote:
> Remove the word “the.”

Done.

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:178: // for example a GC scavenging root set pointers
from stack.
On 2013/03/20 13:34:58, Mark Mentovai wrote:
> And here, “stack” needs “the.”

Done.

https://codereview.chromium.org/12951003/diff/8001/base/compiler_specific.h#n...
base/compiler_specific.h:180: #else // MEMORY_SANITIZER
On 2013/03/20 13:34:58, Mark Mentovai wrote:
> At least two spaces between the last non-whitespace thing on a line and the
> opening // of a line comment. Same on line 182.

Done.

[Mark Mentovai, 2013-03-20 13:41:02]

LGTM

[eugenis, 2013-03-20 13:41:31]

Thanks!

[commit-bot: I haz the power, 2013-03-20 13:41:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eugenis@chromium.org/12951003/11001

[commit-bot: I haz the power, 2013-03-20 14:09:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eugenis@chromium.org/12951003/11001

[commit-bot: I haz the power, 2013-03-20 21:18:25]

Change committed as 189406