Index: include/lldb/Breakpoint/BreakpointSiteList.h =================================================================== --- include/lldb/Breakpoint/BreakpointSiteList.h +++ include/lldb/Breakpoint/BreakpointSiteList.h @@ -15,6 +15,7 @@ #include #include #include +#include // Other libraries and framework includes // Project includes @@ -28,21 +29,16 @@ /// @brief Class that manages lists of BreakpointSite shared pointers. //---------------------------------------------------------------------- class BreakpointSiteList { - // At present Process directly accesses the map of BreakpointSites so it can - // do quick lookups into the map (using GetMap). - // FIXME: Find a better interface for this. - friend class Process; - public: //------------------------------------------------------------------ /// Default constructor makes an empty list. //------------------------------------------------------------------ - BreakpointSiteList(); + BreakpointSiteList() = default; //------------------------------------------------------------------ /// Destructor, currently does nothing. //------------------------------------------------------------------ - ~BreakpointSiteList(); + ~BreakpointSiteList() = default; //------------------------------------------------------------------ /// Add a BreakpointSite to the list. @@ -130,8 +126,12 @@ bool BreakpointSiteContainsBreakpoint(lldb::break_id_t bp_site_id, lldb::break_id_t bp_id); - void ForEach(std::function const &callback); + using ModifyingCallback = std::function; + bool ForEach(const ModifyingCallback &callback); + using Callback = std::function; + bool ForEach(const Callback &callback) const; + //------------------------------------------------------------------ /// Removes the breakpoint site given by \b breakID from this list. /// @@ -154,12 +154,12 @@ //------------------------------------------------------------------ bool RemoveByAddress(lldb::addr_t addr); - bool FindInRange(lldb::addr_t lower_bound, lldb::addr_t upper_bound, - BreakpointSiteList &bp_site_list) const; + using Range = std::list; + using Filter = std::add_pointer::type; + using Lock = std::unique_lock; - typedef void (*BreakpointSiteSPMapFunc)(lldb::BreakpointSiteSP &bp, - void *baton); - + Lock FindInRange(lldb::addr_t lower_bound, lldb::addr_t upper_bound, + Range &bp_site_list, Filter filter = nullptr) const; //------------------------------------------------------------------ /// Enquires of the breakpoint site on in this list with ID \a breakID whether /// we should stop for the breakpoint or not. Index: source/Breakpoint/BreakpointSiteList.cpp =================================================================== --- source/Breakpoint/BreakpointSiteList.cpp +++ source/Breakpoint/BreakpointSiteList.cpp @@ -11,21 +11,17 @@ // C Includes // C++ Includes +#include +#include // Other libraries and framework includes // Project includes #include "lldb/Utility/Stream.h" -#include using namespace lldb; using namespace lldb_private; -BreakpointSiteList::BreakpointSiteList() : m_mutex(), m_bp_site_list() {} - -BreakpointSiteList::~BreakpointSiteList() {} - // Add breakpoint site to the list. However, if the element already exists in -// the -// list, then we don't add it, and return LLDB_INVALID_BREAK_ID. +// the list, then we don't add it, and return LLDB_INVALID_BREAK_ID. lldb::break_id_t BreakpointSiteList::Add(const BreakpointSiteSP &bp) { lldb::addr_t bp_site_load_addr = bp->GetLoadAddress(); @@ -159,48 +155,60 @@ s->Printf("BreakpointSiteList with %u BreakpointSites:\n", (uint32_t)m_bp_site_list.size()); s->IndentMore(); - collection::const_iterator pos; - collection::const_iterator end = m_bp_site_list.end(); - for (pos = m_bp_site_list.begin(); pos != end; ++pos) - pos->second.get()->Dump(s); + + for (const auto &pair : m_bp_site_list) + pair.second->Dump(s); s->IndentLess(); } -void BreakpointSiteList::ForEach( - std::function const &callback) { +bool BreakpointSiteList::ForEach(const ModifyingCallback &callback) { std::lock_guard guard(m_mutex); - for (auto pair : m_bp_site_list) - callback(pair.second.get()); + for (auto &pair : m_bp_site_list) + if (!callback(*pair.second)) + return false; + + return true; } -bool BreakpointSiteList::FindInRange(lldb::addr_t lower_bound, - lldb::addr_t upper_bound, - BreakpointSiteList &bp_site_list) const { +bool BreakpointSiteList::ForEach(const Callback &callback) const { + std::lock_guard guard(m_mutex); + for (const auto &pair : m_bp_site_list) + if (!callback(*pair.second)) + return false; + + return true; +} + +BreakpointSiteList::Lock +BreakpointSiteList::FindInRange(lldb::addr_t lower_bound, + lldb::addr_t upper_bound, + BreakpointSiteList::Range &bp_site_list, + BreakpointSiteList::Filter filter) const { if (lower_bound > upper_bound) - return false; + return Lock(); - std::lock_guard guard(m_mutex); - collection::const_iterator lower, upper, pos; - lower = m_bp_site_list.lower_bound(lower_bound); - if (lower == m_bp_site_list.end() || (*lower).first >= upper_bound) - return false; + Lock lock(m_mutex); + auto lower = m_bp_site_list.lower_bound(lower_bound); // This is one tricky bit. The breakpoint might overlap the bottom end of the - // range. So we grab the - // breakpoint prior to the lower bound, and check that that + its byte size - // isn't in our range. + // range. So we grab the breakpoint prior to the lower bound, + // and check that that + its byte size isn't in our range. if (lower != m_bp_site_list.begin()) { - collection::const_iterator prev_pos = lower; - prev_pos--; + auto prev_pos = std::prev(lower); const BreakpointSiteSP &prev_bp = (*prev_pos).second; if (prev_bp->GetLoadAddress() + prev_bp->GetByteSize() > lower_bound) - bp_site_list.Add(prev_bp); + lower = prev_pos; } - upper = m_bp_site_list.upper_bound(upper_bound); + if (lower == m_bp_site_list.end() || (*lower).first >= upper_bound) + return Lock(); - for (pos = lower; pos != upper; pos++) { - bp_site_list.Add((*pos).second); + auto upper = m_bp_site_list.upper_bound(upper_bound); + + for (auto pos = lower; pos != upper; ++pos) { + if (nullptr == filter || filter(*(*pos).second)) + bp_site_list.push_back((*pos).second); } - return true; + + return std::move(lock); } Index: source/Target/Process.cpp =================================================================== --- source/Target/Process.cpp +++ source/Target/Process.cpp @@ -1813,9 +1813,8 @@ } void Process::DisableAllBreakpointSites() { - m_breakpoint_site_list.ForEach([this](BreakpointSite *bp_site) -> void { - // bp_site->SetEnabled(true); - DisableBreakpointSite(bp_site); + m_breakpoint_site_list.ForEach([this](BreakpointSite &bp_site) { + return DisableBreakpointSite(&bp_site).Success(); }); } @@ -1961,29 +1960,26 @@ size_t Process::RemoveBreakpointOpcodesFromBuffer(addr_t bp_addr, size_t size, uint8_t *buf) const { size_t bytes_removed = 0; - BreakpointSiteList bp_sites_in_range; + BreakpointSiteList::Range bp_sites_in_range; - if (m_breakpoint_site_list.FindInRange(bp_addr, bp_addr + size, - bp_sites_in_range)) { - bp_sites_in_range.ForEach([bp_addr, size, - buf](BreakpointSite *bp_site) -> void { - if (bp_site->GetType() == BreakpointSite::eSoftware) { - addr_t intersect_addr; - size_t intersect_size; - size_t opcode_offset; - if (bp_site->IntersectsRange(bp_addr, size, &intersect_addr, - &intersect_size, &opcode_offset)) { - assert(bp_addr <= intersect_addr && intersect_addr < bp_addr + size); - assert(bp_addr < intersect_addr + intersect_size && - intersect_addr + intersect_size <= bp_addr + size); - assert(opcode_offset + intersect_size <= bp_site->GetByteSize()); - size_t buf_offset = intersect_addr - bp_addr; - ::memcpy(buf + buf_offset, - bp_site->GetSavedOpcodeBytes() + opcode_offset, - intersect_size); - } - } - }); + auto optional_lock = m_breakpoint_site_list.FindInRange( + bp_addr, bp_addr + size, bp_sites_in_range, + [](const BreakpointSite &bp_site) { return bp_site.IsEnabled(); }); + + for (auto &bp_site : bp_sites_in_range) { + addr_t intersect_addr; + size_t intersect_size; + size_t opcode_offset; + if (bp_site->IntersectsRange(bp_addr, size, &intersect_addr, + &intersect_size, &opcode_offset)) { + assert(bp_addr <= intersect_addr && intersect_addr < bp_addr + size); + assert(bp_addr < intersect_addr + intersect_size && + intersect_addr + intersect_size <= bp_addr + size); + assert(opcode_offset + intersect_size <= bp_site->GetByteSize()); + size_t buf_offset = intersect_addr - bp_addr; + ::memcpy(buf + buf_offset, bp_site->GetSavedOpcodeBytes() + opcode_offset, + intersect_size); + } } return bytes_removed; } @@ -2407,7 +2403,7 @@ } size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size, - Status &error) { + Status &status) { #if defined(ENABLE_MEMORY_CACHING) m_memory_cache.Flush(addr, size); #endif @@ -2417,72 +2413,36 @@ m_mod_id.BumpMemoryID(); - // We need to write any data that would go where any current software traps - // (enabled software breakpoints) any software traps (breakpoints) that we - // may have placed in our tasks memory. + Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS)); + auto update_status = [&status, log, this](const Status &op_status) { + if (op_status.Fail()) { + if (log) + log->Printf("Process pid %" PRIu64 ": %s", GetID(), + op_status.AsCString()); - BreakpointSiteList bp_sites_in_range; + if (!status.Fail()) + status.SetErrorString( + "failed to update one or more breakpoints in range"); + } + }; - if (m_breakpoint_site_list.FindInRange(addr, addr + size, - bp_sites_in_range)) { - // No breakpoint sites overlap - if (bp_sites_in_range.IsEmpty()) - return WriteMemoryPrivate(addr, buf, size, error); - else { - const uint8_t *ubuf = (const uint8_t *)buf; - uint64_t bytes_written = 0; + BreakpointSiteList::Range enabled_bp_sites_in_range; - bp_sites_in_range.ForEach([this, addr, size, &bytes_written, &ubuf, - &error](BreakpointSite *bp) -> void { + auto optional_lock = m_breakpoint_site_list.FindInRange( + addr, addr + size, enabled_bp_sites_in_range, + [](const BreakpointSite &bp_site) { return bp_site.IsEnabled(); }); - if (error.Success()) { - addr_t intersect_addr; - size_t intersect_size; - size_t opcode_offset; - const bool intersects = bp->IntersectsRange( - addr, size, &intersect_addr, &intersect_size, &opcode_offset); - UNUSED_IF_ASSERT_DISABLED(intersects); - assert(intersects); - assert(addr <= intersect_addr && intersect_addr < addr + size); - assert(addr < intersect_addr + intersect_size && - intersect_addr + intersect_size <= addr + size); - assert(opcode_offset + intersect_size <= bp->GetByteSize()); + for (auto &bp_site : enabled_bp_sites_in_range) + update_status(DisableBreakpointSite(bp_site.get())); - // Check for bytes before this breakpoint - const addr_t curr_addr = addr + bytes_written; - if (intersect_addr > curr_addr) { - // There are some bytes before this breakpoint that we need to - // just write to memory - size_t curr_size = intersect_addr - curr_addr; - size_t curr_bytes_written = WriteMemoryPrivate( - curr_addr, ubuf + bytes_written, curr_size, error); - bytes_written += curr_bytes_written; - if (curr_bytes_written != curr_size) { - // We weren't able to write all of the requested bytes, we - // are done looping and will return the number of bytes that - // we have written so far. - if (error.Success()) - error.SetErrorToGenericError(); - } - } - // Now write any bytes that would cover up any software breakpoints - // directly into the breakpoint opcode buffer - ::memcpy(bp->GetSavedOpcodeBytes() + opcode_offset, - ubuf + bytes_written, intersect_size); - bytes_written += intersect_size; - } - }); + size_t written = WriteMemoryPrivate(addr, buf, size, status); - if (bytes_written < size) - WriteMemoryPrivate(addr + bytes_written, ubuf + bytes_written, - size - bytes_written, error); - } - } else { - return WriteMemoryPrivate(addr, buf, size, error); + if (status.Success()) { + for (auto &bp_site : enabled_bp_sites_in_range) + update_status(EnableBreakpointSite(bp_site.get())); } - // Write any remaining bytes after the last breakpoint if we have any left - return 0; // bytes_written; + return written; } size_t Process::WriteScalarToMemory(addr_t addr, const Scalar &scalar, Index: unittests/Process/CMakeLists.txt =================================================================== --- unittests/Process/CMakeLists.txt +++ unittests/Process/CMakeLists.txt @@ -1,3 +1,4 @@ +add_subdirectory(common) add_subdirectory(gdb-remote) if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android") add_subdirectory(Linux) Index: unittests/Process/common/CMakeLists.txt =================================================================== --- /dev/null +++ unittests/Process/common/CMakeLists.txt @@ -0,0 +1,7 @@ +add_lldb_unittest(LLDBProcessTests + ProcessWriteMemoryTest.cpp + + LINK_LIBS + lldbBreakpoint + lldbTarget + ) \ No newline at end of file Index: unittests/Process/common/ProcessWriteMemoryTest.cpp =================================================================== --- /dev/null +++ unittests/Process/common/ProcessWriteMemoryTest.cpp @@ -0,0 +1,289 @@ +//===-- ProcessorWriteMemoryTest.cpp -------------------------- -*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "lldb/Breakpoint/BreakpointLocation.h" +#include "lldb/Breakpoint/BreakpointResolver.h" +#include "lldb/Core/Debugger.h" +#include "lldb/Core/PluginManager.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Target/Process.h" + +#include "gtest/gtest.h" + +#include +#include +#include +#include + +using Status = lldb_private::Status; + +class FakeProcess : public lldb_private::Process { + using lldb_private::Process::Process; + +public: + static lldb_private::ConstString GetName() { + return lldb_private::ConstString("fake-process"); + } + + size_t ReadMemoryAsIs(lldb::addr_t addr, void *buf, size_t size, + Status &error) { + return DoReadMemory(addr, buf, size, error); + } + +private: + lldb_private::ConstString GetPluginName() override { return GetName(); } + + uint32_t GetPluginVersion() override { return 0; } + + bool CanDebug(lldb::TargetSP, bool) override { return true; } + Status DoDestroy() override { return Status(); } + void RefreshStateAfterStop() override {} + + size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size, + Status &error) override { + auto mem_size = m_memory.size(); + auto to_copy = + addr + size <= mem_size ? size : addr >= mem_size ? 0 : mem_size - addr; + memcpy(buf, m_memory.data() + addr, to_copy); + memset(static_cast(buf) + to_copy, 0, size - to_copy); + return size; + } + + size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size, + Status &error) override { + auto mem_size = m_memory.size(); + if (addr + size > mem_size) + m_memory.resize(addr + size); + + memcpy(m_memory.data() + addr, buf, size); + return size; + } + + Status EnableBreakpointSite(lldb_private::BreakpointSite *bp_site) override { + return EnableSoftwareBreakpoint(bp_site); + } + + Status DisableBreakpointSite(lldb_private::BreakpointSite *bp_site) override { + return DisableSoftwareBreakpoint(bp_site); + } + + bool UpdateThreadList(lldb_private::ThreadList &old_thread_list, + lldb_private::ThreadList &new_thread_list) override { + new_thread_list.Update(old_thread_list); + return true; + } + + std::vector m_memory; +}; + +using FakeProcessSP = std::shared_ptr; + +class FakePlatform : public lldb_private::Platform { +public: + FakePlatform() : lldb_private::Platform(false) {} + + using TrapOpcode = std::array; + static const TrapOpcode &GetTrapOpcode() { + static const TrapOpcode trap_opcode = {0x1, 0x2, 0x3, 0x4}; + return trap_opcode; + } + +private: + size_t GetSoftwareBreakpointTrapOpcode( + lldb_private::Target &target, + lldb_private::BreakpointSite *bp_site) override { + assert(bp_site); + auto &trap_opcode = GetTrapOpcode(); + return bp_site->SetTrapOpcode(trap_opcode.data(), trap_opcode.size()) + ? trap_opcode.size() + : 0; + } + const char *GetDescription() override { return ""; } + + lldb_private::ConstString GetPluginName() override { + return lldb_private::ConstString(); + } + + uint32_t GetPluginVersion() override { return 0; } + + bool GetSupportedArchitectureAtIndex(uint32_t, + lldb_private::ArchSpec &) override { + return false; + } + + lldb::ProcessSP Attach(lldb_private::ProcessAttachInfo &, + lldb_private::Debugger &debugger, + lldb_private::Target *target, Status &error) { + return lldb::ProcessSP(); + } + + void CalculateTrapHandlerSymbolNames() override {} +}; + +struct WriteMemoryTest : public ::testing::Test { + void SetUp() override { + lldb_private::HostInfo::Initialize(); + + lldb_private::PluginManager::RegisterPlugin( + FakeProcess::GetName(), nullptr, + [](lldb::TargetSP target, lldb::ListenerSP listener, + const lldb_private::FileSpec *) { + return lldb::ProcessSP(new FakeProcess(target, listener)); + }); + + platform = std::make_shared(); + lldb_private::Platform::SetHostPlatform(platform); + + debugger = lldb_private::Debugger::CreateInstance(nullptr, nullptr); + debugger->GetPlatformList().SetSelectedPlatform(platform); + debugger->GetTargetList().CreateTarget(*debugger, llvm::StringRef(), + lldb_private::ArchSpec(), false, + platform, target); + + ASSERT_TRUE(target); + + process = std::static_pointer_cast( + target->CreateProcess(debugger->GetListener(), + FakeProcess::GetName().GetStringRef(), nullptr)); + ASSERT_TRUE(process); + InitializeBuffers(); + } + + void TearDown() override { lldb_private::HostInfo::Terminate(); } + + void SetBreakpoint(lldb::addr_t addr) { + ASSERT_TRUE( + target->CreateBreakpoint(lldb_private::Address(addr), false, false)); + } + + void CheckSavedOpcodes(); + void CheckTrapOpcode(lldb::addr_t addr); + void WriteAndCheckData(lldb::addr_t addr, size_t count); + + void InitializeBuffers() { + wbuf.fill(0xffu); + rbuf.fill(0u); + } + + lldb::DebuggerSP debugger; + lldb::PlatformSP platform; + lldb::TargetSP target; + FakeProcessSP process; + std::array wbuf, rbuf; + const FakePlatform::TrapOpcode &trap_opcode = FakePlatform::GetTrapOpcode(); +}; + +void WriteMemoryTest::CheckSavedOpcodes() { + auto result = process->GetBreakpointSiteList().ForEach( + [this](const lldb_private::BreakpointSite &bp_site) { + bool are_same = + (0 == memcmp(bp_site.GetSavedOpcodeBytes(), + wbuf.data() + bp_site.GetLoadAddress(), + bp_site.GetByteSize())); + return are_same; + }); + ASSERT_TRUE(result); +} + +void WriteMemoryTest::CheckTrapOpcode(lldb::addr_t addr) { + Status status; + auto read = + process->ReadMemoryAsIs(addr, rbuf.data(), trap_opcode.size(), status); + ASSERT_TRUE(status.Success() && trap_opcode.size() == read); + ASSERT_TRUE(0 == memcmp(trap_opcode.data(), rbuf.data(), trap_opcode.size())); +} + +void WriteMemoryTest::WriteAndCheckData(lldb::addr_t addr, size_t count) { + Status status; + auto written = process->WriteMemory(addr, wbuf.data() + addr, count, status); + ASSERT_TRUE(status.Success() && count == written); + + auto read = process->ReadMemory(0, rbuf.data(), rbuf.size(), status); + ASSERT_TRUE(status.Success() && rbuf.size() == read); + + ASSERT_TRUE(0 == memcmp(wbuf.data(), rbuf.data(), rbuf.size())); +} + +TEST_F(WriteMemoryTest, NoBreakpointsInRange) { + // Without any breakpoints. + WriteAndCheckData(0, wbuf.size()); + + // Breakpoints are located beyond the area that will be re-written. + const size_t offset = trap_opcode.size(); + const size_t count = wbuf.size() - 2 * offset; + + const lldb::addr_t bp1_addr = 0; + SetBreakpoint(bp1_addr); + const lldb::addr_t bp2_addr = wbuf.size() - offset; + SetBreakpoint(bp2_addr); + + // SetBreakpoint is not fake;) + ASSERT_EQ(2u, target->GetBreakpointList().GetSize()); + ASSERT_EQ(2u, process->GetBreakpointSiteList().GetSize()); + + // Write and check memory between breakpoints. + for (size_t i = offset; i < count; ++i) + --wbuf[i]; + WriteAndCheckData(offset, count); + + // Breakpoints still here. + ASSERT_EQ(2u, process->GetBreakpointSiteList().GetSize()); + + // Breakpoint opcode before area is not damaged. + CheckTrapOpcode(bp1_addr); + + // Breakpoint opcode after area is not damaged. + CheckTrapOpcode(bp2_addr); + + // Saved opcodes are updated. + CheckSavedOpcodes(); +} + +TEST_F(WriteMemoryTest, BreakpointOverlapsLowerBound) { + WriteAndCheckData(0, wbuf.size()); + + const lldb::addr_t bp_addr = 0; + const lldb::addr_t bp_size = trap_opcode.size(); + const lldb::addr_t offset = bp_addr + bp_size / 2; + SetBreakpoint(bp_addr); + + // Write memory starting from middle of breakpoint. + for (size_t i = offset; i < bp_size + offset; ++i) + --wbuf[i]; + WriteAndCheckData(offset, bp_size); + + // Breakpoint still here. + ASSERT_EQ(1u, process->GetBreakpointSiteList().GetSize()); + + // Breakpoint opcode is not damaged. + CheckTrapOpcode(bp_addr); + + // A half of saved opcode is updated. Quite absurd and dangerous situation... + CheckSavedOpcodes(); +} + +TEST_F(WriteMemoryTest, BreakpointInRange) { + WriteAndCheckData(0, wbuf.size()); + + const lldb::addr_t bp_addr = wbuf.size() / 2; + const lldb::addr_t bp_size = trap_opcode.size(); + SetBreakpoint(bp_addr); + + wbuf.fill(0u); + WriteAndCheckData(0, wbuf.size()); + + // Breakpoint still here. + ASSERT_EQ(1u, process->GetBreakpointSiteList().GetSize()); + + // Breakpoint opcode is not damaged. + CheckTrapOpcode(bp_addr); + + // Saved opcode is updated. + CheckSavedOpcodes(); +}