1329150 - Remove ENABLE_ARM_LR_SAVING and its code

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

On ARM we put the value of the LR register into the profiler buffer but never exposed it through in the actual profile object (bug 788289). We should stop putting these values into the buffer.

Comment 1

[Nicholas Nethercote [inactive]]

Julian, want to take this one? Looks pretty trivial, just removing the #ifdef'd code.

Comment 2

[Julian Seward [:jseward]]

Markus, I'm a bit confused about this.  With ENABLE_ARM_LR_SAVING
enabled, we copy the link register into TickSample::lr, and that
is then "used" in two places:

(1) GeckoSampler.cpp, doSampleStackTrace():
#ifdef ENABLE_ARM_LR_SAVING
    aProfile.addTag(ProfileEntry('L', (void*)aSample->lr));
#endif

(2) GeckoSampler.cpp, mergeStacksIntoProfile():
      JS::ProfilingFrameIterator::RegisterState registerState;
      registerState.pc = aSample->pc;
      registerState.sp = aSample->sp;
#ifdef ENABLE_ARM_LR_SAVING
      registerState.lr = aSample->lr;
#endif

Which of these uses do you want to remove?  It seems to me that
we could remove (1), but removing (2) will mean we give the JS
world uninitialised junk in registerState.lr.

Should we enable ENABLE_ARM_LR_SAVING always (I mean, always
copy the value into TickSample), remove (1), but keep (2) ?
Or something else?

Comment 3

[Markus Stange [:mstange]]

Oh, I hadn't seen (2)! I was referring to (1), because those profile entries are never consumed, and I thought (1) was the only user of the LR register values we could remove all of ENABLE_ARM_LR_SAVING.
I've found one user of ProfilingFrameIterator::RegisterState::lr, at http://searchfox.org/mozilla-central/rev/ed04f7a44925dace34f2d2e15ef9c0f2809d70b0/js/src/wasm/WasmFrameIterator.cpp#698 so it looks like we can't remove this after all.

We should still remove (1) though, i.e. stop creating 'L' ProfileEntries.

Comment 4

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #3)
> Oh, I hadn't seen (2)! I was referring to (1), because those profile entries
> are never consumed, and I thought (1) was the only user of the LR register
> values we could remove all of ENABLE_ARM_LR_SAVING.

Sorry for the broken sentence. I meant:

I thought (1) was the only user of the LR register values, so I thought we could remove all ENABLE_ARM_LR_SAVING code.

Comment 5

[Julian Seward [:jseward]]

Attachment: bug1329150-1.diff

Proposed fix, as per comment #3.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8829813 [details] [diff] [review]
bug1329150-1.diff

Review of attachment 8829813 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, sorry for the delay.

Comment 7

[Julian Seward [:jseward]]

Marcus, could you please upgrade that to an r+ then?  Sorry, I should
have asked for it directly.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8829813 [details] [diff] [review]
bug1329150-1.diff

Review of attachment 8829813 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, of course.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb5373470c1
Remove ENABLE_ARM_LR_SAVING and its code.  r=mstange.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7bb5373470c1