Tag all tracked objects, including Tasks, with the program counter at the site of FROM_HERE.

Tag all tracked objects, including Tasks, with the program counter at the site of FROM_HERE.

This is to make it easier to determine the site Tasks are posted from in release builds, especially when only a minidump is available. It should help diagnose http://crbug.com/81499.

I added a debug function to alias variables so that the optimizer will not strip them out if they are not live.

The semantics of the MessageLoop::PostTask functions is changed and it is wrong but I am not sure what semantics are intended. It seems location information was no longer being tracked for Tasks wrapped as Closures and I don't know if this was intended. PTAL. Update: this has since been fixed.

TEST=Set breakpoint in TaskClosureAdapter::Run and very that the post site can be located in an optimized build.
BUG=81499
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85991

[apatrick_chromium, 2011-05-17 22:48:04]

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc
File base/message_loop.cc (left):

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc#oldcode770
base/message_loop.cc:770: #if defined(TRACK_ALL_TASK_OBJECTS)
Removing this is wrong but what it does overlaps with Tracked::SetBirthPlace.

[darin (slow to review), 2011-05-17 22:56:11]

what a cool idea!!  jar should review this too.

[awong, 2011-05-17 23:54:01]

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc
File base/message_loop.cc (left):

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc#oldcode770
base/message_loop.cc:770: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/05/17 22:48:04, apatrick_chromium wrote:
> Removing this is wrong but what it does overlaps with Tracked::SetBirthPlace.

Corrently...we were trying to make it so Task doesn't inherit from Tracked. The
idea was to try and keep the Births in the PendingTask struct

If you remove all these task->SetBirthPlace() calls and return to having this
ifdef, I think you can find the needed Location higher up in the callstack in
the PendingTask struct for the task being Run().

http://codereview.chromium.org/7039020/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7039020/diff/1/base/tracked.h#newcode81
base/tracked.h:81: void* program_counter_;
This does fundamentally change the nature of this object.  If you look at the
rest of the definition of Location, it's written such that it is effectively a
constant.

I recall jar@ saying that this was done carefully to allow the compiler to
possibly even elide instances since the "class" is effectively a constant.

Adding in program_counter() breaks that performance characteristic.  This
functionality seems like a worthwhile inclusion, but we are changing some of the
fundamental semantics of tracked objects.

[awong, 2011-05-17 23:54:55]

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc
File base/message_loop.cc (left):

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc#oldcode770
base/message_loop.cc:770: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/05/17 23:54:01, awong wrote:
> On 2011/05/17 22:48:04, apatrick_chromium wrote:
> > Removing this is wrong but what it does overlaps with
Tracked::SetBirthPlace.
> 
> Corrently...we were trying to make it so Task doesn't inherit from Tracked.
The
> idea was to try and keep the Births in the PendingTask struct
> 
> If you remove all these task->SetBirthPlace() calls and return to having this
> ifdef, I think you can find the needed Location higher up in the callstack in
> the PendingTask struct for the task being Run().

Corrently -> Correct.

One of these days I will learn to make less tipos. I sware.

[cpu_(ooo_6.6-7.5), 2011-05-18 00:52:25]

I think something like this will be of great help for the set of developers that
stare at crashes. 

The code folding that the optimizers do really messes with your brain.

http://codereview.chromium.org/7039020/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7039020/diff/1/base/tracked.h#newcode81
base/tracked.h:81: void* program_counter_;
But what if the program counter is const void* ?

On 2011/05/17 23:54:01, awong wrote:
> This does fundamentally change the nature of this object.  If you look at the
> rest of the definition of Location, it's written such that it is effectively a
> constant.
> 
> I recall jar@ saying that this was done carefully to allow the compiler to
> possibly even elide instances since the "class" is effectively a constant.
> 
> Adding in program_counter() breaks that performance characteristic.  This
> functionality seems like a worthwhile inclusion, but we are changing some of
the
> fundamental semantics of tracked objects.

[awong, 2011-05-18 01:02:45]

http://codereview.chromium.org/7039020/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7039020/diff/1/base/tracked.h#newcode81
base/tracked.h:81: void* program_counter_;
I don't think that const really matters too much (we should add it anyways
though).

The difference is that __FUNCTION__, __FILE__, and __LINE__ are all constants
that can be resolved in at compile time.

GetProgramCounterRegister() is specifically not such a thing.

I'm not saying there's necessarily a problem here...but I just remember jar@
noting that the Location was carefully crafted to be compile time resolvable.



On 2011/05/18 00:52:26, cpu wrote:
> But what if the program counter is const void* ?
> 
> On 2011/05/17 23:54:01, awong wrote:
> > This does fundamentally change the nature of this object.  If you look at
the
> > rest of the definition of Location, it's written such that it is effectively
a
> > constant.
> > 
> > I recall jar@ saying that this was done carefully to allow the compiler to
> > possibly even elide instances since the "class" is effectively a constant.
> > 
> > Adding in program_counter() breaks that performance characteristic.  This
> > functionality seems like a worthwhile inclusion, but we are changing some of
> the
> > fundamental semantics of tracked objects.
>

[jar (doing other things), 2011-05-18 01:32:33]

It is an interesting effort.  I think it may be very helpful.  

It is also a delicate affair to deceive (and restrain) the optimizer.  I suspect
it may work, and I have a mild paranoid issue about performance in this code...
but I think I may perhaps be too paranoid.

It will be very good to closely watch the perf results when you do land, to be
sure you haven't induced a less-than-apparent change.

Two points:

First, I think the code removal in message_loop.cc may be wrong (as you noted),
and not sufficiently replaced by the code you added.  See details below in the
code comment.

Second, I had convinced myself (historically) that the use of TrackedObjects was
going to be zero cost in a release build.  I convinced myself that the base
class was sufficiently under-used (when compiled for PRODUCTION), that the
optimizer could completely remove all references to the associated code.  For
instance, all the __FILE__ arguments are unused in the called functions, and a
global optimizer should have effectively removed those args from the arguments
lists, and then decided that the underlying constants are unused :-).  Various
pieces of this *might* have changed with the refactoring.... but that was the
goal, and what I *thought* was achieved.  It would be good to look at a link map
and see that the code was/is being discarded.

In DEBUG mode, (or when the task tracking feature was explicitly enabled), only
then would there be a memory cost (bunch of constant strings for filenames etc.)
and run-time cost (arguments passed, building the database of info, and updating
it once it had reached "full size.").  I'm a little wary that this change may
have an impact on that plan.  For example, you have now added a member to the
class that *is* used in a PRODUCTION build. Hence the class instances can
*sometimes* have data, and an optimizer can't be so callous about concluding
that the base instances are always effectively empty.   I'd have to study the
impact a bit to be sure this won't pull in all the code (is it that much??) for
processing and updating TrackedObjects.

The message loop was also (historically) a perf sensitive area.  I suspect that
the extra calls to fetch the current instruction pointer, and pass that around,
are low compared with the cost of a lock etc. I'm pretty convinced that your
anti-aliasing trick won't hurt the optimization by precluding code motion (right
before calling task->Run()), but it would be worth seeing what the optimizer
actually generated in these code snippets (...but more critically, we can see if
it has any measurable perf impact).

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc
File base/message_loop.cc (left):

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc#oldcode770
base/message_loop.cc:770: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/05/17 22:48:04, apatrick_chromium wrote:
> Removing this is wrong but what it does overlaps with Tracked::SetBirthPlace.

It is similar to, but is not replaced by, what you injected with the 4
SetBirthPlace() calls above (example: new line 315).

It is OK to add the non-debugging code above, but it seems incorrect to remove
this code.  This code drives the task-counting system, which provides stats seen
in about:tasks.  I suspect those stats would be lost with this removal.

The replacement code provides some complex data (execution address) that can
plausibly be used to find the source of the task that was run.  (This decoding
is not even trivial to do in a debugger, I think).

That is only a small part of the functionality provided here.

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc#newcode278
base/message_loop.cc:278: CHECK(task);
These checks are probably not needed, now that you are about to deref the
instance in a method call (and presumably write to a slot close to the start of
the instance).

[jar (doing other things), 2011-05-18 01:51:55]

On 2011/05/18 01:02:45, awong wrote:

> 
> The difference is that __FUNCTION__, __FILE__, and __LINE__ are all constants
> that can be resolved in at compile time.
> 
> GetProgramCounterRegister() is specifically not such a thing.
> 
> I'm not saying there's necessarily a problem here...but I just remember jar@
> noting that the Location was carefully crafted to be compile time resolvable.


The big thing I stayed away from was calling the allocator, by using "const
char*" strings for file and function, and counting on them living forever.  That
bought a lot of perf, and allowed pointer comparisons to be used instead of
string comparisons.

I sorta-kind-a-think he can get away with passing in this integral (actually,
pointer) value.  I'm wary of code-folding, and note that the "location" is no
longer as constant as it used to be.  I can imagine some
optimizer-generated-thunk that set up the first several parameters (file, line,
function), and then jumped to common code to do the request for an instruction
pointer.   ...but I guess even when the happened, it would hopefully be a rarer
event than our complete blindness today (re: can't see who constructed the task,
and often, can't even see what the task was).

As you noted, there is also a change in the nature of this instance (it is not
such a clear constant, that can be optimized to a great extent at link time),
but I *think* that the fact that this instruction pointer is not (I think)
propagated deep into the debug code (for distinguishing locations in the
database) makes this change less critical (on the debug variant where we truly
track stuff).

[jar (doing other things), 2011-05-18 02:47:28]

I'm not sure... but I just tried to use about:tasks in a today's TOT build, and
it appears that the recent refactor may have broken some of the FROM_HERE
recording.  Most things show up with NoFunctionName and NeedToSetBirthPlace.

As a result, you might want to hold off landing a bit until we debug back to
status quo.  Hopefully it was just a dropped call somewhere in the recent
refactor(s)... but it would be nice not to get in too much deeper until we get
back the functionality (that I'm hoping this CL will preserve).

[darin (slow to review), 2011-05-18 04:07:25]

Though the full functionality of task tracking may have regressed recently,
I think Al is looking
to enable some functionality in release builds that can help him track down
critical crash bugs.
Maybe we can find a way to proceed despite the recent regressions?
 Of course if they are easy
to fix, we should just do it.

-Darin



On Tue, May 17, 2011 at 7:47 PM, <jar@chromium.org> wrote:

> I'm not sure... but I just tried to use about:tasks in a today's TOT build,
> and
> it appears that the recent refactor may have broken some of the FROM_HERE
> recording.  Most things show up with NoFunctionName and
> NeedToSetBirthPlace.
>
> As a result, you might want to hold off landing a bit until we debug back
> to
> status quo.  Hopefully it was just a dropped call somewhere in the recent
> refactor(s)... but it would be nice not to get in too much deeper until we
> get
> back the functionality (that I'm hoping this CL will preserve).
>
>
> http://codereview.chromium.org/7039020/
>

[awong, 2011-05-18 20:19:22]

I think I found the problem with tracked objects. See description of this CL for
the diagnosis and proposed fix.

  http://codereview.chromium.org/7029038

With this, I think we have enough information to proceed with Alastair's CL
independent of correcting tracked objects.

-Albert


On 2011/05/18 04:07:25, darin wrote:
> Though the full functionality of task tracking may have regressed recently,
> I think Al is looking
> to enable some functionality in release builds that can help him track down
> critical crash bugs.
> Maybe we can find a way to proceed despite the recent regressions?
>  Of course if they are easy
> to fix, we should just do it.
> 
> -Darin
> 
> 
> 
> On Tue, May 17, 2011 at 7:47 PM, <mailto:jar@chromium.org> wrote:
> 
> > I'm not sure... but I just tried to use about:tasks in a today's TOT build,
> > and
> > it appears that the recent refactor may have broken some of the FROM_HERE
> > recording.  Most things show up with NoFunctionName and
> > NeedToSetBirthPlace.
> >
> > As a result, you might want to hold off landing a bit until we debug back
> > to
> > status quo.  Hopefully it was just a dropped call somewhere in the recent
> > refactor(s)... but it would be nice not to get in too much deeper until we
> > get
> > back the functionality (that I'm hoping this CL will preserve).
> >
> >
> > http://codereview.chromium.org/7039020/
> >

[awong, 2011-05-18 20:37:15]

Random preemptive kibitz...

Just noticed the new patchsets.  It occurs to me that all we're doing now is
using the Location to grab the $PC one stack frame ahead of PostTask().  The
actual location isn't really stored inside the BirthOnThread object anymore (and
really can't be).  Would it be just as effective to grab the PC in PostTask, or
even PendingTask::PendingTask, and leave Location unchanged?  It'd be 2 stack
frames higher, but I think it still might give us what we need.

-Albert


On 2011/05/18 20:19:22, awong wrote:
> I think I found the problem with tracked objects. See description of this CL
for
> the diagnosis and proposed fix.
> 
>   http://codereview.chromium.org/7029038
> 
> With this, I think we have enough information to proceed with Alastair's CL
> independent of correcting tracked objects.
> 
> -Albert
> 
> 
> On 2011/05/18 04:07:25, darin wrote:
> > Though the full functionality of task tracking may have regressed recently,
> > I think Al is looking
> > to enable some functionality in release builds that can help him track down
> > critical crash bugs.
> > Maybe we can find a way to proceed despite the recent regressions?
> >  Of course if they are easy
> > to fix, we should just do it.
> > 
> > -Darin
> > 
> > 
> > 
> > On Tue, May 17, 2011 at 7:47 PM, <mailto:jar@chromium.org> wrote:
> > 
> > > I'm not sure... but I just tried to use about:tasks in a today's TOT
build,
> > > and
> > > it appears that the recent refactor may have broken some of the FROM_HERE
> > > recording.  Most things show up with NoFunctionName and
> > > NeedToSetBirthPlace.
> > >
> > > As a result, you might want to hold off landing a bit until we debug back
> > > to
> > > status quo.  Hopefully it was just a dropped call somewhere in the recent
> > > refactor(s)... but it would be nice not to get in too much deeper until we
> > > get
> > > back the functionality (that I'm hoping this CL will preserve).
> > >
> > >
> > > http://codereview.chromium.org/7039020/
> > >

[apatrick_chromium, 2011-05-18 20:49:28]

On 2011/05/18 20:37:15, awong wrote:
> Random preemptive kibitz...
> 
> Just noticed the new patchsets.  It occurs to me that all we're doing now is
> using the Location to grab the $PC one stack frame ahead of PostTask().  The
> actual location isn't really stored inside the BirthOnThread object anymore
(and
> really can't be).  Would it be just as effective to grab the PC in PostTask,
or
> even PendingTask::PendingTask, and leave Location unchanged?  It'd be 2 stack
> frames higher, but I think it still might give us what we need.
> 
> -Albert
> 
> 
> On 2011/05/18 20:19:22, awong wrote:
> > I think I found the problem with tracked objects. See description of this CL
> for
> > the diagnosis and proposed fix.
> > 
> >   http://codereview.chromium.org/7029038
> > 
> > With this, I think we have enough information to proceed with Alastair's CL
> > independent of correcting tracked objects.
> > 
> > -Albert
> > 
> > 
> > On 2011/05/18 04:07:25, darin wrote:
> > > Though the full functionality of task tracking may have regressed
recently,
> > > I think Al is looking
> > > to enable some functionality in release builds that can help him track
down
> > > critical crash bugs.
> > > Maybe we can find a way to proceed despite the recent regressions?
> > >  Of course if they are easy
> > > to fix, we should just do it.
> > > 
> > > -Darin
> > > 
> > > 
> > > 
> > > On Tue, May 17, 2011 at 7:47 PM, <mailto:jar@chromium.org> wrote:
> > > 
> > > > I'm not sure... but I just tried to use about:tasks in a today's TOT
> build,
> > > > and
> > > > it appears that the recent refactor may have broken some of the
FROM_HERE
> > > > recording.  Most things show up with NoFunctionName and
> > > > NeedToSetBirthPlace.
> > > >
> > > > As a result, you might want to hold off landing a bit until we debug
back
> > > > to
> > > > status quo.  Hopefully it was just a dropped call somewhere in the
recent
> > > > refactor(s)... but it would be nice not to get in too much deeper until
we
> > > > get
> > > > back the functionality (that I'm hoping this CL will preserve).
> > > >
> > > >
> > > > http://codereview.chromium.org/7039020/
> > > >

I could get the PC of the caller in MessageLoop::PostTask but that would give
less useful information in some cases. For example, if a task is posted with
BrowserThread::PostTask, BrowserThread::PostTask will show up as the site the
task was posted from, which is true, but I'm more interested in the caller of
BrowserThread::PostTask. FROM_HERE will invoke GetProgramCounterRegister closer
to the bug.

It would be possible to walk the call stack N frames and store an array of PCs
in PendingTask. That would be a bit more work though and I have a top crasher to
fix :) I'm happy to revert this once I have the information I want from the
canary channel.

[eroman%chromium.org, 2011-05-18 20:52:44]

http://codereview.chromium.org/7039020/diff/11002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7039020/diff/11002/base/message_loop.cc#newcod...
base/message_loop.cc:477: base::debug::Alias(&program_counter);
Since you are only targeting MSVC anyway, how about:

#if defined(OS_WIN)
#pragma warning (disable: 4748)
#pragma optimize( "", off )

<CODE THAT YOU DON'T WANT OPTIMIZED HERE>

#if defined(OS_WIN)
#pragma optimize( "", on )
#pragma warning (default: 4748)
#endif



I have used that pattern before to make sure local variables and functions don't
get optimized away, it works pretty well to preserve information in the
minidumps.

[awong, 2011-05-18 21:01:18]

On 2011/05/18 20:49:28, apatrick_chromium wrote:
> On 2011/05/18 20:37:15, awong wrote:
> > Random preemptive kibitz...
> > 
> > Just noticed the new patchsets.  It occurs to me that all we're doing now is
> > using the Location to grab the $PC one stack frame ahead of PostTask().  The
> > actual location isn't really stored inside the BirthOnThread object anymore
> (and
> > really can't be).  Would it be just as effective to grab the PC in PostTask,
> or
> > even PendingTask::PendingTask, and leave Location unchanged?  It'd be 2
stack
> > frames higher, but I think it still might give us what we need.
> > 
> > -Albert
> > 
> > 
> > On 2011/05/18 20:19:22, awong wrote:
> > > I think I found the problem with tracked objects. See description of this
CL
> > for
> > > the diagnosis and proposed fix.
> > > 
> > >   http://codereview.chromium.org/7029038
> > > 
> > > With this, I think we have enough information to proceed with Alastair's
CL
> > > independent of correcting tracked objects.
> > > 
> > > -Albert
> > > 
> > > 
> > > On 2011/05/18 04:07:25, darin wrote:
> > > > Though the full functionality of task tracking may have regressed
> recently,
> > > > I think Al is looking
> > > > to enable some functionality in release builds that can help him track
> down
> > > > critical crash bugs.
> > > > Maybe we can find a way to proceed despite the recent regressions?
> > > >  Of course if they are easy
> > > > to fix, we should just do it.
> > > > 
> > > > -Darin
> > > > 
> > > > 
> > > > 
> > > > On Tue, May 17, 2011 at 7:47 PM, <mailto:jar@chromium.org> wrote:
> > > > 
> > > > > I'm not sure... but I just tried to use about:tasks in a today's TOT
> > build,
> > > > > and
> > > > > it appears that the recent refactor may have broken some of the
> FROM_HERE
> > > > > recording.  Most things show up with NoFunctionName and
> > > > > NeedToSetBirthPlace.
> > > > >
> > > > > As a result, you might want to hold off landing a bit until we debug
> back
> > > > > to
> > > > > status quo.  Hopefully it was just a dropped call somewhere in the
> recent
> > > > > refactor(s)... but it would be nice not to get in too much deeper
until
> we
> > > > > get
> > > > > back the functionality (that I'm hoping this CL will preserve).
> > > > >
> > > > >
> > > > > http://codereview.chromium.org/7039020/
> > > > >
> 
> I could get the PC of the caller in MessageLoop::PostTask but that would give
> less useful information in some cases. For example, if a task is posted with
> BrowserThread::PostTask, BrowserThread::PostTask will show up as the site the
> task was posted from, which is true, but I'm more interested in the caller of
> BrowserThread::PostTask. FROM_HERE will invoke GetProgramCounterRegister
closer
> to the bug.
> 
> It would be possible to walk the call stack N frames and store an array of PCs
> in PendingTask. That would be a bit more work though and I have a top crasher
to
> fix :) I'm happy to revert this once I have the information I want from the
> canary channel.

Agreed that it's much nicer to have PC at FROM_HERE. As for walking the stack,
this sounds overly slow.

Feel free to ignore me here. I'm just being OCD about Location having a field
that will always be unset inside BirthOnThread.

[apatrick_chromium, 2011-05-18 21:09:47]

Ready for another review.

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc
File base/message_loop.cc (left):

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc#oldcode770
base/message_loop.cc:770: #if defined(TRACK_ALL_TASK_OBJECTS)
On 2011/05/18 01:32:34, jar wrote:
> On 2011/05/17 22:48:04, apatrick_chromium wrote:
> > Removing this is wrong but what it does overlaps with
Tracked::SetBirthPlace.
> 
> It is similar to, but is not replaced by, what you injected with the 4
> SetBirthPlace() calls above (example: new line 315).
> 
> It is OK to add the non-debugging code above, but it seems incorrect to remove
> this code.  This code drives the task-counting system, which provides stats
seen
> in about:tasks.  I suspect those stats would be lost with this removal.
> 
> The replacement code provides some complex data (execution address) that can
> plausibly be used to find the source of the task that was run.  (This decoding
> is not even trivial to do in a debugger, I think).
> 
> That is only a small part of the functionality provided here.

I think I see how this works now. This code block of code is back.

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7039020/diff/1/base/message_loop.cc#newcode278
base/message_loop.cc:278: CHECK(task);
On 2011/05/18 01:32:34, jar wrote:
> These checks are probably not needed, now that you are about to deref the
> instance in a method call (and presumably write to a slot close to the start
of
> the instance).

SetBirthPlace calls are gone so the CHECK is not redundant now.

http://codereview.chromium.org/7039020/diff/1/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7039020/diff/1/base/tracked.h#newcode81
base/tracked.h:81: void* program_counter_;
On 2011/05/18 00:52:26, cpu wrote:
> But what if the program counter is const void* ?
> 
> On 2011/05/17 23:54:01, awong wrote:
> > This does fundamentally change the nature of this object.  If you look at
the
> > rest of the definition of Location, it's written such that it is effectively
a
> > constant.
> > 
> > I recall jar@ saying that this was done carefully to allow the compiler to
> > possibly even elide instances since the "class" is effectively a constant.
> > 
> > Adding in program_counter() breaks that performance characteristic.  This
> > functionality seems like a worthwhile inclusion, but we are changing some of
> the
> > fundamental semantics of tracked objects.
> 

I made it "const void* const" for consistency. I don't think it will help the
compiler though.

http://codereview.chromium.org/7039020/diff/1/base/tracked.h#newcode81
base/tracked.h:81: void* program_counter_;
On 2011/05/18 01:02:45, awong wrote:
> I don't think that const really matters too much (we should add it anyways
> though).
> 
> The difference is that __FUNCTION__, __FILE__, and __LINE__ are all constants
> that can be resolved in at compile time.
> 
> GetProgramCounterRegister() is specifically not such a thing.
> 
> I'm not saying there's necessarily a problem here...but I just remember jar@
> noting that the Location was carefully crafted to be compile time resolvable.
> 
> 
> 
> On 2011/05/18 00:52:26, cpu wrote:
> > But what if the program counter is const void* ?
> > 
> > On 2011/05/17 23:54:01, awong wrote:
> > > This does fundamentally change the nature of this object.  If you look at
> the
> > > rest of the definition of Location, it's written such that it is
effectively
> a
> > > constant.
> > > 
> > > I recall jar@ saying that this was done carefully to allow the compiler to
> > > possibly even elide instances since the "class" is effectively a constant.
> > > 
> > > Adding in program_counter() breaks that performance characteristic.  This
> > > functionality seems like a worthwhile inclusion, but we are changing some
of
> > the
> > > fundamental semantics of tracked objects.
> > 
> 

I agree the result of GetProgramCounterRegister() is not compile time
resolvable. I examined the code generated for an optimized windows build prior
to this change (no global optimization) and the compiler was not doing anything
special for FROM_HERE:

5C50BD18  push        293h 
5C50BD1D  push        offset string ".\\browser\\safe_browsing\\safe_bro"...
(5D25874Ch) 
5C50BD22  push        offset string "SafeBrowsingService::MakeDatabas"...
(5D258C2Ch) 
5C50BD27  lea         ecx,[esp+2Ch] 
5C50BD2B  call        dword ptr [__imp_tracked_objects::Location::Location
(5D1B4C98h)] 

This change adds the overhead for invoking GetProgramCounterRegister() and
pushing it as an argument for the Location constructor.

I didn't figure out how to do an LTCG build.

http://codereview.chromium.org/7039020/diff/11002/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/7039020/diff/11002/base/message_loop.cc#newcod...
base/message_loop.cc:477: base::debug::Alias(&program_counter);
On 2011/05/18 20:52:44, eroman%chromium.org wrote:
> Since you are only targeting MSVC anyway, how about:
> 
> #if defined(OS_WIN)
> #pragma warning (disable: 4748)
> #pragma optimize( "", off )
> 
> <CODE THAT YOU DON'T WANT OPTIMIZED HERE>
> 
> #if defined(OS_WIN)
> #pragma optimize( "", on )
> #pragma warning (default: 4748)
> #endif
> 
> 
> 
> I have used that pattern before to make sure local variables and functions
don't
> get optimized away, it works pretty well to preserve information in the
> minidumps.

Done.

[awong, 2011-05-18 21:29:09]

LGTM w/one nit.

http://codereview.chromium.org/7039020/diff/10004/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7039020/diff/10004/base/tracked.h#newcode69
base/tracked.h:69: const void* program_counter() const { return
program_counter_; }
Would it be worth noting somewhere that program_counter() is likely
uninitialized when you do a births->location()?

[apatrick_chromium, 2011-05-19 00:35:13]

On 2011/05/18 21:29:09, awong wrote:
> LGTM w/one nit.
> 
> http://codereview.chromium.org/7039020/diff/10004/base/tracked.h
> File base/tracked.h (right):
> 
> http://codereview.chromium.org/7039020/diff/10004/base/tracked.h#newcode69
> base/tracked.h:69: const void* program_counter() const { return
> program_counter_; }
> Would it be worth noting somewhere that program_counter() is likely
> uninitialized when you do a births->location()?

Okay I will fix that.

Does anyone object to me committing this? The sooner this gets in the canary
channel the better.

[Daniel Berlin, 2011-05-19 18:40:45]

> Okay I will fix that.
> 
> Does anyone object to me committing this? The sooner this gets in the canary
> channel the better.

The patch should be using __builtin_extract_return_address to get the real PC
from the return value of builtin_return_address (it knows how to offset the
addresses properly for SPARC, and decode addresses on platforms that require
frobbing like S/390).

Additionally, Alias() won't work in most IPO modes, and newer GCC'en should see
right through it even outside of whole program mode.
If you really want to convince the compiler to keep it around, store it in a
global variable, which should work on almost all compilers :)

[apatrick_chromium, 2011-05-19 19:17:06]

On 2011/05/19 18:40:45, Daniel Berlin wrote:
> > Okay I will fix that.
> > 
> > Does anyone object to me committing this? The sooner this gets in the canary
> > channel the better.
> 
> The patch should be using __builtin_extract_return_address to get the real PC
> from the return value of builtin_return_address (it knows how to offset the
> addresses properly for SPARC, and decode addresses on platforms that require
> frobbing like S/390).
> 
> Additionally, Alias() won't work in most IPO modes, and newer GCC'en should
see
> right through it even outside of whole program mode.
> If you really want to convince the compiler to keep it around, store it in a
> global variable, which should work on almost all compilers :)

Done. Thanks.

[jar (doing other things), 2011-05-19 19:21:34]

If the global is never used (read), won't the compiler optimize away all
assignments to it??

Jim

On Thu, May 19, 2011 at 12:17 PM, <apatrick@chromium.org> wrote:

> On 2011/05/19 18:40:45, Daniel Berlin wrote:
>
>> > Okay I will fix that.
>> >
>> > Does anyone object to me committing this? The sooner this gets in the
>> canary
>> > channel the better.
>>
>
>  The patch should be using __builtin_extract_return_address to get the real
>> PC
>> from the return value of builtin_return_address (it knows how to offset
>> the
>> addresses properly for SPARC, and decode addresses on platforms that
>> require
>> frobbing like S/390).
>>
>
>  Additionally, Alias() won't work in most IPO modes, and newer GCC'en
>> should
>>
> see
>
>> right through it even outside of whole program mode.
>> If you really want to convince the compiler to keep it around, store it in
>> a
>> global variable, which should work on almost all compilers :)
>>
>
> Done. Thanks.
>
>
> http://codereview.chromium.org/7039020/
>

[jar (doing other things), 2011-05-19 19:28:13]

The hassle with "global variable" is that we don't get heap dumps, but only
stack dumps.  As a result, his effort to put it into the stack was
interesting.

As I warned, it is hard to trick an optimizing compiler. Historically taking
an address did cause lots of optimizers to punt on a lot of things...but I'm
more than wary of the currently proposed technique on today's compilers.
 However, I *suspect* there are ways to cajole it to not optimize.

For example, perhaps the address aliased variable can be pushed (for real,
rather than in a fake __asm construct) into the containing message loop
state structure.  So long as we provide a conceivable way for an application
to access that data from the message loop structures, (such as a silly
about:page??), the optimizer should be precluded from declaring the use dead
and worthy of discarding.

Jim



On Thu, May 19, 2011 at 11:40 AM, <dannyb@google.com> wrote:

>
>  Okay I will fix that.
>>
>
>  Does anyone object to me committing this? The sooner this gets in the
>> canary
>> channel the better.
>>
>
> The patch should be using __builtin_extract_return_address to get the real
> PC
> from the return value of builtin_return_address (it knows how to offset the
> addresses properly for SPARC, and decode addresses on platforms that
> require
> frobbing like S/390).
>
> Additionally, Alias() won't work in most IPO modes, and newer GCC'en should
> see
> right through it even outside of whole program mode.
> If you really want to convince the compiler to keep it around, store it in
> a
> global variable, which should work on almost all compilers :)
>
>
>
>
> http://codereview.chromium.org/7039020/
>

[Daniel Berlin, 2011-05-19 19:29:32]

Only in whole program mode.
Even then, in order to prove that the global is not used,  it needs to
be able to trace all possible call graphs from the main function, and
show that none of them read or write that global.

This call graph includes all external functions.  After all, you have
no proof that strlen doesn't read this global :)
So when i say whole program, i mean "you'd have to have the IR for all
functions including C library functions, etc" so that you know they
don't read the global.
For some classes of functions with well defined semantics (The C
library is a good example here actually), the compiler will often have
tables of what the functions do to the arguments past to it, and
whether they read/write from visible globals.u
But basically, out in the real world,  it isn't going to optimize it
away unless you pass flags telling it that the globals aren't used
where the compiler can't see it.


On Thu, May 19, 2011 at 3:20 PM, Jim Roskind <jar@chromium.org> wrote:
> If the global is never used (read), won't the compiler optimize away all
> assignments to it??
> Jim
>
> On Thu, May 19, 2011 at 12:17 PM, <apatrick@chromium.org> wrote:
>>
>> On 2011/05/19 18:40:45, Daniel Berlin wrote:
>>>
>>> > Okay I will fix that.
>>> >
>>> > Does anyone object to me committing this? The sooner this gets in the
>>> > canary
>>> > channel the better.
>>
>>> The patch should be using __builtin_extract_return_address to get the
>>> real PC
>>> from the return value of builtin_return_address (it knows how to offset
>>> the
>>> addresses properly for SPARC, and decode addresses on platforms that
>>> require
>>> frobbing like S/390).
>>
>>> Additionally, Alias() won't work in most IPO modes, and newer GCC'en
>>> should
>>
>> see
>>>
>>> right through it even outside of whole program mode.
>>> If you really want to convince the compiler to keep it around, store it
>>> in a
>>> global variable, which should work on almost all compilers :)
>>
>> Done. Thanks.
>>
>> http://codereview.chromium.org/7039020/
>
>

[Daniel Berlin, 2011-05-19 19:38:45]

On Thu, May 19, 2011 at 3:28 PM, Daniel Berlin <dannyb@google.com> wrote:
> Only in whole program mode.
> Even then, in order to prove that the global is not used,  it needs to
> be able to trace all possible call graphs from the main function, and
> show that none of them read or write that global.

(Just to be clear, this is a bit of a simplification, you'd really
need to show none of them do something with the global that eventually
causes side-effects, like printing it out
If all you did was read or write it, it could eventually eliminate it).

In reality, if the compiler can prove the value is never used, nothing
you do other than telling the compiler the variable is used in ways it
doesn't understand (gcc has attributes for this, i assume MSVC does
too) will stop it from eliminating it.

All we are changing is how hard it is to prove that it is never used.
Putting it in a global would suffice for how chrome is currently
compiled and is likely to be compiled in the future, AFAIK.

[Daniel Berlin, 2011-05-19 19:41:40]

On Thu, May 19, 2011 at 3:18 PM, Jim Roskind <jar@chromium.org> wrote:
> The hassle with "global variable" is that we don't get heap dumps, but only
> stack dumps.  As a result, his effort to put it into the stack was
> interesting.
> As I warned, it is hard to trick an optimizing compiler. Historically taking
> an address did cause lots of optimizers to punt on a lot of things...

Not anymore.
In fact, i spent a lot of time making that true for GCC about 6 years ago :)

> but I'm more than wary of the currently proposed technique on today's
compilers.
>  However, I *suspect* there are ways to cajole it to not optimize.

Storing it to a global will suffice for the real world for now, unless
the compiler is broken, you've passed flags telling it it can assume
some things that violate the various standards, or you are handing it
all of the IR for literally the entire program (including external
libs/etc) at once.


> For example, perhaps the address aliased variable can be pushed (for real,
> rather than in a fake __asm construct) into the containing message loop
> state structure.
> So long as we provide a conceivable way for an application
> to access that data from the message loop structures, (such as a silly
> about:page??), the optimizer should be precluded from declaring the use dead
> and worthy of discarding.

You want to be careful here, you don't want to use a mechanism that is
going to lose you a lot of other optimizations.  The nice thing about
globals is the compiler can see most of the functions don't touch
them, and will happily optimize around those functions that it can't
tell about.

Once you start pushing aliased variables into structures, you make the
job of pointer analysis and dependent optimizations *significantly*
harder.

[darin (slow to review), 2011-05-19 21:00:03]

LGTM

http://codereview.chromium.org/7039020/diff/19003/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7039020/diff/19003/base/tracked.h#newcode84
base/tracked.h:84: BASE_API const void* GetProgramCounterRegister();
nit: maybe just call this GetProgramCounter()... Register suffix doesn't add
much and you are storing the result as "program_counter"

[apatrick_chromium, 2011-05-19 21:09:44]

http://codereview.chromium.org/7039020/diff/19003/base/tracked.h
File base/tracked.h (right):

http://codereview.chromium.org/7039020/diff/19003/base/tracked.h#newcode84
base/tracked.h:84: BASE_API const void* GetProgramCounterRegister();
On 2011/05/19 21:00:03, darin wrote:
> nit: maybe just call this GetProgramCounter()... Register suffix doesn't add
> much and you are storing the result as "program_counter"

Done.