Closed Bug 1714501 Opened 3 years ago Closed 2 years ago

Registers::SyncPopulate collects information that becomes incorrect for stack-walking

Categories

(Core :: Gecko Profiler, defect, P2)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1779257

People

(Reporter: mozbugz, Unassigned)

References

Details

(Whiteboard: [fxp])

Found by :glandium while debugging stack walkers for bug 1712674. Trying to summarize here, and think of solutions:

Registers::SyncPopulate is used twice in the profiler, before calling the appropriate native stack walker.

The issue is that the call may not be inlined, in which case the register information it collects refers to the Registers::SyncPopulate function call. So when it returns, in particular the stack register(s) (SP, and possibly BP) now refer to the Registers::SyncPopulate function that just ended, and when calling other functions and eventually the stack walker, that stack area will be modified.

The effect may depend on the platform. For example:
On Windows, it's only used to record the "leaf function" (and it's probably not a good thing either, see bug 1695618), and the native stack walker will start from a newly-acquired correct CONTEXT anyway.
On Mac, the FramePointerStackWalk does use these registers. But I don't remember seeing problems around that, I'm guessing because luckily whatever function is called after Registers::SyncPopulate happens to have the same BP (base pointer, the address where only PC and BP were pushed at the beginning of the function call), so the stack walker can proceed from there.

One solution may be to force the function to be inlined, so that its information will still be valid when calling the native stack walker from under the same function. (A quick attempt with MOZ_ALWAYS_INLINE didn't make it inline! More work needed...)

Or maybe we could get rid of this dangerous call?
We are considering getting rid of the "leaf" profiler feature anyway (bug 1571089), in which case I think we could remove this function from the profiler code, and maybe move part of it to the stack walker(s) like FramePointerStackWalk that require a starting point in the current thread.

On Mac, Registers::SyncPopulate does get the values for the caller (which means it would not work as expected if it were inlined). On Linux x64, it probably doesn't cause a problem because LUL would find the right caller frame based on the unwind info for the Registers::SyncPopulate function (since the PC would point there and would be correct). On other platforms, I'm not entirely sure of the status (like it might as well be borked on Linux x86, because I think we're using FramePointerStackWalk there instead of LUL ; but maybe it's also saved by the fact that BP is still saved at the right place on the call to the next function after Register::SyncPopulate, but that's relying on tail-call optimization not happening).

Whiteboard: [fxp]

This has been fixed by Bug 1779257.

Status: NEW → RESOLVED
Closed: 2 years ago
Duplicate of bug: 1779257
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.