[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

2020-08-25 Thread Nicholas Allegra via Phabricator via lldb-commits 
comex added a comment.

In D86388#2234418 , @jingham wrote:

> I'm confused as to how this patch actually fixes the problem.  When the 
> thread gets removed from the thread list, it should get Destroy called on it 
> - which should set m_destroy_called, causing IsValid to return false..  So I 
> am not clear under what circumstances FindThreadByID will fail, but the 
> cached thread shared pointer's IsValid is still true?  If IsValid is holding 
> true over the thread's removal from the thread list, then I'm worried that 
> this change will keep us using the old ThreadSP that was reported the next 
> time we stopped and this thread ID was represented by a different ThreadSP.

Hmm… I’m confused by your comment.  This patch isn’t meant to address a 
situation where IsValid is true but FindThreadByID fails.  It addresses the 
situation where a ThreadPlan already has a cached thread pointer (and would 
like to reuse it without calling FindThreadByID at all), but IsValid is false 
because the thread was removed from the thread list since it was cached.

The existing code doesn’t check IsValid; it assumes that the thread list can’t 
be changed until a resume happens, at which point WillResume will be called and 
reset the cached thread pointer to null.  However, in the buggy case I found, 
GetThread is called again after WillResume has already run.  GetThread sets the 
cached thread pointer back to non-null, and then later when the thread list 
actually changes, the pointer becomes dangling.

With this patch, the pointer never gets reset to null, so it can end up 
pointing to a thread that has been removed from the list.  But now it’s a 
shared_ptr, so it at least keeps the thread object alive.  And every time 
GetThread is called, it checks (using IsValid) whether the thread has been 
removed.  If so, GetThread throws out the cached pointer and falls back to 
calling FindThreadByID.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86388/new/

https://reviews.llvm.org/D86388

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D79219: [CMake] Simplify CMake handling for zlib

2020-08-25 Thread Mateusz Mikuła via Phabricator via lldb-commits 
mati865 added a comment.

@phosek

> Looks like that's an issue introduced by D86134 
>  or D86245 .

Indeed, I apologize for bothering you. Should I move the discussion to one of 
patches created by  @haampie?

Continuing the discussion with `if( CMAKE_FIND_LIBRARY_PREFIXES )` and `if( 
CMAKE_FIND_LIBRARY_SUFFIXES )` from https://reviews.llvm.org/D86245 changed to 
false `llvm-config --system-libs` prints `-llibz.dll.a` so we need those 
replaces to bring back correct `-lz` value.

> Can you print the value of ${CMAKE_FIND_LIBRARY_PREFIXES} and 
> ${CMAKE_FIND_LIBRARY_SUFFIXES} in your build?

  -- CMAKE_FIND_LIBRARY_PREFIXES=lib;
  -- CMAKE_FIND_LIBRARY_SUFFIXES=.dll.a;.a;.lib

Which matches 
https://gitlab.kitware.com/cmake/cmake/-/blob/269d1a86249ea037a2884133daffc8c44a38d926/Modules/Platform/Windows-GNU.cmake


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79219/new/

https://reviews.llvm.org/D79219

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D85820: Use find_library for ncurses

2020-08-25 Thread Harmen Stoppels via Phabricator via lldb-commits 
haampie added a comment.

@gkistanova It's true that this change has lead to more issues I could ever 
imagine, but I think the link you provided is the last remaining problem.

Pinging @phosek for a similar issue w.r.t. zlib: since 
https://reviews.llvm.org/D79219 zlib gets disabled on static builds of LLVM.  
The reason is `find_package(ZLIB)` finds a shared library, and 
`check_symbol_exists(compress2 zlib.h HAVE_ZLIB)` tries to statically link to 
that -- it can't so zlib is disabled. I guess that's a regression too?

What would be the best way forward @phosek? A quick fix for ncurses is to add a 
simple `check_symbol_exists` test too, but that would in practice just disable 
ncurses in static builds. And additionally we could find static libs by adding 
explicit names like `libtinfo.a` to find_library when `LLVM_BUILD_STATIC=ON`. 
This trick won't help for zlib though, the find_library stuff is hidden inside 
find_package, and there is no way to target static libs it seems.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85820/new/

https://reviews.llvm.org/D85820

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86521: Revert "Use find_library for ncurses"

2020-08-25 Thread Harmen Stoppels via Phabricator via lldb-commits 
haampie created this revision.
haampie added reviewers: JDevlieghere, phosek, gkistanova.
Herald added subscribers: llvm-commits, lldb-commits, Sanitizers, hiraditya, 
mgorny.
Herald added projects: Sanitizers, LLDB, LLVM.
haampie requested review of this revision.

The introduction of find_library for ncurses caused more issues than it solved 
problems. The current open issue is it makes the static build of LLVM fail. 
It's better to revert for now, and get back to it later.

Revert "[CMake] Fix an issue where get_system_libname creates an empty regex 
capture on windows"

This reverts commit 1ed1e16ab83f55d85c90ae43a05cbe08a00c20e0 
.

Revert "Fix msan build"

This reverts commit 34fe9613dda3c7d8665b609136a8c12deb122382 
.

Revert "[CMake] Always mark terminfo as unavailable on Windows"

This reverts commit 76bf26236f6fd453343666c3cd91de8f74ffd89d 
.

Revert "[CMake] Fix OCaml build failure because of absolute path in system libs"

This reverts commit 8e4acb82f71ad4effec8895b8fc957189ce95933 
.

Revert "[CMake] Don't look for terminfo libs when LLVM_ENABLE_TERMINFO=OFF"

This reverts commit 495f91fd33d492941c39424a32cf24bcfe192f35 
.

Revert "Use find_library for ncurses"

This reverts commit a52173a3e56553d7b795bcf3cdadcf6433117107 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86521

Files:
  compiler-rt/cmake/config-ix.cmake
  compiler-rt/lib/xray/tests/CMakeLists.txt
  lldb/source/Core/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/include/llvm/Config/config.h.cmake
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Unix/Process.inc
  llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
@@ -284,9 +284,9 @@
   }
 
   if (llvm_enable_terminfo) {
-values += [ "LLVM_ENABLE_TERMINFO=1" ]
+values += [ "HAVE_TERMINFO=1" ]
   } else {
-values += [ "LLVM_ENABLE_TERMINFO=" ]
+values += [ "HAVE_TERMINFO=" ]
   }
 
   if (llvm_enable_dia_sdk) {
Index: llvm/lib/Support/Unix/Process.inc
===
--- llvm/lib/Support/Unix/Process.inc
+++ llvm/lib/Support/Unix/Process.inc
@@ -313,7 +313,7 @@
   return getColumns();
 }
 
-#ifdef LLVM_ENABLE_TERMINFO
+#ifdef HAVE_TERMINFO
 // We manually declare these extern functions because finding the correct
 // headers from various terminfo, curses, or other sources is harder than
 // writing their specs down.
@@ -323,12 +323,12 @@
 extern "C" int tigetnum(char *capname);
 #endif
 
-#ifdef LLVM_ENABLE_TERMINFO
+#ifdef HAVE_TERMINFO
 static ManagedStatic TermColorMutex;
 #endif
 
 static bool terminalHasColors(int fd) {
-#ifdef LLVM_ENABLE_TERMINFO
+#ifdef HAVE_TERMINFO
   // First, acquire a global lock because these C routines are thread hostile.
   std::lock_guard G(*TermColorMutex);
 
Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -2,19 +2,6 @@
   set(imported_libs ZLIB::ZLIB)
 endif()
 
-function(get_system_libname libpath libname)
-  get_filename_component(libpath ${libpath} NAME)
-  if( CMAKE_FIND_LIBRARY_PREFIXES )
-string(REPLACE ";" "|" PREFIXES "${CMAKE_FIND_LIBRARY_PREFIXES}")
-string(REGEX REPLACE "^(${PREFIXES})" "" libpath ${libpath})
-  endif()
-  if( CMAKE_FIND_LIBRARY_SUFFIXES )
-string(REPLACE ";" "|" SUFFIXES "${CMAKE_FIND_LIBRARY_SUFFIXES}")
-string(REGEX REPLACE "(${SUFFIXES})$" "" libpath ${libpath})
-  endif()
-  set(${libname} "${libpath}" PARENT_SCOPE)
-endfunction()
-
 if( MSVC OR MINGW )
   # libuuid required for FOLDERID_Profile usage in lib/Support/Windows/Path.inc.
   # advapi32 required for CryptAcquireContextW in lib/Support/Windows/Path.inc.
@@ -34,8 +21,10 @@
 STRING(REGEX REPLACE "^lib" "" Backtrace_LIBFILE ${Backtrace_LIBFILE})
 set(system_libs ${system_libs} ${Backtrace_LIBFILE})
   endif()
-  if( LLVM_ENABLE_TERMINFO )
-set(imported_libs ${imported_libs} "${TERMINFO_LIB}")
+  if(LLVM_ENABLE_TERMINFO)
+if(HAVE_TERMINFO)
+  set(system_libs ${system_libs} ${TERMINFO_LIBS})
+endif()
   endif()
   if( LLVM_ENABLE_THREADS AND (HAVE_LIBATOMIC OR HAVE_CXX_LIBATOMICS64) )
 set(system_libs ${system_libs} atomic)
@@ -220,15 +209,20 @@
   if(NOT zlib_library)
 get_property(zlib_library TARGET ZLIB::ZLIB PROPE

[Lldb-commits] [PATCH] D85820: Use find_library for ncurses

2020-08-25 Thread Galina via Phabricator via lldb-commits 
gkistanova reopened this revision.
gkistanova added a comment.
This revision is now accepted and ready to land.

@haampie Are you working on fixing the 
http://lab.llvm.org:8011/builders/lld-perf-testsuite bot? This patch has broken 
it.

  FAILED: bin/llvm-tblgen 
  : && /usr/bin/c++  -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3 
 -static -fno-pie -Wl,-allow-shlib-undefined
-Wl,-rpath-link,/home/buildslave/slave_as-bldslv8/lld-perf-testsuite/build/./lib
  -Wl,-O3 -Wl,--gc-sections 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmMatcherEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/AsmWriterInst.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/Attributes.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CallingConvEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeEmitterGen.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenDAGPatterns.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenHwModes.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenInstruction.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenMapTable.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenSchedule.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenTarget.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherGen.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcherOpt.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DAGISelMatcher.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DFAPacketizerEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DirectiveEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/DisassemblerEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/ExegesisEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/FastISelEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/FixedLenDecoderEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/GICombinerEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/GlobalISelEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/InfoByHwMode.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrInfoEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/InstrDocsEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/IntrinsicEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptParserEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/OptRSTEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/PredicateExpander.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/PseudoLoweringEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/RISCVCompressInstEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterBankEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/RegisterInfoEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/SDNodeProperties.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/SearchableTableEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/SubtargetFeatureInfo.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/TableGen.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/Types.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86DisassemblerTables.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86EVEX2VEXTablesEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86FoldTablesEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86ModRMFilters.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/X86RecognizableInstr.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/WebAssemblyDisassemblerEmitter.cpp.o 
utils/TableGen/CMakeFiles/llvm-tblgen.dir/CTagsEmitter.cpp.o  -o 
bin/llvm-tblgen  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libLLVMSupport.a  
lib/libLLVMTableGen.a  -lpthread  lib/libLLVMTableGenGlobalISel.a  
lib/libLLVMTableGen.a  lib/libLLVMSupport.a  -lrt  -ldl  -lpthread  -lm  
/usr/lib/x86_64-linux-gnu/libtinfo.so  lib/libLLVMDemangle.a && :
  /usr/bin/ld: attempted static link of dynamic object 
`/usr/lib/x86_64-linux-gnu/libtinfo.so'

It seems the patch is quite problematic. Maybe reverting it at this point and 
making another approach is the way to go?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85820/new/

https://reviews.llvm.org/D85820

___
lldb-comm

[Lldb-commits] [PATCH] D79219: [CMake] Simplify CMake handling for zlib

2020-08-25 Thread Petr Hosek via Phabricator via lldb-commits 
phosek added a comment.

In D79219#2231927 , @mati865 wrote:

> @phosek in MSYS2 (targeting x86_64-w64-windows-gnu) Zlib works properly for 
> LLVM 10 but with master I'm now seeing:
>
>   -- Constructing LLVMBuild project information
>   -- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a
>   CMake Error at lib/Support/CMakeLists.txt:9 (string):
> string sub-command REGEX, mode REPLACE: regex "^(lib|)" matched an empty
> string.
>   Call Stack (most recent call first):
> lib/Support/CMakeLists.txt:226 (get_system_libname)
>
> `-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a` was printed by my 
> change to help debugging it.
> FYI `zlib_library` is set here: 
> https://github.com/llvm/llvm-project/blob/8e06bf6b3a2e8d25e56cd52dca0cf3ff1b37b5d1/llvm/lib/Support/CMakeLists.txt#L218

Looks like that's an issue introduced by D86134 
 or D86245 . 
Can you print the value of `${CMAKE_FIND_LIBRARY_PREFIXES}` and 
`${CMAKE_FIND_LIBRARY_SUFFIXES}` in your build?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79219/new/

https://reviews.llvm.org/D79219

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

2020-08-25 Thread Joseph Tremoulet via Phabricator via lldb-commits 
JosephTremoulet added a comment.

> And its parent 'raise' is set so as well because of 
> 'GetNextFrame()->m_frame_type == eTrapHandlerFrame '.

Sorry, can you clarify what code path you're referring to here?  I see two 
occurrences of `GetNextFrame()->m_frame_type == eTrapHandlerFrame` in the code, 
and three occurrences of `m_frame_type = eTrapHandlerFrame`, but it's not clear 
how one of the former leads to one of the latter, or if I'm misunderstanding 
what you're saying.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86417/new/

https://reviews.llvm.org/D86417

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

2020-08-25 Thread Luboš Luňák via Phabricator via lldb-commits 
llunak added a comment.

In D86417#2236337 , @JosephTremoulet 
wrote:

>> And its parent 'raise' is set so as well because of 
>> 'GetNextFrame()->m_frame_type == eTrapHandlerFrame '.
>
> Sorry, can you clarify what code path you're referring to here?  I see two 
> occurrences of `GetNextFrame()->m_frame_type == eTrapHandlerFrame` in the 
> code, and three occurrences of `m_frame_type = eTrapHandlerFrame`, but it's 
> not clear how one of the former leads to one of the latter, or if I'm 
> misunderstanding what you're saying.

The one in RegisterContextUnwind::GetFullUnwindPlanForFrame(). It sets 
behaves_like_zeroth_frame, which in turn leads to executing the if() block 
later down in the function.

After applying the following patch

  diff --git a/lldb/source/Target/RegisterContextUnwind.cpp 
b/lldb/source/Target/RegisterContextUnwind.cpp
  index cf9336857ba..1de27ca764e 100644
  --- a/lldb/source/Target/RegisterContextUnwind.cpp
  +++ b/lldb/source/Target/RegisterContextUnwind.cpp
  @@ -1,3 +1,4 @@
  +#include 
   //===-- RegisterContextUnwind.cpp 
-===//
   //
   // Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
  @@ -263,6 +264,7 @@ void RegisterContextUnwind::InitializeZerothFrame() {
   // RegisterContextUnwind "below" it to provide things like its current pc 
value.
   
   void RegisterContextUnwind::InitializeNonZerothFrame() {
  +std::cerr << __PRETTY_FUNCTION__ << ":" << 
GetSymbolOrFunctionName(m_sym_ctx).AsCString("") << std::endl;
 Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
 if (IsFrameZero()) {
   m_frame_type = eNotAValidFrame;
  @@ -637,6 +639,7 @@ bool RegisterContextUnwind::IsFrameZero() const { return 
m_frame_number == 0; }
   //   the function, maybe backed up by 1, -1 if unknown
   
   UnwindPlanSP RegisterContextUnwind::GetFastUnwindPlanForFrame() {
  +std::cerr << __PRETTY_FUNCTION__ << ":" << 
GetSymbolOrFunctionName(m_sym_ctx).AsCString("") << std::endl;
 UnwindPlanSP unwind_plan_sp;
 ModuleSP pc_module_sp(m_current_pc.GetModule());
   
  @@ -689,6 +692,7 @@ UnwindPlanSP 
RegisterContextUnwind::GetFastUnwindPlanForFrame() {
   //   the function, maybe backed up by 1, -1 if unknown
   
   UnwindPlanSP RegisterContextUnwind::GetFullUnwindPlanForFrame() {
  +std::cerr << __PRETTY_FUNCTION__ << ":" << 
GetSymbolOrFunctionName(m_sym_ctx).AsCString("") << std::endl;
 UnwindPlanSP unwind_plan_sp;
 UnwindPlanSP arch_default_unwind_plan_sp;
 ExecutionContext exe_ctx(m_thread.shared_from_this());
  @@ -1763,6 +1767,7 @@ bool 
RegisterContextUnwind::ForceSwitchToFallbackUnwindPlan() {
   
   void RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan(
   lldb::UnwindPlanSP unwind_plan) {
  +std::cerr << __PRETTY_FUNCTION__ << ":" << 
GetSymbolOrFunctionName(m_sym_ctx).AsCString("") << std::endl;
 if (unwind_plan->GetUnwindPlanForSignalTrap() != eLazyBoolYes) {
   // Unwind plan does not indicate trap handler.  Do nothing.  We may
   // already be flagged as trap handler flag due to the symbol being
  @@ -1775,6 +1780,7 @@ void 
RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan(
 }
   
 m_frame_type = eTrapHandlerFrame;
  +std::cerr << "SET:" << GetSymbolOrFunctionName(m_sym_ctx).AsCString("") 
<< std::endl;
   
 if (m_current_offset_backed_up_one != m_current_offset) {
   // We backed up the pc by 1 to compute the symbol context, but

and then executing manually what TestHandleAbort.py does directly in lldb, then 
doing the 'bt' command when the breakpoint at "Set a breakpoint here" results 
in this output:

  (lldb) bt
  void lldb_private::RegisterContextUnwind::InitializeNonZerothFrame():
  lldb::UnwindPlanSP 
lldb_private::RegisterContextUnwind::GetFastUnwindPlanForFrame():__restore_rt
  lldb::UnwindPlanSP 
lldb_private::RegisterContextUnwind::GetFullUnwindPlanForFrame():__restore_rt
  void 
lldb_private::RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan(lldb::UnwindPlanSP):__restore_rt
  void lldb_private::RegisterContextUnwind::InitializeNonZerothFrame():
  lldb::UnwindPlanSP 
lldb_private::RegisterContextUnwind::GetFastUnwindPlanForFrame():raise
  lldb::UnwindPlanSP 
lldb_private::RegisterContextUnwind::GetFullUnwindPlanForFrame():raise
  void 
lldb_private::RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan(lldb::UnwindPlanSP):raise
  SET:raise
  void lldb_private::RegisterContextUnwind::InitializeNonZerothFrame():
  lldb::UnwindPlanSP 
lldb_private::RegisterContextUnwind::GetFastUnwindPlanForFrame():abort
  lldb::UnwindPlanSP 
lldb_private::RegisterContextUnwind::GetFullUnwindPlanForFrame():abort
  void 
lldb_private::RegisterContextUnwind::PropagateTrapHandlerFlagFromUnwindPlan(lldb::UnwindPlanSP):abort
  SET:abort
  void lldb_private::RegisterContextUnwind::InitializeNonZerothFrame():
  lldb::UnwindPlanSP 
lldb_private::RegisterCont

[Lldb-commits] [PATCH] D86493: [lldb][NFC] Remove unused/misnamed SetObjectModificationTime

2020-08-25 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Sometimes these odd leaf APIs are used by custom downstream variants of LLDB, 
but without any comment pointing into that direction and no unit test, I think 
this makes sense to remove. Also, we can easily add it back in later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86493/new/

https://reviews.llvm.org/D86493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86493: [lldb][NFC] Remove unused/misnamed SetObjectModificationTime

2020-08-25 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision.
JDevlieghere added a comment.

Nice


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86493/new/

https://reviews.llvm.org/D86493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86493: [lldb][NFC] Remove unused/misnamed SetObjectModificationTime

2020-08-25 Thread Dave Lee via Phabricator via lldb-commits 
kastiglione added a comment.

In D86493#2236454 , @aprantl wrote:

> Sometimes these odd leaf APIs are used by custom downstream variants of LLDB

I wondered about other consumers, how often is it the case that mainline lldb 
has code used only downstream?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86493/new/

https://reviews.llvm.org/D86493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86493: [lldb][NFC] Remove unused/misnamed SetObjectModificationTime

2020-08-25 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added a comment.

The largest downstream LLDB fork that I know about is swift-lldb, which is also 
open source and thus easy to check. There also exists a Rust version, and many 
custom forks for various specialized hardware. Putting patches up for review 
here gives the maintainers a chance to voice their concerns. For larger 
architectural changes we usually have a thread on lldb-dev to make sure we 
captured everyone's attention. We're under no obligation to not break anyone's 
private fork, but if there is an easy/reasonable way to make everyone happy we 
usually tend to choose that path.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86493/new/

https://reviews.llvm.org/D86493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86497: [lldb] Add reproducer verifier

2020-08-25 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 287710.
JDevlieghere added a comment.
Herald added a subscriber: ormris.

- Add tests
- Extract common code from `CommandObjectReproducerDump` and 
`CommandObjectReproducerVerify` into `GetLoaderFromPathOrCurrent`
- Address code review feedback


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86497/new/

https://reviews.llvm.org/D86497

Files:
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Commands/CommandObjectReproducer.cpp
  lldb/source/Commands/Options.td
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/test/Shell/Reproducer/TestDebugSymbols.test
  lldb/test/Shell/Reproducer/TestVerify.test
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1159,6 +1159,17 @@
   return ExternalContentsPrefixDir;
 }
 
+void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
+  IsFallthrough = Fallthrough;
+}
+
+std::vector RedirectingFileSystem::getRoots() const {
+  std::vector R;
+  for (const auto &Root : Roots)
+R.push_back(Root->getName());
+  return R;
+}
+
 void RedirectingFileSystem::dump(raw_ostream &OS) const {
   for (const auto &Root : Roots)
 dumpEntry(OS, Root.get());
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -749,6 +749,10 @@
 
   StringRef getExternalContentsPrefixDir() const;
 
+  void setFallthrough(bool Fallthrough);
+
+  std::vector getRoots() const;
+
   void dump(raw_ostream &OS) const;
   void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Index: lldb/test/Shell/Reproducer/TestVerify.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestVerify.test
@@ -0,0 +1,15 @@
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture --capture-path %t.repro %t.out
+# RUN: %lldb --replay %t.repro
+
+# RUN: echo "/bogus/home/dir" > %t.repro/home.txt
+# RUN: echo "/bogus/current/working/dir" > %t.repro/cwd.txt
+# RUN: rm %t.repro/root/%S/Inputs/GDBRemoteCapture.in
+
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %s
+# CHECK: warning: working directory '/bogus/current/working/dir' not in VFS
+# CHECK: warning: home directory '/bogus/current/working/dir' not in VFS
+
+# RUN: echo "CHECK: %S/Inputs/GDBRemoteCapture.in" > %t.check
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %t.check
Index: lldb/test/Shell/Reproducer/TestDebugSymbols.test
===
--- lldb/test/Shell/Reproducer/TestDebugSymbols.test
+++ lldb/test/Shell/Reproducer/TestDebugSymbols.test
@@ -12,3 +12,7 @@
 # DUMP: uuid: AD52358C-94F8-3796-ADD6-B20FFAC00E5C
 # DUMP-NEXT: module path: /path/to/unstripped/executable
 # DUMP-NEXT: symbol path: /path/to/foo.dSYM/Contents/Resources/DWARF/foo
+
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %s --check-prefix VERIFY
+# VERIFY: warning: '/path/to/unstripped/executable': module path for AD52358C-94F8-3796-ADD6-B20FFAC00E5C not in VFS
+# VERIFY: warning: '/path/to/foo.dSYM/Contents/Resources/DWARF/foo': symbol path for AD52358C-94F8-3796-ADD6-B20FFAC00E5C not in VFS
Index: lldb/source/Utility/ReproducerProvider.cpp
===
--- lldb/source/Utility/ReproducerProvider.cpp
+++ lldb/source/Utility/ReproducerProvider.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Utility/ReproducerProvider.h"
 #include "lldb/Utility/ProcessInfo.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace lldb_private;
@@ -160,6 +161,87 @@
 FileSpec(it->symbol_path));
 }
 
+void Verifier::Verify(
+std::function error_callback,
+std::function warning_callback) const {
+  if (!m_loader) {
+error_callback("invalid loader");
+return;
+  }
+
+  FileSpec vfs_mapping = m_loader->GetFile();
+  ErrorOr> buffer =
+  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
+  if (!buffer) {
+error_callback("unable to read files: " + buffer.getError().message());
+return;
+  }
+
+  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
+  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
+  if (!vfs) {
+error_callback("unable to initialize the virtual file system");
+return;
+  }
+
+  auto &redirecting_vfs = static_cast(*vfs);
+  redirecting_vfs.setFallthrough(false);
+
+  llvm::Expected

[Lldb-commits] [PATCH] D86493: [lldb][NFC] Remove unused/misnamed SetObjectModificationTime

2020-08-25 Thread Adrian Prantl via Phabricator via lldb-commits 
aprantl added a comment.

I should also mention that we are actively looking into ways to upstream the 
swift-lldb code into llvm.org. Now that both branches are licensed under Apache 
the only challenge left is to make the Swift support a real language *plugin* 
that can be conditionally compiled.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86493/new/

https://reviews.llvm.org/D86493

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86497: [lldb] Add reproducer verifier

2020-08-25 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 287713.
JDevlieghere added a comment.

- Make CHECK-line more specific


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86497/new/

https://reviews.llvm.org/D86497

Files:
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Commands/CommandObjectReproducer.cpp
  lldb/source/Commands/Options.td
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/test/Shell/Reproducer/TestDebugSymbols.test
  lldb/test/Shell/Reproducer/TestVerify.test
  llvm/include/llvm/Support/VirtualFileSystem.h
  llvm/lib/Support/VirtualFileSystem.cpp

Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1159,6 +1159,17 @@
   return ExternalContentsPrefixDir;
 }
 
+void RedirectingFileSystem::setFallthrough(bool Fallthrough) {
+  IsFallthrough = Fallthrough;
+}
+
+std::vector RedirectingFileSystem::getRoots() const {
+  std::vector R;
+  for (const auto &Root : Roots)
+R.push_back(Root->getName());
+  return R;
+}
+
 void RedirectingFileSystem::dump(raw_ostream &OS) const {
   for (const auto &Root : Roots)
 dumpEntry(OS, Root.get());
Index: llvm/include/llvm/Support/VirtualFileSystem.h
===
--- llvm/include/llvm/Support/VirtualFileSystem.h
+++ llvm/include/llvm/Support/VirtualFileSystem.h
@@ -749,6 +749,10 @@
 
   StringRef getExternalContentsPrefixDir() const;
 
+  void setFallthrough(bool Fallthrough);
+
+  std::vector getRoots() const;
+
   void dump(raw_ostream &OS) const;
   void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Index: lldb/test/Shell/Reproducer/TestVerify.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestVerify.test
@@ -0,0 +1,15 @@
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture --capture-path %t.repro %t.out
+# RUN: %lldb --replay %t.repro
+
+# RUN: echo "/bogus/home/dir" > %t.repro/home.txt
+# RUN: echo "/bogus/current/working/dir" > %t.repro/cwd.txt
+
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %s
+# CHECK: warning: working directory '/bogus/current/working/dir' not in VFS
+# CHECK: warning: home directory '/bogus/current/working/dir' not in VFS
+
+# RUN: rm %t.repro/root/%S/Inputs/GDBRemoteCapture.in
+# RUN: echo "CHECK: warning: '%S/Inputs/GDBRemoteCapture.in': No such file or directory" > %t.check
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %t.check
Index: lldb/test/Shell/Reproducer/TestDebugSymbols.test
===
--- lldb/test/Shell/Reproducer/TestDebugSymbols.test
+++ lldb/test/Shell/Reproducer/TestDebugSymbols.test
@@ -12,3 +12,7 @@
 # DUMP: uuid: AD52358C-94F8-3796-ADD6-B20FFAC00E5C
 # DUMP-NEXT: module path: /path/to/unstripped/executable
 # DUMP-NEXT: symbol path: /path/to/foo.dSYM/Contents/Resources/DWARF/foo
+
+# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %s --check-prefix VERIFY
+# VERIFY: warning: '/path/to/unstripped/executable': module path for AD52358C-94F8-3796-ADD6-B20FFAC00E5C not in VFS
+# VERIFY: warning: '/path/to/foo.dSYM/Contents/Resources/DWARF/foo': symbol path for AD52358C-94F8-3796-ADD6-B20FFAC00E5C not in VFS
Index: lldb/source/Utility/ReproducerProvider.cpp
===
--- lldb/source/Utility/ReproducerProvider.cpp
+++ lldb/source/Utility/ReproducerProvider.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Utility/ReproducerProvider.h"
 #include "lldb/Utility/ProcessInfo.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace lldb_private;
@@ -160,6 +161,87 @@
 FileSpec(it->symbol_path));
 }
 
+void Verifier::Verify(
+std::function error_callback,
+std::function warning_callback) const {
+  if (!m_loader) {
+error_callback("invalid loader");
+return;
+  }
+
+  FileSpec vfs_mapping = m_loader->GetFile();
+  ErrorOr> buffer =
+  vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
+  if (!buffer) {
+error_callback("unable to read files: " + buffer.getError().message());
+return;
+  }
+
+  IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML(
+  std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
+  if (!vfs) {
+error_callback("unable to initialize the virtual file system");
+return;
+  }
+
+  auto &redirecting_vfs = static_cast(*vfs);
+  redirecting_vfs.setFallthrough(false);
+
+  llvm::Expected working_dir =
+  GetDirectoryFrom(m_loader);
+  if (working_dir) {
+if (!vfs->exists(*working_dir))
+  warning_callback("wor

[Lldb-commits] [PATCH] D86389: [lldb] Add a SymbolFileProvider to record and replay calls to dsymForUUID

2020-08-25 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor added a comment.

Pretty sure this broke some tests 
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/23476/

  Note: Google Test filter = 
SymbolsTest.LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile
  [==] Running 1 test from 1 test case.
  [--] Global test environment set-up.
  [--] 1 test from SymbolsTest
  [ RUN  ] 
SymbolsTest.LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile
  Assertion failed: (hasVal), function getValue, file 
/Users/buildslave/jenkins/workspace/lldb-cmake@2/llvm-project/llvm/include/llvm/ADT/Optional.h,
 line 73.
  0  SymbolTests  0x00010943f8a5 
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
  1  SymbolTests  0x00010943e836 llvm::sys::RunSignalHandlers() 
+ 198
  2  SymbolTests  0x00010943fe86 SignalHandler(int) + 262
  3  libsystem_platform.dylib 0x7fff6e8b35fd _sigtramp + 29
  4  libsystem_platform.dylib 0x _sigtramp + 
12297688647425029322
  5  libsystem_c.dylib0x7fff6e785808 abort + 120
  6  libsystem_c.dylib0x7fff6e784ac6 err + 0
  7  SymbolTests  0x00010a8f0bf5 
lldb_private::repro::Reproducer::Instance() + 133
  8  SymbolTests  0x0001094a7261 
LocateMacOSXFilesUsingDebugSymbols(lldb_private::ModuleSpec const&, 
lldb_private::ModuleSpec&) + 433
  9  SymbolTests  0x0001094894dd 
lldb_private::Symbols::LocateExecutableSymbolFile(lldb_private::ModuleSpec 
const&, lldb_private::FileSpecList const&) + 5741
  10 SymbolTests  0x00010932f232 
SymbolsTest_LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile_Test::TestBody()
 + 274
  11 SymbolTests  0x00010944869f testing::Test::Run() + 527
  12 SymbolTests  0x000109449516 testing::TestInfo::Run() + 566
  13 SymbolTests  0x000109449d47 testing::TestCase::Run() + 247
  14 SymbolTests  0x000109452877 
testing::internal::UnitTestImpl::RunAllTests() + 1287
  15 SymbolTests  0x0001094522fb testing::UnitTest::Run() + 171
  16 SymbolTests  0x000109441209 main + 121
  17 libdyld.dylib0x7fff6e6b6cc9 start + 1
  18 libdyld.dylib0x0002 start + 18446603338663629626


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86389/new/

https://reviews.llvm.org/D86389

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86211: [lldb] Don't ask for QOS_CLASS_UNSPECIFIED queue in TestQueues

2020-08-25 Thread Raphael Isemann via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7de7fe5d0e3f: [lldb] Don't ask for 
QOS_CLASS_UNSPECIFIED queue in TestQueues (authored by teemperor).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86211/new/

https://reviews.llvm.org/D86211

Files:
  lldb/test/API/macosx/queues/TestQueues.py
  lldb/test/API/macosx/queues/main.c


Index: lldb/test/API/macosx/queues/main.c
===
--- lldb/test/API/macosx/queues/main.c
+++ lldb/test/API/macosx/queues/main.c
@@ -136,15 +136,9 @@
 while (1)
 sleep (10);
   });
-dispatch_async (dispatch_get_global_queue(QOS_CLASS_UNSPECIFIED, 0), ^{
-pthread_setname_np ("unspecified QoS");
-atomic_fetch_add(&thread_count, 1);
-while (1)
-sleep (10);
-  });
 
 // Unfortunately there is no pthread_barrier on darwin.
-while ((atomic_load(&thread_count) < 13) || (finished_enqueueing_work == 
0))
+while ((atomic_load(&thread_count) < 12) || (finished_enqueueing_work == 
0))
 sleep (1);
 
 stopper ();
Index: lldb/test/API/macosx/queues/TestQueues.py
===
--- lldb/test/API/macosx/queues/TestQueues.py
+++ lldb/test/API/macosx/queues/TestQueues.py
@@ -192,7 +192,6 @@
 user_initiated_thread = lldb.SBThread()
 user_interactive_thread = lldb.SBThread()
 utility_thread = lldb.SBThread()
-unspecified_thread = lldb.SBThread()
 background_thread = lldb.SBThread()
 for th in process.threads:
 if th.GetName() == "user initiated QoS":
@@ -201,8 +200,6 @@
 user_interactive_thread = th
 if th.GetName() == "utility QoS":
 utility_thread = th
-if th.GetName() == "unspecified QoS":
-unspecified_thread = th
 if th.GetName() == "background QoS":
 background_thread = th
 
@@ -213,9 +210,6 @@
 user_interactive_thread.IsValid(),
 "Found user interactive QoS thread")
 self.assertTrue(utility_thread.IsValid(), "Found utility QoS thread")
-self.assertTrue(
-unspecified_thread.IsValid(),
-"Found unspecified QoS thread")
 self.assertTrue(
 background_thread.IsValid(),
 "Found background QoS thread")
@@ -248,16 +242,6 @@
 stream.GetData(), "Utility",
 "utility QoS thread name is valid")
 stream.Clear()
-self.assertTrue(
-unspecified_thread.GetInfoItemByPathAsString(
-"requested_qos.printable_name",
-stream),
-"Get QoS printable string for unspecified QoS thread")
-qosName = stream.GetData()
-self.assertTrue(
-qosName == "User Initiated" or qosName == "Default",
-"unspecified QoS thread name is valid: " + str(qosName))
-stream.Clear()
 self.assertTrue(
 background_thread.GetInfoItemByPathAsString(
 "requested_qos.printable_name",


Index: lldb/test/API/macosx/queues/main.c
===
--- lldb/test/API/macosx/queues/main.c
+++ lldb/test/API/macosx/queues/main.c
@@ -136,15 +136,9 @@
 while (1)
 sleep (10);
   });
-dispatch_async (dispatch_get_global_queue(QOS_CLASS_UNSPECIFIED, 0), ^{
-pthread_setname_np ("unspecified QoS");
-atomic_fetch_add(&thread_count, 1);
-while (1)
-sleep (10);
-  });
 
 // Unfortunately there is no pthread_barrier on darwin.
-while ((atomic_load(&thread_count) < 13) || (finished_enqueueing_work == 0))
+while ((atomic_load(&thread_count) < 12) || (finished_enqueueing_work == 0))
 sleep (1);
 
 stopper ();
Index: lldb/test/API/macosx/queues/TestQueues.py
===
--- lldb/test/API/macosx/queues/TestQueues.py
+++ lldb/test/API/macosx/queues/TestQueues.py
@@ -192,7 +192,6 @@
 user_initiated_thread = lldb.SBThread()
 user_interactive_thread = lldb.SBThread()
 utility_thread = lldb.SBThread()
-unspecified_thread = lldb.SBThread()
 background_thread = lldb.SBThread()
 for th in process.threads:
 if th.GetName() == "user initiated QoS":
@@ -201,8 +200,6 @@
 user_interactive_thread = th
 if th.GetName() == "utility QoS":
 utility_thread = th
-if th.GetName() == "unspecified QoS":
-unspecified_thread = th
 if th.GetName() == "background QoS":
 background_thread = th
 
@@ -213,9 +210,6 @@

[Lldb-commits] [lldb] 7de7fe5 - [lldb] Don't ask for QOS_CLASS_UNSPECIFIED queue in TestQueues

2020-08-25 Thread Raphael Isemann via lldb-commits 

Author: Raphael Isemann
Date: 2020-08-25T20:13:33+02:00
New Revision: 7de7fe5d0e3f7f4d28e1dde42df4a7defa564f11

URL: 
https://github.com/llvm/llvm-project/commit/7de7fe5d0e3f7f4d28e1dde42df4a7defa564f11
DIFF: 
https://github.com/llvm/llvm-project/commit/7de7fe5d0e3f7f4d28e1dde42df4a7defa564f11.diff

LOG: [lldb] Don't ask for QOS_CLASS_UNSPECIFIED queue in TestQueues

TestQueues is curiously failing for me as my queue for QOS_CLASS_UNSPECIFIED
is named "Utility" and not "User Initiated" or "Default". While debugging, this
I noticed that this test isn't actually using this API right from what I 
understand. The API documentation
for `dispatch_get_global_queue` specifies for the parameter: "You may specify 
the value
QOS_CLASS_USER_INTERACTIVE, QOS_CLASS_USER_INITIATED, QOS_CLASS_UTILITY, or 
QOS_CLASS_BACKGROUND."

QOS_CLASS_UNSPECIFIED isn't listed as one of the supported values. 
swift-corelibs-libdispatch
even checks for this value and returns a DISPATCH_BAD_INPUT. The
libdispatch shipped on macOS seems to also check for QOS_CLASS_UNSPECIFIED and 
seems to
instead cause a "client crash", but somehow this doesn't trigger in this test 
and instead we just
get whatever queue

This patch just removes that part of the test as it appears the code is just 
incorrect.

Reviewed By: jasonmolenda

Differential Revision: https://reviews.llvm.org/D86211

Added: 


Modified: 
lldb/test/API/macosx/queues/TestQueues.py
lldb/test/API/macosx/queues/main.c

Removed: 




diff  --git a/lldb/test/API/macosx/queues/TestQueues.py 
b/lldb/test/API/macosx/queues/TestQueues.py
index e177daa54fa3..711c99a7d400 100644
--- a/lldb/test/API/macosx/queues/TestQueues.py
+++ b/lldb/test/API/macosx/queues/TestQueues.py
@@ -192,7 +192,6 @@ def queues(self):
 user_initiated_thread = lldb.SBThread()
 user_interactive_thread = lldb.SBThread()
 utility_thread = lldb.SBThread()
-unspecified_thread = lldb.SBThread()
 background_thread = lldb.SBThread()
 for th in process.threads:
 if th.GetName() == "user initiated QoS":
@@ -201,8 +200,6 @@ def queues(self):
 user_interactive_thread = th
 if th.GetName() == "utility QoS":
 utility_thread = th
-if th.GetName() == "unspecified QoS":
-unspecified_thread = th
 if th.GetName() == "background QoS":
 background_thread = th
 
@@ -213,9 +210,6 @@ def queues(self):
 user_interactive_thread.IsValid(),
 "Found user interactive QoS thread")
 self.assertTrue(utility_thread.IsValid(), "Found utility QoS thread")
-self.assertTrue(
-unspecified_thread.IsValid(),
-"Found unspecified QoS thread")
 self.assertTrue(
 background_thread.IsValid(),
 "Found background QoS thread")
@@ -248,16 +242,6 @@ def queues(self):
 stream.GetData(), "Utility",
 "utility QoS thread name is valid")
 stream.Clear()
-self.assertTrue(
-unspecified_thread.GetInfoItemByPathAsString(
-"requested_qos.printable_name",
-stream),
-"Get QoS printable string for unspecified QoS thread")
-qosName = stream.GetData()
-self.assertTrue(
-qosName == "User Initiated" or qosName == "Default",
-"unspecified QoS thread name is valid: " + str(qosName))
-stream.Clear()
 self.assertTrue(
 background_thread.GetInfoItemByPathAsString(
 "requested_qos.printable_name",

diff  --git a/lldb/test/API/macosx/queues/main.c 
b/lldb/test/API/macosx/queues/main.c
index 3978b92bff1a..2bf390b1330a 100644
--- a/lldb/test/API/macosx/queues/main.c
+++ b/lldb/test/API/macosx/queues/main.c
@@ -136,15 +136,9 @@ int main (int argc, const char **argv)
 while (1)
 sleep (10);
   });
-dispatch_async (dispatch_get_global_queue(QOS_CLASS_UNSPECIFIED, 0), ^{
-pthread_setname_np ("unspecified QoS");
-atomic_fetch_add(&thread_count, 1);
-while (1)
-sleep (10);
-  });
 
 // Unfortunately there is no pthread_barrier on darwin.
-while ((atomic_load(&thread_count) < 13) || (finished_enqueueing_work == 
0))
+while ((atomic_load(&thread_count) < 12) || (finished_enqueueing_work == 
0))
 sleep (1);
 
 stopper ();



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] ef76686 - [lldb] Initialize reproducers in LocateSymbolFileTest

2020-08-25 Thread Raphael Isemann via lldb-commits 

Author: Raphael Isemann
Date: 2020-08-25T20:26:43+02:00
New Revision: ef76686916d40f20c782ed3967130bd2e0105b31

URL: 
https://github.com/llvm/llvm-project/commit/ef76686916d40f20c782ed3967130bd2e0105b31
DIFF: 
https://github.com/llvm/llvm-project/commit/ef76686916d40f20c782ed3967130bd2e0105b31.diff

LOG: [lldb] Initialize reproducers in LocateSymbolFileTest

Since a842950b62b6d029a392c3c312c6495d6368c2a4 this test started using
the reproducer subsystem but we never initialized it in the test. The
Subsystem takes an argument, so we can't use the usual SubsystemRAII at the
moment to do this for us.

This just adds the initialize/terminate calls to get the test passing again.

Added: 


Modified: 
lldb/unittests/Symbol/LocateSymbolFileTest.cpp

Removed: 




diff  --git a/lldb/unittests/Symbol/LocateSymbolFileTest.cpp 
b/lldb/unittests/Symbol/LocateSymbolFileTest.cpp
index 268faeaf1dbb..a2f9be56635d 100644
--- a/lldb/unittests/Symbol/LocateSymbolFileTest.cpp
+++ b/lldb/unittests/Symbol/LocateSymbolFileTest.cpp
@@ -14,6 +14,7 @@
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/Reproducer.h"
 
 using namespace lldb_private;
 
@@ -27,15 +28,22 @@ class SymbolsTest : public ::testing::Test {
 TEST_F(
 SymbolsTest,
 
TerminateLocateExecutableSymbolFileForUnknownExecutableAndUnknownSymbolFile) {
+  EXPECT_THAT_ERROR(
+  repro::Reproducer::Initialize(repro::ReproducerMode::Off, llvm::None),
+  llvm::Succeeded());
   ModuleSpec module_spec;
   FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
   FileSpec symbol_file_spec =
   Symbols::LocateExecutableSymbolFile(module_spec, search_paths);
   EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
+  repro::Reproducer::Terminate();
 }
 
 TEST_F(SymbolsTest,
LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile) {
+  EXPECT_THAT_ERROR(
+  repro::Reproducer::Initialize(repro::ReproducerMode::Off, llvm::None),
+  llvm::Succeeded());
   ModuleSpec module_spec;
   // using a GUID here because the symbol file shouldn't actually exist on disk
   module_spec.GetSymbolFileSpec().SetFile(
@@ -44,4 +52,5 @@ TEST_F(SymbolsTest,
   FileSpec symbol_file_spec =
   Symbols::LocateExecutableSymbolFile(module_spec, search_paths);
   EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
+  repro::Reproducer::Terminate();
 }



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86389: [lldb] Add a SymbolFileProvider to record and replay calls to dsymForUUID

2020-08-25 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor added a comment.

Seems like this were just some missing calls to initialize/terminate. I added 
them in an ugly hotfix commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86389/new/

https://reviews.llvm.org/D86389

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

2020-08-25 Thread Jim Ingham via Phabricator via lldb-commits 
jingham added a comment.

What's calling into the thread plans when WillResume has already been called?  
That seems wrong, since the thread list is in an uncertain state, having been 
cleared out for resume and not reset after the stop.  It seems to me it would 
be a better fix to ensure that we aren't doing that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86388/new/

https://reviews.llvm.org/D86388

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 5212206 - [lldb] Make Reproducer compatbile with SubsystemRAII (NFC)

2020-08-25 Thread Jonas Devlieghere via lldb-commits 

Author: Jonas Devlieghere
Date: 2020-08-25T13:00:04-07:00
New Revision: 521220690ab7741e382344319b2a9d458be3eb41

URL: 
https://github.com/llvm/llvm-project/commit/521220690ab7741e382344319b2a9d458be3eb41
DIFF: 
https://github.com/llvm/llvm-project/commit/521220690ab7741e382344319b2a9d458be3eb41.diff

LOG: [lldb] Make Reproducer compatbile with SubsystemRAII  (NFC)

Make Reproducer compatbile with SubsystemRAII and use it in
LocateSymbolFileTest.

Added: 


Modified: 
lldb/include/lldb/Utility/Reproducer.h
lldb/source/Utility/Reproducer.cpp
lldb/unittests/Symbol/LocateSymbolFileTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Reproducer.h 
b/lldb/include/lldb/Utility/Reproducer.h
index d3d6589a7ef87..d6cde44850901 100644
--- a/lldb/include/lldb/Utility/Reproducer.h
+++ b/lldb/include/lldb/Utility/Reproducer.h
@@ -197,6 +197,7 @@ class Reproducer {
   static Reproducer &Instance();
   static llvm::Error Initialize(ReproducerMode mode,
 llvm::Optional root);
+  static void Initialize();
   static bool Initialized();
   static void Terminate();
 

diff  --git a/lldb/source/Utility/Reproducer.cpp 
b/lldb/source/Utility/Reproducer.cpp
index 9276c7449d7b2..68c64195f55ee 100644
--- a/lldb/source/Utility/Reproducer.cpp
+++ b/lldb/source/Utility/Reproducer.cpp
@@ -73,6 +73,10 @@ llvm::Error Reproducer::Initialize(ReproducerMode mode,
   return Error::success();
 }
 
+void Reproducer::Initialize() {
+  llvm::cantFail(Initialize(repro::ReproducerMode::Off, llvm::None));
+}
+
 bool Reproducer::Initialized() { return InstanceImpl().operator bool(); }
 
 void Reproducer::Terminate() {

diff  --git a/lldb/unittests/Symbol/LocateSymbolFileTest.cpp 
b/lldb/unittests/Symbol/LocateSymbolFileTest.cpp
index a2f9be56635d1..c51b1ba490461 100644
--- a/lldb/unittests/Symbol/LocateSymbolFileTest.cpp
+++ b/lldb/unittests/Symbol/LocateSymbolFileTest.cpp
@@ -21,29 +21,22 @@ using namespace lldb_private;
 namespace {
 class SymbolsTest : public ::testing::Test {
 public:
-  SubsystemRAII subsystems;
+  SubsystemRAII subsystems;
 };
 } // namespace
 
 TEST_F(
 SymbolsTest,
 
TerminateLocateExecutableSymbolFileForUnknownExecutableAndUnknownSymbolFile) {
-  EXPECT_THAT_ERROR(
-  repro::Reproducer::Initialize(repro::ReproducerMode::Off, llvm::None),
-  llvm::Succeeded());
   ModuleSpec module_spec;
   FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
   FileSpec symbol_file_spec =
   Symbols::LocateExecutableSymbolFile(module_spec, search_paths);
   EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
-  repro::Reproducer::Terminate();
 }
 
 TEST_F(SymbolsTest,
LocateExecutableSymbolFileForUnknownExecutableAndMissingSymbolFile) {
-  EXPECT_THAT_ERROR(
-  repro::Reproducer::Initialize(repro::ReproducerMode::Off, llvm::None),
-  llvm::Succeeded());
   ModuleSpec module_spec;
   // using a GUID here because the symbol file shouldn't actually exist on disk
   module_spec.GetSymbolFileSpec().SetFile(
@@ -52,5 +45,4 @@ TEST_F(SymbolsTest,
   FileSpec symbol_file_spec =
   Symbols::LocateExecutableSymbolFile(module_spec, search_paths);
   EXPECT_TRUE(symbol_file_spec.GetFilename().IsEmpty());
-  repro::Reproducer::Terminate();
 }



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86493: [lldb][NFC] Remove unused/misnamed SetObjectModificationTime

2020-08-25 Thread Dave Lee via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG66c48802918d: Remove unused/misnamed 
SetObjectModificationTime (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86493/new/

https://reviews.llvm.org/D86493

Files:
  lldb/include/lldb/Core/Module.h


Index: lldb/include/lldb/Core/Module.h
===
--- lldb/include/lldb/Core/Module.h
+++ lldb/include/lldb/Core/Module.h
@@ -506,10 +506,6 @@
 return m_object_mod_time;
   }
 
-  void SetObjectModificationTime(const llvm::sys::TimePoint<> &mod_time) {
-m_mod_time = mod_time;
-  }
-
   /// This callback will be called by SymbolFile implementations when
   /// parsing a compile unit that contains SDK information.
   /// \param sysroot will be added to the path remapping dictionary.


Index: lldb/include/lldb/Core/Module.h
===
--- lldb/include/lldb/Core/Module.h
+++ lldb/include/lldb/Core/Module.h
@@ -506,10 +506,6 @@
 return m_object_mod_time;
   }
 
-  void SetObjectModificationTime(const llvm::sys::TimePoint<> &mod_time) {
-m_mod_time = mod_time;
-  }
-
   /// This callback will be called by SymbolFile implementations when
   /// parsing a compile unit that contains SDK information.
   /// \param sysroot will be added to the path remapping dictionary.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 66c4880 - Remove unused/misnamed SetObjectModificationTime

2020-08-25 Thread Dave Lee via lldb-commits 

Author: Dave Lee
Date: 2020-08-25T14:49:34-07:00
New Revision: 66c48802918d90e6a90d4f8da9c10889d5bc20dd

URL: 
https://github.com/llvm/llvm-project/commit/66c48802918d90e6a90d4f8da9c10889d5bc20dd
DIFF: 
https://github.com/llvm/llvm-project/commit/66c48802918d90e6a90d4f8da9c10889d5bc20dd.diff

LOG: Remove unused/misnamed SetObjectModificationTime

Remove `SetObjectModificationTime` which is not currently used, and assigns to 
the wrong member.

Differential Revision: https://reviews.llvm.org/D86493

Added: 


Modified: 
lldb/include/lldb/Core/Module.h

Removed: 




diff  --git a/lldb/include/lldb/Core/Module.h b/lldb/include/lldb/Core/Module.h
index 8bd70ab16b5a..9eb7477730c1 100644
--- a/lldb/include/lldb/Core/Module.h
+++ b/lldb/include/lldb/Core/Module.h
@@ -506,10 +506,6 @@ class Module : public std::enable_shared_from_this,
 return m_object_mod_time;
   }
 
-  void SetObjectModificationTime(const llvm::sys::TimePoint<> &mod_time) {
-m_mod_time = mod_time;
-  }
-
   /// This callback will be called by SymbolFile implementations when
   /// parsing a compile unit that contains SDK information.
   /// \param sysroot will be added to the path remapping dictionary.



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

2020-08-25 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added a comment.

A large part of this patch is concerned with parsing which worries me from a 
maintenance perspective. Did you consider using Yaml I/O 
? While I'm not a particularly big fan of 
the format, the benefits of being able to (de)serialize any class by 
implementing the appropriate traits are quite significant. We already have 
traits implemented for a bunch of utility classes, such as `ArchSpec` and 
`FileSpec` which we could reuse for this. I know changing the format would be 
invasive, but I think it might be worth it in the long term.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85705/new/

https://reviews.llvm.org/D85705

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

2020-08-25 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added a comment.

Hi, I'll review this problem / suggested patch in a bit but I think it might be 
helpful to outline what the intention of all this is.  Let's say we have a 
stack of

  frame #0 - handler_func()
  frame #1 - magic_function_called_from_kernel()
  frame #2 - doing_work()

`doing_work()` was executing along when an asynchronous signal came in - a 
SIGINT or something.  The kernel stops it, saves its register context (all the 
registers) into a buffer on the stack, and invokes 
`magic_function_called_from_kernel`.  m_f_c_f_k calls the `handler_func` that 
the program registered as the handler for SIGINT earlier.

In this situation, there are two *special* things about `doing_work` that the 
unwinder must do.

First, normally when you are off of the currently-executing stack frame (frame 
0), only registers that are callee-spilled aka non-volatile are retrievable.  
For instance, on the x86 SysV ABI, rdi is an argument register.  In frame 0, 
you can print rdi.  On frame #1, rdi may have been modified since it called 
frame 0, so lldb won't let you retrieve it.  However, when a stack frame has 
been interrupted asynchronously and we have a full saved register context, we 
can retrieve any register.  Printing stack frame 2's $rdi is possible and 
valid.  frame #2 behaves like a zeroth stack frame, because the next frame -- 
frame #1, magic_function_called_from_kernel, is a trap handler and has the full 
register context.

The second thing that is different about frame #2 is how we look up the source 
line and symbol context.  When we set the source line & block scope & function 
scope searching for a frame up the stack, we subtract 1 from the return-pc 
value.  Why do we subtract 1?  Two important reasons.  First, a function may 
end by calling a no-return function like abort() and the x86 ABI will push the 
return address on the stack with the next pc value.  But if the callq abort is 
the last instruction, then the return address points to the next function!  
Also, there are cases (often with optimized code) where a variable is in a 
register up until the function call (and is still retrievable) but after the 
function call, it may be considered dead because it's a different code path 
through the function.  So doing source line / block scope / function scope / 
location list lookups with $return-pc - 1 is very important.  However, when we 
interrupt a function asynchronously, like `doing_work` here, we may be 
legitimately on the very first instruction of `doing_work`.  Backing up the pc 
by 1 would have us claiming that we're in the *previous* function.

So this is why we have all this code for computing behaves_like_zeroth_frame in 
these areas.  In my original design, I conflated the two important points here: 
 That we have a full saved register context and that the function may have been 
interrupted asynchronously, so we need to treat this as if it's a 
currently-executing stack frame, even though it's in the middle of the stack.  
If either of these is not true:  if the NextFrame (frame #1 in this case) does 
NOT have a full register context, or this frame (frame #2) was NOT interrupted 
asynchronously, then treating frame #2 as behaves_like_zeroth_frame is 
incorrect and will lead to bugs.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86417/new/

https://reviews.llvm.org/D86417

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

2020-08-25 Thread walter erquinigo via Phabricator via lldb-commits 
wallace added a comment.

That's a very good idea! I'll revisit this patch then. I was not fond of doing 
the manual parsing but I hadn't found a more automated way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85705/new/

https://reviews.llvm.org/D85705

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

2020-08-25 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Looking better. The main thing we need to do it modify StructuredData classes a 
bit by moving a lot of the static functions we are using here into the 
appropriate classes. See inlined comments.




Comment at: lldb/source/Target/Trace.cpp:59
+  "plugin": <<>>,
+  "triple": string, // llvm-triple
+  "processes": [

Do we want an optional "trace-file" here in case trace files can contain all 
data for all processes?



Comment at: lldb/source/Target/Trace.cpp:62
+{
+  "pid": integer,
+  "threads": [

Do we want an optional "trace-file" here in case trace files can contain all 
data for a single process?





Comment at: lldb/source/Target/Trace.cpp:66
+  "tid": integer,
+  "traceFile": string // path to trace file relative to the settings 
file
+}

Do all trace files have one file per thread? Should this be "trace-file" 
instead of "traceFile"? 

Might make sense to make this optional and also include an optional traceFile 
at the process level and possibly one at the root level? See inline comments 
above



Comment at: lldb/source/Target/Trace.cpp:71
+{
+  "file": string, // path to trace file relative to the settings file
+  "loadAddress": string, // address in hex or decimal form

This should probably be optional as we won't always have the file copied into 
the trace directory?

Do we want a "system-path" for the original path here?



Comment at: lldb/source/Target/Trace.cpp:72
+  "file": string, // path to trace file relative to the settings file
+  "loadAddress": string, // address in hex or decimal form
+  "uuid"?: string,

"load-address" instead of "loadAddress" and should be an integer instead of a 
string.



Comment at: lldb/source/Target/Trace.cpp:114-123
+bool Trace::ExtractDictionaryFromArray(const StructuredData::Array &array,
+   size_t index,
+   StructuredData::Dictionary *&dict,
+   Status &error) {
+  StructuredData::ObjectSP item = array.GetItemAtIndex(index);
+  dict = item->GetAsDictionary();
+  if (!dict)

This entire function should go into the StructuredData::Array class for re-use



Comment at: lldb/source/Target/Trace.cpp:125-133
+bool Trace::ExtractOptionalStringFromDictionary(
+const StructuredData::Dictionary &dict, const char *field, StringRef 
&value,
+Status &error) {
+  if (!dict.HasKey(field))
+return true;
+  if (dict.GetValueForKeyAsString(field, value))
+return true;

This entire function should go into StructuredData::Dictionary so other code 
can re-use this.



Comment at: lldb/source/Target/Trace.cpp:139
+  lldb::tid_t tid;
+  if (!thread.GetValueForKeyAsInteger("tid", tid))
+return SetMissingFieldError(error, "tid", "integer", thread);

Seems like we should modify GetValueForKeyAsInteger to take a pointer to an 
error as the 3rd parameter here? Then that can return an appropriate error in 
case there is a "tid" entry, but it isn't an integer.





Comment at: lldb/source/Target/Trace.cpp:151
+  StructuredData::Array *threads = nullptr;
+  if (!process.GetValueForKeyAsArray("threads", threads))
+return SetMissingFieldError(error, "threads", "array", process);

Seems like we should modify GetValueForKeyAsArray to take an pointer to an 
error as the 3rd parameter here? Then that can return an appropriate error in 
case there is a "threads" entry, but it isn't an array.



Comment at: lldb/source/Target/Trace.cpp:156
+StructuredData::Dictionary *thread = nullptr;
+if (!ExtractDictionaryFromArray(*threads, i, thread, error))
+  return false;

This functionality should be moved into 
StructuredData::Array::ExtractItemAsArray(...)



Comment at: lldb/source/Target/Trace.cpp:168
+  StringRef load_address_str;
+  if (!module.GetValueForKeyAsString("loadAddress", load_address_str))
+return SetMissingFieldError(error, "loadAddress", "string", module);

"load-address" might be a better identifier and it is also weird to store an 
address as a string. Should be an integer.



Comment at: lldb/source/Target/Trace.cpp:193-194
+
+  ModuleSP module_sp =
+  target_sp->GetOrCreateModule(module_spec, /*notify*/ false, &error);
+  return error.Success();

This will need to be able to fail gracefully here. We might not always have the 
module file on disk. We might need to create a Placeholder module like the 
ProcessMinidump does.


Repository:
  rG LLVM G

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

2020-08-25 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

After speaking with Walter a bit, key names in the JSON should be camel case. 
Many languages (Swift and JavaScript) can auto import JSON and create objects 
and use the key names as member variable names, and if they are camelCase, then 
no name conversion needs to happen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85705/new/

https://reviews.llvm.org/D85705

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

2020-08-25 Thread Greg Clayton via Phabricator via lldb-commits 
clayborg added a comment.

In D85705#2237397 , @JDevlieghere 
wrote:

> A large part of this patch is concerned with parsing which worries me from a 
> maintenance perspective. Did you consider using Yaml I/O 
> ? While I'm not a particularly big fan of 
> the format, the benefits of being able to (de)serialize any class by 
> implementing the appropriate traits are quite significant. We already have 
> traits implemented for a bunch of utility classes, such as `ArchSpec` and 
> `FileSpec` which we could reuse for this. I know changing the format would be 
> invasive, but I think it might be worth it in the long term.

So the nice thing about using StructuredData is it can be in any format: JSON, 
XML, Apple property list, YAML etc. It seems like the functions that were added 
to ArchSpec and FileSpec for the YAML I/O could be converted to use 
StructuredData and then any of the formats would work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85705/new/

https://reviews.llvm.org/D85705

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

2020-08-25 Thread Nicholas Allegra via Phabricator via lldb-commits 
comex added a comment.

The sequence I found is:

- `WillResume`
- `DoResume` sends `eBroadcastBitAsyncContinue` to 
`ProcessGDBRemote::AsyncThread`
- `AsyncThread` calls `process->SetPrivateState(eStateRunning);`
- …which sends `eBroadcastBitStateChanged` back to the main thread, handled by 
`Process::HandlePrivateEvent`
- …which ends up in this stack, when it tries to figure out whether to report 
the state change to the user:
  - `Process::HandlePrivateEvent` ->
  - `Process::ShouldBroadcastEvent` ->
  - `ThreadList::ShouldReportRun` ->
  - `Thread::ShouldReportRun` ->
  - `ThreadPlan::ShouldReportRun` ->
  - `ThreadPlan::GetPreviousPlan` ->
  - `ThreadPlan::GetThread`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86388/new/

https://reviews.llvm.org/D86388

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

2020-08-25 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added a comment.

If I understand correctly, in

  frame #0: 0x004006bb a.out`handler(sig=6) at main.c:7:5
  frame #1: 0x77a555a0 libc.so.6`__restore_rt
  frame #2: 0x77a55520 libc.so.6`raise + 272
  frame #3: 0x77a56b01 libc.so.6`abort + 337

lldb thinks that both frames 1 & 2 are trap handler frames.  They have full 
register context available for the frame above them on the stack (that is, 
frames 2 & 3) and frames 2 & 3 were interrupted asynchronously.  This doesn't 
sound right?  How do we decide what is a trap handler frame?  One way is to 
look for the 'S' augmentation in the eh_frame / dwarf debug_frame CIE/FDE for 
the function -

  unwind_plan.SetUnwindPlanForSignalTrap(
strchr(cie->augmentation, 'S') ? eLazyBoolYes : eLazyBoolNo);

The other way is from the Platform `CalculateTrapHandlerSymbolNames` method.  
PlatformLinux sets these to

  void PlatformLinux::CalculateTrapHandlerSymbolNames() {
m_trap_handlers.push_back(ConstString("_sigtramp"));
m_trap_handlers.push_back(ConstString("__kernel_rt_sigreturn"));
m_trap_handlers.push_back(ConstString("__restore_rt"));

is one of these wrong?

Maybe start with a simpler question -- does `abort` call `raise`?  Like, 
through a normal CALLQ?  Does `raise` call `__restore_rt`?  Through a normal 
CALLQ?  Do any of these have the 'S' augmentation string in their eh_frame? 
`UnwindPlan::Dump` should print whether an unwindplan says it is a trap handler 
or not, but it doesn't currently (sigh).  If it did, you could do `image 
show-unwind -n raise` and see what lldb thinks it is.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86417/new/

https://reviews.llvm.org/D86417

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

2020-08-25 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added a comment.

separate from the fact that `UnwindPlan::Dump` does not print 
`m_plan_is_for_signal_trap` for an UnwindPlan, what we need to see from a real 
trap handler -- in lldb's terminology -- is that we have restore rules for all 
of the registers.

If we're getting things marked as a trap handler which are not, maybe we should 
change the unwind rules to *explicitly* mark any registers that the UnwindPlan 
doesn't supply as "unavailable".  If `raise` doesn't have a save rule for 
`rdi`, that means that the value of rdi in `raise` has been unmodified from the 
caller function, and it could be supplied (invalidly).  In the middle of a 
stack, it isn't such a big deal because the next frame down the stack (e.g. 
frame 1) will not pass rdi up the stack, I suppose.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86417/new/

https://reviews.llvm.org/D86417

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 99d187a - Update UnwindPlan dump to list if it is a trap handler func; also Command

2020-08-25 Thread Jason Molenda via lldb-commits 

Author: Jason Molenda
Date: 2020-08-25T20:53:59-07:00
New Revision: 99d187a003c9bd4bdc42c17e5563bd80f4e159e9

URL: 
https://github.com/llvm/llvm-project/commit/99d187a003c9bd4bdc42c17e5563bd80f4e159e9
DIFF: 
https://github.com/llvm/llvm-project/commit/99d187a003c9bd4bdc42c17e5563bd80f4e159e9.diff

LOG: Update UnwindPlan dump to list if it is a trap handler func; also Command

Update the "image show-unwind" command output to show if the function
being shown is listed as a user-setting or platform trap handler.

Update the individual UnwindPlan dumps to show whether the unwind plan
is registered as a trap handler.

Added: 


Modified: 
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Symbol/UnwindPlan.cpp
lldb/test/Shell/Minidump/Windows/arm-fp-unwind.test
lldb/test/Shell/SymbolFile/Breakpad/stack-cfi-parsing.test
lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
lldb/test/Shell/SymbolFile/Breakpad/unwind-via-stack-cfi.test
lldb/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win.test
lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectTarget.cpp 
b/lldb/source/Commands/CommandObjectTarget.cpp
index b6af481090d7..30fdaf9ec9a2 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -3421,10 +3421,35 @@ class CommandObjectTargetModulesShowUnwind : public 
CommandObjectParsed {
 continue;
 
   result.GetOutputStream().Printf(
-  "UNWIND PLANS for %s`%s (start addr 0x%" PRIx64 ")\n\n",
+  "UNWIND PLANS for %s`%s (start addr 0x%" PRIx64 ")\n",
   sc.module_sp->GetPlatformFileSpec().GetFilename().AsCString(),
   funcname.AsCString(), start_addr);
 
+  Args args;
+  target->GetUserSpecifiedTrapHandlerNames(args);
+  size_t count = args.GetArgumentCount();
+  for (size_t i = 0; i < count; i++) {
+const char *trap_func_name = args.GetArgumentAtIndex(i);
+if (strcmp(funcname.GetCString(), trap_func_name) == 0)
+  result.GetOutputStream().Printf(
+  "This function is "
+  "treated as a trap handler function via user setting.\n");
+  }
+  PlatformSP platform_sp(target->GetPlatform());
+  if (platform_sp) {
+const std::vector trap_handler_names(
+platform_sp->GetTrapHandlerSymbolNames());
+for (ConstString trap_name : trap_handler_names) {
+  if (trap_name == funcname) {
+result.GetOutputStream().Printf(
+"This function's "
+"name is listed by the platform as a trap handler.\n");
+  }
+}
+  }
+
+  result.GetOutputStream().Printf("\n");
+
   UnwindPlanSP non_callsite_unwind_plan =
   func_unwinders_sp->GetUnwindPlanAtNonCallSite(*target, *thread);
   if (non_callsite_unwind_plan) {

diff  --git a/lldb/source/Symbol/UnwindPlan.cpp 
b/lldb/source/Symbol/UnwindPlan.cpp
index e8906f38e2ff..278a79d8c0ec 100644
--- a/lldb/source/Symbol/UnwindPlan.cpp
+++ b/lldb/source/Symbol/UnwindPlan.cpp
@@ -527,6 +527,18 @@ void UnwindPlan::Dump(Stream &s, Thread *thread, 
lldb::addr_t base_addr) const {
 s.Printf("not specified.\n");
 break;
   }
+  s.Printf("This UnwindPlan is for a trap handler function: ");
+  switch (m_plan_is_for_signal_trap) {
+  case eLazyBoolYes:
+s.Printf("yes.\n");
+break;
+  case eLazyBoolNo:
+s.Printf("no.\n");
+break;
+  case eLazyBoolCalculate:
+s.Printf("not specified.\n");
+break;
+  }
   if (m_plan_valid_address_range.GetBaseAddress().IsValid() &&
   m_plan_valid_address_range.GetByteSize() > 0) {
 s.PutCString("Address range of this UnwindPlan: ");

diff  --git a/lldb/test/Shell/Minidump/Windows/arm-fp-unwind.test 
b/lldb/test/Shell/Minidump/Windows/arm-fp-unwind.test
index ed871ac544a8..7c056b612a4e 100644
--- a/lldb/test/Shell/Minidump/Windows/arm-fp-unwind.test
+++ b/lldb/test/Shell/Minidump/Windows/arm-fp-unwind.test
@@ -12,6 +12,7 @@ CHECK: Assembly language inspection UnwindPlan:
 CHECK-NEXT: This UnwindPlan originally sourced from EmulateInstructionARM
 CHECK-NEXT: This UnwindPlan is sourced from the compiler: no.
 CHECK-NEXT: This UnwindPlan is valid at all instruction locations: yes.
+CHECK-NEXT: This UnwindPlan is for a trap handler function: no.
 CHECK-NEXT: row[0]:0: CFA=sp +0 =>
 CHECK-NEXT: row[1]:4: CFA=sp +8 => fp=[CFA-8] lr=[CFA-4]
 CHECK-NEXT: row[2]:6: CFA=fp +8 => fp=[CFA-8] lr=[CFA-4]

diff  --git a/lldb/test/Shell/SymbolFile/Breakpad/stack-cfi-parsing.test 
b/lldb/test/Shell/SymbolFile/Breakpad/stack-cfi-parsing.test
index dd98939aa82e..539b8096b58b 100644
--- a/lldb/test/Shell/SymbolFile/Breakpad/stack-cfi-parsing.test
+++ b/lldb/test/Shell/SymbolFile/Breakpad/stack-cfi-parsing.test
@@ -10,6 +10,7 @@ image show-unwind -n func0
 # CHECK-N

[Lldb-commits] [lldb] b1e856d - Ah, one test too many updated. This one should be unmodified.

2020-08-25 Thread Jason Molenda via lldb-commits 

Author: Jason Molenda
Date: 2020-08-25T21:03:39-07:00
New Revision: b1e856d3a9019c355baa186075699d95be7b2735

URL: 
https://github.com/llvm/llvm-project/commit/b1e856d3a9019c355baa186075699d95be7b2735
DIFF: 
https://github.com/llvm/llvm-project/commit/b1e856d3a9019c355baa186075699d95be7b2735.diff

LOG: Ah, one test too many updated.  This one should be unmodified.

Added: 


Modified: 
lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test

Removed: 




diff  --git a/lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test 
b/lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test
index 23133a801c27..b4f0cc4c402c 100644
--- a/lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test
+++ b/lldb/test/Shell/SymbolFile/target-symbols-add-unwind.test
@@ -24,4 +24,3 @@ image show-unwind -n main
 # CHECK: debug_frame UnwindPlan:
 # CHECK-NEXT: This UnwindPlan originally sourced from DWARF CFI
 # CHECK-NEXT: This UnwindPlan is sourced from the compiler: yes.
-# CHECK-NEXT: This UnwindPlan is for a trap handler function: no.



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

2020-08-25 Thread Jason Molenda via Phabricator via lldb-commits 
jasonmolenda added a comment.

Minor followup on the 'image show-unwind' output -- I just landed a patch to 
print when a function or unwindplan are marked as being a trap handler.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86417/new/

https://reviews.llvm.org/D86417

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits