Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement instruction_addr_adjustment #42533

Merged
merged 11 commits into from
Feb 1, 2023
23 changes: 21 additions & 2 deletions src/sentry/lang/native/processing.py
Original file line number Diff line number Diff line change
Expand Up @@ -302,15 +302,24 @@ def _handles_frame(data, frame):
return is_native_platform(platform) and frame.get("instruction_addr") is not None


def get_frames_for_symbolication(frames, data, modules):
def get_frames_for_symbolication(
frames,
data,
modules,
adjustment=None,
):
modules_by_debug_id = None
rv = []
adjustment = adjustment or "auto"

for frame in reversed(frames):
if not _handles_frame(data, frame):
continue
s_frame = dict(frame)

if adjustment == "none":
s_frame["adjust_instruction_addr"] = False

# validate and expand addressing modes. If we can't validate and
# expand it, we keep None which is absolute. That's not great but
# at least won't do damage.
Expand Down Expand Up @@ -342,6 +351,13 @@ def get_frames_for_symbolication(frames, data, modules):
s_frame["addr_mode"] = sanitized_addr_mode
rv.append(s_frame)

if len(rv) > 0:
first_frame = rv[0]
if adjustment == "all":
first_frame["adjust_instruction_addr"] = True
elif adjustment == "all_but_first":
first_frame["adjust_instruction_addr"] = False

return rv


Expand All @@ -362,7 +378,10 @@ def process_payload(data):
{
"registers": sinfo.stacktrace.get("registers") or {},
"frames": get_frames_for_symbolication(
sinfo.stacktrace.get("frames") or (), data, modules
sinfo.stacktrace.get("frames") or (),
data,
modules,
sinfo.stacktrace.get("instruction_addr_adjustment"),
),
}
for sinfo in stacktrace_infos
Expand Down
40 changes: 32 additions & 8 deletions src/sentry/profiles/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

from copy import deepcopy
from datetime import datetime
from time import sleep, time
from typing import Any, List, Mapping, MutableMapping, Optional, Tuple
Expand Down Expand Up @@ -225,18 +226,41 @@ def _normalize(profile: Profile, organization: Organization) -> None:
def _prepare_frames_from_profile(profile: Profile) -> Tuple[List[Any], List[Any]]:
modules = profile["debug_meta"]["images"]

# NOTE: the usage of `adjust_instruction_addr` assumes that all
# the profilers on all the platforms are walking stacks right from a
# suspended threads cpu context

# in the sample format, we have a frames key containing all the frames
if "version" in profile:
stacktraces = [{"registers": {}, "frames": profile["profile"]["frames"]}]
frames = profile["profile"]["frames"]

for stack in profile["profile"]["stacks"]:
if len(stack) > 0:
# Make a deep copy of the leaf frame with adjust_instruction_addr = False
# and append it to the list. This ensures correct behavior
# if the leaf frame also shows up in the middle of another stack.
first_frame_idx = stack[0]
frame = deepcopy(frames[first_frame_idx])
frame["adjust_instruction_addr"] = False
frames.append(frame)
stack[0] = len(frames) - 1

stacktraces = [{"registers": {}, "frames": [dict(frame) for frame in frames]}]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not transform the frame to a dict in the append above so you don't have to loop once more?

# in the original format, we need to gather frames from all samples
else:
stacktraces = [
{
"registers": {},
"frames": s["frames"],
}
for s in profile["sampled_profile"]["samples"]
]
stacktraces = []
for s in profile["sampled_profile"]["samples"]:
frames = [dict(frame) for frame in s["frames"]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you're casting as a dict again, it's already a dict.


if len(frames) > 0:
frames[0]["adjust_instruction_addr"] = False

stacktraces.append(
{
"registers": {},
"frames": frames,
}
)
return (modules, stacktraces)


Expand Down
48 changes: 48 additions & 0 deletions tests/sentry/lang/native/test_processing.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,54 @@ def test_filter_frames():
assert len(filtered_frames) == 0


def test_instruction_addr_adjustment_auto():
frames = [
{"instruction_addr": "0xdeadbeef", "platform": "native"},
{"instruction_addr": "0xbeefdead", "platform": "native"},
]

processed_frames = get_frames_for_symbolication(frames, None, None, None)

assert "adjust_instruction_addr" not in processed_frames[0].keys()
assert "adjust_instruction_addr" not in processed_frames[1].keys()


def test_instruction_addr_adjustment_all():
frames = [
{"instruction_addr": "0xdeadbeef", "platform": "native"},
{"instruction_addr": "0xbeefdead", "platform": "native"},
]

processed_frames = get_frames_for_symbolication(frames, None, None, "all")

assert processed_frames[0]["adjust_instruction_addr"]
assert "adjust_instruction_addr" not in processed_frames[1].keys()


def test_instruction_addr_adjustment_all_but_first():
frames = [
{"instruction_addr": "0xdeadbeef", "platform": "native"},
{"instruction_addr": "0xbeefdead", "platform": "native"},
]

processed_frames = get_frames_for_symbolication(frames, None, None, "all_but_first")

assert not processed_frames[0]["adjust_instruction_addr"]
assert "adjust_instruction_addr" not in processed_frames[1].keys()


def test_instruction_addr_adjustment_none():
frames = [
{"instruction_addr": "0xdeadbeef", "platform": "native"},
{"instruction_addr": "0xbeefdead", "platform": "native"},
]

processed_frames = get_frames_for_symbolication(frames, None, None, "none")

assert not processed_frames[0]["adjust_instruction_addr"]
assert not processed_frames[1]["adjust_instruction_addr"]


@pytest.mark.django_db
@mock.patch("sentry.lang.native.processing.Symbolicator")
def test_il2cpp_symbolication(mock_symbolicator, default_project):
Expand Down
50 changes: 50 additions & 0 deletions tests/sentry/profiles/consumers/test_process.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from copy import copy
from datetime import datetime
from unittest.mock import Mock, patch

Expand All @@ -6,6 +7,7 @@
from arroyo.types import BrokerValue, Message, Partition, Topic

from sentry.profiles.consumers.process.factory import ProcessProfileStrategyFactory
from sentry.profiles.task import _prepare_frames_from_profile
from sentry.testutils.cases import TestCase
from sentry.utils import json

Expand Down Expand Up @@ -56,3 +58,51 @@ def test_basic_profile_to_celery(self, process_profile_task):
)

process_profile_task.assert_called_with(profile=profile)


def test_adjust_instruction_addr_sample_format():
original_frames = [
{"instruction_addr": "0xdeadbeef"},
{"instruction_addr": "0xbeefdead"},
{"instruction_addr": "0xfeedface"},
]
profile = {
"version": "1",
"profile": {
"frames": copy(original_frames),
"stacks": [[1, 0], [0, 1, 2]],
},
"debug_meta": {"images": []},
}

_, stacktraces = _prepare_frames_from_profile(profile)
assert profile["profile"]["stacks"] == [[3, 0], [4, 1, 2]]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correctly asserting that _prepare_frames_from_profile is mutating its parameter which is nice.
However, is that mutation also being observed end to end when the request comes back from symbolicator?

frames = stacktraces[0]["frames"]

for i in range(3):
assert frames[i] == original_frames[i]

assert frames[3] == {"instruction_addr": "0xbeefdead", "adjust_instruction_addr": False}
assert frames[4] == {"instruction_addr": "0xdeadbeef", "adjust_instruction_addr": False}


def test_adjust_instruction_addr_original_format():
profile = {
"sampled_profile": {
"samples": [
{
"frames": [
{"instruction_addr": "0xdeadbeef", "platform": "native"},
{"instruction_addr": "0xbeefdead", "platform": "native"},
],
}
]
},
"debug_meta": {"images": []},
}

_, stacktraces = _prepare_frames_from_profile(profile)
frames = stacktraces[0]["frames"]

assert not frames[0]["adjust_instruction_addr"]
assert "adjust_instruction_addr" not in frames[1]