Commit 9099ab87 authored by Mike Wittman's avatar Mike Wittman Committed by Commit Bot

[Sampling profiler] Clean up Unwinder::CanUnwindFrom interface

Passes |frame| by const reference rather than const pointer, to be
consistent with style.

Bug: 1035855
Change-Id: I783441fcfcbca46147bffc428a838a3920856f0c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091650
Commit-Queue: Scott Violet <sky@chromium.org>
Auto-Submit: Mike Wittman <wittman@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarEtienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751500}
parent 848fb7ef
...@@ -21,15 +21,15 @@ ChromeUnwinderAndroid::ChromeUnwinderAndroid( ...@@ -21,15 +21,15 @@ ChromeUnwinderAndroid::ChromeUnwinderAndroid(
ChromeUnwinderAndroid::~ChromeUnwinderAndroid() = default; ChromeUnwinderAndroid::~ChromeUnwinderAndroid() = default;
bool ChromeUnwinderAndroid::CanUnwindFrom(const Frame* current_frame) const { bool ChromeUnwinderAndroid::CanUnwindFrom(const Frame& current_frame) const {
return current_frame->module == chrome_module_; return current_frame.module == chrome_module_;
} }
UnwindResult ChromeUnwinderAndroid::TryUnwind(RegisterContext* thread_context, UnwindResult ChromeUnwinderAndroid::TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
ModuleCache* module_cache, ModuleCache* module_cache,
std::vector<Frame>* stack) const { std::vector<Frame>* stack) const {
DCHECK(CanUnwindFrom(&stack->back())); DCHECK(CanUnwindFrom(stack->back()));
do { do {
const ModuleCache::Module* module = stack->back().module; const ModuleCache::Module* module = stack->back().module;
uintptr_t pc = RegisterContextInstructionPointer(thread_context); uintptr_t pc = RegisterContextInstructionPointer(thread_context);
...@@ -44,7 +44,7 @@ UnwindResult ChromeUnwinderAndroid::TryUnwind(RegisterContext* thread_context, ...@@ -44,7 +44,7 @@ UnwindResult ChromeUnwinderAndroid::TryUnwind(RegisterContext* thread_context,
stack->emplace_back(RegisterContextInstructionPointer(thread_context), stack->emplace_back(RegisterContextInstructionPointer(thread_context),
module_cache->GetModuleForAddress( module_cache->GetModuleForAddress(
RegisterContextInstructionPointer(thread_context))); RegisterContextInstructionPointer(thread_context)));
} while (CanUnwindFrom(&stack->back())); } while (CanUnwindFrom(stack->back()));
return UnwindResult::UNRECOGNIZED_FRAME; return UnwindResult::UNRECOGNIZED_FRAME;
} }
......
...@@ -25,7 +25,7 @@ class BASE_EXPORT ChromeUnwinderAndroid : public Unwinder { ...@@ -25,7 +25,7 @@ class BASE_EXPORT ChromeUnwinderAndroid : public Unwinder {
ChromeUnwinderAndroid& operator=(const ChromeUnwinderAndroid&) = delete; ChromeUnwinderAndroid& operator=(const ChromeUnwinderAndroid&) = delete;
// Unwinder: // Unwinder:
bool CanUnwindFrom(const Frame* current_frame) const override; bool CanUnwindFrom(const Frame& current_frame) const override;
UnwindResult TryUnwind(RegisterContext* thread_context, UnwindResult TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
ModuleCache* module_cache, ModuleCache* module_cache,
......
...@@ -215,11 +215,8 @@ TEST(ChromeUnwinderAndroidTest, CanUnwindFrom) { ...@@ -215,11 +215,8 @@ TEST(ChromeUnwinderAndroidTest, CanUnwindFrom) {
ChromeUnwinderAndroid unwinder(cfi_table.get(), chrome_module.get()); ChromeUnwinderAndroid unwinder(cfi_table.get(), chrome_module.get());
Frame chrome_frame{0x1100, chrome_module.get()}; EXPECT_TRUE(unwinder.CanUnwindFrom({0x1100, chrome_module.get()}));
EXPECT_TRUE(unwinder.CanUnwindFrom(&chrome_frame)); EXPECT_FALSE(unwinder.CanUnwindFrom({0x2100, non_chrome_module.get()}));
Frame non_chrome_frame{0x2100, non_chrome_module.get()};
EXPECT_FALSE(unwinder.CanUnwindFrom(&non_chrome_frame));
} }
TEST(ChromeUnwinderAndroidTest, TryUnwind) { TEST(ChromeUnwinderAndroidTest, TryUnwind) {
......
...@@ -10,7 +10,7 @@ ...@@ -10,7 +10,7 @@
namespace base { namespace base {
bool NativeUnwinderAndroid::CanUnwindFrom(const Frame* current_frame) const { bool NativeUnwinderAndroid::CanUnwindFrom(const Frame& current_frame) const {
return false; return false;
} }
......
...@@ -21,7 +21,7 @@ class NativeUnwinderAndroid : public Unwinder { ...@@ -21,7 +21,7 @@ class NativeUnwinderAndroid : public Unwinder {
NativeUnwinderAndroid& operator=(const NativeUnwinderAndroid&) = delete; NativeUnwinderAndroid& operator=(const NativeUnwinderAndroid&) = delete;
// Unwinder // Unwinder
bool CanUnwindFrom(const Frame* current_frame) const override; bool CanUnwindFrom(const Frame& current_frame) const override;
UnwindResult TryUnwind(RegisterContext* thread_context, UnwindResult TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
ModuleCache* module_cache, ModuleCache* module_cache,
......
...@@ -130,8 +130,8 @@ NativeUnwinderMac::NativeUnwinderMac(ModuleCache* module_cache) ...@@ -130,8 +130,8 @@ NativeUnwinderMac::NativeUnwinderMac(ModuleCache* module_cache)
GetSigtrampRange(&sigtramp_start_, &sigtramp_end_); GetSigtrampRange(&sigtramp_start_, &sigtramp_end_);
} }
bool NativeUnwinderMac::CanUnwindFrom(const Frame* current_frame) const { bool NativeUnwinderMac::CanUnwindFrom(const Frame& current_frame) const {
return current_frame->module && current_frame->module->IsNative(); return current_frame.module && current_frame.module->IsNative();
} }
UnwindResult NativeUnwinderMac::TryUnwind(x86_thread_state64_t* thread_context, UnwindResult NativeUnwinderMac::TryUnwind(x86_thread_state64_t* thread_context,
......
...@@ -22,7 +22,7 @@ class NativeUnwinderMac : public Unwinder { ...@@ -22,7 +22,7 @@ class NativeUnwinderMac : public Unwinder {
NativeUnwinderMac& operator=(const NativeUnwinderMac&) = delete; NativeUnwinderMac& operator=(const NativeUnwinderMac&) = delete;
// Unwinder: // Unwinder:
bool CanUnwindFrom(const Frame* current_frame) const override; bool CanUnwindFrom(const Frame& current_frame) const override;
UnwindResult TryUnwind(RegisterContext* thread_context, UnwindResult TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
ModuleCache* module_cache, ModuleCache* module_cache,
......
...@@ -11,8 +11,8 @@ ...@@ -11,8 +11,8 @@
namespace base { namespace base {
bool NativeUnwinderWin::CanUnwindFrom(const Frame* current_frame) const { bool NativeUnwinderWin::CanUnwindFrom(const Frame& current_frame) const {
return current_frame->module && current_frame->module->IsNative(); return current_frame.module && current_frame.module->IsNative();
} }
// Attempts to unwind the frame represented by the context values. If // Attempts to unwind the frame represented by the context values. If
......
...@@ -19,7 +19,7 @@ class NativeUnwinderWin : public Unwinder { ...@@ -19,7 +19,7 @@ class NativeUnwinderWin : public Unwinder {
NativeUnwinderWin& operator=(const NativeUnwinderWin&) = delete; NativeUnwinderWin& operator=(const NativeUnwinderWin&) = delete;
// Unwinder: // Unwinder:
bool CanUnwindFrom(const Frame* current_frame) const override; bool CanUnwindFrom(const Frame& current_frame) const override;
UnwindResult TryUnwind(RegisterContext* thread_context, UnwindResult TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
ModuleCache* module_cache, ModuleCache* module_cache,
......
...@@ -158,7 +158,7 @@ std::vector<Frame> StackSamplerImpl::WalkStack(ModuleCache* module_cache, ...@@ -158,7 +158,7 @@ std::vector<Frame> StackSamplerImpl::WalkStack(ModuleCache* module_cache,
// unwinder if it thinks it can unwind from the current frame, otherwise use // unwinder if it thinks it can unwind from the current frame, otherwise use
// the native unwinder. // the native unwinder.
Unwinder* unwinder = Unwinder* unwinder =
aux_unwinder && aux_unwinder->CanUnwindFrom(&stack.back()) aux_unwinder && aux_unwinder->CanUnwindFrom(stack.back())
? aux_unwinder ? aux_unwinder
: native_unwinder; : native_unwinder;
......
...@@ -116,7 +116,7 @@ class TestUnwinder : public Unwinder { ...@@ -116,7 +116,7 @@ class TestUnwinder : public Unwinder {
stack_copy_(stack_copy), stack_copy_(stack_copy),
stack_copy_bottom_(stack_copy_bottom) {} stack_copy_bottom_(stack_copy_bottom) {}
bool CanUnwindFrom(const Frame* current_frame) const override { return true; } bool CanUnwindFrom(const Frame& current_frame) const override { return true; }
UnwindResult TryUnwind(RegisterContext* thread_context, UnwindResult TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
...@@ -148,7 +148,7 @@ class CallRecordingUnwinder : public Unwinder { ...@@ -148,7 +148,7 @@ class CallRecordingUnwinder : public Unwinder {
update_modules_was_invoked_ = true; update_modules_was_invoked_ = true;
} }
bool CanUnwindFrom(const Frame* current_frame) const override { return true; } bool CanUnwindFrom(const Frame& current_frame) const override { return true; }
UnwindResult TryUnwind(RegisterContext* thread_context, UnwindResult TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
...@@ -238,7 +238,7 @@ class FakeTestUnwinder : public Unwinder { ...@@ -238,7 +238,7 @@ class FakeTestUnwinder : public Unwinder {
FakeTestUnwinder(const FakeTestUnwinder&) = delete; FakeTestUnwinder(const FakeTestUnwinder&) = delete;
FakeTestUnwinder& operator=(const FakeTestUnwinder&) = delete; FakeTestUnwinder& operator=(const FakeTestUnwinder&) = delete;
bool CanUnwindFrom(const Frame* current_frame) const override { bool CanUnwindFrom(const Frame& current_frame) const override {
bool can_unwind = results_[current_unwind_].can_unwind; bool can_unwind = results_[current_unwind_].can_unwind;
// NB: If CanUnwindFrom() returns false then TryUnwind() will not be // NB: If CanUnwindFrom() returns false then TryUnwind() will not be
// invoked, so current_unwind_ is guarantee to be incremented only once for // invoked, so current_unwind_ is guarantee to be incremented only once for
......
...@@ -550,7 +550,7 @@ class TestAuxUnwinder : public Unwinder { ...@@ -550,7 +550,7 @@ class TestAuxUnwinder : public Unwinder {
TestAuxUnwinder(const TestAuxUnwinder&) = delete; TestAuxUnwinder(const TestAuxUnwinder&) = delete;
TestAuxUnwinder& operator=(const TestAuxUnwinder&) = delete; TestAuxUnwinder& operator=(const TestAuxUnwinder&) = delete;
bool CanUnwindFrom(const Frame* current_frame) const override { return true; } bool CanUnwindFrom(const Frame& current_frame) const override { return true; }
UnwindResult TryUnwind(RegisterContext* thread_context, UnwindResult TryUnwind(RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
......
...@@ -58,7 +58,7 @@ class Unwinder { ...@@ -58,7 +58,7 @@ class Unwinder {
// of frames. Note that if the unwinder returns true it may still legitmately // of frames. Note that if the unwinder returns true it may still legitmately
// fail to unwind; e.g. in the case of a native unwind for a function that // fail to unwind; e.g. in the case of a native unwind for a function that
// doesn't have unwind information. // doesn't have unwind information.
virtual bool CanUnwindFrom(const Frame* current_frame) const = 0; virtual bool CanUnwindFrom(const Frame& current_frame) const = 0;
// Attempts to unwind the frame represented by the context values. // Attempts to unwind the frame represented by the context values.
// Walks the native frames on the stack pointed to by the stack pointer in // Walks the native frames on the stack pointed to by the stack pointer in
......
...@@ -127,8 +127,8 @@ void V8Unwinder::UpdateModules(base::ModuleCache* module_cache) { ...@@ -127,8 +127,8 @@ void V8Unwinder::UpdateModules(base::ModuleCache* module_cache) {
code_ranges_.ExpandCapacityIfNecessary(required_code_ranges_capacity_); code_ranges_.ExpandCapacityIfNecessary(required_code_ranges_capacity_);
} }
bool V8Unwinder::CanUnwindFrom(const base::Frame* current_frame) const { bool V8Unwinder::CanUnwindFrom(const base::Frame& current_frame) const {
const base::ModuleCache::Module* module = current_frame->module; const base::ModuleCache::Module* module = current_frame.module;
if (!module) if (!module)
return false; return false;
const auto loc = modules_.find(module); const auto loc = modules_.find(module);
......
...@@ -24,7 +24,7 @@ class V8Unwinder : public base::Unwinder { ...@@ -24,7 +24,7 @@ class V8Unwinder : public base::Unwinder {
// Unwinder: // Unwinder:
void OnStackCapture() override; void OnStackCapture() override;
void UpdateModules(base::ModuleCache* module_cache) override; void UpdateModules(base::ModuleCache* module_cache) override;
bool CanUnwindFrom(const base::Frame* current_frame) const override; bool CanUnwindFrom(const base::Frame& current_frame) const override;
base::UnwindResult TryUnwind(base::RegisterContext* thread_context, base::UnwindResult TryUnwind(base::RegisterContext* thread_context,
uintptr_t stack_top, uintptr_t stack_top,
base::ModuleCache* module_cache, base::ModuleCache* module_cache,
......
...@@ -403,8 +403,7 @@ TEST(V8UnwinderTest, CanUnwindFrom_V8Module) { ...@@ -403,8 +403,7 @@ TEST(V8UnwinderTest, CanUnwindFrom_V8Module) {
const base::ModuleCache::Module* module = module_cache.GetModuleForAddress(1); const base::ModuleCache::Module* module = module_cache.GetModuleForAddress(1);
ASSERT_NE(nullptr, module); ASSERT_NE(nullptr, module);
base::Frame frame{1, module}; EXPECT_TRUE(unwinder.CanUnwindFrom({1, module}));
EXPECT_TRUE(unwinder.CanUnwindFrom(&frame));
} }
TEST(V8UnwinderTest, CanUnwindFrom_OtherModule) { TEST(V8UnwinderTest, CanUnwindFrom_OtherModule) {
...@@ -420,8 +419,7 @@ TEST(V8UnwinderTest, CanUnwindFrom_OtherModule) { ...@@ -420,8 +419,7 @@ TEST(V8UnwinderTest, CanUnwindFrom_OtherModule) {
const base::ModuleCache::Module* other_module_ptr = other_module.get(); const base::ModuleCache::Module* other_module_ptr = other_module.get();
module_cache.AddCustomNativeModule(std::move(other_module)); module_cache.AddCustomNativeModule(std::move(other_module));
base::Frame frame{1, other_module_ptr}; EXPECT_FALSE(unwinder.CanUnwindFrom({1, other_module_ptr}));
EXPECT_FALSE(unwinder.CanUnwindFrom(&frame));
} }
TEST(V8UnwinderTest, CanUnwindFrom_NullModule) { TEST(V8UnwinderTest, CanUnwindFrom_NullModule) {
...@@ -434,8 +432,7 @@ TEST(V8UnwinderTest, CanUnwindFrom_NullModule) { ...@@ -434,8 +432,7 @@ TEST(V8UnwinderTest, CanUnwindFrom_NullModule) {
unwinder.OnStackCapture(); unwinder.OnStackCapture();
unwinder.UpdateModules(&module_cache); unwinder.UpdateModules(&module_cache);
base::Frame frame{20, nullptr}; EXPECT_FALSE(unwinder.CanUnwindFrom({20, nullptr}));
EXPECT_FALSE(unwinder.CanUnwindFrom(&frame));
} }
// Checks that unwinding from C++ through JavaScript and back into C++ succeeds. // Checks that unwinding from C++ through JavaScript and back into C++ succeeds.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment