[Lldb-commits] [lldb] 801c786 - [lldb][ARM/AArch64] Update disasm flags to latest v8.7a ISA

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2021-01-07T10:51:47Z
New Revision: 801c7866e6d4fba81c97d27ca56c9e05ba7b157a

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

LOG: [lldb][ARM/AArch64] Update disasm flags to latest v8.7a ISA

Add optional memory tagging extension on AArch64.

Use isAArch64() instead of listing the AArch64 triples,
which fixes us not recognising aarch64_be.

Reviewed By: omjavaid

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

Added: 


Modified: 
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp 
b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index 6427d8d176c8..8a2d3232a39a 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1056,7 +1056,7 @@ DisassemblerLLVMC::DisassemblerLLVMC(const ArchSpec &arch,
   thumb_arch_name.erase(0, 3);
   thumb_arch_name.insert(0, "thumb");
 } else {
-  thumb_arch_name = "thumbv8.2a";
+  thumb_arch_name = "thumbv8.7a";
 }
 thumb_arch.GetTriple().setArchName(llvm::StringRef(thumb_arch_name));
   }
@@ -1068,7 +1068,7 @@ DisassemblerLLVMC::DisassemblerLLVMC(const ArchSpec &arch,
   // specified)
   if (triple.getArch() == llvm::Triple::arm &&
   triple.getSubArch() == llvm::Triple::NoSubArch)
-triple.setArchName("armv8.2a");
+triple.setArchName("armv8.7a");
 
   std::string features_str = "";
   const char *triple_str = triple.getTriple().c_str();
@@ -1137,16 +1137,13 @@ DisassemblerLLVMC::DisassemblerLLVMC(const ArchSpec 
&arch,
   features_str += "+dspr2,";
   }
 
-  // If any AArch64 variant, enable the ARMv8.5 ISA with SVE extensions so we
-  // can disassemble newer instructions.
-  if (triple.getArch() == llvm::Triple::aarch64 || 
-  triple.getArch() == llvm::Triple::aarch64_32)
-features_str += "+v8.5a,+sve2";
+  // If any AArch64 variant, enable latest ISA with any optional
+  // extensions like SVE.
+  if (triple.isAArch64()) {
+features_str += "+v8.7a,+sve2,+mte";
 
-  if ((triple.getArch() == llvm::Triple::aarch64 ||
-   triple.getArch() == llvm::Triple::aarch64_32)
-  && triple.getVendor() == llvm::Triple::Apple) {
-cpu = "apple-latest";
+if (triple.getVendor() == llvm::Triple::Apple)
+  cpu = "apple-latest";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this



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

[Lldb-commits] [PATCH] D94084: [lldb][ARM/AArch64] Update disasm flags to latest v8.7a ISA

[David Spickett via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG801c7866e6d4: [lldb][ARM/AArch64] Update disasm flags to 
latest v8.7a ISA (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94084

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1056,7 +1056,7 @@
   thumb_arch_name.erase(0, 3);
   thumb_arch_name.insert(0, "thumb");
 } else {
-  thumb_arch_name = "thumbv8.2a";
+  thumb_arch_name = "thumbv8.7a";
 }
 thumb_arch.GetTriple().setArchName(llvm::StringRef(thumb_arch_name));
   }
@@ -1068,7 +1068,7 @@
   // specified)
   if (triple.getArch() == llvm::Triple::arm &&
   triple.getSubArch() == llvm::Triple::NoSubArch)
-triple.setArchName("armv8.2a");
+triple.setArchName("armv8.7a");
 
   std::string features_str = "";
   const char *triple_str = triple.getTriple().c_str();
@@ -1137,16 +1137,13 @@
   features_str += "+dspr2,";
   }
 
-  // If any AArch64 variant, enable the ARMv8.5 ISA with SVE extensions so we
-  // can disassemble newer instructions.
-  if (triple.getArch() == llvm::Triple::aarch64 || 
-  triple.getArch() == llvm::Triple::aarch64_32)
-features_str += "+v8.5a,+sve2";
+  // If any AArch64 variant, enable latest ISA with any optional
+  // extensions like SVE.
+  if (triple.isAArch64()) {
+features_str += "+v8.7a,+sve2,+mte";
 
-  if ((triple.getArch() == llvm::Triple::aarch64 ||
-   triple.getArch() == llvm::Triple::aarch64_32)
-  && triple.getVendor() == llvm::Triple::Apple) {
-cpu = "apple-latest";
+if (triple.getVendor() == llvm::Triple::Apple)
+  cpu = "apple-latest";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1056,7 +1056,7 @@
   thumb_arch_name.erase(0, 3);
   thumb_arch_name.insert(0, "thumb");
 } else {
-  thumb_arch_name = "thumbv8.2a";
+  thumb_arch_name = "thumbv8.7a";
 }
 thumb_arch.GetTriple().setArchName(llvm::StringRef(thumb_arch_name));
   }
@@ -1068,7 +1068,7 @@
   // specified)
   if (triple.getArch() == llvm::Triple::arm &&
   triple.getSubArch() == llvm::Triple::NoSubArch)
-triple.setArchName("armv8.2a");
+triple.setArchName("armv8.7a");
 
   std::string features_str = "";
   const char *triple_str = triple.getTriple().c_str();
@@ -1137,16 +1137,13 @@
   features_str += "+dspr2,";
   }
 
-  // If any AArch64 variant, enable the ARMv8.5 ISA with SVE extensions so we
-  // can disassemble newer instructions.
-  if (triple.getArch() == llvm::Triple::aarch64 || 
-  triple.getArch() == llvm::Triple::aarch64_32)
-features_str += "+v8.5a,+sve2";
+  // If any AArch64 variant, enable latest ISA with any optional
+  // extensions like SVE.
+  if (triple.isAArch64()) {
+features_str += "+v8.7a,+sve2,+mte";
 
-  if ((triple.getArch() == llvm::Triple::aarch64 ||
-   triple.getArch() == llvm::Triple::aarch64_32)
-  && triple.getVendor() == llvm::Triple::Apple) {
-cpu = "apple-latest";
+if (triple.getVendor() == llvm::Triple::Apple)
+  cpu = "apple-latest";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D93951: [vscode] Improve runInTerminal and support linux

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

I'll follow your recommendations.

Regarding Windows, mkfifo can be replaced with CreateNamedPipe and should lead 
to the same behavior. Sadly, I don't have a Windows machine where I could even 
compile it... So this should be left for someone else or for when I can get my 
laptop from the office.

And why are fifo files needed? Well, when you open a normal text file, c++ 
doesn't just know when the file gets updated. Its pointer to the end of the 
file is set when you open the file. So if you want to get updates, you have to 
reopen the file in a polling fashion. Fifo files, on the other hand, which are 
the same as pipes, behave the same way as stdin, so c++ keeps reading it until 
it's empty and closed at OS level, aware of new data being written to it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93951

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

[Lldb-commits] [PATCH] D93951: [vscode] Improve runInTerminal and support linux

[walter erquinigo via Phabricator via lldb-commits]

wallace marked an inline comment as done.
wallace added a comment.

I'll think about just writing this as tcp sockets. That would for sure be cross 
platform


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93951

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

[Lldb-commits] [PATCH] D93951: [vscode] Improve runInTerminal and support linux

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 5 inline comments as done.
wallace added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2994
+  // signal to prevent being paused forever.
+  const char *timeout_env_var = getenv("LLDB_VSCODE_RIT_TIMEOUT_IN_MS");
+  int timeout_in_ms =

clayborg wrote:
> Why are we using env vars for some things and options values for others? We 
> should probably pass this in as an argument. We are also polluting the env 
> vars of the new process we will exec if we leave this in the environment
I'm only using this for testing, so that the python tests doesn't take 10 
seconds waiting for the timeout to happen. The user shouldn't need to change 
this for any reason.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3001
+  const char *target = target_arg.getValue();
+  execv(target, argv + target_arg.getIndex() + 2);
+

clayborg wrote:
> I assume env vars are being inherited from this process?
yes, that's the default behavior



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3018
+  llvm::SmallString<256> program_path(argv[0]);
+  llvm::sys::fs::make_absolute(program_path);
+  g_vsc.debug_adaptor_path = program_path.str().str();

clayborg wrote:
> Might be worth checking if there is a llvm file system call to determine the 
> program path already?
it seems there isn't


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93951

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

[Lldb-commits] [PATCH] D94244: [lldb] Bump the required SWIG version to 3

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: LLDB.
Herald added a subscriber: mgorny.
JDevlieghere requested review of this revision.

Bump the required SWIG version to 3. If my memory serves me well we last bumped 
the required SWIG version to 2 for Python 3. At that time SWIG 3 had already 
been around for a while so everyone I know was already using that.

It appears that SWIG 3 is the only version that officially supports C++11 which 
we're using in the typemap. SWIG 3 was released in 2014 so I think it's 
reasonable to make that the minimally supported version.

https://bugs.llvm.org/show_bug.cgi?id=48685


https://reviews.llvm.org/D94244

Files:
  lldb/cmake/modules/FindLuaAndSwig.cmake
  lldb/cmake/modules/FindPythonAndSwig.cmake
  lldb/docs/resources/build.rst


Index: lldb/docs/resources/build.rst
===
--- lldb/docs/resources/build.rst
+++ lldb/docs/resources/build.rst
@@ -34,7 +34,7 @@
 scripting support.
 
 * `Python `_
-* `SWIG `_ 2 or later.
+* `SWIG `_ 3 or later.
 
 Optional Dependencies
 *
Index: lldb/cmake/modules/FindPythonAndSwig.cmake
===
--- lldb/cmake/modules/FindPythonAndSwig.cmake
+++ lldb/cmake/modules/FindPythonAndSwig.cmake
@@ -38,7 +38,7 @@
 if(Python3_LIBRARIES AND Python3_INCLUDE_DIRS AND Python3_EXECUTABLE AND 
SWIG_EXECUTABLE)
   set(PYTHONANDSWIG_FOUND TRUE)
 else()
-  find_package(SWIG 2.0)
+  find_package(SWIG 3.0)
   if (SWIG_FOUND)
   FindPython3()
   else()
Index: lldb/cmake/modules/FindLuaAndSwig.cmake
===
--- lldb/cmake/modules/FindLuaAndSwig.cmake
+++ lldb/cmake/modules/FindLuaAndSwig.cmake
@@ -7,7 +7,7 @@
 if(LUA_LIBRARIES AND LUA_INCLUDE_DIR AND SWIG_EXECUTABLE)
   set(LUAANDSWIG_FOUND TRUE)
 else()
-  find_package(SWIG 2.0)
+  find_package(SWIG 3.0)
   if (SWIG_FOUND)
 find_package(Lua 5.3)
 if(LUA_FOUND AND SWIG_FOUND)


Index: lldb/docs/resources/build.rst
===
--- lldb/docs/resources/build.rst
+++ lldb/docs/resources/build.rst
@@ -34,7 +34,7 @@
 scripting support.
 
 * `Python `_
-* `SWIG `_ 2 or later.
+* `SWIG `_ 3 or later.
 
 Optional Dependencies
 *
Index: lldb/cmake/modules/FindPythonAndSwig.cmake
===
--- lldb/cmake/modules/FindPythonAndSwig.cmake
+++ lldb/cmake/modules/FindPythonAndSwig.cmake
@@ -38,7 +38,7 @@
 if(Python3_LIBRARIES AND Python3_INCLUDE_DIRS AND Python3_EXECUTABLE AND SWIG_EXECUTABLE)
   set(PYTHONANDSWIG_FOUND TRUE)
 else()
-  find_package(SWIG 2.0)
+  find_package(SWIG 3.0)
   if (SWIG_FOUND)
   FindPython3()
   else()
Index: lldb/cmake/modules/FindLuaAndSwig.cmake
===
--- lldb/cmake/modules/FindLuaAndSwig.cmake
+++ lldb/cmake/modules/FindLuaAndSwig.cmake
@@ -7,7 +7,7 @@
 if(LUA_LIBRARIES AND LUA_INCLUDE_DIR AND SWIG_EXECUTABLE)
   set(LUAANDSWIG_FOUND TRUE)
 else()
-  find_package(SWIG 2.0)
+  find_package(SWIG 3.0)
   if (SWIG_FOUND)
 find_package(Lua 5.3)
 if(LUA_FOUND AND SWIG_FOUND)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D93926: [lldb] Don't remove the lldb.debugger var after exiting the interpreter

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D93926#2481929 , @labath wrote:

> In D93926#2479749 , @JDevlieghere 
> wrote:
>
>> Your argument is correct because the interactive script interpreter always 
>> belongs to a single debugger.
>
> I'm not sure that even this is true (for python, anyway). IIUC all 
> SBDebuggers share the same python interpreter (because python does not allow 
> the creation of completely independent python contexts). We do some trickery 
> to make it it appear as if these were independent, but I am not sure the 
> trickery extends to the `lldb` module...

It depends on your definition of Python interpreter. The `Debugger` owns the 
`ScriptInterpreterSP` which is what would decide which debugger to put in the 
convenience variable, but you're right that they all share the same "python 
instance" under the hood.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93926

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

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D94064#2483578 , @dblaikie wrote:

> In D94064#2481925 , @labath wrote:
>
>> I don't think that simply setting func_lo_pc to zero will be sufficient to 
>> make this work. I would expect this to break in more complicated scenarios 
>> (like, even if we just have two of these functions). I think the only reason 
>> it works in this case is because for this particular function, it's base 
>> address (relative to the CU base) *is* zero.
>
> I certainly don't have high confidence in the change, to be sure - but I 
> think it's a bit more robust than that.

Yes, it seems it is, though just by a bit. :)

The thing which is missing is the location list on the variable DIE -- without 
it, it does not matter which address gets put here (so long as it does not 
trigger the assertion), as the value is totally unused. The necessity of the 
location list is kind of obvious from my description of the problem, though 
even I did not realize when I was writing it that this requires a more 
elaborate test case.

Adjusting your test case to produce a location list for `i` I got this 
(probably not minimal) snippet (add -O1 to CFLAGS):

  #define optnone __attribute__((optnone))
  #define noinline __attribute__((noinline))
  
  void optnone noinline stop() {}
  void optnone noinline consume(int i) {}
  void noinline f1(int x) {
int i = x;
stop();
consume(i);
i = 5;
consume(i);
  }
  int main() {
int j = 12;
f1(j);
stop();
  }

Here lldb fails to print the value of `i`, but if I rearrange the code so that 
the `f1` functions comes first, the value is printed correctly:

  #define optnone __attribute__((optnone))
  #define noinline __attribute__((noinline))
  
  void optnone noinline stop();
  void optnone noinline consume(int i);
  void noinline f1(int x) {
int i = x;
stop();
consume(i);
i = 5;
consume(i);
  }
  void optnone noinline stop() {}
  void optnone noinline consume(int i) {}
  int main() {
int j = 12;
f1(j);
stop();
  }



>> The purpose of func_lo_pc is pretty weird, but it's essentially used for 
>> runtime relocation of location lists within the function -- we take the 
>> static "base" of the function, and the runtime "base", and the difference 
>> between the two produces the load bias. Applying the same bias to all 
>> variable location lists "relocates" the variables. (The reason this is so 
>> convoluted is reading location lists straight from (unrelocated) .o files on 
>> mac. I seriously considered changing this when working on debug_rnglists, 
>> but it turned out it wasn't really necessary, so I let it go.)
>
> Yep, I figured a bunch of this was for DWARF in unrelocated MachO files - and 
> that they wouldn't be able to/want to use Split DWARF or this feature (which 
> is particularly relevant to Split DWARF). Does any of this logic apply 
> outside that unrelocated MachO scenario?

Yes, this code is used everywhere, though it's role is less important (and one 
that could be implemented in an easier manner, were it not for MachO) -- it 
adjusts the variable address for ASLR, i.e., the variable/function being loaded 
at a different address than what the static debug info says.

>> LLDB's representation of a function (lldb_private::Function) assumes that 
>> all functions will be contiguous (Function::GetAddressRange returns a single 
>> range). If we make it so that this range matches the first block of the 
>> function (maybe that's what happens now),
>
> Actually seems to adjust for discontiguous ranges and not just pick the start 
> of the first (as it appears in the DWARF - which isn't guaranteed to be 
> ordered in any way) range, instead finding the smallest and largest 
> addresses: 
> https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L2357

Ah.. interesting. In that case, I believe things should work if we use the same 
algorithm to compute `func_lo_pc`. We may still run into weird issues for 
functions which are truly discontinuous, but it should be ok for single-range 
range lists, at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94064

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

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

In D94063#2483546 , @dblaikie wrote:

> If it's better to write it using C++ source and custom clang flags I can do 
> that instead (it'll be an -mllvm flag - looks like there's one other test 
> that does that: `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py: 
>dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) - 
> means the test will be a bit more convoluted to tickle the subprogram ranges, 
> but not much - just takes two functions+function-sections.

I certainly wouldn't want to drop the existing test. However, it could be 
useful to have c++ test too. This one could feature a more complicated 
executable, and be more open-ended/exploratory and test end-to-end 
functionality (including compiler integration), instead of a targeted "did we 
parse DW_AT_ranges correctly" regression test. Then this test could go into the 
`API` test category, as we have the ability to run those kinds of tests against 
different compilers.

However, all of that is strictly optional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

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

[Lldb-commits] [PATCH] D70277: [Signal] Allow llvm clients to opt into one-shot SIGPIPE handling

[Nick Desaulniers via Phabricator via lldb-commits]

nickdesaulniers added a comment.

This causes:

  $ clang -E -fno-integrated-cc1 x.c | tee foo.txt |head

to crash. If I revert 9a3f892d018238dce5181e458905311db8e682f5 
 and 
4624e83ce7b124545b55e45ba13f2d900ed65654 
, the 
crash goes away.

This is causing our build logs to be flooded with:

  LLVM ERROR: IO failure on output stream: Broken pipe

where x.c looks like:

  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70277

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

[Lldb-commits] [lldb] 274afac - lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via lldb-commits]

Author: David Blaikie
Date: 2021-01-07T14:28:03-08:00
New Revision: 274afac9a17f43e5396a0d6c7a0741702596a7bd

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

LOG: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

gcc already produces debug info with this form
-freorder-block-and-partition
clang produces this sort of thing with -fbasic-block-sections and with a
coming-soon tweak to use ranges in DWARFv5 where they can allow greater
reuse of debug_addr than the low/high_pc forms.

This fixes the case of breaking on a function name, but leaves broken
printing a variable - a follow-up commit will add that and improve the
test case to match.

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

Added: 
lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
index 3eca911f4837..421298802645 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -687,13 +687,15 @@ const char *DWARFDebugInfoEntry::GetPubname(const 
DWARFUnit *cu) const {
 /// table, except that the actual DIE offset for the function is placed in the
 /// table instead of the compile unit offset.
 void DWARFDebugInfoEntry::BuildFunctionAddressRangeTable(
-const DWARFUnit *cu, DWARFDebugAranges *debug_aranges) const {
+DWARFUnit *cu, DWARFDebugAranges *debug_aranges) const {
   if (m_tag) {
 if (m_tag == DW_TAG_subprogram) {
-  dw_addr_t lo_pc = LLDB_INVALID_ADDRESS;
-  dw_addr_t hi_pc = LLDB_INVALID_ADDRESS;
-  if (GetAttributeAddressRange(cu, lo_pc, hi_pc, LLDB_INVALID_ADDRESS)) {
-debug_aranges->AppendRange(GetOffset(), lo_pc, hi_pc);
+  DWARFRangeList ranges;
+  GetAttributeAddressRanges(cu, ranges,
+/*check_hi_lo_pc=*/true);
+  for (const auto &r : ranges) {
+debug_aranges->AppendRange(GetOffset(), r.GetRangeBase(),
+   r.GetRangeEnd());
   }
 }
 

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
index 534d92e1feb9..0ba56a0a4161 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
@@ -42,7 +42,7 @@ class DWARFDebugInfoEntry {
   bool operator==(const DWARFDebugInfoEntry &rhs) const;
   bool operator!=(const DWARFDebugInfoEntry &rhs) const;
 
-  void BuildFunctionAddressRangeTable(const DWARFUnit *cu,
+  void BuildFunctionAddressRangeTable(DWARFUnit *cu,
   DWARFDebugAranges *debug_aranges) const;
 
   bool Extract(const lldb_private::DWARFDataExtractor &data,

diff  --git a/lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s 
b/lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
new file mode 100644
index ..1ff883c67f9e
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -0,0 +1,159 @@
+   .text
+   .file   "main.c"
+   .globl  main# -- Begin function main
+   .p2align4, 0x90
+   .type   main,@function
+main:   # @main
+.Lfunc_begin0:
+   .file   1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
+   .loc1 1 0   # main.c:1:0
+   .cfi_startproc
+# %bb.0:# %entry
+   pushq   %rbp
+   .cfi_def_cfa_offset 16
+   .cfi_offset %rbp, -16
+   movq%rsp, %rbp
+   .cfi_def_cfa_register %rbp
+   xorl%eax, %eax
+.Ltmp0:
+   .loc1 2 7 prologue_end  # main.c:2:7
+   movl$3, -4(%rbp)
+   .loc1 3 1   # main.c:3:1
+   popq%rbp
+   .cfi_def_cfa %rsp, 8
+   retq
+.Ltmp1:
+.Lfunc_end0:
+   .size   main, .Lfunc_end0-main
+   .cfi_endproc
+# -- End function
+   .section.debug_abbrev,"",@progbits
+   .byte   1   # Abbreviation Code
+   .byte   17  # DW_TAG_compile_unit
+   .byte   1   # DW_CHILDREN_yes
+   .byte   37  # DW_AT_producer
+   .byte   14  # DW_FORM_strp
+   .byte   19  # DW_AT_language

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG274afac9a17f: lldb: Add support for DW_AT_ranges on 
DW_TAG_subprograms (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
  lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Index: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -0,0 +1,17 @@
+# REQUIRES: x86
+# RUN: %clang -target x86_64-pc-linux -g -O0 %S/Inputs/subprogram_ranges.s -o %t.out
+# RUN: %lldb -b -s %s %t.out 2>&1 | FileCheck %s
+
+# Test breaking on symbols and printing variables when a DW_TAG_subprogram uses
+# DW_AT_ranges instead of DW_AT_low_pc/DW_AT_high_pc.  While the assembly here
+# is a bit unrealistic - it's a single-entry range using DWARFv4 which isn't
+# useful for anything (a single-entry range with DWARFv5 can reduce address
+# relocations, and multi-entry ranges can be used for function sections), but
+# it's the simplest thing to test. If anyone's updating this test at some
+# point, feel free to replace it with another equivalent test if it's
+# especially useful, but don't dismiss it as pointless just because it's a bit
+# weird.
+
+b main
+# CHECK: (lldb) b main
+# CHECK-NEXT: Breakpoint 1: where = subprogram_ranges.test.tmp.out`main + 6 at main.c:2:7
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/subprogram_ranges.s
@@ -0,0 +1,159 @@
+	.text
+	.file	"main.c"
+	.globl	main# -- Begin function main
+	.p2align	4, 0x90
+	.type	main,@function
+main:   # @main
+.Lfunc_begin0:
+	.file	1 "/usr/local/google/home/blaikie/dev/scratch" "main.c"
+	.loc	1 1 0   # main.c:1:0
+	.cfi_startproc
+# %bb.0:# %entry
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset %rbp, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register %rbp
+	xorl	%eax, %eax
+.Ltmp0:
+	.loc	1 2 7 prologue_end  # main.c:2:7
+	movl	$3, -4(%rbp)
+	.loc	1 3 1   # main.c:3:1
+	popq	%rbp
+	.cfi_def_cfa %rsp, 8
+	retq
+.Ltmp1:
+.Lfunc_end0:
+	.size	main, .Lfunc_end0-main
+	.cfi_endproc
+# -- End function
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	1   # DW_CHILDREN_yes
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	16  # DW_AT_stmt_list
+	.byte	23  # DW_FORM_sec_offset
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	17  # DW_AT_low_pc
+	.byte	1   # DW_FORM_addr
+	.byte	18  # DW_AT_high_pc
+	.byte	6   # DW_FORM_data4
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	2   # Abbreviation Code
+	.byte	46  # DW_TAG_subprogram
+	.byte	1   # DW_CHILDREN_yes
+	.byte	85  # DW_AT_ranges
+	.byte	23  # DW_FORM_sec_offset
+	.byte	64  # DW_AT_frame_base
+	.byte	24  # DW_FORM_exprloc
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	58  # DW_AT_decl_file
+	.byte	11  # DW_FORM_data1
+	.byte	59  # DW_AT_decl_line
+	.byte	11  # DW_FORM_data1
+	.byte	73  # DW_AT_type
+	.byte	19  # DW_FORM_ref4
+	.byte	63  # DW_AT_external
+	.byte	25  # DW_FORM_flag_present
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	3

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D94063#2485271 , @labath wrote:

> In D94063#2483546 , @dblaikie wrote:
>
>> If it's better to write it using C++ source and custom clang flags I can do 
>> that instead (it'll be an -mllvm flag - looks like there's one other test 
>> that does that: `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py:
>> dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) 
>> - means the test will be a bit more convoluted to tickle the subprogram 
>> ranges, but not much - just takes two functions+function-sections.
>
> I certainly wouldn't want to drop the existing test.

Ah, what's the tradeoff between the different test types here?

> However, it could be useful to have c++ test too. This one could feature a 
> more complicated executable, and be more open-ended/exploratory and test 
> end-to-end functionality (including compiler integration), instead of a 
> targeted "did we parse DW_AT_ranges correctly" regression test. Then this 
> test could go into the `API` test category, as we have the ability to run 
> those kinds of tests against different compilers.

Does this include support for custom compiler flags (it'd currently take a 
non-official/internal-only llvm flag to create the DW_AT_ranges on a subprogram 
that I have in mind, for instance)?

> However, all of that is strictly optional.

I'll consider it for a separate commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94063

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

[Lldb-commits] [lldb] 15f5971 - [LLDB][RISCV] Add RISC-V ArchSpec and rv32/rv64 variant detection

[Luís Marques via lldb-commits]

Author: Luís Marques
Date: 2021-01-07T23:02:55Z
New Revision: 15f5971150684b656005cfd5b744c1a34477ff60

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

LOG: [LLDB][RISCV] Add RISC-V ArchSpec and rv32/rv64 variant detection

Adds the RISC-V ArchSpec bits contributed by @simoncook as part of D62732,
plus logic to distinguish between riscv32 and riscv64 based on ELF class.

The patch follows the implementation approach previously used for MIPS.
It defines RISC-V architecture subtypes and inspects the ELF header,
namely the ELF class, to detect the right subtype.

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

Added: 
lldb/test/Shell/ObjectFile/ELF/riscv-arch.yaml

Modified: 
lldb/include/lldb/Utility/ArchSpec.h
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/source/Utility/ArchSpec.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/ArchSpec.h 
b/lldb/include/lldb/Utility/ArchSpec.h
index a727d5ca4f79..b35766d3d9cf 100644
--- a/lldb/include/lldb/Utility/ArchSpec.h
+++ b/lldb/include/lldb/Utility/ArchSpec.h
@@ -92,6 +92,12 @@ class ArchSpec {
 eARM_abi_hard_float = 0x0400
   };
 
+  enum RISCVSubType {
+eRISCVSubType_unknown,
+eRISCVSubType_riscv32,
+eRISCVSubType_riscv64,
+  };
+
   enum Core {
 eCore_arm_generic,
 eCore_arm_armv4,
@@ -184,6 +190,9 @@ class ArchSpec {
 eCore_hexagon_hexagonv4,
 eCore_hexagon_hexagonv5,
 
+eCore_riscv32,
+eCore_riscv64,
+
 eCore_uknownMach32,
 eCore_uknownMach64,
 

diff  --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp 
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 82a08a235084..cad9ce218b10 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -296,9 +296,23 @@ static uint32_t mipsVariantFromElfFlags (const 
elf::ELFHeader &header) {
   return arch_variant;
 }
 
+static uint32_t riscvVariantFromElfFlags(const elf::ELFHeader &header) {
+  uint32_t fileclass = header.e_ident[EI_CLASS];
+  switch (fileclass) {
+  case llvm::ELF::ELFCLASS32:
+return ArchSpec::eRISCVSubType_riscv32;
+  case llvm::ELF::ELFCLASS64:
+return ArchSpec::eRISCVSubType_riscv64;
+  default:
+return ArchSpec::eRISCVSubType_unknown;
+  }
+}
+
 static uint32_t subTypeFromElfHeader(const elf::ELFHeader &header) {
   if (header.e_machine == llvm::ELF::EM_MIPS)
 return mipsVariantFromElfFlags(header);
+  else if (header.e_machine == llvm::ELF::EM_RISCV)
+return riscvVariantFromElfFlags(header);
 
   return LLDB_INVALID_CPUTYPE;
 }

diff  --git a/lldb/source/Utility/ArchSpec.cpp 
b/lldb/source/Utility/ArchSpec.cpp
index b0cbb269b18b..8025f37c4b38 100644
--- a/lldb/source/Utility/ArchSpec.cpp
+++ b/lldb/source/Utility/ArchSpec.cpp
@@ -212,6 +212,11 @@ static const CoreDefinition g_core_definitions[] = {
 {eByteOrderLittle, 4, 4, 4, llvm::Triple::hexagon,
  ArchSpec::eCore_hexagon_hexagonv5, "hexagonv5"},
 
+{eByteOrderLittle, 4, 2, 4, llvm::Triple::riscv32, ArchSpec::eCore_riscv32,
+ "riscv32"},
+{eByteOrderLittle, 8, 2, 4, llvm::Triple::riscv64, ArchSpec::eCore_riscv64,
+ "riscv64"},
+
 {eByteOrderLittle, 4, 4, 4, llvm::Triple::UnknownArch,
  ArchSpec::eCore_uknownMach32, "unknown-mach-32"},
 {eByteOrderLittle, 8, 4, 4, llvm::Triple::UnknownArch,
@@ -395,6 +400,10 @@ static const ArchDefinitionEntry g_elf_arch_entries[] = {
  0xu, 0xu}, // ARC
 {ArchSpec::eCore_avr, llvm::ELF::EM_AVR, LLDB_INVALID_CPUTYPE,
  0xu, 0xu}, // AVR
+{ArchSpec::eCore_riscv32, llvm::ELF::EM_RISCV,
+ ArchSpec::eRISCVSubType_riscv32, 0xu, 0xu}, // riscv32
+{ArchSpec::eCore_riscv64, llvm::ELF::EM_RISCV,
+ ArchSpec::eRISCVSubType_riscv64, 0xu, 0xu}, // riscv64
 };
 
 static const ArchDefinition g_elf_arch_def = {

diff  --git a/lldb/test/Shell/ObjectFile/ELF/riscv-arch.yaml 
b/lldb/test/Shell/ObjectFile/ELF/riscv-arch.yaml
new file mode 100644
index ..7fbf2059c74e
--- /dev/null
+++ b/lldb/test/Shell/ObjectFile/ELF/riscv-arch.yaml
@@ -0,0 +1,24 @@
+# RUN: yaml2obj --docnum=1 %s > %t32
+# RUN: yaml2obj --docnum=2 %s > %t64
+# RUN: lldb-test object-file %t32 | FileCheck --check-prefix=CHECK-RV32 %s
+# RUN: lldb-test object-file %t64 | FileCheck --check-prefix=CHECK-RV64 %s
+
+# CHECK-RV32: Architecture: riscv32--
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_RISCV
+...
+
+# CHECK-RV64: Architecture: riscv64--
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_RISCV
+...



___

Re: [Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[Jim Ingham via lldb-commits]

> On Jan 7, 2021, at 2:29 PM, David Blaikie via Phabricator via lldb-commits 
>  wrote:
> 
> dblaikie added a comment.
> 
> In D94063#2485271 , @labath wrote:
> 
>> In D94063#2483546 , @dblaikie wrote:
>> 
>>> If it's better to write it using C++ source and custom clang flags I can do 
>>> that instead (it'll be an -mllvm flag - looks like there's one other test 
>>> that does that: `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py:   
>>>  dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm 
>>> -accel-tables=Dwarf"))`) - means the test will be a bit more convoluted to 
>>> tickle the subprogram ranges, but not much - just takes two 
>>> functions+function-sections.
>> 
>> I certainly wouldn't want to drop the existing test.
> 
> Ah, what's the tradeoff between the different test types here?

This is my take (this has been a contentious issue so I'm sure there are other 
takes...):

The "Shell" tests use pattern matching against the lldb command line output.  
They are useful for testing the details of the command interaction.  You can 
also do that using pexpect in the API tests, but the Python 2.7 version of 
pexpect seemed really flakey so we switched to shell tests for this sort of 
thing.

Because you are matching against text output that isn't API, they are less 
stable.  For instance if we changed anything in the "break set" output, your 
test would fail(*).  And because you are picking details out of that text, the 
tests are less precise.  You either have to match more of the command line than 
you are actually testing for, which isn't a good practice, or you run the risk 
of finding the text you were looking for in a directory path or other unrelated 
part of the output.  Also they are harder to debug if you can't reproduce the 
failure locally, since it isn't easy to add internal checks/output to the test 
to try hypotheses.  Whenever I have run into failures of this sort the first 
thing I do is convert the test to an API test...

But the main benefit of the "Shell" tests is that you can write tests without 
having to know Python or learn the lldb Python API's.  And if you are coming 
from clang you already know how FileCheck tests work, so that's a bonus.  I 
think it's legit to require that folks actually working on lldb learn the SB 
API's.  But we were persuaded that it wasn't fair to impose that on people not 
working on lldb, and yet such folks do sometimes need to write tests for 
lldb...  So for simple tests, the Shell tests are an okay option.  But really, 
there's nothing you can do in a Shell test that you can't do in an API test.

The "API" tests use the Python SB API's - though they also have the ability to 
run commands and do expect type checks on the output so for single commands 
they work much as the shell tests do (there's even a FileCheck style assert 
IIRC).  They are a little more verbose than shell tests (though we've reduced 
the boilerplate significantly over the years).  And of course you have to know 
the SB API's.  But for instance, if you wanted to know that a breakpoint was 
set on line 5 of foo.c, you can set the breakpoint, then ask the resultant 
SBBreakpoint object what it's file & line numbers were directly.  So once 
you've gotten familiar with the setup, IMO you can write much higher quality 
tests with the API tests.


Jim

(*) I am personally not at all in favor of the Shell tests, but that's in part 
because back in the day I was asked to make a simple and useful change to the 
output of the gdb "break" command but then righting the gdb testsuite - which 
is all based on expecting the results of various gdb commands - was so tedious 
that we ended up dropping the change instead.  I don't want to get to that 
place with lldb, but the hope is that as long as we mostly write API tests, we 
can avoid encumbering the current command outputs too heavily...

Jim

> 
>> However, it could be useful to have c++ test too. This one could feature a 
>> more complicated executable, and be more open-ended/exploratory and test 
>> end-to-end functionality (including compiler integration), instead of a 
>> targeted "did we parse DW_AT_ranges correctly" regression test. Then this 
>> test could go into the `API` test category, as we have the ability to run 
>> those kinds of tests against different compilers.
> 
> Does this include support for custom compiler flags (it'd currently take a 
> non-official/internal-only llvm flag to create the DW_AT_ranges on a 
> subprogram that I have in mind, for instance)?
> 
>> However, all of that is strictly optional.
> 
> I'll consider it for a separate commit.
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D94063/new/
> 
> https://reviews.llvm.org/D94063
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> https://lists.llvm.o

[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: jingham, mib.
Herald added a subscriber: arphaman.
JDevlieghere requested review of this revision.

Replace uses of `GetModuleAtIndexUnlocked` and 
`GetModulePointerAtIndexUnlocked` with the `ModuleIterable` and 
`ModuleIterableNoLocking` where applicable.


https://reviews.llvm.org/D94271

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Utility/Iterable.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Core/SearchFilter.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
  lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
  lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3076,13 +3076,8 @@
 }
   }
   // Figure out which one is the executable, and set that in our target:
-  const ModuleList &target_modules = GetTarget().GetImages();
-  std::lock_guard guard(target_modules.GetMutex());
-  size_t num_modules = target_modules.GetSize();
   ModuleSP new_executable_module_sp;
-
-  for (size_t i = 0; i < num_modules; i++) {
-ModuleSP module_sp(target_modules.GetModuleAtIndexUnlocked(i));
+  for (ModuleSP module_sp : GetTarget().GetImages().Modules()) {
 if (module_sp && module_sp->IsExecutable()) {
   if (GetTarget().GetExecutableModulePointer() != module_sp.get())
 new_executable_module_sp = module_sp;
Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -36,13 +36,8 @@
 
   Target &target = process_sp->GetTarget();
 
-  const ModuleList &target_modules = target.GetImages();
-  std::lock_guard guard(target_modules.GetMutex());
-  const size_t num_modules = target_modules.GetSize();
-  for (size_t i = 0; i < num_modules; ++i) {
-Module *module_pointer = target_modules.GetModulePointerAtIndexUnlocked(i);
-
-const Symbol *symbol = module_pointer->FindFirstSymbolWithNameAndType(
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
+const Symbol *symbol = module_sp->FindFirstSymbolWithNameAndType(
 ConstString("__asan_get_alloc_stack"), lldb::eSymbolTypeAny);
 
 if (symbol != nullptr)
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
@@ -452,15 +452,11 @@
   if (process_sp) {
 Target &target = process_sp->GetTarget();
 
-const ModuleList &target_modules = target.GetImages();
-std::lock_guard guard(target_modules.GetMutex());
-size_t num_modules = target_modules.GetSize();
 if (!m_objc_module_sp) {
-  for (size_t i = 0; i < num_modules; i++) {
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
 if (ObjCLanguageRuntime::Get(*process_sp)
-->IsModuleObjCLibrary(
-target_modules.GetModuleAtIndexUnlocked(i))) {
-  m_objc_module_sp = target_modules.GetModuleAtIndexUnlocked(i);
+->IsModuleObjCLibrary(module_sp)) {
+  m_objc_module_sp = module_sp;
   break;
 }
   }
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -1590,10 +1590,7 @@
 
 ObjCLanguageRuntime *objc_runtime = ObjCLanguageRuntime::Get(*process);
 if (objc_runtime) {
-  const ModuleList &images = process->GetTarget().GetImages();
-  std::lock_guard guard(images.GetMutex());
-  for (size_t i = 0; i < images.GetSize(); ++i) {
-lldb::ModuleSP mod_sp = images.GetModuleAtIndexUnlocked(i);
+  for (lldb::ModuleSP mod_sp : process->GetTarget().GetImages().Modules()) {
 if (objc_ru

[Lldb-commits] [PATCH] D94136: [lldb] Make ModuleList iterable (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere abandoned this revision.
JDevlieghere added a comment.

Abandoning this in favor of using the ModuleIterable everywhere: 
https://reviews.llvm.org/D94271


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

https://reviews.llvm.org/D94136

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

Re: [Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[David Blaikie via lldb-commits]

On Thu, Jan 7, 2021 at 3:37 PM Jim Ingham via lldb-commits
 wrote:
>
>
>
> > On Jan 7, 2021, at 2:29 PM, David Blaikie via Phabricator via lldb-commits 
> >  wrote:
> >
> > dblaikie added a comment.
> >
> > In D94063#2485271 , @labath wrote:
> >
> >> In D94063#2483546 , @dblaikie 
> >> wrote:
> >>
> >>> If it's better to write it using C++ source and custom clang flags I can 
> >>> do that instead (it'll be an -mllvm flag - looks like there's one other 
> >>> test that does that: 
> >>> `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py:
> >>> dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) - 
> >>> means the test will be a bit more convoluted to tickle the subprogram 
> >>> ranges, but not much - just takes two functions+function-sections.
> >>
> >> I certainly wouldn't want to drop the existing test.
> >
> > Ah, what's the tradeoff between the different test types here?
>
> This is my take (this has been a contentious issue so I'm sure there are 
> other takes...):
>
> The "Shell" tests use pattern matching against the lldb command line output.  
> They are useful for testing the details of the command interaction.  You can 
> also do that using pexpect in the API tests, but the Python 2.7 version of 
> pexpect seemed really flakey so we switched to shell tests for this sort of 
> thing.
>
> Because you are matching against text output that isn't API, they are less 
> stable.  For instance if we changed anything in the "break set" output, your 
> test would fail(*).  And because you are picking details out of that text, 
> the tests are less precise.  You either have to match more of the command 
> line than you are actually testing for, which isn't a good practice, or you 
> run the risk of finding the text you were looking for in a directory path or 
> other unrelated part of the output.  Also they are harder to debug if you 
> can't reproduce the failure locally, since it isn't easy to add internal 
> checks/output to the test to try hypotheses.  Whenever I have run into 
> failures of this sort the first thing I do is convert the test to an API 
> test...
>
> But the main benefit of the "Shell" tests is that you can write tests without 
> having to know Python or learn the lldb Python API's.  And if you are coming 
> from clang you already know how FileCheck tests work, so that's a bonus.  I 
> think it's legit to require that folks actually working on lldb learn the SB 
> API's.  But we were persuaded that it wasn't fair to impose that on people 
> not working on lldb, and yet such folks do sometimes need to write tests for 
> lldb...  So for simple tests, the Shell tests are an okay option.  But 
> really, there's nothing you can do in a Shell test that you can't do in an 
> API test.
>
> The "API" tests use the Python SB API's - though they also have the ability 
> to run commands and do expect type checks on the output so for single 
> commands they work much as the shell tests do (there's even a FileCheck style 
> assert IIRC).  They are a little more verbose than shell tests (though we've 
> reduced the boilerplate significantly over the years).  And of course you 
> have to know the SB API's.  But for instance, if you wanted to know that a 
> breakpoint was set on line 5 of foo.c, you can set the breakpoint, then ask 
> the resultant SBBreakpoint object what it's file & line numbers were 
> directly.  So once you've gotten familiar with the setup, IMO you can write 
> much higher quality tests with the API tests.
>
>
> Jim
>
> (*) I am personally not at all in favor of the Shell tests, but that's in 
> part because back in the day I was asked to make a simple and useful change 
> to the output of the gdb "break" command but then righting the gdb testsuite 
> - which is all based on expecting the results of various gdb commands - was 
> so tedious that we ended up dropping the change instead.  I don't want to get 
> to that place with lldb, but the hope is that as long as we mostly write API 
> tests, we can avoid encumbering the current command outputs too heavily...


Thanks for the context, I really appreciate it.

The aspect I was especially curious about here was less in regards to
the mechanics of how the behavior is examined/tested (between shell
and SB API) but more in regards to source code versus assembly - where
the assembly can more explicitly target some DWARF feature, but isn't
especially portable - whereas the source code could be portable to
test on different architectures, but might require either more
contortions to reliably produce the desired DWARF, or possibly extra
compiler flags (that was especialyl of interest since Pavel mentioned
these tests could be portable across compilers, so how would I specify
any necessary custom flags to get clang to produce the desired DWARF
(& the tests would be fairly useless for other compilers without those
flags/feature

[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM apart from few nits.




Comment at: lldb/include/lldb/Core/ModuleList.h:485-486
   typedef AdaptedIterable
   ModuleIterableNoLocking;
-  ModuleIterableNoLocking ModulesNoLocking() {
+  ModuleIterableNoLocking ModulesNoLocking() const {
 return ModuleIterableNoLocking(m_modules);

nit: Can we rename that to ModuleIterableNo**n**Locking and 
ModulesNo**n**Locking ?

Sounds more correct to me ^^



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1404
   std::lock_guard guard(module_list.GetMutex());
   const size_t num_modules = module_list.GetSize();
   if (num_modules > 0) {

Same here, we could remove the check and just use `module_list.GetSize()` in 
the printf.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2279
   std::lock_guard guard(target_modules.GetMutex());
   const size_t num_modules = target_modules.GetSize();
   if (num_modules > 0) {

Same.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:3911-3914
   } else {
 result.AppendError("the target has no associated executable images");
 result.SetStatus(eReturnStatusFailed);
 return false;

Early return instead to get rid of `num_modules` ?



Comment at: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:406
+  Target &target = m_process->GetTarget();
+  const size_t num_modules = target.GetImages().GetSize();
 

This doesn't seem to be used anymore.


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

https://reviews.llvm.org/D94271

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

[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 315276.
JDevlieghere marked 5 inline comments as done.
JDevlieghere added a comment.

Address @mib's code review feedback.


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

https://reviews.llvm.org/D94271

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Utility/Iterable.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Core/SearchFilter.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
  lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
  lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3076,13 +3076,8 @@
 }
   }
   // Figure out which one is the executable, and set that in our target:
-  const ModuleList &target_modules = GetTarget().GetImages();
-  std::lock_guard guard(target_modules.GetMutex());
-  size_t num_modules = target_modules.GetSize();
   ModuleSP new_executable_module_sp;
-
-  for (size_t i = 0; i < num_modules; i++) {
-ModuleSP module_sp(target_modules.GetModuleAtIndexUnlocked(i));
+  for (ModuleSP module_sp : GetTarget().GetImages().Modules()) {
 if (module_sp && module_sp->IsExecutable()) {
   if (GetTarget().GetExecutableModulePointer() != module_sp.get())
 new_executable_module_sp = module_sp;
Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -36,13 +36,8 @@
 
   Target &target = process_sp->GetTarget();
 
-  const ModuleList &target_modules = target.GetImages();
-  std::lock_guard guard(target_modules.GetMutex());
-  const size_t num_modules = target_modules.GetSize();
-  for (size_t i = 0; i < num_modules; ++i) {
-Module *module_pointer = target_modules.GetModulePointerAtIndexUnlocked(i);
-
-const Symbol *symbol = module_pointer->FindFirstSymbolWithNameAndType(
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
+const Symbol *symbol = module_sp->FindFirstSymbolWithNameAndType(
 ConstString("__asan_get_alloc_stack"), lldb::eSymbolTypeAny);
 
 if (symbol != nullptr)
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
@@ -452,15 +452,11 @@
   if (process_sp) {
 Target &target = process_sp->GetTarget();
 
-const ModuleList &target_modules = target.GetImages();
-std::lock_guard guard(target_modules.GetMutex());
-size_t num_modules = target_modules.GetSize();
 if (!m_objc_module_sp) {
-  for (size_t i = 0; i < num_modules; i++) {
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
 if (ObjCLanguageRuntime::Get(*process_sp)
-->IsModuleObjCLibrary(
-target_modules.GetModuleAtIndexUnlocked(i))) {
-  m_objc_module_sp = target_modules.GetModuleAtIndexUnlocked(i);
+->IsModuleObjCLibrary(module_sp)) {
+  m_objc_module_sp = module_sp;
   break;
 }
   }
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -1590,10 +1590,7 @@
 
 ObjCLanguageRuntime *objc_runtime = ObjCLanguageRuntime::Get(*process);
 if (objc_runtime) {
-  const ModuleList &images = process->GetTarget().GetImages();
-  std::lock_guard guard(images.GetMutex());
-  for (size_t i = 0; i < images.GetSize(); ++i) {
-lldb::ModuleSP mod_sp = images.GetModuleAtIndexUnlocked(i);
+  for (lldb::ModuleSP mod_sp : process->GetTarget().GetImages().Modules()) {
 if (objc_runtime->IsModuleObjCLibrary(mod_sp)) {
   const Symbol *symbol =
   mod_

[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Core/ModuleList.h:485-486
   typedef AdaptedIterable
   ModuleIterableNoLocking;
-  ModuleIterableNoLocking ModulesNoLocking() {
+  ModuleIterableNoLocking ModulesNoLocking() const {
 return ModuleIterableNoLocking(m_modules);

mib wrote:
> nit: Can we rename that to ModuleIterableNo**n**Locking and 
> ModulesNo**n**Locking ?
> 
> Sounds more correct to me ^^
If you feel strongly about it I can do it as a pre-commit change, but 
personally I don't think it's worth the churn.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1404
   std::lock_guard guard(module_list.GetMutex());
   const size_t num_modules = module_list.GetSize();
   if (num_modules > 0) {

mib wrote:
> Same here, we could remove the check and just use `module_list.GetSize()` in 
> the printf.
There's a few places in this file where that's true, but here we still need it 
for both the early return and the print statement. I'm being pedantic, but 
generally `::size` is only guaranteed to be `O(n)` as opposed to `::empty` 
which is `O(1)`. I think having the temporary variable here is fine. 



Comment at: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:406
+  Target &target = m_process->GetTarget();
+  const size_t num_modules = target.GetImages().GetSize();
 

mib wrote:
> This doesn't seem to be used anymore.
It is below, but I've inlined it. 


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

https://reviews.llvm.org/D94271

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

[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/include/lldb/Core/ModuleList.h:485-486
   typedef AdaptedIterable
   ModuleIterableNoLocking;
-  ModuleIterableNoLocking ModulesNoLocking() {
+  ModuleIterableNoLocking ModulesNoLocking() const {
 return ModuleIterableNoLocking(m_modules);

JDevlieghere wrote:
> mib wrote:
> > nit: Can we rename that to ModuleIterableNo**n**Locking and 
> > ModulesNo**n**Locking ?
> > 
> > Sounds more correct to me ^^
> If you feel strongly about it I can do it as a pre-commit change, but 
> personally I don't think it's worth the churn.
Not really, it can remain as is.


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

https://reviews.llvm.org/D94271

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

[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:1404
   std::lock_guard guard(module_list.GetMutex());
   const size_t num_modules = module_list.GetSize();
   if (num_modules > 0) {

JDevlieghere wrote:
> mib wrote:
> > Same here, we could remove the check and just use `module_list.GetSize()` 
> > in the printf.
> There's a few places in this file where that's true, but here we still need 
> it for both the early return and the print statement. I'm being pedantic, but 
> generally `::size` is only guaranteed to be `O(n)` as opposed to `::empty` 
> which is `O(1)`. I think having the temporary variable here is fine. 
Sounds good then!



Comment at: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:406
+  Target &target = m_process->GetTarget();
+  const size_t num_modules = target.GetImages().GetSize();
 

JDevlieghere wrote:
> mib wrote:
> > This doesn't seem to be used anymore.
> It is below, but I've inlined it. 
Oh! The code was collapsed --' My bad!


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

https://reviews.llvm.org/D94271

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

Re: [Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

[Jim Ingham via lldb-commits]

> On Jan 7, 2021, at 3:57 PM, David Blaikie  wrote:
> 
> On Thu, Jan 7, 2021 at 3:37 PM Jim Ingham via lldb-commits
>  wrote:
>> 
>> 
>> 
>>> On Jan 7, 2021, at 2:29 PM, David Blaikie via Phabricator via lldb-commits 
>>>  wrote:
>>> 
>>> dblaikie added a comment.
>>> 
>>> In D94063#2485271 , @labath wrote:
>>> 
 In D94063#2483546 , @dblaikie 
 wrote:
 
> If it's better to write it using C++ source and custom clang flags I can 
> do that instead (it'll be an -mllvm flag - looks like there's one other 
> test that does that: 
> `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py:
> dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) - 
> means the test will be a bit more convoluted to tickle the subprogram 
> ranges, but not much - just takes two functions+function-sections.
 
 I certainly wouldn't want to drop the existing test.
>>> 
>>> Ah, what's the tradeoff between the different test types here?
>> 
>> This is my take (this has been a contentious issue so I'm sure there are 
>> other takes...):
>> 
>> The "Shell" tests use pattern matching against the lldb command line output. 
>>  They are useful for testing the details of the command interaction. You can 
>> also do that using pexpect in the API tests, but the Python 2.7 version of 
>> pexpect seemed really flakey so we switched to shell tests for this sort of 
>> thing.
>> 
>> Because you are matching against text output that isn't API, they are less 
>> stable.  For instance if we changed anything in the "break set" output, your 
>> test would fail(*).  And because you are picking details out of that text, 
>> the tests are less precise.  You either have to match more of the command 
>> line than you are actually testing for, which isn't a good practice, or you 
>> run the risk of finding the text you were looking for in a directory path or 
>> other unrelated part of the output.  Also they are harder to debug if you 
>> can't reproduce the failure locally, since it isn't easy to add internal 
>> checks/output to the test to try hypotheses.  Whenever I have run into 
>> failures of this sort the first thing I do is convert the test to an API 
>> test...
>> 
>> But the main benefit of the "Shell" tests is that you can write tests 
>> without having to know Python or learn the lldb Python API's.  And if you 
>> are coming from clang you already know how FileCheck tests work, so that's a 
>> bonus.  I think it's legit to require that folks actually working on lldb 
>> learn the SB API's.  But we were persuaded that it wasn't fair to impose 
>> that on people not working on lldb, and yet such folks do sometimes need to 
>> write tests for lldb...  So for simple tests, the Shell tests are an okay 
>> option.  But really, there's nothing you can do in a Shell test that you 
>> can't do in an API test.
>> 
>> The "API" tests use the Python SB API's - though they also have the ability 
>> to run commands and do expect type checks on the output so for single 
>> commands they work much as the shell tests do (there's even a FileCheck 
>> style assert IIRC).  They are a little more verbose than shell tests (though 
>> we've reduced the boilerplate significantly over the years).  And of course 
>> you have to know the SB API's.  But for instance, if you wanted to know that 
>> a breakpoint was set on line 5 of foo.c, you can set the breakpoint, then 
>> ask the resultant SBBreakpoint object what it's file & line numbers were 
>> directly.  So once you've gotten familiar with the setup, IMO you can write 
>> much higher quality tests with the API tests.
>> 
>> 
>> Jim
>> 
>> (*) I am personally not at all in favor of the Shell tests, but that's in 
>> part because back in the day I was asked to make a simple and useful change 
>> to the output of the gdb "break" command but then righting the gdb testsuite 
>> - which is all based on expecting the results of various gdb commands - was 
>> so tedious that we ended up dropping the change instead.  I don't want to 
>> get to that place with lldb, but the hope is that as long as we mostly write 
>> API tests, we can avoid encumbering the current command outputs too 
>> heavily...
> 
> 
> Thanks for the context, I really appreciate it.
> 
> The aspect I was especially curious about here was less in regards to
> the mechanics of how the behavior is examined/tested (between shell
> and SB API) but more in regards to source code versus assembly - where
> the assembly can more explicitly target some DWARF feature, but isn't
> especially portable - whereas the source code could be portable to
> test on different architectures, but might require either more
> contortions to reliably produce the desired DWARF, or possibly extra
> compiler flags (that was especialyl of interest since Pavel mentioned
> these tests could be portable across compilers, so how would 

[Lldb-commits] [PATCH] D70277: [Signal] Allow llvm clients to opt into one-shot SIGPIPE handling

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

Unfortunately I can’t get to this right now, but will take a look first thing 
tomorrow morning.

A shot in the dark: looks like your workflow causes a clang process to be sent 
SIGPIPE more than once. The second time, it exits with ioerr. I expected this 
to be the pre-patch behavior, so I’m confused as to why the reverts appear to 
help.

If you need a quick fix, I’d suggest adding a SIG_IGN for SIGPIPE to the clang 
driver. Those reverts could regress lldb and I’m really not sure why they work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70277

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

[Lldb-commits] [lldb] 696775d - Fix subprogram_ranges.test by explicitly using lld

[David Blaikie via lldb-commits]

Author: David Blaikie
Date: 2021-01-07T19:53:17-08:00
New Revision: 696775d96ecd20aacb7935541995a5554bb32ba8

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

LOG: Fix subprogram_ranges.test by explicitly using lld

Seems consistent with the way other tests in this directory do linking

Added: 


Modified: 
lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Removed: 




diff  --git a/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test 
b/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
index 13186e39b9d1..a6fa5f8ce7a5 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ b/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -1,6 +1,8 @@
 # REQUIRES: x86
-# RUN: %clang -target x86_64-pc-linux -g -O0 %S/Inputs/subprogram_ranges.s -o 
%t.out
-# RUN: %lldb -b -s %s %t.out 2>&1 | FileCheck %s
+# REQUIRES: lld
+# RUN: %clang -target x86_64-pc-linux -g -O0 %S/Inputs/subprogram_ranges.s -o 
%t.o -c
+# RUN: ld.lld %t.o -o %t
+# RUN: %lldb -b -s %s %t 2>&1 | FileCheck %s
 
 # Test breaking on symbols and printing variables when a DW_TAG_subprogram uses
 # DW_AT_ranges instead of DW_AT_low_pc/DW_AT_high_pc.  While the assembly here



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

[Lldb-commits] [lldb] 2ff36e7 - lldb subprogram_ranges.test - remove dependence on temp file name

[David Blaikie via lldb-commits]

Author: David Blaikie
Date: 2021-01-07T20:04:22-08:00
New Revision: 2ff36e79291486b489ae26418daa1b123473b405

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

LOG: lldb subprogram_ranges.test - remove dependence on temp file name

Added: 


Modified: 
lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test

Removed: 




diff  --git a/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test 
b/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
index a6fa5f8ce7a5..740cd35b0f3b 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
+++ b/lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test
@@ -16,4 +16,4 @@
 
 b main
 # CHECK: (lldb) b main
-# CHECK-NEXT: Breakpoint 1: where = subprogram_ranges.test.tmp.out`main + 6 at 
main.c:2:7
+# CHECK-NEXT: Breakpoint 1: where = {{.*}}`main + 6 at main.c:2:7



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

[Lldb-commits] [PATCH] D94284: [lldb] Make DoReadMemory a protected method.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: LLDB.
JDevlieghere requested review of this revision.

DoReadMemory is LLDB's internal implementation and shouldn't be called directly.


https://reviews.llvm.org/D94284

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
  lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp

Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -291,8 +291,8 @@
   jit_descriptor jit_desc;
   const size_t jit_desc_size = sizeof(jit_desc);
   Status error;
-  size_t bytes_read = m_process->DoReadMemory(m_jit_descriptor_addr, &jit_desc,
-  jit_desc_size, error);
+  size_t bytes_read = m_process->ReadMemory(m_jit_descriptor_addr, &jit_desc,
+jit_desc_size, error);
   if (bytes_read != jit_desc_size || !error.Success()) {
 LLDB_LOGF(log, "JITLoaderGDB::%s failed to read JIT descriptor",
   __FUNCTION__);
Index: lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
===
--- lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
+++ lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
@@ -246,7 +246,7 @@
 return std::string();
 
   for (;;) {
-size = m_process->DoReadMemory(addr, &c, 1, error);
+size = m_process->ReadMemory(addr, &c, 1, error);
 if (size != 1 || error.Fail())
   return std::string();
 if (c == 0)
Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -400,7 +400,7 @@
 *read_error = false;
 
   // Read the mach header and see whether it looks like a kernel
-  if (process->DoReadMemory (addr, &header, sizeof(header), error) !=
+  if (process->ReadMemory(addr, &header, sizeof(header), error) !=
   sizeof(header)) {
 if (read_error)
   *read_error = true;
@@ -790,7 +790,7 @@
 
   // For the kernel, we really do need an on-disk file copy of the binary
   // to do anything useful. This will force a call to dsymForUUID if it
-  // exists, instead of depending on the DebugSymbols preferences being 
+  // exists, instead of depending on the DebugSymbols preferences being
   // set.
   if (IsKernel()) {
 if (Symbols::DownloadObjectAndSymbolFile(module_spec, true)) {
Index: lldb/include/lldb/Target/Process.h
===
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -1414,35 +1414,6 @@
   /// this process.
   virtual bool WarnBeforeDetach() const { return true; }
 
-  /// Actually do the reading of memory from a process.
-  ///
-  /// Subclasses must override this function and can return fewer bytes than
-  /// requested when memory requests are too large. This class will break up
-  /// the memory requests and keep advancing the arguments along as needed.
-  ///
-  /// \param[in] vm_addr
-  /// A virtual load address that indicates where to start reading
-  /// memory from.
-  ///
-  /// \param[in] size
-  /// The number of bytes to read.
-  ///
-  /// \param[out] buf
-  /// A byte buffer that is at least \a size bytes long that
-  /// will receive the memory bytes.
-  ///
-  /// \param[out] error
-  /// An error that indicates the success or failure of this
-  /// operation. If error indicates success (error.Success()),
-  /// then the value returned can be trusted, otherwise zero
-  /// will be returned.
-  ///
-  /// \return
-  /// The number of bytes that were actually read into \a buf.
-  /// Zero is returned in the case of an error.
-  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
-  Status &error) = 0;
-
   /// Read of memory from a process.
   ///
   /// This function will read memory from the current process's address space
@@ -2570,6 +2541,35 @@
 bool trap_exceptions = false);
 
 protected:
+  /// Actually do the reading of memory from a process.
+  ///
+  /// Subclasses must override this function and can return fewer bytes than
+  /// requested when memory requests are too large. This class will break up
+  /// the memory requests and keep advancing the arguments along as needed.
+  ///
+  /// \param[in] vm_addr
+  /// A virtual load address t

[Lldb-commits] [PATCH] D94271: [lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf2e05855deb3: [lldb] Access the ModuleList through iterators 
where possible (NFC) (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94271

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Utility/Iterable.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Core/SearchFilter.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
  lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
  lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3076,13 +3076,8 @@
 }
   }
   // Figure out which one is the executable, and set that in our target:
-  const ModuleList &target_modules = GetTarget().GetImages();
-  std::lock_guard guard(target_modules.GetMutex());
-  size_t num_modules = target_modules.GetSize();
   ModuleSP new_executable_module_sp;
-
-  for (size_t i = 0; i < num_modules; i++) {
-ModuleSP module_sp(target_modules.GetModuleAtIndexUnlocked(i));
+  for (ModuleSP module_sp : GetTarget().GetImages().Modules()) {
 if (module_sp && module_sp->IsExecutable()) {
   if (GetTarget().GetExecutableModulePointer() != module_sp.get())
 new_executable_module_sp = module_sp;
Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -36,13 +36,8 @@
 
   Target &target = process_sp->GetTarget();
 
-  const ModuleList &target_modules = target.GetImages();
-  std::lock_guard guard(target_modules.GetMutex());
-  const size_t num_modules = target_modules.GetSize();
-  for (size_t i = 0; i < num_modules; ++i) {
-Module *module_pointer = target_modules.GetModulePointerAtIndexUnlocked(i);
-
-const Symbol *symbol = module_pointer->FindFirstSymbolWithNameAndType(
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
+const Symbol *symbol = module_sp->FindFirstSymbolWithNameAndType(
 ConstString("__asan_get_alloc_stack"), lldb::eSymbolTypeAny);
 
 if (symbol != nullptr)
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
@@ -452,15 +452,11 @@
   if (process_sp) {
 Target &target = process_sp->GetTarget();
 
-const ModuleList &target_modules = target.GetImages();
-std::lock_guard guard(target_modules.GetMutex());
-size_t num_modules = target_modules.GetSize();
 if (!m_objc_module_sp) {
-  for (size_t i = 0; i < num_modules; i++) {
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
 if (ObjCLanguageRuntime::Get(*process_sp)
-->IsModuleObjCLibrary(
-target_modules.GetModuleAtIndexUnlocked(i))) {
-  m_objc_module_sp = target_modules.GetModuleAtIndexUnlocked(i);
+->IsModuleObjCLibrary(module_sp)) {
+  m_objc_module_sp = module_sp;
   break;
 }
   }
Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -1590,10 +1590,7 @@
 
 ObjCLanguageRuntime *objc_runtime = ObjCLanguageRuntime::Get(*process);
 if (objc_runtime) {
-  const ModuleList &images = process->GetTarget().GetImages();
-  std::lock_guard guard(images.GetMutex());
-  for (size_t i = 0; i < images.GetSize(); ++i) {
-lldb::ModuleSP mod_sp = images.GetModuleAtIndexUnlocked(i);
+  for (lldb::ModuleSP mod_sp : process->GetTarget().GetImages().Modules()) {
 

[Lldb-commits] [lldb] f2e0585 - [lldb] Access the ModuleList through iterators where possible (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-01-07T21:06:36-08:00
New Revision: f2e05855deb39125a30a67b63a5e524792805768

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

LOG: [lldb] Access the ModuleList through iterators where possible (NFC)

Replace uses of GetModuleAtIndexUnlocked and
GetModulePointerAtIndexUnlocked with the ModuleIterable and
ModuleIterableNoLocking where applicable.

Differential revision: https://reviews.llvm.org/D94271

Added: 


Modified: 
lldb/include/lldb/Core/ModuleList.h
lldb/include/lldb/Utility/Iterable.h
lldb/source/Breakpoint/Breakpoint.cpp
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Core/ModuleList.cpp
lldb/source/Core/SearchFilter.cpp
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp
lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
lldb/source/Target/Process.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/ModuleList.h 
b/lldb/include/lldb/Core/ModuleList.h
index 1609f0f77c56..46a718f08f04 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -237,20 +237,6 @@ class ModuleList {
   /// \see ModuleList::GetSize()
   Module *GetModulePointerAtIndex(size_t idx) const;
 
-  /// Get the module pointer for the module at index \a idx without acquiring
-  /// the ModuleList mutex.  This MUST already have been acquired with
-  /// ModuleList::GetMutex and locked for this call to be safe.
-  ///
-  /// \param[in] idx
-  /// An index into this module collection.
-  ///
-  /// \return
-  /// A pointer to a Module which can by nullptr if \a idx is out
-  /// of range.
-  ///
-  /// \see ModuleList::GetSize()
-  Module *GetModulePointerAtIndexUnlocked(size_t idx) const;
-
   /// Find compile units by partial or full path.
   ///
   /// Finds all compile units that match \a path in all of the modules and
@@ -491,11 +477,13 @@ class ModuleList {
   typedef LockingAdaptedIterable
   ModuleIterable;
-  ModuleIterable Modules() { return ModuleIterable(m_modules, GetMutex()); }
+  ModuleIterable Modules() const {
+return ModuleIterable(m_modules, GetMutex());
+  }
 
   typedef AdaptedIterable
   ModuleIterableNoLocking;
-  ModuleIterableNoLocking ModulesNoLocking() {
+  ModuleIterableNoLocking ModulesNoLocking() const {
 return ModuleIterableNoLocking(m_modules);
   }
 };

diff  --git a/lldb/include/lldb/Utility/Iterable.h 
b/lldb/include/lldb/Utility/Iterable.h
index 3f9b8b1e4c57..5c38e46feb92 100644
--- a/lldb/include/lldb/Utility/Iterable.h
+++ b/lldb/include/lldb/Utility/Iterable.h
@@ -170,7 +170,7 @@ template 
 class LockingAdaptedIterable : public AdaptedIterable {
 public:
-  LockingAdaptedIterable(C &container, MutexType &mutex)
+  LockingAdaptedIterable(const C &container, MutexType &mutex)
   : AdaptedIterable(container), m_mutex(&mutex) {
 m_mutex->lock();
   }

diff  --git a/lldb/source/Breakpoint/Breakpoint.cpp 
b/lldb/source/Breakpoint/Breakpoint.cpp
index 06d08ecdb3eb..d7bca308ca99 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -508,7 +508,6 @@ void Breakpoint::ModulesChanged(ModuleList &module_list, 
bool load,
 "delete_locations: %i\n",
 module_list.GetSize(), load, delete_locations);
 
-  std::lock_guard guard(module_list.GetMutex());
   if (load) {
 // The logic for handling new modules is:
 // 1) If the filter rejects this module, then skip it. 2) Run through the
@@ -525,7 +524,7 @@ void Breakpoint::ModulesChanged(ModuleList &module_list, 
bool load,
 // them after the locations pass.  Have to do it this way because resolving
 // breakpoints will add new locations potentially.
 
-for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
+for (ModuleSP module_sp : module_list.Modules()) {
   bool seen = false;
   if (!m_filter_sp->ModulePasses(module_sp))
 continue;
@@ -589,9 +588,7 @@ void Breakpoint::ModulesChanged(ModuleList &module_list, 
bool load,
 else
   removed_locations_event = nullptr;
 
-size_t num_modules = module_list.GetSize();
-for (size_t i = 0; i < num_modules; i++) {
-  ModuleSP module_sp(module_list.G

[Lldb-commits] [PATCH] D94284: [lldb] Make DoReadMemory a protected method.

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG57e0cd356287: [lldb] Make DoReadMemory a protected method. 
(authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94284

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
  lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp

Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -291,8 +291,8 @@
   jit_descriptor jit_desc;
   const size_t jit_desc_size = sizeof(jit_desc);
   Status error;
-  size_t bytes_read = m_process->DoReadMemory(m_jit_descriptor_addr, &jit_desc,
-  jit_desc_size, error);
+  size_t bytes_read = m_process->ReadMemory(m_jit_descriptor_addr, &jit_desc,
+jit_desc_size, error);
   if (bytes_read != jit_desc_size || !error.Success()) {
 LLDB_LOGF(log, "JITLoaderGDB::%s failed to read JIT descriptor",
   __FUNCTION__);
Index: lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
===
--- lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
+++ lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
@@ -246,7 +246,7 @@
 return std::string();
 
   for (;;) {
-size = m_process->DoReadMemory(addr, &c, 1, error);
+size = m_process->ReadMemory(addr, &c, 1, error);
 if (size != 1 || error.Fail())
   return std::string();
 if (c == 0)
Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -400,7 +400,7 @@
 *read_error = false;
 
   // Read the mach header and see whether it looks like a kernel
-  if (process->DoReadMemory (addr, &header, sizeof(header), error) !=
+  if (process->ReadMemory(addr, &header, sizeof(header), error) !=
   sizeof(header)) {
 if (read_error)
   *read_error = true;
@@ -790,7 +790,7 @@
 
   // For the kernel, we really do need an on-disk file copy of the binary
   // to do anything useful. This will force a call to dsymForUUID if it
-  // exists, instead of depending on the DebugSymbols preferences being 
+  // exists, instead of depending on the DebugSymbols preferences being
   // set.
   if (IsKernel()) {
 if (Symbols::DownloadObjectAndSymbolFile(module_spec, true)) {
Index: lldb/include/lldb/Target/Process.h
===
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -1414,35 +1414,6 @@
   /// this process.
   virtual bool WarnBeforeDetach() const { return true; }
 
-  /// Actually do the reading of memory from a process.
-  ///
-  /// Subclasses must override this function and can return fewer bytes than
-  /// requested when memory requests are too large. This class will break up
-  /// the memory requests and keep advancing the arguments along as needed.
-  ///
-  /// \param[in] vm_addr
-  /// A virtual load address that indicates where to start reading
-  /// memory from.
-  ///
-  /// \param[in] size
-  /// The number of bytes to read.
-  ///
-  /// \param[out] buf
-  /// A byte buffer that is at least \a size bytes long that
-  /// will receive the memory bytes.
-  ///
-  /// \param[out] error
-  /// An error that indicates the success or failure of this
-  /// operation. If error indicates success (error.Success()),
-  /// then the value returned can be trusted, otherwise zero
-  /// will be returned.
-  ///
-  /// \return
-  /// The number of bytes that were actually read into \a buf.
-  /// Zero is returned in the case of an error.
-  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
-  Status &error) = 0;
-
   /// Read of memory from a process.
   ///
   /// This function will read memory from the current process's address space
@@ -2570,6 +2541,35 @@
 bool trap_exceptions = false);
 
 protected:
+  /// Actually do the reading of memory from a process.
+  ///
+  /// Subclasses must override this function and can return fewer bytes than
+  /// requested when memory requests are too large. This class will break up
+  /// the memory requests and ke

[Lldb-commits] [lldb] 57e0cd3 - [lldb] Make DoReadMemory a protected method.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-01-07T21:06:36-08:00
New Revision: 57e0cd356287321c4847a9e0a9177516dae0cbc1

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

LOG: [lldb] Make DoReadMemory a protected method.

DoReadMemory is LLDB's internal implementation and shouldn't be called
directly.

Differential revision: https://reviews.llvm.org/D94284

Added: 


Modified: 
lldb/include/lldb/Target/Process.h

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/HexagonDYLDRendezvous.cpp
lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index e8dd8847e87a..6f30787f7e5b 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -1414,35 +1414,6 @@ class Process : public 
std::enable_shared_from_this,
   /// this process.
   virtual bool WarnBeforeDetach() const { return true; }
 
-  /// Actually do the reading of memory from a process.
-  ///
-  /// Subclasses must override this function and can return fewer bytes than
-  /// requested when memory requests are too large. This class will break up
-  /// the memory requests and keep advancing the arguments along as needed.
-  ///
-  /// \param[in] vm_addr
-  /// A virtual load address that indicates where to start reading
-  /// memory from.
-  ///
-  /// \param[in] size
-  /// The number of bytes to read.
-  ///
-  /// \param[out] buf
-  /// A byte buffer that is at least \a size bytes long that
-  /// will receive the memory bytes.
-  ///
-  /// \param[out] error
-  /// An error that indicates the success or failure of this
-  /// operation. If error indicates success (error.Success()),
-  /// then the value returned can be trusted, otherwise zero
-  /// will be returned.
-  ///
-  /// \return
-  /// The number of bytes that were actually read into \a buf.
-  /// Zero is returned in the case of an error.
-  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
-  Status &error) = 0;
-
   /// Read of memory from a process.
   ///
   /// This function will read memory from the current process's address space
@@ -2570,6 +2541,35 @@ void PruneThreadPlans();
 bool trap_exceptions = false);
 
 protected:
+  /// Actually do the reading of memory from a process.
+  ///
+  /// Subclasses must override this function and can return fewer bytes than
+  /// requested when memory requests are too large. This class will break up
+  /// the memory requests and keep advancing the arguments along as needed.
+  ///
+  /// \param[in] vm_addr
+  /// A virtual load address that indicates where to start reading
+  /// memory from.
+  ///
+  /// \param[in] size
+  /// The number of bytes to read.
+  ///
+  /// \param[out] buf
+  /// A byte buffer that is at least \a size bytes long that
+  /// will receive the memory bytes.
+  ///
+  /// \param[out] error
+  /// An error that indicates the success or failure of this
+  /// operation. If error indicates success (error.Success()),
+  /// then the value returned can be trusted, otherwise zero
+  /// will be returned.
+  ///
+  /// \return
+  /// The number of bytes that were actually read into \a buf.
+  /// Zero is returned in the case of an error.
+  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+  Status &error) = 0;
+
   void SetState(lldb::EventSP &event_sp);
 
   lldb::StateType GetPrivateState();

diff  --git 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index d0d5a99b28ed..fd1916d296d5 100644
--- 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -400,7 +400,7 @@ DynamicLoaderDarwinKernel::ReadMachHeader(addr_t addr, 
Process *process, llvm::M
 *read_error = false;
 
   // Read the mach header and see whether it looks like a kernel
-  if (process->DoReadMemory (addr, &header, sizeof(header), error) !=
+  if (process->ReadMemory(addr, &header, sizeof(header), error) !=
   sizeof(header)) {
 if (read_error)
   *read_error = true;
@@ -790,7 +790,7 @@ bool 
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
 
   // For the kernel, we really do need an on-disk file copy of the binary
   // to do anything useful. This will force a call to dsymForUUID if it
-  // exists, instead of depending on

[Lldb-commits] [PATCH] D93926: [lldb] Don't remove the lldb.debugger var after exiting the interpreter

[Nathan Lanza via Phabricator via lldb-commits]

lanza added a comment.

What's the boundaries of the stable API, then? This was a public API that was 
removed and broke a plugin I used for vim (and I'll be this isn't the only 
case, just I'm maybe the only one who has worked on lldb before whose tool 
broke). The author used `threading.Thread(someFunc, (debugger,))` to listen on 
a socket for fetch requests from lldb outside of the prompt. Not the most 
beautiful of implementations, but it worked for years on top of a promised 
public stable API.

As far as I know, the only other ways to do this would be to use the 
listener/event forwarding mechanism Greg used to set up the curses based GUI in 
`Debugger::HandleProcessEvent` or to write an entire new frontend analogous to 
the lldb tool itself. The latter implementation is a couple orders of magnitude 
more work to implement for a simple plugin author like this. The former isn't 
exposed in the SBAPI.

Maybe the SBAPI needs access to the `Debugger::m_forward_listener_sp` for GUI 
based usages? Something like:

  class SBForwardEventListener:
  // called for process events
  def HandleProcessEvent(self, event: lldb.SBEvent):
  // called for thread events
  def HandleThreadEvent(self, event: lldb.SBEvent):
  // called for breakpoint events
  def HandleBreakpointEvent(self, event: lldb.SBEvent):

for the developer can invoke

  def __lldb_init_module(...):
  debugger.RegisterForwardEventListener(MyListener())

and be informed for the same whenever the curses GUI would be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93926

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

[Lldb-commits] [PATCH] D93926: [lldb] Don't remove the lldb.debugger var after exiting the interpreter

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D93926#2486013 , @lanza wrote:

> What's the boundaries of the stable API, then? This was a public API that was 
> removed and broke a plugin I used for vim. The author used 
> `threading.Thread(someFunc, (debugger,))` to listen on a socket for fetch 
> requests from lldb outside of the prompt. Not the most beautiful of 
> implementations, but it worked for years on top of a promised public stable 
> API.

The debugger variable is still present in the lldb namespace, it's `None` 
instead of a default constructed debugger, so I would argue that this is not an 
API breaking change. It has always been documented that those variables are 
only valid in the interactive script interpreter 
(https://lldb.llvm.org/use/python-reference.html#embedded-python-interpreter) 
so it's also not really a policy change. I totally understand how annoying it 
is that this breaks things (I had to fix incorrect uses myself, such as in the 
crashlog/symbolication script) but I think it's worth it because it prevents 
accidental misuse.

> As far as I know, the only other ways to do this would be to use the 
> listener/event forwarding mechanism Greg used to set up the curses based GUI 
> in `Debugger::HandleProcessEvent` or to write an entire new frontend 
> analogous to the lldb tool itself. The latter implementation is a couple 
> orders of magnitude more work to implement for a simple plugin author like 
> this. The former isn't exposed in the SBAPI.

I'm not sure I understand how this is a problem for the event listener. Why 
can't you save the debugger in `__lldb_init_module` and start the event 
listening thread from there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93926

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

[Lldb-commits] [PATCH] D93926: [lldb] Don't remove the lldb.debugger var after exiting the interpreter

[Nathan Lanza via Phabricator via lldb-commits]

lanza added a comment.

I guess if the intention of that is maintain the debugger instance around it 
should work, but at the moment it segfaults pretty quick with Xcode's lldb 
using:

  import threading
  import lldb
  import time
  
  def background(debugger: lldb.SBDebugger):
  while True:
  time.sleep(1)
  print(debugger)
  
  def __lldb_init_module(
debugger: lldb.SBDebugger,
internal_dict: dict
  ):
  threading.Thread()
  threading.Thread(target=background, args=(debugger,)).start()

and

  $ lldb a.out
  (lldb) command script import test.py
  (lldb) // do a few things
  0  lldb0x0001058d8575 
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
  1  lldb0x0001058d7b55 
llvm::sys::RunSignalHandlers() + 85
  2  lldb0x0001058d8dd6 
SignalHandler(int) + 262
  3  libsystem_platform.dylib0x7fff20394d7d _sigtramp + 
29
  4  libsystem_platform.dylib0x00010762d680 _sigtramp + 
18446603344394422560
  5  liblldbPluginScriptInterpreterPython3.dylib 0x0001059f1802 
_wrap_SBDebugger___str__(_object*, _object*) + 130
  6  Python3 0x000105bf6453 
cfunction_call_varargs + 323
  7  Python3 0x000105bf5dd6 
_PyObject_MakeTpCall + 374
  8  Python3 0x000105cd613c 
call_function + 652
  9  Python3 0x000105cd26d6 
_PyEval_EvalFrameDefault + 29782
  10 Python3 0x000105bf678d 
function_code_fastcall + 237
  11 Python3 0x000105bf7192 
_PyObject_FastCall_Prepend + 178
  12 Python3 0x000105c4e527 slot_tp_str 
+ 183
  13 Python3 0x000105c37a62 
PyObject_Str + 146
  14 Python3 0x000105c09b95 
PyFile_WriteObject + 149
  15 Python3 0x000105cc96f2 
builtin_print + 450
  16 Python3 0x000105c34a82 
cfunction_vectorcall_FASTCALL_KEYWORDS + 130
  17 Python3 0x000105cd6012 
call_function + 354
  18 Python3 0x000105cd278a 
_PyEval_EvalFrameDefault + 29962
  19 Python3 0x000105bf678d 
function_code_fastcall + 237
  20 Python3 0x000105bf6124 
PyVectorcall_Call + 100
  21 Python3 0x000105cd2a34 
_PyEval_EvalFrameDefault + 30644
  22 Python3 0x000105bf678d 
function_code_fastcall + 237
  23 Python3 0x000105cd6012 
call_function + 354
  24 Python3 0x000105cd26b9 
_PyEval_EvalFrameDefault + 29753
  25 Python3 0x000105bf678d 
function_code_fastcall + 237
  26 Python3 0x000105cd6012 
call_function + 354
  27 Python3 0x000105cd26b9 
_PyEval_EvalFrameDefault + 29753
  28 Python3 0x000105bf678d 
function_code_fastcall + 237
  29 Python3 0x000105bf91e2 
method_vectorcall + 322
  30 Python3 0x000105bf6124 
PyVectorcall_Call + 100
  31 Python3 0x000105d7800a t_bootstrap 
+ 74
  32 Python3 0x000105d29829 
pythread_wrapper + 25
  33 libsystem_pthread.dylib 0x7fff20350950 
_pthread_start + 224
  34 libsystem_pthread.dylib 0x7fff2034c47b 
thread_start + 15
  fish: 'lldb' terminated by signal SIGSEGV (Address boundary error)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93926

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

[Lldb-commits] [PATCH] D93926: [lldb] Don't remove the lldb.debugger var after exiting the interpreter

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D93926#2486038 , @lanza wrote:

> I guess if the intention of that is maintain the debugger instance around it 
> should work, but at the moment it segfaults pretty quick with Xcode's lldb 
> using:

Hmm, the debugger somehow gets borked when it's passed into the `background` 
method. If I call `GetID()` on it, it returns a totally bogus value. I need to 
read up on how these objects are passed to new threads. The following script 
works as expected:

  import threading
  import lldb
  import time
  
  def background(debugger_id: int):
  debugger = lldb.SBDebugger.FindDebuggerWithID(debugger_id)
  while True:
  time.sleep(1)
  print(debugger)
  
  def __lldb_init_module(
debugger: lldb.SBDebugger,
internal_dict: dict
  ):
  threading.Thread()
  threading.Thread(target=background, args=(debugger.GetID(),)).start()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93926

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

[Lldb-commits] [PATCH] D93926: [lldb] Don't remove the lldb.debugger var after exiting the interpreter

[Nathan Lanza via Phabricator via lldb-commits]

lanza abandoned this revision.
lanza added a comment.

Great, that does indeed seem to work properly. Thanks, Jonas!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93926

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