roll clang 148039:148911

roll clang 148039:148911

BUG=97532, 111806, 111208
(maybe?)
TEST=none
TBR=hans

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

[Nico, 2012-01-29 00:28:36]

Quite intrusive this time :-/

shess: FYI for the objc_zombie.mm change. If you can think of a way to not
access isa directly, that would be swell.

[commit-bot: I haz the power, 2012-01-29 00:28:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/9297028/7001

[commit-bot: I haz the power, 2012-01-29 04:44:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/9297028/7001

[commit-bot: I haz the power, 2012-01-29 09:02:12]

Change committed as 119637

[Alexei Svitkine (slow), 2012-01-29 17:21:33]

> shess: FYI for the objc_zombie.mm change. If you can think of a way to not
> access isa directly, that would be swell.

Could we use object_setClass() here?

[Nico, 2012-01-29 17:42:43]

On 2012/01/29 17:21:33, Alexei Svitkine wrote:
> > shess: FYI for the objc_zombie.mm change. If you can think of a way to not
> > access isa directly, that would be swell.
> 
> Could we use object_setClass() here?

See the comment 4 lines up that says "we don't use object_setClass() here
because that uses a memory barrier", hence me cc'ing shess :-)

[Brad Chen, 2012-01-30 21:45:29]

This change seems to have induced a couple of problems in the Valgrind FYI bots
for MacOS. These bots use a suppressions file that depends on the call stack at
the time a problem is detected. This change seemed to have caused certain call
frames to appear multiple times on the stack. In the future the memory sheriff's
would surely be grateful if you could do a valgrind/mac try before a clang roll,
or help watch the waterfall after the roll. Or you could join the Memory Sheriff
rotation...

On 2012/01/29 17:42:43, Nico wrote:
> On 2012/01/29 17:21:33, Alexei Svitkine wrote:
> > > shess: FYI for the objc_zombie.mm change. If you can think of a way to not
> > > access isa directly, that would be swell.
> > 
> > Could we use object_setClass() here?
> 
> See the comment 4 lines up that says "we don't use object_setClass() here
> because that uses a memory barrier", hence me cc'ing shess :-)

[Nico, 2012-01-30 22:06:50]

On Mon, Jan 30, 2012 at 1:45 PM,  <bradchen@google.com> wrote:
> This change seems to have induced a couple of problems in the Valgrind FYI
> bots
> for MacOS. These bots use a suppressions file that depends on the call stack
> at
> the time a problem is detected. This change seemed to have caused certain
> call
> frames to appear multiple times on the stack. In the future the memory
> sheriff's
> would surely be grateful if you could do a valgrind/mac try before a clang
> roll,
> or help watch the waterfall after the roll. Or you could join the Memory
> Sheriff
> rotation...

He Brad,

sorry, I usually stick around after clang rolls, but this one landed
at 0:30am due to the commit queue acting up. And I am on the Memory
Sheriff rotation :-)

Nico

[Scott Hess - ex-Googler, 2012-02-09 23:41:21]

On 2012/01/29 17:42:43, Nico wrote:
> On 2012/01/29 17:21:33, Alexei Svitkine wrote:
> > > shess: FYI for the objc_zombie.mm change. If you can think of a way to not
> > > access isa directly, that would be swell.
> > 
> > Could we use object_setClass() here?
> 
> See the comment 4 lines up that says "we don't use object_setClass() here
> because that uses a memory barrier", hence me cc'ing shess :-)

I guess that means I shouldn't bother to continue with my change to use
object_setClass().  Man, whoever wrote this was a genius!

Reading the runtime, I can't for the life of me tell if the use of
OSAtomicCompareAndSwapPtrBarrier() would be of material impact or not.  Setting
things directly is definitely thread-safe, insofar as anyone touching self for
any reason outside this function is ipso facto incorrect.

Would using static casting manage it?
  *static_cast<Class**>(self) = newClass;
This seems materially more brittle with little gain over using the pragma.  In
practice it's likely impossible for Apple to change the layout, and the CrZombie
class also requires this layout.  So I suppose:
  static_cast<CrZombie*>(self)->isa = newClass;
might be adequate?

[Worst-case scenario if the runtime changes is probably a crash without the
additional diagnostics, which isn't that bad of a worst case.]

[Nico, 2012-02-09 23:46:19]

On 2012/02/09 23:41:21, shess wrote:
> On 2012/01/29 17:42:43, Nico wrote:
> > On 2012/01/29 17:21:33, Alexei Svitkine wrote:
> > > > shess: FYI for the objc_zombie.mm change. If you can think of a way to
not
> > > > access isa directly, that would be swell.
> > > 
> > > Could we use object_setClass() here?
> > 
> > See the comment 4 lines up that says "we don't use object_setClass() here
> > because that uses a memory barrier", hence me cc'ing shess :-)
> 
> I guess that means I shouldn't bother to continue with my change to use
> object_setClass().  Man, whoever wrote this was a genius!
> 
> Reading the runtime, I can't for the life of me tell if the use of
> OSAtomicCompareAndSwapPtrBarrier() would be of material impact or not.

This sounds like we might want to give object_setClass a try?