Make LoggingTest.CheckCausesDistinctBreakpoints work on clang win

Make LoggingTest.CheckCausesDistinctBreakpoints work on clang win

Clang coalesces the int3s of the simple implementation of CHECK as
__debugbreak(). To avoid this, add an additional null deref following
the __debugbreak() when __clang__ to uniquify crash locations.

R=thakis@chromium.org
BUG=693713
Review-Url: https://codereview.chromium.org/2701253005
Cr-Commit-Position: refs/heads/master@{#451660}
Committed: https://chromium.googlesource.com/chromium/src/+/92bbdc39154ce7cbbc939d9a4c5ee371691ab811

[scottmg, 2017-02-20 18:55:15]

Description was changed from

==========
Make LoggingTest.CheckCausesDistinctBreakpoints work on clang win

Clang coalesces the int3s of the simple implementation of CHECK as
__debugbreak(). To avoid this, add an additional null deref following
the __debugbreak() when __clang__ to uniquify crash locations.

R=thakis@chromium.org
BUG=693713
==========

to

==========
Make LoggingTest.CheckCausesDistinctBreakpoints work on clang win

Clang coalesces the int3s of the simple implementation of CHECK as
__debugbreak(). To avoid this, add an additional null deref following
the __debugbreak() when __clang__ to uniquify crash locations.

R=thakis@chromium.org
BUG=693713
==========

[scottmg, 2017-02-20 18:55:23]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[scottmg, 2017-02-20 18:55:27]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2017-02-20 18:55:50]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 18:56:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-02-20 19:04:21]

lgtm

Which intrinsics do you want us to add?

[scottmg, 2017-02-20 19:48:37]

On 2017/02/20 19:04:21, Nico wrote:
> lgtm
> 
> Which intrinsics do you want us to add?

All of them! :) Honestly, since it's so fiddly, it's hard to tell because it's
basically "try something, see what all the configs you care about generate,
iterate".

Reid said he already did __fastfail() so I can try that one after the next roll.

__inbyte sort of seems appealing because I think it'd be an invalid instruction
and hopefully short (but takes an argument, so unique). Maybe there's others
that meet that description we could try too, though I don't see any that only
take a unsigned char unfortunately.

__ud2() would also be good to have generally.

[scottmg, 2017-02-20 19:49:27]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2017-02-20 19:49:29]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2017-02-20 19:49:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 21:06:29]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487620169826960, "parent_rev":
"c9723e314368e3adc6d94822045b075d4329855c", "commit_rev":
"92bbdc39154ce7cbbc939d9a4c5ee371691ab811"}

[commit-bot: I haz the power, 2017-02-20 21:07:09]

Description was changed from

==========
Make LoggingTest.CheckCausesDistinctBreakpoints work on clang win

Clang coalesces the int3s of the simple implementation of CHECK as
__debugbreak(). To avoid this, add an additional null deref following
the __debugbreak() when __clang__ to uniquify crash locations.

R=thakis@chromium.org
BUG=693713
==========

to

==========
Make LoggingTest.CheckCausesDistinctBreakpoints work on clang win

Clang coalesces the int3s of the simple implementation of CHECK as
__debugbreak(). To avoid this, add an additional null deref following
the __debugbreak() when __clang__ to uniquify crash locations.

R=thakis@chromium.org
BUG=693713

Review-Url: https://codereview.chromium.org/2701253005
Cr-Commit-Position: refs/heads/master@{#451660}
Committed:
https://chromium.googlesource.com/chromium/src/+/92bbdc39154ce7cbbc939d9a4c5e...
==========

[commit-bot: I haz the power, 2017-02-20 21:07:10]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/92bbdc39154ce7cbbc939d9a4c5e...

[shinyak (Google), 2017-02-21 02:19:22]

Hi, this patch looks causing win-clang compile failure

E:\b\c\b\win_clang\src\third_party\WebKit\Source\core\style\BasicShapes.h(186,1):
 error: implicit conversion from 'int' to 'unsigned char' changes value from 256
to 0 [-Werror,-Wconstant-conversion]
DEFINE_BASICSHAPE_TYPE_CASTS(BasicShapeCircle);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E:\b\c\b\win_clang\src\third_party\WebKit\Source\core\style\BasicShapes.h(77,3):
 note: expanded from macro 'DEFINE_BASICSHAPE_TYPE_CASTS'
  DEFINE_TYPE_CASTS(thisType, BasicShape, value,                 \
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
E:\b\c\b\win_clang\src\third_party\WebKit\Source\wtf\Assertions.h(248,5):  note:
expanded from macro 'DEFINE_TYPE_CASTS'
    CHECK(referencePredicate);                                                \
    ^~~~~~~~~~~~~~~~~~~~~~~~~
E:\b\c\b\win_clang\src\base\logging.h(540,28):  note: expanded from macro
'CHECK'
  UNLIKELY(!(condition)) ? IMMEDIATE_CRASH() : EAT_STREAM_PARAMETERS
                           ^~~~~~~~~~~~~~~~~
E:\b\c\b\win_clang\src\base\logging.h(515,59):  note: expanded from macro
'IMMEDIATE_CRASH'
   (void)(*reinterpret_cast<volatile unsigned char*>(0) = __COUNTER__))
                                                        ~ ^~~~~~~~~~~
<scratch space>(52,1):  note: expanded from here
256
^~~

[shinyak, 2017-02-21 02:27:51]

Making a fix here
https://codereview.chromium.org/2706143002

[Primiano Tucci (use gerrit), 2017-02-21 11:02:06]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-02-21 11:02:07]

https://codereview.chromium.org/2701253005/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2701253005/diff/1/base/logging.h#newcode515
base/logging.h:515: (void)(*reinterpret_cast<volatile unsigned char*>(0) =
__COUNTER__))
I arrived late on this.  I had the same exact problem on Android.

Assuming that:
1. my https://codereview.chromium.org/2705053002 sticks
2. clang on windows supports statement expressions like it does on Linux

I think you can just reuse my 
#define TRAP_SEQUENCE()
 asm volatile("int3; ud2; push %0;" ::"i"(static_cast<unsigned
char>(__COUNTER__)))

Rationale:
1) It takes 4 bytes instead of 8
int3 is 1 byte long
ud2 is 1 byte long
push byte immediate: is 2 bytes long

while *nullptr = __COUNTER__ is 8 bytes long. From objdump:
   1:   c6 04 25 00 00 00 00    movb   $0x0,0x0
   8:   00

2) We can reduce the number of different black magics on logging.h. My
TRAP_SRQUENCE there is just few lines above. I think you can just nicely
re-route the #ifdefs there.

[scottmg, 2017-02-21 16:42:10]

On 2017/02/21 11:02:07, Primiano Tucci wrote:
> https://codereview.chromium.org/2701253005/diff/1/base/logging.h
> File base/logging.h (right):
> 
> https://codereview.chromium.org/2701253005/diff/1/base/logging.h#newcode515
> base/logging.h:515: (void)(*reinterpret_cast<volatile unsigned char*>(0) =
> __COUNTER__))
> I arrived late on this.  I had the same exact problem on Android.
> 
> Assuming that:
> 1. my https://codereview.chromium.org/2705053002 sticks
> 2. clang on windows supports statement expressions like it does on Linux
> 
> I think you can just reuse my 
> #define TRAP_SEQUENCE()
>  asm volatile("int3; ud2; push %0;" ::"i"(static_cast<unsigned
> char>(__COUNTER__)))
> 

Thanks. MSVC cl doesn't support inline asm, so clang-cl doesn't either, so we're
limited to C and intrinsics. I was hopeful that an `in` (via __inbyte()) would
be a relatively short way to crash, but it's good to have your code length as a
benchmark in any case.

> Rationale:
> 1) It takes 4 bytes instead of 8
> int3 is 1 byte long
> ud2 is 1 byte long
> push byte immediate: is 2 bytes long
> 
> while *nullptr = __COUNTER__ is 8 bytes long. From objdump:
>    1:   c6 04 25 00 00 00 00    movb   $0x0,0x0
>    8:   00
> 
> 2) We can reduce the number of different black magics on logging.h. My
> TRAP_SRQUENCE there is just few lines above. I think you can just nicely
> re-route the #ifdefs there.

[Nico, 2017-02-21 16:43:43]

On Tue, Feb 21, 2017 at 11:42 AM, <scottmg@chromium.org> wrote:

> On 2017/02/21 11:02:07, Primiano Tucci wrote:
> > https://codereview.chromium.org/2701253005/diff/1/base/logging.h
> > File base/logging.h (right):
> >
> > https://codereview.chromium.org/2701253005/diff/1/base/
> logging.h#newcode515
> > base/logging.h:515: (void)(*reinterpret_cast<volatile unsigned
> char*>(0) =
> > __COUNTER__))
> > I arrived late on this. I had the same exact problem on Android.
> >
> > Assuming that:
> > 1. my https://codereview.chromium.org/2705053002 sticks
> > 2. clang on windows supports statement expressions like it does on Linux
> >
> > I think you can just reuse my
> > #define TRAP_SEQUENCE()
> > asm volatile("int3; ud2; push %0;" ::"i"(static_cast<unsigned
> > char>(__COUNTER__)))
> >
>
> Thanks. MSVC cl doesn't support inline asm
>

It does, just using __asm {} instead of asm() (at least on 32-bit).


> , so clang-cl doesn't either
>

It does, even on 64-bit, but using __asm {} instead of asm() (not sure if
that helps here)


> , so we're
> limited to C and intrinsics. I was hopeful that an `in` (via __inbyte())
> would
> be a relatively short way to crash, but it's good to have your code length
> as a
> benchmark in any case.
>
> > Rationale:
> > 1) It takes 4 bytes instead of 8
> > int3 is 1 byte long
> > ud2 is 1 byte long
> > push byte immediate: is 2 bytes long
> >
> > while *nullptr = __COUNTER__ is 8 bytes long. From objdump:
> > 1: c6 04 25 00 00 00 00 movb $0x0,0x0
> > 8: 00
> >
> > 2) We can reduce the number of different black magics on logging.h. My
> > TRAP_SRQUENCE there is just few lines above. I think you can just nicely
> > re-route the #ifdefs there.
>
>
>
> https://codereview.chromium.org/2701253005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2017-02-21 17:13:41]

On 2017/02/21 16:43:43, Nico wrote:
> On Tue, Feb 21, 2017 at 11:42 AM, <mailto:scottmg@chromium.org> wrote:
> 
> > On 2017/02/21 11:02:07, Primiano Tucci wrote:
> > > https://codereview.chromium.org/2701253005/diff/1/base/logging.h
> > > File base/logging.h (right):
> > >
> > > https://codereview.chromium.org/2701253005/diff/1/base/
> > logging.h#newcode515
> > > base/logging.h:515: (void)(*reinterpret_cast<volatile unsigned
> > char*>(0) =
> > > __COUNTER__))
> > > I arrived late on this. I had the same exact problem on Android.
> > >
> > > Assuming that:
> > > 1. my https://codereview.chromium.org/2705053002 sticks
> > > 2. clang on windows supports statement expressions like it does on Linux
> > >
> > > I think you can just reuse my
> > > #define TRAP_SEQUENCE()
> > > asm volatile("int3; ud2; push %0;" ::"i"(static_cast<unsigned
> > > char>(__COUNTER__)))
> > >
> >
> > Thanks. MSVC cl doesn't support inline asm
> >
> 
> It does, just using __asm {} instead of asm() (at least on 32-bit).

Sorry, yeah, I meant x64 (we were discussing in email and I forgot that context
here).

> 
> 
> > , so clang-cl doesn't either
> >
> 
> It does, even on 64-bit, but using __asm {} instead of asm() (not sure if
> that helps here)

Huh, oops. I did try it, honest. The error looked like "i don't understand
__asm", but I must have screwed up in the logging macro expansions.

OK, so ... we can't collapse the implementations (because of asm"" vs. __asm{}),
and I don't think clang-cl enables statement expressions (?). So we'd end up
with the same number of #ifs I think.

But we can emit shorter code for clang-cl's arm of the #ifs. I can do that.

> 
> 
> > , so we're
> > limited to C and intrinsics. I was hopeful that an `in` (via __inbyte())
> > would
> > be a relatively short way to crash, but it's good to have your code length
> > as a
> > benchmark in any case.
> >
> > > Rationale:
> > > 1) It takes 4 bytes instead of 8
> > > int3 is 1 byte long
> > > ud2 is 1 byte long
> > > push byte immediate: is 2 bytes long
> > >
> > > while *nullptr = __COUNTER__ is 8 bytes long. From objdump:
> > > 1: c6 04 25 00 00 00 00 movb $0x0,0x0
> > > 8: 00
> > >
> > > 2) We can reduce the number of different black magics on logging.h. My
> > > TRAP_SRQUENCE there is just few lines above. I think you can just nicely
> > > re-route the #ifdefs there.
> >
> >
> >
> > https://codereview.chromium.org/2701253005/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2017-02-21 17:22:42]

On 2017/02/21 17:13:41, scottmg wrote:
> On 2017/02/21 16:43:43, Nico wrote:
> > On Tue, Feb 21, 2017 at 11:42 AM, <mailto:scottmg@chromium.org> wrote:
> > 
> > > On 2017/02/21 11:02:07, Primiano Tucci wrote:
> > > > https://codereview.chromium.org/2701253005/diff/1/base/logging.h
> > > > File base/logging.h (right):
> > > >
> > > > https://codereview.chromium.org/2701253005/diff/1/base/
> > > logging.h#newcode515
> > > > base/logging.h:515: (void)(*reinterpret_cast<volatile unsigned
> > > char*>(0) =
> > > > __COUNTER__))
> > > > I arrived late on this. I had the same exact problem on Android.
> > > >
> > > > Assuming that:
> > > > 1. my https://codereview.chromium.org/2705053002 sticks
> > > > 2. clang on windows supports statement expressions like it does on Linux
> > > >
> > > > I think you can just reuse my
> > > > #define TRAP_SEQUENCE()
> > > > asm volatile("int3; ud2; push %0;" ::"i"(static_cast<unsigned
> > > > char>(__COUNTER__)))
> > > >
> > >
> > > Thanks. MSVC cl doesn't support inline asm
> > >
> > 
> > It does, just using __asm {} instead of asm() (at least on 32-bit).
> 
> Sorry, yeah, I meant x64 (we were discussing in email and I forgot that
context
> here).
> 
> > 
> > 
> > > , so clang-cl doesn't either
> > >
> > 
> > It does, even on 64-bit, but using __asm {} instead of asm() (not sure if
> > that helps here)
> 
> Huh, oops. I did try it, honest. The error looked like "i don't understand
> __asm", but I must have screwed up in the logging macro expansions.

(I realized what I did wrong, I used ; to separate instructions, where the
required separator is "__asm" in macros. Doh!)

> 
> OK, so ... we can't collapse the implementations (because of asm"" vs.
__asm{}),
> and I don't think clang-cl enables statement expressions (?). So we'd end up
> with the same number of #ifs I think.
> 
> But we can emit shorter code for clang-cl's arm of the #ifs. I can do that.
> 
> > 
> > 
> > > , so we're
> > > limited to C and intrinsics. I was hopeful that an `in` (via __inbyte())
> > > would
> > > be a relatively short way to crash, but it's good to have your code length
> > > as a
> > > benchmark in any case.
> > >
> > > > Rationale:
> > > > 1) It takes 4 bytes instead of 8
> > > > int3 is 1 byte long
> > > > ud2 is 1 byte long
> > > > push byte immediate: is 2 bytes long
> > > >
> > > > while *nullptr = __COUNTER__ is 8 bytes long. From objdump:
> > > > 1: c6 04 25 00 00 00 00 movb $0x0,0x0
> > > > 8: 00
> > > >
> > > > 2) We can reduce the number of different black magics on logging.h. My
> > > > TRAP_SRQUENCE there is just few lines above. I think you can just nicely
> > > > re-route the #ifdefs there.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2701253005/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.