Prevent code folding in the ThreadWatcher ThreadUnresponsive_X functions.

Prevent code folding in the ThreadWatcher ThreadUnresponsive_X functions.

Compiler optimizations were folding all the CHECK(false) lines into a single,
causing them to bucket the same on the crash server. This alters the bodies of
the functions to avoid folding.

Inlining will still happen, but the DWARF data should still be able to identify
the instructions as coming from these functions. If not, a noinline attribute
will need to be applied.

BUG=134254
TEST=Crash stats.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146414

[jar (doing other things), 2012-07-03 16:43:25]

drive by commentary:

The way I usually avoid this confusion is be re-implementing CHECK(false) with
an explicit (unique) segv instigation in each case.  This doesn't waste memory
creating strings, and doesn't leave openings for other optimizations (I think). 
Code might look like the following if you wanted to make this a macro:

#define UNIQUE_CHECK_FALSE(small_enum)  do { \
    char * p = reinterpret_cast<char*>(NULL);  \
    p += small_enum;  \
    *p = 0;  // instigate segv.  \
  } while(0);

You could equally well have written the above as a function.

Or you could do something like:

#define UNIQUE_CHECK_FALSE()  do { \
    char * p = reinterpret_cast<char*>(NULL);  \
    p += __LINE__ % 1000;  \
    *p = 0;  // instigate segv.  \
  } while(0);

...and then you don't have to write an enum... and you get the line number of
your crash (and you count on the first 1000 bytes of memory as being read-only).

The code is tiny.... the crashreport tells what address was accessed to cause
the segv, so you know what the enum (or __LINE__) was.

YMMV.

[eroman, 2012-07-03 19:00:38]

lgtm

https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/thread_watcher.cc (right):

https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
chrome/browser/metrics/thread_watcher.cc:43: CHECK(false) << __FUNCTION__;
I am slightly surprised that CHECK(false) doesn't already generate unique
bodies. Since CHECK() macro will include the filename and line number, and the
line numbers should be different for each of the CHECK().

I suspect NOINLINE may be necessary here instead, but if this works then fine!

[Robert Sesek, 2012-07-10 18:43:09]

https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/thread_watcher.cc (right):

https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
chrome/browser/metrics/thread_watcher.cc:43: CHECK(false) << __FUNCTION__;
On 2012/07/03 19:00:38, eroman wrote:
> I am slightly surprised that CHECK(false) doesn't already generate unique
> bodies. Since CHECK() macro will include the filename and line number, and the
> line numbers should be different for each of the CHECK().
> 
> I suspect NOINLINE may be necessary here instead, but if this works then fine!

In official release builds, it looks like CHECK() gets turned into
base::debug::BreakDebugger, which won't generate the file/line data.

[commit-bot: I haz the power, 2012-07-10 18:43:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/10700080/1

[jar (doing other things), 2012-07-10 20:05:23]

https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/thread_watcher.cc (right):

https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
chrome/browser/metrics/thread_watcher.cc:43: CHECK(false) << __FUNCTION__;
On 2012/07/10 18:43:09, rsesek wrote:
> On 2012/07/03 19:00:38, eroman wrote:
> > I am slightly surprised that CHECK(false) doesn't already generate unique
> > bodies. Since CHECK() macro will include the filename and line number, and
the
> > line numbers should be different for each of the CHECK().
> > 
> > I suspect NOINLINE may be necessary here instead, but if this works then
fine!
> 
> In official release builds, it looks like CHECK() gets turned into
> base::debug::BreakDebugger, which won't generate the file/line data.

Reading the header file, it appears that they discard as much as possible to
avoid bloating the binary.  The approach you've taken here, reverses this trend,
and tries to force unique string names (function names) into the binary.   In
fact, it uses more memory than __FILE__ plus __LINE__ would, as the __FILE__
string would be shared in a const data segment, and only appear once.

Although the total memory used by your pattern in this case is not that great (a
couple of hundred bytes), the pattern seems worse than the segv approach I
mentioned.

Is there a reason why you prefer this that I'm missing?

[Robert Sesek, 2012-07-12 15:14:18]

On 2012/07/10 20:05:23, jar wrote:
>
https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
> File chrome/browser/metrics/thread_watcher.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10700080/diff/1/chrome/browser/metrics...
> chrome/browser/metrics/thread_watcher.cc:43: CHECK(false) << __FUNCTION__;
> On 2012/07/10 18:43:09, rsesek wrote:
> > On 2012/07/03 19:00:38, eroman wrote:
> > > I am slightly surprised that CHECK(false) doesn't already generate unique
> > > bodies. Since CHECK() macro will include the filename and line number, and
> the
> > > line numbers should be different for each of the CHECK().
> > > 
> > > I suspect NOINLINE may be necessary here instead, but if this works then
> fine!
> > 
> > In official release builds, it looks like CHECK() gets turned into
> > base::debug::BreakDebugger, which won't generate the file/line data.
> 
> Reading the header file, it appears that they discard as much as possible to
> avoid bloating the binary.  The approach you've taken here, reverses this
trend,
> and tries to force unique string names (function names) into the binary.   In
> fact, it uses more memory than __FILE__ plus __LINE__ would, as the __FILE__
> string would be shared in a const data segment, and only appear once.
> 
> Although the total memory used by your pattern in this case is not that great
(a
> couple of hundred bytes), the pattern seems worse than the segv approach I
> mentioned.
> 
> Is there a reason why you prefer this that I'm missing?

I think your approach is clever, but I don't see why making exception code
unique is important. I just want to be able to easily differentiate the buckets
on the crash server. What you suggested would do that, but I think this can be
done without an obscure macro. If we're going to quibble over fewer than 256
bytes, then I think what I've done in PS2 should be sufficient. It's just
derefing a NULL pointer and writing the line into it. The important piece
(AFAIU) is getting unique instructions for each of these functions, s.t. the
compiler doesn't fold them together. Inlining, at least on Mac, shouldn't matter
because the full DWARF data will still be able to map these back to the right
function.

[jar (doing other things), 2012-07-12 18:16:44]

lgtm

[commit-bot: I haz the power, 2012-07-12 18:18:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/10700080/4

[eroman, 2012-07-12 18:33:44]

Still LGTM.

Note that for our Windows builds this sort of trick sometimes gets defeated by
whole program optimization, since the number has to be unique across the entire
program. We have had several functions that crashed intentionally by writing
*((int*)0) = 0;, and the symbol name that would appear in callstack was pretty
arbitrary. So the author would then go and tweak the address to something more
random. Meh.

[commit-bot: I haz the power, 2012-07-12 19:33:56]

Change committed as 146414

[jar (doing other things), 2012-07-12 20:05:37]

The stack (above the segv function) gets you "close," and the number should
clinch uniqueness.

My only minor concern with this game is being sure the number is not "too
large," such that you don't get the SEGV on a reserved page.  His line numbers
are plenty small, so this is not a problem.

If we wanted to be fancy, we'd have some header with a mother-of-all exit codes,
and use that.  We'd then have unique codes across the system, and could even
move to fancier games (if we were afraid we had too many codes), such as
explicitly pushing the code on the stack, and then crashing.

I've found the segv-offset game, plus the stack above it, to be very distinctive
though.  I also like that the offset is in-my-face when I look at a crash dump
(and don't have to dig through stacks with a debugger to get everything right).


On 2012/07/12 18:33:44, eroman wrote:
> Still LGTM.
> 
> Note that for our Windows builds this sort of trick sometimes gets defeated by
> whole program optimization, since the number has to be unique across the
entire
> program. We have had several functions that crashed intentionally by writing
> *((int*)0) = 0;, and the symbol name that would appear in callstack was pretty
> arbitrary. So the author would then go and tweak the address to something more
> random. Meh.