Re: [Lldb-commits] [PATCH] D21221: Fix for PrintStackTraces
ravitheja added a comment. Well its not possible for the assembly unwinder to do the correct thing in this situation , as the function is entered through a stub function (which has push instruction) and then a jump. The point of this patch is to catch when to use the CFI or the assembly unwinder. http://reviews.llvm.org/D21221 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D21280: Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets
omjavaid created this revision.
omjavaid added reviewers: labath, clayborg.
omjavaid added a subscriber: lldb-commits.
Herald added subscribers: danalbert, tberghammer, rengolin, aemerson.
This patch adds logic to make sure we can install watchpoints at 1,2 and 4 byte
alligned addresses.
ptrace interface allows watchpoints at 8-byte alligned addresses. Therefor for
all lower allignment levels we have to watch full 8-bytes.
We will ignore all irrelevant watchpoint trigger exceptions and will continue
the target after stepping over the watchpoint instruction.
In worst case while watching a 8-byte array like byteArr[8] we may have to
ignore 7 false watchpoint hits if we install watchpoint at the last byte in the
array.
However overall advantage of this solution overwhelms this disadvantage. We now
have all watchpoint tests passing on AArch64 Linux (HiKey board) and Arm64
Android (Nexus9).
http://reviews.llvm.org/D21280
Files:
source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2059,7 +2059,8 @@
{
WatchpointSP wp_sp;
ArchSpec::Core core = GetTarget().GetArchitecture().GetCore();
-if (core >= ArchSpec::kCore_mips_first && core <= ArchSpec::kCore_mips_last)
+if ((core >= ArchSpec::kCore_mips_first && core <= ArchSpec::kCore_mips_last) ||
+(core >= ArchSpec::eCore_arm_arm64 && core <= ArchSpec::eCore_arm_aarch64))
wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_hit_addr);
if (!wp_sp)
wp_sp = GetTarget().GetWatchpointList().FindByAddress(wp_addr);
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
@@ -74,6 +74,9 @@
GetWatchpointHitIndex(uint32_t &wp_index, lldb::addr_t trap_addr) override;
lldb::addr_t
+GetWatchpointHitAddress (uint32_t wp_index) override;
+
+lldb::addr_t
GetWatchpointAddress (uint32_t wp_index) override;
uint32_t
@@ -161,6 +164,8 @@
struct DREG
{
lldb::addr_t address; // Breakpoint/watchpoint address value.
+lldb::addr_t hit_addr; // Address at which last watchpoint trigger exception occurred.
+lldb::addr_t real_addr; // Address value that should cause target to stop.
uint32_t control; // Breakpoint/watchpoint control value.
uint32_t refcount; // Serves as enable/disable and refernce counter.
};
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -566,6 +566,7 @@
return LLDB_INVALID_INDEX32;
uint32_t control_value = 0, wp_index = 0;
+lldb::addr_t real_addr = addr;
// Check if we are setting watchpoint other than read/write/access
// Also update watchpoint flag to match AArch64 write-read bit configuration.
@@ -588,9 +589,23 @@
return LLDB_INVALID_INDEX32;
// Check 8-byte alignment for hardware watchpoint target address.
-// TODO: Add support for watching un-aligned addresses
+// Below is a hack to recalculate address and size in order to
+// make sure we can watch non 8-byte alligned addresses as well.
if (addr & 0x07)
-return LLDB_INVALID_INDEX32;
+{
+uint8_t watch_mask = (addr & 0x07) + size;
+
+if (watch_mask > 0x08)
+return LLDB_INVALID_INDEX32;
+else if (watch_mask <= 0x02)
+size = 2;
+else if (watch_mask <= 0x04)
+size = 4;
+else
+size = 8;
+
+addr = addr & (~0x07);
+}
// Setup control value
control_value = watch_flags << 3;
@@ -620,6 +635,7 @@
if ((m_hwp_regs[wp_index].control & 1) == 0)
{
// Update watchpoint in local cache
+m_hwp_regs[wp_index].real_addr = real_addr;
m_hwp_regs[wp_index].address = addr;
m_hwp_regs[wp_index].control = control_value;
m_hwp_regs[wp_index].refcount = 1;
@@ -801,6 +817,7 @@
if (m_hwp_regs[wp_index].refcount >= 1 &
Re: [Lldb-commits] [PATCH] D20368: Remove Platform usages from NativeProcessLinux
nitesh.jain added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:438 @@ +437,3 @@ +// The auxiliary vector consists of a sequence of key-value pairs, where key and value are of +// the pointer size for the architecture that the process is running on. We can use this to +// detect the process architecture. We will try reading the vector as if it were a 64-bit labath wrote: > nitesh.jain wrote: > > There are three flavours of ABI, > > > > | ABI | pointer-size | Arch | Processs Type > > | O32 |4 | Mips32 | 32 bit > > | N32|4 | Mips64 | 32 bit > > | N64|8 | Mips64 | 64 bit > > > > So not sure whether it will work for "N32", I will check and let you know > > asap. > Uh-oh. :) > > So, I don't think the ABI should matter much to the native register context. > What matters here is the registers and their sizes. I'm guessing the N32 > thing is something like the x32 intel abi, wher all registers are 64-bit, but > sizeof(void*) is 32-bit. In that case this function will not work correctly, > as it will detect it as 32-bit (but the NT_PRSTATUS-based one would, I guess). > > Does N32 work on pre-3.13 kernels? (Grepping the source seems to find > mentions of it, but i don't know how well it actually worked). > > I guess is back to drawing board with this one again. Could you please send > me the contents of the N32 auxiliary vector (LD_SHOW_AUXV as well as the > actual binary contents). I presume the one you sent me earlier was O32. > > cheers, > pl In N32 ABI, all GPR register are 64 bit and sizeof(void *) is 32 bit. Yes, N32 ABI works on pre-3.13 kernels. I have send earlier aux for O32 ABI . Thanks, Nj http://reviews.llvm.org/D20368 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D20565: Add MemoryRegionInfo to SB API
hhellyer updated this revision to Diff 60517.
hhellyer added a comment.
Updated to rebase on the latest changes to MemoryRegionInfo.h
http://reviews.llvm.org/D20565
Files:
include/lldb/API/LLDB.h
include/lldb/API/SBDefines.h
include/lldb/API/SBMemoryRegionInfo.h
include/lldb/API/SBMemoryRegionInfoList.h
include/lldb/API/SBProcess.h
include/lldb/API/SBStream.h
include/lldb/Target/MemoryRegionInfo.h
include/lldb/Target/Process.h
include/lldb/lldb-forward.h
source/API/CMakeLists.txt
source/API/SBMemoryRegionInfo.cpp
source/API/SBMemoryRegionInfoList.cpp
source/API/SBProcess.cpp
Index: source/API/SBProcess.cpp
===
--- source/API/SBProcess.cpp
+++ source/API/SBProcess.cpp
@@ -23,6 +23,7 @@
#include "lldb/Core/State.h"
#include "lldb/Core/Stream.h"
#include "lldb/Core/StreamFile.h"
+#include "lldb/Target/MemoryRegionInfo.h"
#include "lldb/Target/Process.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/SystemRuntime.h"
@@ -36,6 +37,8 @@
#include "lldb/API/SBDebugger.h"
#include "lldb/API/SBEvent.h"
#include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBMemoryRegionInfo.h"
+#include "lldb/API/SBMemoryRegionInfoList.h"
#include "lldb/API/SBThread.h"
#include "lldb/API/SBThreadCollection.h"
#include "lldb/API/SBStream.h"
@@ -1476,3 +1479,74 @@
error.ref() = PluginManager::SaveCore(process_sp, core_file);
return error;
}
+
+lldb::SBError
+SBProcess::GetMemoryRegionInfo (lldb::addr_t load_addr, SBMemoryRegionInfo &sb_region_info)
+{
+lldb::SBError sb_error;
+ProcessSP process_sp(GetSP());
+MemoryRegionInfoSP region_info_sp = std::make_shared();
+if (process_sp)
+{
+Process::StopLocker stop_locker;
+if (stop_locker.TryLock(&process_sp->GetRunLock()))
+{
+std::lock_guard guard(process_sp->GetTarget().GetAPIMutex());
+sb_error.ref() = process_sp->GetMemoryRegionInfo(load_addr, *region_info_sp);
+if( sb_error.Success() ) {
+sb_region_info.ref() = *region_info_sp;
+}
+}
+else
+{
+Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
+if (log)
+log->Printf ("SBProcess(%p)::GetMemoryRegionInfo() => error: process is running",
+ static_cast(process_sp.get()));
+sb_error.SetErrorString("process is running");
+}
+}
+else
+{
+sb_error.SetErrorString ("SBProcess is invalid");
+}
+return sb_error;
+}
+
+lldb::SBMemoryRegionInfoList
+SBProcess::GetMemoryRegions()
+{
+lldb::SBError sb_error;
+lldb::SBMemoryRegionInfoList sb_region_list;
+ProcessSP process_sp(GetSP());
+if (process_sp)
+{
+Process::StopLocker stop_locker;
+if (stop_locker.TryLock(&process_sp->GetRunLock()))
+{
+std::lock_guard guard(process_sp->GetTarget().GetAPIMutex());
+std::vector region_list;
+sb_error.ref() = process_sp->GetMemoryRegions(region_list);
+if( sb_error.Success() ) {
+std::vector::iterator end = region_list.end();
+for( std::vector::iterator it = region_list.begin(); it != end; it++ ) {
+SBMemoryRegionInfo sb_region_info(it->get());
+sb_region_list.Append(sb_region_info);
+}
+}
+}
+else
+{
+Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_API));
+if (log)
+log->Printf ("SBProcess(%p)::GetMemoryRegionInfo() => error: process is running",
+ static_cast(process_sp.get()));
+sb_error.SetErrorString("process is running");
+}
+}
+else
+{
+sb_error.SetErrorString ("SBProcess is invalid");
+}
+return sb_region_list;
+}
Index: source/API/SBMemoryRegionInfoList.cpp
===
--- /dev/null
+++ source/API/SBMemoryRegionInfoList.cpp
@@ -0,0 +1,162 @@
+//===-- SBMemoryRegionInfoList.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/API/SBMemoryRegionInfo.h"
+#include "lldb/API/SBMemoryRegionInfoList.h"
+#include "lldb/API/SBStream.h"
+#include "lldb/Core/Log.h"
+#include "lldb/Target/MemoryRegionInfo.h"
+
+#include
+
+using namespace lldb;
+using namespace lldb_private;
+
+class MemoryRegionInfoListImpl
+{
+public:
+MemoryRegionInfoListImpl () :
+m_regions()
+{
+}
+
+MemoryRegionInfoListImpl (const MemoryRegionInfoListImpl& rhs) :
+m_regions(rhs.m_regions)
+{
+}
+
+
Re: [Lldb-commits] [PATCH] D21221: Fix for PrintStackTraces
ravitheja updated this revision to Diff 60533.
ravitheja added a comment.
Making EH Frame CFI the full unwinder
when artificial symbols are found.
http://reviews.llvm.org/D21221
Files:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
source/Plugins/Process/Utility/RegisterContextLLDB.cpp
Index: source/Plugins/Process/Utility/RegisterContextLLDB.cpp
===
--- source/Plugins/Process/Utility/RegisterContextLLDB.cpp
+++ source/Plugins/Process/Utility/RegisterContextLLDB.cpp
@@ -210,12 +210,28 @@
m_frame_type = eNormalFrame;
}
+// We've set m_frame_type and m_sym_ctx before these calls.
+
+m_fast_unwind_plan_sp = GetFastUnwindPlanForFrame ();
+m_full_unwind_plan_sp = GetFullUnwindPlanForFrame ();
+
// If we were able to find a symbol/function, set addr_range to the bounds of that symbol/function.
// else treat the current pc value as the start_pc and record no offset.
if (addr_range.GetBaseAddress().IsValid())
{
m_start_pc = addr_range.GetBaseAddress();
-if (m_current_pc.GetSection() == m_start_pc.GetSection())
+if (m_sym_ctx.symbol->IsSynthetic())
+{
+// The current offset should be recalculated here. The m_current_offset is
+// calculated from the base address of the symbol. The symbol can lie in the PLT
+// (Procedure Linkage Table) i.e its a symbol stub for external call. In this case
+// the base address for the unwindplan and the base address of the symbol maybe different, hence
+// the m_current_offset will be wrong.
+AddressRange unwind_address_range = m_full_unwind_plan_sp->GetAddressRange();
+if (unwind_address_range.ContainsFileAddress(m_current_pc))
+m_current_offset = m_current_pc.GetOffset() - unwind_address_range.GetBaseAddress().GetOffset();
+}
+else if (m_current_pc.GetSection() == m_start_pc.GetSection())
{
m_current_offset = m_current_pc.GetOffset() - m_start_pc.GetOffset();
}
@@ -236,11 +252,6 @@
m_current_offset_backed_up_one = -1;
}
-// We've set m_frame_type and m_sym_ctx before these calls.
-
-m_fast_unwind_plan_sp = GetFastUnwindPlanForFrame ();
-m_full_unwind_plan_sp = GetFullUnwindPlanForFrame ();
-
UnwindPlan::RowSP active_row;
lldb::RegisterKind row_register_kind = eRegisterKindGeneric;
if (m_full_unwind_plan_sp && m_full_unwind_plan_sp->PlanValidAtAddress (m_current_pc))
@@ -255,36 +266,14 @@
}
}
-if (!active_row.get())
-{
-UnwindLogMsg ("could not find an unwindplan row for this frame's pc");
-m_frame_type = eNotAValidFrame;
-return;
-}
-
-
if (!ReadCFAValueForRow (row_register_kind, active_row, m_cfa))
{
// Try the fall back unwind plan since the
// full unwind plan failed.
-FuncUnwindersSP func_unwinders_sp;
-UnwindPlanSP call_site_unwind_plan;
bool cfa_status = false;
+if (TryFallbackUnwindPlan())
+cfa_status = true;
-if (m_sym_ctx_valid)
-{
-func_unwinders_sp = pc_module_sp->GetObjectFile()->GetUnwindTable().GetFuncUnwindersContainingAddress (m_current_pc, m_sym_ctx);
-}
-
-if(func_unwinders_sp.get() != nullptr)
-call_site_unwind_plan = func_unwinders_sp->GetUnwindPlanAtCallSite(process->GetTarget(), m_current_offset_backed_up_one);
-
-if (call_site_unwind_plan.get() != nullptr)
-{
-m_fallback_unwind_plan_sp = call_site_unwind_plan;
-if(TryFallbackUnwindPlan())
-cfa_status = true;
-}
if (!cfa_status)
{
UnwindLogMsg ("could not read CFA value for first frame.");
@@ -881,6 +870,8 @@
// call GetUnwindPlanAtCallSite() -- because CallSite may return an unwind plan sourced from
// either eh_frame (that's what we intend) or compact unwind (this won't work)
unwind_plan_sp = func_unwinders_sp->GetEHFrameUnwindPlan (process->GetTarget(), m_current_offset_backed_up_one);
+m_fallback_unwind_plan_sp = func_unwinders_sp->GetUnwindPlanAtNonCallSite (process->GetTarget(), m_thread, m_current_offset_backed_up_one);
+
if (unwind_plan_sp && unwind_plan_sp->PlanValidAtAddress (m_current_pc))
{
UnwindLogMsgVerbose ("frame uses %s for full UnwindPlan because the DynamicLoader suggested we prefer it",
@@ -1608,8 +1599,8 @@
// If a compiler generated unwind plan failed, trying the arch default unwindplan
// isn't going to do any better.
-if (m_full_unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes)
-return false;
+//if (m_full_unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes)
+//return false;
Re: [Lldb-commits] [PATCH] D21221: Fix for PrintStackTraces
ravitheja updated this revision to Diff 60539.
ravitheja added a comment.
Checks for nullptr
http://reviews.llvm.org/D21221
Files:
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
source/Plugins/Process/Utility/RegisterContextLLDB.cpp
Index: source/Plugins/Process/Utility/RegisterContextLLDB.cpp
===
--- source/Plugins/Process/Utility/RegisterContextLLDB.cpp
+++ source/Plugins/Process/Utility/RegisterContextLLDB.cpp
@@ -210,12 +210,28 @@
m_frame_type = eNormalFrame;
}
+// We've set m_frame_type and m_sym_ctx before these calls.
+
+m_fast_unwind_plan_sp = GetFastUnwindPlanForFrame ();
+m_full_unwind_plan_sp = GetFullUnwindPlanForFrame ();
+
// If we were able to find a symbol/function, set addr_range to the bounds of that symbol/function.
// else treat the current pc value as the start_pc and record no offset.
if (addr_range.GetBaseAddress().IsValid())
{
m_start_pc = addr_range.GetBaseAddress();
-if (m_current_pc.GetSection() == m_start_pc.GetSection())
+if (m_sym_ctx.symbol != nullptr && m_sym_ctx.symbol->IsSynthetic())
+{
+// The current offset should be recalculated here. The m_current_offset is
+// calculated from the base address of the symbol. The symbol can lie in the PLT
+// (Procedure Linkage Table) i.e its a symbol stub for external call. In this case
+// the base address for the unwindplan and the base address of the symbol maybe different, hence
+// the m_current_offset will be wrong.
+AddressRange unwind_address_range = m_full_unwind_plan_sp->GetAddressRange();
+if (unwind_address_range.ContainsFileAddress(m_current_pc))
+m_current_offset = m_current_pc.GetOffset() - unwind_address_range.GetBaseAddress().GetOffset();
+}
+else if (m_current_pc.GetSection() == m_start_pc.GetSection())
{
m_current_offset = m_current_pc.GetOffset() - m_start_pc.GetOffset();
}
@@ -236,11 +252,6 @@
m_current_offset_backed_up_one = -1;
}
-// We've set m_frame_type and m_sym_ctx before these calls.
-
-m_fast_unwind_plan_sp = GetFastUnwindPlanForFrame ();
-m_full_unwind_plan_sp = GetFullUnwindPlanForFrame ();
-
UnwindPlan::RowSP active_row;
lldb::RegisterKind row_register_kind = eRegisterKindGeneric;
if (m_full_unwind_plan_sp && m_full_unwind_plan_sp->PlanValidAtAddress (m_current_pc))
@@ -255,36 +266,14 @@
}
}
-if (!active_row.get())
-{
-UnwindLogMsg ("could not find an unwindplan row for this frame's pc");
-m_frame_type = eNotAValidFrame;
-return;
-}
-
-
if (!ReadCFAValueForRow (row_register_kind, active_row, m_cfa))
{
// Try the fall back unwind plan since the
// full unwind plan failed.
-FuncUnwindersSP func_unwinders_sp;
-UnwindPlanSP call_site_unwind_plan;
bool cfa_status = false;
+if (TryFallbackUnwindPlan())
+cfa_status = true;
-if (m_sym_ctx_valid)
-{
-func_unwinders_sp = pc_module_sp->GetObjectFile()->GetUnwindTable().GetFuncUnwindersContainingAddress (m_current_pc, m_sym_ctx);
-}
-
-if(func_unwinders_sp.get() != nullptr)
-call_site_unwind_plan = func_unwinders_sp->GetUnwindPlanAtCallSite(process->GetTarget(), m_current_offset_backed_up_one);
-
-if (call_site_unwind_plan.get() != nullptr)
-{
-m_fallback_unwind_plan_sp = call_site_unwind_plan;
-if(TryFallbackUnwindPlan())
-cfa_status = true;
-}
if (!cfa_status)
{
UnwindLogMsg ("could not read CFA value for first frame.");
@@ -881,6 +870,8 @@
// call GetUnwindPlanAtCallSite() -- because CallSite may return an unwind plan sourced from
// either eh_frame (that's what we intend) or compact unwind (this won't work)
unwind_plan_sp = func_unwinders_sp->GetEHFrameUnwindPlan (process->GetTarget(), m_current_offset_backed_up_one);
+m_fallback_unwind_plan_sp = func_unwinders_sp->GetUnwindPlanAtNonCallSite (process->GetTarget(), m_thread, m_current_offset_backed_up_one);
+
if (unwind_plan_sp && unwind_plan_sp->PlanValidAtAddress (m_current_pc))
{
UnwindLogMsgVerbose ("frame uses %s for full UnwindPlan because the DynamicLoader suggested we prefer it",
@@ -1608,8 +1599,8 @@
// If a compiler generated unwind plan failed, trying the arch default unwindplan
// isn't going to do any better.
-if (m_full_unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes)
-return false;
+//if (m_full_unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes)
+//return false;
// Get the call
[Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2
cameron314 created this revision.
cameron314 added reviewers: clayborg, labath, zturner.
cameron314 added a subscriber: lldb-commits.
This is the follow-up to D19122, which was accepted but subsequently reverted
due to a bug it introduced (that I didn't see during local testing on Windows
but which manifested quite often on Linux). That bug (a race between the
Process object was being destroyed and the thread terminating, caused by the
join not being done under certain conditions) is fixed in this version of the
patch.
This patch fixes various races between the time the private state thread is
signaled to exit and the time it actually exits (during which it no longer
responds to events). Previously, this was consistently causing 2-second timeout
delays on process detach/stop for us.
This also prevents crashes that were caused by the thread controlling its own
owning pointer while the controller was using it (copying the thread wrapper is
not enough to mitigate this, since the internal thread object was getting reset
anyway). Again, we were seeing this consistently.
For what it's worth, I've run the test suite with this change (on Linux)
without any regressions, and the number of reruns dropped from 15 to 0 for me
(though that last part may be coincidence).
http://reviews.llvm.org/D21296
Files:
include/lldb/Target/Process.h
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4088,7 +4088,7 @@
void
Process::StopPrivateStateThread ()
{
-if (PrivateStateThreadIsValid ())
+if (m_private_state_thread.IsJoinable ())
ControlPrivateStateThread (eBroadcastInternalStateControlStop);
else
{
@@ -4110,21 +4110,23 @@
if (log)
log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
{
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
-{
-if (log)
-log->Printf ("Sending control event of type: %d.", signal);
-// Send the control event and wait for the receipt or for the private state
-// thread to exit
-std::shared_ptr event_receipt_sp(new EventDataReceipt());
-m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
+// Broadcast the event.
+// It is important to do this outside of the if below, because
+// it's possible that the thread state is invalid but that the
+// thread is waiting on a control event instead of simply being
+// on its way out (this should not happen, but it apparently can).
+if (log)
+log->Printf ("Sending control event of type: %d.", signal);
+std::shared_ptr event_receipt_sp(new EventDataReceipt());
+m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
-bool receipt_received = false;
+// Wait for the event receipt or for the private state thread to exit
+bool receipt_received = false;
+if (PrivateStateThreadIsValid())
+{
while (!receipt_received)
{
bool timed_out = false;
@@ -4137,23 +4139,24 @@
if (!receipt_received)
{
// Check if the private state thread is still around. If it isn't then we are done waiting
-if (!m_private_state_thread.IsJoinable())
-break; // Private state thread exited, we are done
+if (!PrivateStateThreadIsValid())
+break; // Private state thread exited or is exiting, we are done
}
}
-
-if (signal == eBroadcastInternalStateControlStop)
-{
-thread_result_t result = NULL;
-private_state_thread.Join(&result);
-}
}
-else
+
+if (signal == eBroadcastInternalStateControlStop)
{
-if (log)
-log->Printf ("Private state thread already dead, no need to signal it to stop.");
+thread_result_t result = NULL;
+m_private_state_thread.Join(&result);
+m_private_state_thread.Reset();
}
}
+else
+{
+if (log)
+log->Printf("Private state thread already dead, no need to signal it to stop.");
+}
}
void
@@ -4446,7 +4449,6 @@
// try to change it on the way out.
if (!is_secondary_thread)
m_public_run_lock.SetStopped();
-m_private_state_thread.Reset();
return NULL;
Re: [Lldb-commits] [PATCH] D21280: Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Does this patch handle being able to share an 8 byte watchpoint between two watchpoints? Lets say you have an 8 byte array named "a" and watch to watch a[0] and a[3] and a[7]. You should be able to allow the watchpoints to share the 1 watchpoint. This patch is fine as is, just something to think about for a future patch. http://reviews.llvm.org/D21280 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. This looks like it would work for normal operation. I am not sure it will work when an extra private state thread is spun up. In certain circumstances we need to create more private state threads. This happens when you have an expression that is run from the private state thread. Since the private state thread is what controls the actual process, we can't run an expression from the current private state thread, so we spin up new ones. The current code is doing some tricky things to deal with this, and that was part of the reason Process::ControlPrivateStateThread() was making a copy of the current value of "m_private_state_thread" into a local variable named "private_state_thread": HostThread private_state_thread(m_private_state_thread); See the code in "Process::RunThreadPlan()" around the code: if (m_private_state_thread.EqualsThread(Host::GetCurrentThread())) The new loop as written in Process::ControlPrivateStateThread() could end up using m_private_state_thread with differing contents in the "if (m_private_state_thread.IsJoinable())" if statement. Jim Ingham made these changes, so we should add him to the reviewer list. I am going to mark as "Request Changes" so we can address any such issues, but Jim should chime in on this before we proceed. The way this normally happens is an expression is being run and while handling the expression on the private state thread, we need to run another expression (like a call to "mmap" in the debugged process so we can allocate memory), so we need to spin up another private state thread to handle the needed starts and stops. Only one of these threads will be actively grabbing events at a single time, so your patch might just work, but I want to get some more eyes on this to be sure. Please add Jim Ingham as a reviewer. http://reviews.llvm.org/D21296 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2
cameron314 added a comment. @clayborg: Thanks for having a look! I've added Jim Ingham as a reviewer. @jingham, I'd appreciate if you could take a few minutes to look this over. Right, I'd seen the backup/restore of the thread. As far as I can tell it should still work; the code in `ControlPrivateStateThread` has no idea it's working with a temporary thread, just as it didn't know before (in fact, if you look carefully at the code in the present tip of the trunk, a recent change seems to have introduced a mix of using both `private_state_thread` and `m_private_state_thread`, probably by accident). `m_private_state_thread` cannot be reset to the backup during a control event, since the first thing that's done before restoring the backup thread is to stop the temporary thread. http://reviews.llvm.org/D21296 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2
clayborg added a comment. Ok, as long as Jim agrees, then I will give it the go ahead. http://reviews.llvm.org/D21296 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2
jingham accepted this revision. jingham added a comment. This looks okay to me. http://reviews.llvm.org/D21296 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good if Jim is happy. http://reviews.llvm.org/D21296 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21152: Hunt for unused memory properly when dealing with processes that can tell us about memory mappings
tfiala accepted this revision. tfiala added a comment. This revision is now accepted and ready to land. LGTM. Repository: rL LLVM http://reviews.llvm.org/D21152 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21280: Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets
labath requested changes to this revision. labath added a comment. This revision now requires changes to proceed. The overall change looks good, but please also add a test which specifically tests for watchpoints at unaligned addresses. Last time I checked, we all watchpoint tests were passing (at least on android) even without this patch, so it looks like our tests coverage is not sufficient here. http://reviews.llvm.org/D21280 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r272635 - Add support to PlatformRemoteiOS, PlatformRemoteAppleWatch, and
Author: jmolenda
Date: Mon Jun 13 22:49:13 2016
New Revision: 272635
URL: http://llvm.org/viewvc/llvm-project?rev=272635&view=rev
Log:
Add support to PlatformRemoteiOS, PlatformRemoteAppleWatch, and
PlatformRemoteAppleTV to check the target.exec-search-paths
directories for files after looking in the SDK. An additional
wrinkle is that the remote file path may be something like
".../UIFoundation.framework/UIFoundation" and in
target.exec-search-paths we will have "UIFoundation.framework".
Looking for just the filename of the path is not sufficient -
we need to also look for it by the parent directories because
this may be a darwin bundle/framework like the UIFoundation
example.
We really need to make a PlatformRemoteAppleDevice and have
PlatformRemoteiOS, PlatformRemoteAppleWatch, and PlatformRemoteAppleTV
inherit from it. These three classes are 98% identical code.
Modified:
lldb/trunk/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.cpp
lldb/trunk/source/Plugins/Platform/MacOSX/PlatformRemoteAppleWatch.cpp
lldb/trunk/source/Plugins/Platform/MacOSX/PlatformRemoteiOS.cpp
Modified: lldb/trunk/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.cpp?rev=272635&r1=272634&r2=272635&view=diff
==
--- lldb/trunk/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.cpp
(original)
+++ lldb/trunk/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.cpp Mon Jun
13 22:49:13 2016
@@ -880,6 +880,76 @@ PlatformRemoteAppleTV::GetSharedModule (
if (error.Success())
return error;
+// See if the file is present in any of the module_search_paths_ptr
directories.
+if (!module_sp && module_search_paths_ptr && platform_file)
+{
+// create a vector of all the file / directory names in platform_file
+// e.g. this might be
/System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
+//
+// We'll need to look in the module_search_paths_ptr directories for
+// both "UIFoundation" and "UIFoundation.framework" -- most likely the
+// latter will be the one we find there.
+
+FileSpec platform_pull_apart (platform_file);
+std::vector path_parts;
+ConstString unix_root_dir("/");
+while (true)
+{
+ConstString part = platform_pull_apart.GetLastPathComponent();
+platform_pull_apart.RemoveLastPathComponent();
+if (part.IsEmpty() || part == unix_root_dir)
+break;
+path_parts.push_back (part.AsCString());
+}
+const size_t path_parts_size = path_parts.size();
+
+size_t num_module_search_paths = module_search_paths_ptr->GetSize();
+for (size_t i = 0; i < num_module_search_paths; ++i)
+{
+// Create a new FileSpec with this module_search_paths_ptr
+// plus just the filename ("UIFoundation"), then the parent
+// dir plus filename ("UIFoundation.framework/UIFoundation")
+// etc - up to four names (to handle
"Foo.framework/Contents/MacOS/Foo")
+
+for (size_t j = 0; j < 4 && j < path_parts_size - 1; ++j)
+{
+FileSpec path_to_try
(module_search_paths_ptr->GetFileSpecAtIndex (i));
+
+// Add the components backwards. For
.../PrivateFrameworks/UIFoundation.framework/UIFoundation
+// path_parts is
+// [0] UIFoundation
+// [1] UIFoundation.framework
+// [2] PrivateFrameworks
+//
+// and if 'j' is 2, we want to append path_parts[1] and then
path_parts[0], aka
+// 'UIFoundation.framework/UIFoundation', to the
module_search_paths_ptr path.
+
+for (int k = j; k >= 0; --k)
+{
+path_to_try.AppendPathComponent (path_parts[k]);
+}
+
+if (path_to_try.Exists())
+{
+ModuleSpec new_module_spec (module_spec);
+new_module_spec.GetFileSpec() = path_to_try;
+Error new_error (Platform::GetSharedModule
(new_module_spec,
+process,
+module_sp,
+NULL,
+
old_module_sp_ptr,
+
did_create_ptr));
+
+if (module_sp)
+{
+module_sp->SetPlatformFileSpec (path_to_try);
+return new_error;
+}
+}
+}
+
Re: [Lldb-commits] [PATCH] D21221: Fix for PrintStackTraces
labath added a comment. I think this idea in general is more acceptable, but we'll need an OK from Jason on the details. Also, we should figure out a way to add a test for this. We should definitely add a more deterministic test and not rely on TestPrintStackTraces to check this functionality. Is it possible to write a self-contained test replicating these conditions, i.e. without depending on unwinding from standard library functions? In http://reviews.llvm.org/D21221#455818, @ravitheja wrote: > Well its not possible for the assembly unwinder to do the correct thing in > this situation , as the function is entered through a stub function (which > has push instruction) and then a jump. The point of this patch is to catch > when to use the CFI or the assembly unwinder. Note I said the "default" unwinder. If the assembly unwinder does not work, then maybe it should not be the default in this case. Also, I seem to recall that the first plan being tried is the "augmented CFI", but that is very conservative in what it accepts as valid input. So maybe if we made it smarter (as in, recognize that in this case it does not need to do any augmentation and just pass through the eh_frame plan), then this and a lot of other problems would go away. Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1602 @@ -1610,3 +1601,3 @@ // isn't going to do any better. -if (m_full_unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes) -return false; +//if (m_full_unwind_plan_sp->GetSourcedFromCompiler() == eLazyBoolYes) +//return false; What is this supposed to do? http://reviews.llvm.org/D21221 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21221: Fix for PrintStackTraces
jasonmolenda added a comment. Hi Ravi, sorry for not having time to look at this patch yet - I was out of the office Friday and was catching up on everything today. I'll look at this tomorrow. My initial reactions are to be a little worried. It sounds like you have functions that are non-ABI conforming -- which is fine for functions that don't have external linkage -- and you're tweaking the unwinder until it works for this case. When we were discussing this on lldb-dev a couple of weeks ago, the functions in question had no eh_frame code. Why are you forcing eh_frame for frame 0? Is this a different case than the one you were debugging ten days ago? One initial comment is I don't know about adding AlwaysRelyOnEHUnwindInfo to the DynamicLoader - the section of code you added this check to in RegisterContextLLDB is there because we had one or two solibs on darwin where we had to use eh_frame, hence it made sense to ask the DynamicLoader plugin. If we were to add something like AlwaysRelyOnEHUnwindInfo for an entire platform, I'd probably put it at FuncUnwinders::GetUnwindPlanAtNonCallSite. But I really don't think that's a good idea unless there's a concrete example where a non-ABI-conforming function actually has eh_frame that describes how to correctly unwind from it. (my initial opinion is that a debugger trying to unwind out of non-ABI conforming functions without hand-written eh_frame describing how to do it, are in the "it would be nice if it worked" camp; I'd be reluctant to make changes that could compromise lldb otherwise to support this kind of scenario.) But I really haven't given the patch or the discussion a fair read-through and thought about it more - I'll do that tomorrow. http://reviews.llvm.org/D21221 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D21280: Allow installing watchpoints at less than 8-byte alligned addresses for AArch64 targets
omjavaid added a comment. In http://reviews.llvm.org/D21280#457196, @labath wrote: > The overall change looks good, but please also add a test which specifically > tests for watchpoints at unaligned addresses. Last time I checked, we all > watchpoint tests were passing (at least on android) even without this patch, > so it looks like our tests coverage is not sufficient here. Patch causes no regressions and following tests fail without this patch on Nexus 9 and arm-linux hikey board: No test currently fully tests the functionality supported by this patch. So I ll add a new test that tests different watchpoint sizes. UNEXPECTED SUCCESS: test_watchpoint_command_can_disable_a_watchpoint_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py) UNEXPECTED SUCCESS: test_watchpoint_command_can_disable_a_watchpoint_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py) UNEXPECTED SUCCESS: test_watchpoint_command_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py) UNEXPECTED SUCCESS: test_watchpoint_command_dwarf (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py) UNEXPECTED SUCCESS: test_watchpoint_command_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandPython.py) UNEXPECTED SUCCESS: test_watchpoint_command_dwo (functionalities/watchpoint/watchpoint_commands/command/TestWatchpointCommandLLDB.py) UNEXPECTED SUCCESS: test_watchpoint_cond_api_dwarf (python_api/watchpoint/condition/TestWatchpointConditionAPI.py) UNEXPECTED SUCCESS: test_watchpoint_cond_api_dwo (python_api/watchpoint/condition/TestWatchpointConditionAPI.py) UNEXPECTED SUCCESS: test_watchpoint_cond_dwarf (functionalities/watchpoint/watchpoint_commands/condition/TestWatchpointConditionCmd.py) UNEXPECTED SUCCESS: test_watchpoint_cond_dwo (functionalities/watchpoint/watchpoint_commands/condition/TestWatchpointConditionCmd.py) UNEXPECTED SUCCESS: test_with_python_api_dwarf (functionalities/watchpoint/watchpoint_events/TestWatchpointEvents.py) UNEXPECTED SUCCESS: test_with_python_api_dwo (functionalities/watchpoint/watchpoint_events/TestWatchpointEvents.py) In http://reviews.llvm.org/D21280#456670, @clayborg wrote: > Does this patch handle being able to share an 8 byte watchpoint between two > watchpoints? Lets say you have an 8 byte array named "a" and watch to watch > a[0] and a[3] and a[7]. You should be able to allow the watchpoints to share > the 1 watchpoint. This patch is fine as is, just something to think about for > a future patch. This functionality may not work with current patch and may require more work. I will follow up with another patch after adding support for this kind of scenario. http://reviews.llvm.org/D21280 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
