[Lldb-commits] [PATCH] D106348: Write the # of addressable bits into Mach-O corefiles, read it back out again

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 360717.
jasonmolenda added a comment.

Update patch to address Jonas' suggestion of not duplicating the code to write 
the LC_NOTE load command & payloads (I was annoyed by this too but didn't deal 
with it in my first pass).  Also add a test, although it only is really tested 
on an arm64e testsuite run where the compiler is doing PAC auth on return 
addresses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106348

Files:
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/test/API/macosx/lc-note/addrable-bits/Makefile
  lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
  lldb/test/API/macosx/lc-note/addrable-bits/main.c

Index: lldb/test/API/macosx/lc-note/addrable-bits/main.c
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/main.c
@@ -0,0 +1,12 @@
+int pat (int in) { 
+  return in + 5; // break here 
+}
+
+int tat (int in) { return pat(in + 10); }
+
+int mat (int in) { return tat(in + 15); }
+
+int main() {
+ int (*matp)(int) = mat;
+ return matp(10);
+}
Index: lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
@@ -0,0 +1,59 @@
+"""Test that corefiles with LC_NOTE "addrable bits" load command, creating and reading."""
+
+
+
+import os
+import re
+import subprocess
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestAddrableBitsCorefile(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def initial_setup(self):
+self.build()
+self.exe = self.getBuildArtifact("a.out")
+self.corefile = self.getBuildArtifact("corefile")
+
+@skipIf(archs=no_match(['arm64e']))
+@skipUnlessDarwin
+def test_lc_note_addrable_bits(self):
+self.initial_setup()
+
+self.target = self.dbg.CreateTarget(self.exe)
+err = lldb.SBError()
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self, "break here",
+  lldb.SBFileSpec('main.c'))
+self.assertEqual(process.IsValid(), True)
+
+found_main = False
+for f in thread.frames:
+  if f.GetFunctionName() == "main":
+found_main = True
+self.assertTrue(found_main)
+
+cmdinterp = self.dbg.GetCommandInterpreter()
+res = lldb.SBCommandReturnObject()
+cmdinterp.HandleCommand("process save-core %s" % self.corefile, res)
+self.assertTrue(res.Succeeded(), True)
+process.Kill()
+self.dbg.DeleteTarget(target)
+
+target = self.dbg.CreateTarget('')
+process = target.LoadCore(self.corefile)
+self.assertTrue(process.IsValid(), True)
+thread = process.GetSelectedThread()
+
+found_main = False
+for f in thread.frames:
+  if f.GetFunctionName() == "main":
+found_main = True
+self.assertTrue(found_main)
+
Index: lldb/test/API/macosx/lc-note/addrable-bits/Makefile
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -541,6 +541,11 @@
   if (arch.IsValid())
 GetTarget().SetArchitecture(arch);
 
+  addr_t address_mask = core_objfile->GetAddressMask();
+  if (address_mask != 0) {
+SetCodeAddressMask(address_mask);
+SetDataAddressMask(address_mask);
+  }
   return error;
 }
 
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -15,6 +15,7 @@
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/RangeMap.h"
+#include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UUID.h"
 
 // This class needs to be hidden as eventually belongs in a plugin that
@@ -113,6 +114,8 @@
 
   std::string GetIdentifierString() override;
 
+  lldb::addr_t GetAddressMask() override;
+
   bool GetCorefileMainBinaryInfo(lldb::addr_t &address,

[Lldb-commits] [PATCH] D106348: Write the # of addressable bits into Mach-O corefiles, read it back out again

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 360722.
jasonmolenda added a comment.

One more bit I meant to do -- added output to "process status --verbose" to 
show the addressing masks in effect.  There's no other way to check what lldb 
is currently using for ptrauth masks currently, to debug/confirm what lldb is 
using.

There's the target.process.virtual-addressable-bits setting, but this is 
separate from what lldb might pick up dynamically (from lldb-server, 
debugserver, a core file) so querying that setting will show 0 if lldb is using 
the dynamically determined mask.

I couldn't think of any place better to put this, this will do for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106348

Files:
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/test/API/macosx/lc-note/addrable-bits/Makefile
  lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
  lldb/test/API/macosx/lc-note/addrable-bits/main.c

Index: lldb/test/API/macosx/lc-note/addrable-bits/main.c
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/main.c
@@ -0,0 +1,12 @@
+int pat (int in) { 
+  return in + 5; // break here 
+}
+
+int tat (int in) { return pat(in + 10); }
+
+int mat (int in) { return tat(in + 15); }
+
+int main() {
+ int (*matp)(int) = mat;
+ return matp(10);
+}
Index: lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
@@ -0,0 +1,59 @@
+"""Test that corefiles with LC_NOTE "addrable bits" load command, creating and reading."""
+
+
+
+import os
+import re
+import subprocess
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestAddrableBitsCorefile(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def initial_setup(self):
+self.build()
+self.exe = self.getBuildArtifact("a.out")
+self.corefile = self.getBuildArtifact("corefile")
+
+@skipIf(archs=no_match(['arm64e']))
+@skipUnlessDarwin
+def test_lc_note_addrable_bits(self):
+self.initial_setup()
+
+self.target = self.dbg.CreateTarget(self.exe)
+err = lldb.SBError()
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self, "break here",
+  lldb.SBFileSpec('main.c'))
+self.assertEqual(process.IsValid(), True)
+
+found_main = False
+for f in thread.frames:
+  if f.GetFunctionName() == "main":
+found_main = True
+self.assertTrue(found_main)
+
+cmdinterp = self.dbg.GetCommandInterpreter()
+res = lldb.SBCommandReturnObject()
+cmdinterp.HandleCommand("process save-core %s" % self.corefile, res)
+self.assertTrue(res.Succeeded(), True)
+process.Kill()
+self.dbg.DeleteTarget(target)
+
+target = self.dbg.CreateTarget('')
+process = target.LoadCore(self.corefile)
+self.assertTrue(process.IsValid(), True)
+thread = process.GetSelectedThread()
+
+found_main = False
+for f in thread.frames:
+  if f.GetFunctionName() == "main":
+found_main = True
+self.assertTrue(found_main)
+
Index: lldb/test/API/macosx/lc-note/addrable-bits/Makefile
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -541,6 +541,11 @@
   if (arch.IsValid())
 GetTarget().SetArchitecture(arch);
 
+  addr_t address_mask = core_objfile->GetAddressMask();
+  if (address_mask != 0) {
+SetCodeAddressMask(address_mask);
+SetDataAddressMask(address_mask);
+  }
   return error;
 }
 
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -15,6 +15,7 @@
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/RangeMap.h"
+#include "lldb/Utility

[Lldb-commits] [lldb] cdc6f8d - Read and write a LC_NOTE "addrable bits" for addressing mask

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2021-07-22T01:06:44-07:00
New Revision: cdc6f8d728208d2f06c1c632a41d930e172b4fb5

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

LOG: Read and write a LC_NOTE "addrable bits" for addressing mask

This patch adds code to process save-core for Mach-O files which
embeds an "addrable bits" LC_NOTE when the process is using a
code address mask (e.g. AArch64 v8.3 with ptrauth aka arm64e).
Add code to ObjectFileMachO to read that LC_NOTE from corefiles,
and ProcessMachCore to set the process masks based on it when reading
a corefile back in.

Also have "process status --verbose" print the current address masks
that lldb is using internally to strip ptrauth bits off of addresses.

Differential Revision: https://reviews.llvm.org/D106348
rdar://68630113

Added: 
lldb/test/API/macosx/lc-note/addrable-bits/Makefile
lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
lldb/test/API/macosx/lc-note/addrable-bits/main.c

Modified: 
lldb/include/lldb/Symbol/ObjectFile.h
lldb/include/lldb/Target/Process.h
lldb/source/Commands/CommandObjectProcess.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

Removed: 




diff  --git a/lldb/include/lldb/Symbol/ObjectFile.h 
b/lldb/include/lldb/Symbol/ObjectFile.h
index bf1417d4dc190..24bb3b106eb34 100644
--- a/lldb/include/lldb/Symbol/ObjectFile.h
+++ b/lldb/include/lldb/Symbol/ObjectFile.h
@@ -495,6 +495,16 @@ class ObjectFile : public 
std::enable_shared_from_this,
   return std::string();
   }
 
+  /// Some object files may have the number of bits used for addressing
+  /// embedded in them, e.g. a Mach-O core file using an LC_NOTE.  These
+  /// object files can return the address mask that should be used in
+  /// the Process.
+  /// \return
+  /// The mask will have bits set which aren't used for addressing --
+  /// typically, the high bits.
+  /// Zero is returned when no address bits mask is available.
+  virtual lldb::addr_t GetAddressMask() { return 0; }
+
   /// When the ObjectFile is a core file, lldb needs to locate the "binary" in
   /// the core file.  lldb can iterate over the pages looking for a valid
   /// binary, but some core files may have metadata  describing where the main

diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index 2457ac31c19db..1f671a17a04b4 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -2899,7 +2899,8 @@ void PruneThreadPlans();
   std::atomic m_finalizing;
 
   /// Mask for code an data addresses. The default value (0) means no mask is
-  /// set.
+  /// set.  The bits set to 1 indicate bits that are NOT significant for
+  /// addressing.
   /// @{
   lldb::addr_t m_code_address_mask = 0;
   lldb::addr_t m_data_address_mask = 0;

diff  --git a/lldb/source/Commands/CommandObjectProcess.cpp 
b/lldb/source/Commands/CommandObjectProcess.cpp
index e4f67c04ec256..00fb4d669f499 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -29,6 +29,8 @@
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/State.h"
 
+#include 
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -1341,6 +1343,18 @@ class CommandObjectProcessStatus : public 
CommandObjectParsed {
  num_frames, num_frames_with_source, stop_format);
 
 if (m_options.m_verbose) {
+  addr_t code_mask = process->GetCodeAddressMask();
+  addr_t data_mask = process->GetDataAddressMask();
+  if (code_mask != 0) {
+int bits = std::bitset<64>(~code_mask).count();
+result.AppendMessageWithFormat(
+"Addressable code address mask: 0x%" PRIx64 "\n", code_mask);
+result.AppendMessageWithFormat(
+"Addressable data address mask: 0x%" PRIx64 "\n", data_mask);
+result.AppendMessageWithFormat(
+"Number of bits used in addressing (code): %d\n", bits);
+  }
+
   PlatformSP platform_sp = process->GetTarget().GetPlatform();
   if (!platform_sp) {
 result.AppendError("Couldn'retrieve the target's platform");

diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index e7652cffb1c81..a14c1e3a1ff8c 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -62,6 +62,7 @@
 #include 
 #endif
 
+#include 
 #include 
 
 #if LLVM_SUPPORT_XCODE_SIGNPOSTS
@@ -5571,6 +5572,46 @@ std::string ObjectFileMachO::GetIdentifierString() {
   return result;

[Lldb-commits] [PATCH] D106348: Write the # of addressable bits into Mach-O corefiles, read it back out again

[Jason Molenda via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcdc6f8d72820: Read and write a LC_NOTE "addrable 
bits" for addressing mask (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106348

Files:
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/test/API/macosx/lc-note/addrable-bits/Makefile
  lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
  lldb/test/API/macosx/lc-note/addrable-bits/main.c

Index: lldb/test/API/macosx/lc-note/addrable-bits/main.c
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/main.c
@@ -0,0 +1,12 @@
+int pat (int in) { 
+  return in + 5; // break here 
+}
+
+int tat (int in) { return pat(in + 10); }
+
+int mat (int in) { return tat(in + 15); }
+
+int main() {
+ int (*matp)(int) = mat;
+ return matp(10);
+}
Index: lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/TestAddrableBitsCorefile.py
@@ -0,0 +1,59 @@
+"""Test that corefiles with LC_NOTE "addrable bits" load command, creating and reading."""
+
+
+
+import os
+import re
+import subprocess
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestAddrableBitsCorefile(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def initial_setup(self):
+self.build()
+self.exe = self.getBuildArtifact("a.out")
+self.corefile = self.getBuildArtifact("corefile")
+
+@skipIf(archs=no_match(['arm64e']))
+@skipUnlessDarwin
+def test_lc_note_addrable_bits(self):
+self.initial_setup()
+
+self.target = self.dbg.CreateTarget(self.exe)
+err = lldb.SBError()
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self, "break here",
+  lldb.SBFileSpec('main.c'))
+self.assertEqual(process.IsValid(), True)
+
+found_main = False
+for f in thread.frames:
+  if f.GetFunctionName() == "main":
+found_main = True
+self.assertTrue(found_main)
+
+cmdinterp = self.dbg.GetCommandInterpreter()
+res = lldb.SBCommandReturnObject()
+cmdinterp.HandleCommand("process save-core %s" % self.corefile, res)
+self.assertTrue(res.Succeeded(), True)
+process.Kill()
+self.dbg.DeleteTarget(target)
+
+target = self.dbg.CreateTarget('')
+process = target.LoadCore(self.corefile)
+self.assertTrue(process.IsValid(), True)
+thread = process.GetSelectedThread()
+
+found_main = False
+for f in thread.frames:
+  if f.GetFunctionName() == "main":
+found_main = True
+self.assertTrue(found_main)
+
Index: lldb/test/API/macosx/lc-note/addrable-bits/Makefile
===
--- /dev/null
+++ lldb/test/API/macosx/lc-note/addrable-bits/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -541,6 +541,11 @@
   if (arch.IsValid())
 GetTarget().SetArchitecture(arch);
 
+  addr_t address_mask = core_objfile->GetAddressMask();
+  if (address_mask != 0) {
+SetCodeAddressMask(address_mask);
+SetDataAddressMask(address_mask);
+  }
   return error;
 }
 
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -15,6 +15,7 @@
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/RangeMap.h"
+#include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UUID.h"
 
 // This class needs to be hidden as eventually belongs in a plugin that
@@ -113,6 +114,8 @@
 
   std::string GetIdentifierString() override;
 
+  lldb::addr_t GetAddressMask() override;
+
   bool GetCorefileMainBinaryInfo(lldb::addr_t &address,

[Lldb-commits] [PATCH] D106266: [C++4OpenCL] Add run line standard aliases clc++1.0 and CLC++1.0

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Quick note to prevent some confusion: I saw this patch and realized that the 
LLDB change was only necessary because some Clang code got copy pasted into 
LLDB. I removed that copy in https://reviews.llvm.org/D106537 so if you see 
merge conflicts while merging this, you can just drop the LLDB changes as they 
shouldn't be necessary once my patch landed. Thanks!


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

https://reviews.llvm.org/D106266

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

[Lldb-commits] [lldb] 67c588c - [lldb] Generalize empty record size computation to avoid giving empty C++ structs a size of 0

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-07-22T13:30:48+02:00
New Revision: 67c588c481bb2fb9f72459fbc205c3e1ee8c4b3b

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

LOG: [lldb] Generalize empty record size computation to avoid giving empty C++ 
structs a size of 0

C doesn't allow empty structs but Clang/GCC support them and give them a size 
of 0.

LLDB implements this by checking the tag kind and if it's 
`DW_TAG_structure_type` then
we give it a size of 0 via an empty external RecordLayout. This is done because 
our
internal TypeSystem is always in C++ mode (which means we would give them a size
of 1).

The current check for when we have this special case is currently too lax as 
types with
`DW_TAG_structure_type` can also occur in C++ with types defined using the 
`struct`
keyword. This means that in a C++ program with `struct Empty{};`, LLDB would 
return
`0` for `sizeof(Empty)` even though the correct size is 1.

This patch removes this special case and replaces it with a generic approach 
that just
assigns empty structs the byte_size as specified in DWARF. The GCC/Clang special
case is handles as they both emit an explicit `DW_AT_byte_size` of 0. And if 
another
compiler decides to use a different byte size for this case then this should 
also be
handled by the same code as long as that information is provided via 
`DW_AT_byte_size`.

Reviewed By: werat, shafik

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

Added: 
lldb/test/API/lang/c/sizeof/Makefile
lldb/test/API/lang/c/sizeof/TestCSizeof.py
lldb/test/API/lang/c/sizeof/main.c
lldb/test/API/lang/cpp/sizeof/Makefile
lldb/test/API/lang/cpp/sizeof/TestCPPSizeof.py
lldb/test/API/lang/cpp/sizeof/main.cpp

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 6bbcf46ef34d6..f10fbceaea540 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1685,14 +1685,18 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext &sc,
 die.GetOffset(), attrs.name.GetCString());
   }
 
-  if (tag == DW_TAG_structure_type) // this only applies in C
-  {
+  // If the byte size of the record is specified then overwrite the size
+  // that would be computed by Clang. This is only needed as LLDB's
+  // TypeSystemClang is always in C++ mode, but some compilers such as
+  // GCC and Clang give empty structs a size of 0 in C mode (in contrast to
+  // the size of 1 for empty structs that would be computed in C++ mode).
+  if (attrs.byte_size) {
 clang::RecordDecl *record_decl =
 TypeSystemClang::GetAsRecordDecl(clang_type);
-
 if (record_decl) {
-  GetClangASTImporter().SetRecordLayout(
-  record_decl, ClangASTImporter::LayoutInfo());
+  ClangASTImporter::LayoutInfo layout;
+  layout.bit_size = *attrs.byte_size * 8;
+  GetClangASTImporter().SetRecordLayout(record_decl, layout);
 }
   }
 } else if (clang_type_was_created) {

diff  --git a/lldb/test/API/lang/c/sizeof/Makefile 
b/lldb/test/API/lang/c/sizeof/Makefile
new file mode 100644
index 0..10495940055b6
--- /dev/null
+++ b/lldb/test/API/lang/c/sizeof/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules

diff  --git a/lldb/test/API/lang/c/sizeof/TestCSizeof.py 
b/lldb/test/API/lang/c/sizeof/TestCSizeof.py
new file mode 100644
index 0..5bcbc42e3dfcf
--- /dev/null
+++ b/lldb/test/API/lang/c/sizeof/TestCSizeof.py
@@ -0,0 +1,18 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+self.build()
+self.createTestTarget()
+
+# Empty structs are not allowed in C, but Clang/GCC allow them and
+# give them a size of 0.
+self.expect_expr("sizeof(Empty) == sizeof_empty", result_value="true")
+self.expect_expr("sizeof(SingleMember) == sizeof_single", 
result_value="true")
+self.expect_expr("sizeof(PaddingMember) == sizeof_padding", 
result_value="true")

diff  --git a/lldb/test/API/lang/c/sizeof/main.c 
b/lldb/test/API/lang/c/sizeof/main.c
new file mode 100644
index 0..08bf906edb4af
--- /dev/null
+++ b/lldb/test/API/lang/c/sizeof/main.c
@@ -0,0 +1,21 @@
+struct Empty {};
+struct SingleMember {
+  int i;
+};
+
+struct PaddingMember {
+  int i;
+  char c;
+};
+
+const unsigned sizeof_empty = siz

[Lldb-commits] [PATCH] D105471: [lldb] Generalize empty record size computation to avoid giving empty C++ structs a size of 0

[Raphael Isemann 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 rG67c588c481bb: [lldb] Generalize empty record size 
computation to avoid giving empty C++… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105471

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/c/sizeof/Makefile
  lldb/test/API/lang/c/sizeof/TestCSizeof.py
  lldb/test/API/lang/c/sizeof/main.c
  lldb/test/API/lang/cpp/sizeof/Makefile
  lldb/test/API/lang/cpp/sizeof/TestCPPSizeof.py
  lldb/test/API/lang/cpp/sizeof/main.cpp

Index: lldb/test/API/lang/cpp/sizeof/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/sizeof/main.cpp
@@ -0,0 +1,37 @@
+struct Empty {};
+class EmptyClass {};
+
+struct SingleMember {
+  int i;
+};
+class SingleMemberClass {
+  int i;
+};
+
+struct PaddingMember {
+  int i;
+  char c;
+};
+class PaddingMemberClass {
+  int i;
+  char c;
+};
+
+const unsigned sizeof_empty = sizeof(Empty);
+const unsigned sizeof_empty_class = sizeof(EmptyClass);
+const unsigned sizeof_single = sizeof(SingleMember);
+const unsigned sizeof_single_class = sizeof(SingleMemberClass);
+const unsigned sizeof_padding = sizeof(PaddingMember);
+const unsigned sizeof_padding_class = sizeof(PaddingMemberClass);
+
+int main() {
+  Empty empty;
+  EmptyClass empty_class;
+  SingleMember single;
+  SingleMemberClass single_class;
+  PaddingMember padding;
+  PaddingMemberClass padding_class;
+  // Make sure globals are used.
+  return sizeof_empty + sizeof_empty_class + sizeof_single +
+sizeof_single_class + sizeof_padding + sizeof_padding_class;
+}
Index: lldb/test/API/lang/cpp/sizeof/TestCPPSizeof.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/sizeof/TestCPPSizeof.py
@@ -0,0 +1,20 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+self.build()
+self.createTestTarget()
+
+# Empty structs/classes have size 1 in C++.
+self.expect_expr("sizeof(Empty) == sizeof_empty", result_value="true")
+self.expect_expr("sizeof(EmptyClass) == sizeof_empty_class", result_value="true")
+self.expect_expr("sizeof(SingleMember) == sizeof_single", result_value="true")
+self.expect_expr("sizeof(SingleMemberClass) == sizeof_single_class", result_value="true")
+self.expect_expr("sizeof(PaddingMember) == sizeof_padding", result_value="true")
+self.expect_expr("sizeof(PaddingMemberClass) == sizeof_padding_class", result_value="true")
Index: lldb/test/API/lang/cpp/sizeof/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/sizeof/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/test/API/lang/c/sizeof/main.c
===
--- /dev/null
+++ lldb/test/API/lang/c/sizeof/main.c
@@ -0,0 +1,21 @@
+struct Empty {};
+struct SingleMember {
+  int i;
+};
+
+struct PaddingMember {
+  int i;
+  char c;
+};
+
+const unsigned sizeof_empty = sizeof(struct Empty);
+const unsigned sizeof_single = sizeof(struct SingleMember);
+const unsigned sizeof_padding = sizeof(struct PaddingMember);
+
+int main() {
+  struct Empty empty;
+  struct SingleMember single;
+  struct PaddingMember padding;
+  // Make sure globals are used.
+  return sizeof_empty + sizeof_single + sizeof_padding;
+}
Index: lldb/test/API/lang/c/sizeof/TestCSizeof.py
===
--- /dev/null
+++ lldb/test/API/lang/c/sizeof/TestCSizeof.py
@@ -0,0 +1,18 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+self.build()
+self.createTestTarget()
+
+# Empty structs are not allowed in C, but Clang/GCC allow them and
+# give them a size of 0.
+self.expect_expr("sizeof(Empty) == sizeof_empty", result_value="true")
+self.expect_expr("sizeof(SingleMember) == sizeof_single", result_value="true")
+self.expect_expr("sizeof(PaddingMember) == sizeof_padding", result_value="true")
Index: lldb/test/API/lang/c/sizeof/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/c/sizeof/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFAS

[Lldb-commits] [PATCH] D106266: [C++4OpenCL] Add run line standard aliases clc++1.0 and CLC++1.0

[Justas Janickas via Phabricator via lldb-commits]

Topotuna added a comment.

That is good to know. Thank you for sorting it out


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

https://reviews.llvm.org/D106266

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

[Lldb-commits] [lldb] 12a89e1 - [lldb][NFCI] Remove redundant accessibility heuristic in the DWARF parser

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-07-22T13:36:23+02:00
New Revision: 12a89e14b83ac3db9e44f535a43bb11e7b6c3601

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

LOG: [lldb][NFCI] Remove redundant accessibility heuristic in the DWARF parser

LLDB's DWARF parser has some heuristics for guessing and fixing up the
accessibility of C++ class/struct members after they were already created in the
internal Clang AST. The heuristic is that if a struct/class has a base class,
then it's actually a class and it's members are private unless otherwise
specified.

>From what I can see this heuristic isn't sound and also unnecessary. The idea
that inheritance implies that the `class` keyword was used and the default
visibility is `private` is incorrect. Also both GCC and Clang use
`DW_TAG_structure_type` and `DW_TAG_class_type` for `struct` and `class` types
respectively, so the default visibility we infer from that information is always
correct and there is no need to fix it up.

And finally, the access specifiers we set in the Clang AST are anyway unused
within LLDB. The expression parser explicitly ignores them to give users access
to private members and there is not SBAPI functionality that exposes this
information.

This patch removes all the heuristic code for the reasons above and instead
just relies on the access values we infer from the tag kind and explicit
annotations in DWARF.

This patch is NFCI.

Reviewed By: werat

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index f10fbceaea540..4cf4ea6ed0f4c 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1983,15 +1983,12 @@ bool DWARFASTParserClang::CompleteRecordType(const 
DWARFDIE &die,
 }
 
 std::vector> bases;
-std::vector member_accessibilities;
-bool is_a_class = false;
 // Parse members and base classes first
 std::vector member_function_dies;
 
 DelayedPropertyList delayed_properties;
-ParseChildMembers(die, clang_type, bases, member_accessibilities,
-  member_function_dies, delayed_properties,
-  default_accessibility, is_a_class, layout_info);
+ParseChildMembers(die, clang_type, bases, member_function_dies,
+  delayed_properties, default_accessibility, layout_info);
 
 // Now parse any methods if there were any...
 for (const DWARFDIE &die : member_function_dies)
@@ -2012,31 +2009,6 @@ bool DWARFASTParserClang::CompleteRecordType(const 
DWARFDIE &die,
   }
 }
 
-// If we have a DW_TAG_structure_type instead of a DW_TAG_class_type we
-// need to tell the clang type it is actually a class.
-if (!type_is_objc_object_or_interface) {
-  if (is_a_class && tag_decl_kind != clang::TTK_Class)
-m_ast.SetTagTypeKind(ClangUtil::GetQualType(clang_type),
- clang::TTK_Class);
-}
-
-// Since DW_TAG_structure_type gets used for both classes and
-// structures, we may need to set any DW_TAG_member fields to have a
-// "private" access if none was specified. When we parsed the child
-// members we tracked that actual accessibility value for each
-// DW_TAG_member in the "member_accessibilities" array. If the value
-// for the member is zero, then it was set to the
-// "default_accessibility" which for structs was "public". Below we
-// correct this by setting any fields to "private" that weren't
-// correctly set.
-if (is_a_class && !member_accessibilities.empty()) {
-  // This is a class and all members that didn't have their access
-  // specified are private.
-  m_ast.SetDefaultAccessForRecordFields(
-  m_ast.GetAsRecordDecl(clang_type), eAccessPrivate,
-  &member_accessibilities.front(), member_accessibilities.size());
-}
-
 if (!bases.empty()) {
   // Make sure all base classes refer to complete types and not forward
   // declarations. If we don't do this, clang will crash with an
@@ -2349,7 +2321,6 @@ Function 
*DWARFASTParserClang::ParseFunctionFromDWARF(CompileUnit &comp_unit,
 void DWARFASTParserClang::ParseSingleMember(
 const DWARFDIE &die, const DWARFDIE &parent_die,
 const lldb_private::CompilerType &class_clang_type,
-std::vector &member_accessibilities,

[Lldb-commits] [PATCH] D105463: [lldb][NFC] Remove redundant accessibility heuristic in the DWARF parser

[Raphael Isemann 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 rG12a89e14b83a: [lldb][NFCI] Remove redundant accessibility 
heuristic in the DWARF parser (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105463

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -380,13 +380,6 @@
bool isForwardDecl, bool isInternal,
ClangASTMetadata *metadata = nullptr);
 
-  bool SetTagTypeKind(clang::QualType type, int kind) const;
-
-  bool SetDefaultAccessForRecordFields(clang::RecordDecl *record_decl,
-   int default_accessibility,
-   int *assigned_accessibilities,
-   size_t num_assigned_accessibilities);
-
   // Returns a mask containing bits from the TypeSystemClang::eTypeXXX
   // enumerations
 
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2575,42 +2575,6 @@
   return nullptr;
 }
 
-bool TypeSystemClang::SetTagTypeKind(clang::QualType tag_qual_type,
- int kind) const {
-  const clang::Type *clang_type = tag_qual_type.getTypePtr();
-  if (clang_type) {
-const clang::TagType *tag_type = llvm::dyn_cast(clang_type);
-if (tag_type) {
-  clang::TagDecl *tag_decl =
-  llvm::dyn_cast(tag_type->getDecl());
-  if (tag_decl) {
-tag_decl->setTagKind((clang::TagDecl::TagKind)kind);
-return true;
-  }
-}
-  }
-  return false;
-}
-
-bool TypeSystemClang::SetDefaultAccessForRecordFields(
-clang::RecordDecl *record_decl, int default_accessibility,
-int *assigned_accessibilities, size_t num_assigned_accessibilities) {
-  if (record_decl) {
-uint32_t field_idx;
-clang::RecordDecl::field_iterator field, field_end;
-for (field = record_decl->field_begin(),
-field_end = record_decl->field_end(), field_idx = 0;
- field != field_end; ++field, ++field_idx) {
-  // If no accessibility was assigned, assign the correct one
-  if (field_idx < num_assigned_accessibilities &&
-  assigned_accessibilities[field_idx] == clang::AS_none)
-field->setAccess((clang::AccessSpecifier)default_accessibility);
-}
-return true;
-  }
-  return false;
-}
-
 clang::DeclContext *
 TypeSystemClang::GetDeclContextForType(const CompilerType &type) {
   return GetDeclContextForType(ClangUtil::GetQualType(type));
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -111,10 +111,9 @@
   bool ParseChildMembers(
   const DWARFDIE &die, lldb_private::CompilerType &class_compiler_type,
   std::vector> &base_classes,
-  std::vector &member_accessibilities,
   std::vector &member_function_dies,
   DelayedPropertyList &delayed_properties,
-  lldb::AccessType &default_accessibility, bool &is_a_class,
+  lldb::AccessType &default_accessibility,
   lldb_private::ClangASTImporter::LayoutInfo &layout_info);
 
   size_t
@@ -194,7 +193,6 @@
   void
   ParseSingleMember(const DWARFDIE &die, const DWARFDIE &parent_die,
 const lldb_private::CompilerType &class_clang_type,
-std::vector &member_accessibilities,
 lldb::AccessType default_accessibility,
 DelayedPropertyList &delayed_properties,
 lldb_private::ClangASTImporter::LayoutInfo &layout_info,
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1983,15 +1983,12 @@
 }
 
 std::vector> bases;
-std::vector member_accessibilities;
-bool is_a_class = false;
 // Parse members and base classes first
 std::vector member_function_dies;
 
 DelayedPropertyList delayed_pr

[Lldb-commits] [PATCH] D106483: [LLDB][GUI] Add Platform Plugin Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 360767.
OmarEmaraDev added a comment.

- Rebase on main and fix conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106483

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1454,6 +1454,8 @@
   }
 
   void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+UpdateScrolling();
+
 surface.TitledBox(m_label.c_str());
 
 Rect content_bounds = surface.GetFrame();
@@ -1473,29 +1475,23 @@
   m_choice++;
   }
 
-  // If the cursor moved past the first visible choice, scroll up by one
-  // choice.
-  void ScrollUpIfNeeded() {
-if (m_choice < m_first_visibile_choice)
-  m_first_visibile_choice--;
-  }
+  void UpdateScrolling() {
+if (m_choice > GetLastVisibleChoice()) {
+  m_first_visibile_choice = m_choice - (m_number_of_visible_choices - 1);
+  return;
+}
 
-  // If the cursor moved past the last visible choice, scroll down by one
-  // choice.
-  void ScrollDownIfNeeded() {
-if (m_choice > GetLastVisibleChoice())
-  m_first_visibile_choice++;
+if (m_choice < m_first_visibile_choice)
+  m_first_visibile_choice = m_choice;
   }
 
   HandleCharResult FieldDelegateHandleChar(int key) override {
 switch (key) {
 case KEY_UP:
   SelectPrevious();
-  ScrollUpIfNeeded();
   return eKeyHandled;
 case KEY_DOWN:
   SelectNext();
-  ScrollDownIfNeeded();
   return eKeyHandled;
 default:
   break;
@@ -1509,6 +1505,15 @@
   // Returns the index of the choice.
   int GetChoice() { return m_choice; }
 
+  void SetChoice(const std::string &choice) {
+for (int i = 0; i < GetNumberOfChoices(); i++) {
+  if (choice == m_choices[i]) {
+m_choice = i;
+return;
+  }
+}
+  }
+
 protected:
   std::string m_label;
   int m_number_of_visible_choices;
@@ -1519,6 +1524,29 @@
   int m_first_visibile_choice;
 };
 
+class PlatformPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  PlatformPluginFieldDelegate(Debugger &debugger)
+  : ChoicesFieldDelegate("Platform Plugin", 3, GetPossiblePluginNames()) {
+PlatformSP platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
+if (platform_sp)
+  SetChoice(platform_sp->GetName().AsCString());
+  }
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+size_t i = 0;
+while (auto name = PluginManager::GetPlatformPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+return plugin_name;
+  }
+};
+
 class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
 public:
   ProcessPluginFieldDelegate()
@@ -1941,6 +1969,13 @@
 return delegate;
   }
 
+  PlatformPluginFieldDelegate *AddPlatformPluginField(Debugger &debugger) {
+PlatformPluginFieldDelegate *delegate =
+new PlatformPluginFieldDelegate(debugger);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   ProcessPluginFieldDelegate *AddProcessPluginField() {
 ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
 m_fields.push_back(FieldDelegateUP(delegate));
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 0780034 - [lldb] Fix that `process signal` completion always returns all signals

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-07-22T13:51:21+02:00
New Revision: 078003482e90ff5c7ba047a3d3152f0b0c392b31

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

LOG: [lldb] Fix that `process signal` completion always returns all signals

`CompletionRequest::AddCompletion` adds the given string as completion of the
current command token. `CompletionRequest::TryCompleteCurrentArg` only adds it
if the current token is a prefix of the given string. We're using
`AddCompletion` for the `process signal` handler which means that `process
signal SIGIN` doesn't get uniquely completed to `process signal SIGINT` as we
unconditionally add all other signals (such as `SIGABRT`) as possible
completions.

By using `TryCompleteCurrentArg` we actually do the proper filtering which will
only add `SIGINT` (as that's the only signal with the prefix 'SIGIN' in the
example above).

Reviewed By: mib

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectProcess.cpp
lldb/test/API/functionalities/completion/TestCompletion.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectProcess.cpp 
b/lldb/source/Commands/CommandObjectProcess.cpp
index 00fb4d669f499..7aaba37315000 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1043,7 +1043,7 @@ class CommandObjectProcessSignal : public 
CommandObjectParsed {
 UnixSignalsSP signals = m_exe_ctx.GetProcessPtr()->GetUnixSignals();
 int signo = signals->GetFirstSignalNumber();
 while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
-  request.AddCompletion(signals->GetSignalAsCString(signo), "");
+  request.TryCompleteCurrentArg(signals->GetSignalAsCString(signo));
   signo = signals->GetNextSignalNumber(signo);
 }
   }

diff  --git a/lldb/test/API/functionalities/completion/TestCompletion.py 
b/lldb/test/API/functionalities/completion/TestCompletion.py
index 7e4612ec41e6e..6a52d540184be 100644
--- a/lldb/test/API/functionalities/completion/TestCompletion.py
+++ b/lldb/test/API/functionalities/completion/TestCompletion.py
@@ -164,6 +164,8 @@ def test_process_signal(self):
 
 self.complete_from_to('process signal ',
   'process signal SIG')
+self.complete_from_to('process signal SIGIN',
+  'process signal SIGINT')
 self.complete_from_to('process signal SIGA',
   ['SIGABRT',
'SIGALRM'])



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

[Lldb-commits] [PATCH] D105028: [lldb] Fix that `process signal` completion always returns all signals

[Raphael Isemann 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 rG078003482e90: [lldb] Fix that `process signal` completion 
always returns all signals (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105028

Files:
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/test/API/functionalities/completion/TestCompletion.py


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -164,6 +164,8 @@
 
 self.complete_from_to('process signal ',
   'process signal SIG')
+self.complete_from_to('process signal SIGIN',
+  'process signal SIGINT')
 self.complete_from_to('process signal SIGA',
   ['SIGABRT',
'SIGALRM'])
Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -1043,7 +1043,7 @@
 UnixSignalsSP signals = m_exe_ctx.GetProcessPtr()->GetUnixSignals();
 int signo = signals->GetFirstSignalNumber();
 while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
-  request.AddCompletion(signals->GetSignalAsCString(signo), "");
+  request.TryCompleteCurrentArg(signals->GetSignalAsCString(signo));
   signo = signals->GetNextSignalNumber(signo);
 }
   }


Index: lldb/test/API/functionalities/completion/TestCompletion.py
===
--- lldb/test/API/functionalities/completion/TestCompletion.py
+++ lldb/test/API/functionalities/completion/TestCompletion.py
@@ -164,6 +164,8 @@
 
 self.complete_from_to('process signal ',
   'process signal SIG')
+self.complete_from_to('process signal SIGIN',
+  'process signal SIGINT')
 self.complete_from_to('process signal SIGA',
   ['SIGABRT',
'SIGALRM'])
Index: lldb/source/Commands/CommandObjectProcess.cpp
===
--- lldb/source/Commands/CommandObjectProcess.cpp
+++ lldb/source/Commands/CommandObjectProcess.cpp
@@ -1043,7 +1043,7 @@
 UnixSignalsSP signals = m_exe_ctx.GetProcessPtr()->GetUnixSignals();
 int signo = signals->GetFirstSignalNumber();
 while (signo != LLDB_INVALID_SIGNAL_NUMBER) {
-  request.AddCompletion(signals->GetSignalAsCString(signo), "");
+  request.TryCompleteCurrentArg(signals->GetSignalAsCString(signo));
   signo = signals->GetNextSignalNumber(signo);
 }
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106355: [DWARF5] Only fallback to manual index if no entry was found

[Kim-Anh Tran via Phabricator via lldb-commits]

kimanh added a comment.

Thanks for having a look at the CL!

In D106355#2893959 , @jankratochvil 
wrote:

> Is it really just a microoptimization or can you measure any improvement? 
> That `ManualDWARFIndex::Index` will be called is expected. But there should 
> be `m_units_to_avoid` covering all the units so it will quickly return 
> without indexing anything:

Yes, we are aware that it filters out the units that are already indexed by the 
the name index. In our case we have some third-party libraries that were not 
built by us, and therefore they don't have any name index. Our main focus, is 
however, not to debug those third party libraries necessarily, but only our 
main code that we are compiling. Given that the manual index is taking some 
time to be generated, we could be lazy about generating it only if we need it.

WDYT?

> Maybe there is rather some other more serious bug to fix (I am aware for 
> example D99850  but that would behave 
> differently).

No, there's no serious bug that I'm aware of that is linked to this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106355

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Jeremy Morse via Phabricator via lldb-commits]

jmorse added a comment.

This is going to be excellent for linux targets and similar,

In D106084#2882970 , @probinson wrote:

> + @jmorse who is better placed than I am to say whether this is what Sony 
> would prefer.

Slightly trickier -- our debugger won't resolve symbols across module 
boundaries (similar to the Windows debugger), which will make it hard to debug 
when debug/no-debug code is mixed. Would it be possible to default to 
`-debug-info-kind=limited` if `DebuggerTuning == llvm::DebuggerKind::SCE`? This 
leads to the fewest surprises in a default configuration targeting us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Paul Robinson via Phabricator via lldb-commits]

probinson added a reviewer: jmorse.
probinson added a subscriber: jmorse.
probinson added a comment.

+ @jmorse who is better placed than I am to say whether this is what Sony would 
prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D106084#2886659 , @jmorse wrote:

> This is going to be excellent for linux targets and similar,
>
> In D106084#2882970 , @probinson 
> wrote:
>
>> + @jmorse who is better placed than I am to say whether this is what Sony 
>> would prefer.
>
> Slightly trickier -- our debugger won't resolve symbols across module 
> boundaries (similar to the Windows debugger), which will make it hard to 
> debug when debug/no-debug code is mixed. Would it be possible to default to 
> `-debug-info-kind=limited` if `DebuggerTuning == llvm::DebuggerKind::SCE`? 
> This leads to the fewest surprises in a default configuration targeting us.

It'd be preferable not to split these two cases (current "limited" versus 
"ctor" homing) - because they rely on the same assumption, that the whole 
program is built with debug info (hence the renaming of "limited" a long time 
ago to "standalone-debug" to create a policy/philosophy around what goes in 
each category).

Wouldn't the current "limited" behavior have problems for this shared libraries 
situation too? Sounds like in that case -fstandalone-debug should be used.

(if it's a sliding scale and the problems caused by the current 
-fno-standalone-debug/-flimit-debug-info are not severe enough, but ctor-homing 
would be too severe... I'd probably be inclined to pushback on that being a 
distinction we should draw in-tree & that might be suitable to be kept 
downstream)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106339: Add support to generate Sphinx DOCX documentation

[Louis Dionne via Phabricator via lldb-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

What's the benefit of having docx documentation? We generate HTML 
documentation, which ends up in the website, and that seems strictly superior 
to generating docx. What do you need it for?

The libc++ changes are almost trivial so I would not object to the change on 
that basis, however in general I think it's better to avoid adding support for 
things we won't be using on a regular basis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106339

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

[Lldb-commits] [PATCH] D106339: Add support to generate Sphinx DOCX documentation

[Tony Tye via Phabricator via lldb-commits]

t-tye created this revision.
t-tye added a reviewer: scott.linder.
Herald added subscribers: libcxx-commits, mgorny.
Herald added a reviewer: bollu.
Herald added a reviewer: MaskRay.
Herald added a reviewer: sscalpone.
Herald added a project: libunwind.
Herald added a reviewer: libunwind.
t-tye requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, llvm-commits, openmp-commits, 
lldb-commits, sstefan1.
Herald added projects: clang, LLDB, libc++, OpenMP, LLVM, clang-tools-extra.
Herald added a reviewer: libc++.

Add support for Sphinx docx builder. Requires Sphinx docx builder to be
installed using `pip install docxbuilder`.

Added `SPHINX_OUTPUT_DOCX` CMake variable to control generation of docx
documentation. Defaults to `OFF`.

Added `_INSTALL_SPHINX_DOCX_DIR` CMake variable to control where docx
files are installed. Defaults to `share/doc//.docx`.

Documented new CMake variables in `llvm/docs/CMake.rst`.

Updated description of building documentation is `lld/docs/sphinx_intro.rst`,
`lldb/docs/resources/build.rst`, `llvm/docs/README.txt`, and
`openmp/docs/README.txt`,

Update `clang/docs/CMakeLists.txt` for Sphinx man and docx builders to ensure
.td generated .rst files are available to prevent warnings of missing rst files.
Minor CMake cleanups.

Added `docx` as an additional target for the documentation make.bat scripts.

Added `docx` as an additional target rule in the Makefile.sphinx files.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106339

Files:
  clang-tools-extra/docs/CMakeLists.txt
  clang-tools-extra/docs/conf.py
  clang-tools-extra/docs/make.bat
  clang/docs/CMakeLists.txt
  clang/docs/Makefile.sphinx
  clang/docs/analyzer/conf.py
  clang/docs/analyzer/make.bat
  clang/docs/conf.py
  clang/docs/make.bat
  flang/docs/CMakeLists.txt
  flang/docs/conf.py
  libcxx/docs/CMakeLists.txt
  libcxx/docs/Makefile.sphinx
  libcxx/docs/conf.py
  libunwind/docs/CMakeLists.txt
  libunwind/docs/conf.py
  lld/docs/CMakeLists.txt
  lld/docs/conf.py
  lld/docs/make.bat
  lld/docs/sphinx_intro.rst
  lldb/docs/CMakeLists.txt
  lldb/docs/conf.py
  lldb/docs/resources/build.rst
  llvm/cmake/modules/AddSphinxTarget.cmake
  llvm/cmake/modules/FindSphinx.cmake
  llvm/docs/CMake.rst
  llvm/docs/CMakeLists.txt
  llvm/docs/Makefile.sphinx
  llvm/docs/README.txt
  llvm/docs/conf.py
  llvm/docs/make.bat
  openmp/docs/CMakeLists.txt
  openmp/docs/README.txt
  openmp/docs/conf.py
  polly/docs/CMakeLists.txt
  polly/docs/conf.py

Index: polly/docs/conf.py
===
--- polly/docs/conf.py
+++ polly/docs/conf.py
@@ -28,6 +28,9 @@
 # coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
 extensions = ['sphinx.ext.todo', 'sphinx.ext.mathjax']
 
+if tags.has('builder-docx'):
+extensions.append('docxbuilder')
+
 # Add any paths that contain templates here, relative to this directory.
 templates_path = ['_templates']
 
Index: polly/docs/CMakeLists.txt
===
--- polly/docs/CMakeLists.txt
+++ polly/docs/CMakeLists.txt
@@ -98,6 +98,9 @@
 if (${SPHINX_OUTPUT_MAN})
   add_sphinx_target(man polly)
 endif()
+if (${SPHINX_OUTPUT_DOCX})
+  add_sphinx_target(docx polly)
+endif()
   endif()
 endif()
 
Index: openmp/docs/conf.py
===
--- openmp/docs/conf.py
+++ openmp/docs/conf.py
@@ -28,6 +28,9 @@
 # coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
 extensions = ['sphinx.ext.todo', 'sphinx.ext.mathjax', 'sphinx.ext.intersphinx']
 
+if tags.has('builder-docx'):
+extensions.append('docxbuilder')
+
 # Add any paths that contain templates here, relative to this directory.
 templates_path = ['_templates']
 
Index: openmp/docs/README.txt
===
--- openmp/docs/README.txt
+++ openmp/docs/README.txt
@@ -1,5 +1,5 @@
 OpenMP LLVM Documentation
-==
+=
 
 OpenMP LLVM's documentation is written in reStructuredText, a lightweight
 plaintext markup language (file extension `.rst`). While the
@@ -14,11 +14,15 @@
 cd 
 cmake -DLLVM_ENABLE_SPHINX=true -DSPHINX_OUTPUT_HTML=true 
 make
-$BROWSER /projects/openmp/docs//html/index.html
+$BROWSER /projects/openmp/docs/html/index.html
 
-The mapping between reStructuredText files and generated documentation is
-`docs/Foo.rst` <-> `/projects/openmp/docs//html/Foo.html` <->
-`https://openmp.llvm.org/docs/Foo.html`.
+The correspondence between reStructuredText files and generated HTML pages is:
+
+LLVM project
+`llvm/docs/Foo.rst` <-> `/docs/html/Foo.html` <-> `/share/doc/llvm/html/Foo.html` <-> `https://llvm.org/docs/Foo.html`
+
+Other projects
+`/docs/Foo.rst` <-> `/tools//docs/html/Foo.html` <-> `/share/doc//html/Foo.html`<-> `https://.llvm.org/docs/Foo.html`
 
 If you are interes

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> Wouldn't the current "limited" behavior have problems for this shared 
> libraries situation too? Sounds like in that case -fstandalone-debug should 
> be used.

@jmorse am I remembering correctly, that we require dllimport-style 
annotations, so "limited" actually includes these types even if they aren't 
constructed locally?  I am vague on the details here.  But if ctor homing and 
limited both will consider a dllimport-ed type as requiring a full description, 
that's not a reason to pick one over the other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.

Adrian said "Let's do it!" on the mailing list so LGTM for Darin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Jeremy Morse via Phabricator via lldb-commits]

jmorse accepted this revision.
jmorse added a comment.

In D106084#2887702 , @dblaikie wrote:

> It'd be preferable not to split these two cases (current "limited" versus 
> "ctor" homing) - because they rely on the same assumption, that the whole 
> program is built with debug info (hence the renaming of "limited" a long time 
> ago to "standalone-debug" to create a policy/philosophy around what goes in 
> each category).
>
> Wouldn't the current "limited" behavior have problems for this shared 
> libraries situation too? Sounds like in that case -fstandalone-debug should 
> be used.

I had a dig into what "limited" currently does -- and I think Windows most 
closely matches our setup. Many portions don't have the source available, the 
debugger behaves similarly, and the default debug-info mode there is "Limited". 
I think what saves our bacon is the import/export clause [0, 1], if a class is 
used between libraries then neither Limited or Constructor debug-info will 
suppress its class definition.

With that in mind, I suppose we'll be alright with constructor homing on by 
default. The interfaces between libraries will still have type information 
emitted in Constructor mode, and it's not excessive for those who want to dig 
deeper to run with standalone-debug.

[0] 
https://github.com/llvm/llvm-project/blob/4a30a5c8d9f0fa8a4b6ebd7c82d5335ae7a77521/clang/lib/CodeGen/CGDebugInfo.cpp#L2391
[1] 
https://github.com/llvm/llvm-project/blob/4a30a5c8d9f0fa8a4b6ebd7c82d5335ae7a77521/clang/lib/CodeGen/CGDebugInfo.cpp#L2346


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Jeremy Morse via Phabricator via lldb-commits]

jmorse added a comment.

In D106084#2890541 , @probinson wrote:

> @jmorse am I remembering correctly, that we require dllimport-style 
> annotations, so "limited" actually includes these types even if they aren't 
> constructed locally?  I am vague on the details here.  But if ctor homing and 
> limited both will consider a dllimport-ed type as requiring a full 
> description, that's not a reason to pick one over the other.

For anything shared between modules, indeed it needs the annotations (we try to 
follow Windows here). I believe there can still be un-exported types from the 
other modules headers that qualify for inclusion under "Limited" mode but won't 
for "Constructor",  and that's the benefit of Constructor mode / price if the 
other module isn't built with debug-info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> (hence the renaming of "limited" a long time ago to "standalone-debug" to 
> create a policy/philosophy around what goes in each category).

Sorry, what?  I thought -fstandalone-debug meant FullDebugInfo, and AFAICT 
that's still how the driver handles it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D106084#2890469 , @probinson wrote:

>> (hence the renaming of "limited" a long time ago to "standalone-debug" to 
>> create a policy/philosophy around what goes in each category).
>
> Sorry, what?  I thought -fstandalone-debug meant FullDebugInfo, and AFAICT 
> that's still how the driver handles it?

Sorry, misspoke. -flimit-debug-info is -fno-standalone-debug. I meant the 
terminology changed, not the semantics - the terminology of "standalone" V 
"non-standalone" is the one that gives, in my opinion, the correct framing for 
when you would use one flag or the other. The core assumption of 
-fno-standalone-debug is that you've compiled the whole program with debug info 
enabled, so assumptions can be made about type debug info being carried by some 
other object file. If parts of the program (for instance, only one shared 
library has debug info and it interacts with others that don't) are built 
without debug info, then -fstandalone-debug is the right option to use.

In D106084#2890823 , @jmorse wrote:

> In D106084#2890541 , @probinson 
> wrote:
>
>> @jmorse am I remembering correctly, that we require dllimport-style 
>> annotations, so "limited" actually includes these types even if they aren't 
>> constructed locally?  I am vague on the details here.  But if ctor homing 
>> and limited both will consider a dllimport-ed type as requiring a full 
>> description, that's not a reason to pick one over the other.
>
> For anything shared between modules, indeed it needs the annotations (we try 
> to follow Windows here). I believe there can still be un-exported types from 
> the other modules headers that qualify for inclusion under "Limited" mode but 
> won't for "Constructor",  and that's the benefit of Constructor mode / price 
> if the other module isn't built with debug-info.

I think what I'm missing here: If -fno-standalone-debug is already in use/the 
default and is causing missing types because parts of the program are bulit 
without debug info, then I'm not sure what the rationale is for slicing 
-fstandalone-debug into a "has ctor homing" and a "doesn't have ctor homing" 
strategy. The same general design philosophy applies - that in both cases 
(about 4 cases in total now: types that aren't required to be complete, 
explicitly instantiated templates, vtables, and now ctor homing) the whole 
program must be compiled with debug info enabled for these DWARF size 
optimizations to be sound.

Is there some set of cases where the current set of homing strategies is sound, 
but ctor homing is not sound?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[David Blaikie via Phabricator via lldb-commits]

dblaikie accepted this revision.
dblaikie added subscribers: probinson, aprantl.
dblaikie added a comment.
This revision is now accepted and ready to land.

Please wait for sign-off from @aprantl (or another appropriate Apple 
representative) & @probinson (or another appropriate Sony representative) as 
well - at least to check everyone's ready for this. (Apple I think still uses 
-fstandalone-debug, so won't be immediately relevant, but good to know it's 
coming in case some of their users are using -fno-standalone-debug or they have 
plans in motion, etc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 360646.
Ericson2314 added a comment.

Rebase, fixing conflicts


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

Files:
  clang/tools/scan-build/CMakeLists.txt
  libclc/CMakeLists.txt
  lldb/cmake/modules/FindLibEdit.cmake
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddSphinxTarget.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMInstallSymlink.cmake
  llvm/docs/CMake.rst
  llvm/examples/Bye/CMakeLists.txt
  llvm/include/llvm/CMakeLists.txt
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/lto/CMakeLists.txt
  llvm/tools/opt-viewer/CMakeLists.txt
  llvm/tools/remarks-shlib/CMakeLists.txt
  openmp/runtime/src/CMakeLists.txt

Index: openmp/runtime/src/CMakeLists.txt
===
--- openmp/runtime/src/CMakeLists.txt
+++ openmp/runtime/src/CMakeLists.txt
@@ -323,7 +323,7 @@
 install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_LIB_FILE}\"
   \"${alias}${LIBOMP_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \${CMAKE_INSTALL_PREFIX}/bin)")
 install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_IMP_LIB_FILE}\"
-  \"${alias}${CMAKE_STATIC_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR})")
+  \"${alias}${CMAKE_STATIC_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \"\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR}\")")
   endforeach()
 else()
 
@@ -335,7 +335,7 @@
 foreach(alias IN LISTS LIBOMP_ALIASES)
   install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E create_symlink \"${LIBOMP_LIB_FILE}\"
 \"${alias}${LIBOMP_LIBRARY_SUFFIX}\" WORKING_DIRECTORY
-\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR})")
+\"\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR}\")")
 endforeach()
   endif()
 endif()
Index: llvm/tools/remarks-shlib/CMakeLists.txt
===
--- llvm/tools/remarks-shlib/CMakeLists.txt
+++ llvm/tools/remarks-shlib/CMakeLists.txt
@@ -19,7 +19,7 @@
   endif()
   
   install(FILES ${LLVM_MAIN_INCLUDE_DIR}/llvm-c/Remarks.h
-DESTINATION include/llvm-c
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-c"
 COMPONENT Remarks)
 
   if (APPLE)
Index: llvm/tools/opt-viewer/CMakeLists.txt
===
--- llvm/tools/opt-viewer/CMakeLists.txt
+++ llvm/tools/opt-viewer/CMakeLists.txt
@@ -8,7 +8,7 @@
 
 foreach (file ${files})
   install(PROGRAMS ${file}
-DESTINATION share/opt-viewer
+DESTINATION "${CMAKE_INSTALL_DATADIR}/opt-viewer"
 COMPONENT opt-viewer)
 endforeach (file)
 
Index: llvm/tools/lto/CMakeLists.txt
===
--- llvm/tools/lto/CMakeLists.txt
+++ llvm/tools/lto/CMakeLists.txt
@@ -33,7 +33,7 @@
 ${SOURCES} DEPENDS intrinsics_gen)
 
 install(FILES ${LLVM_MAIN_INCLUDE_DIR}/llvm-c/lto.h
-  DESTINATION include/llvm-c
+  DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-c"
   COMPONENT LTO)
 
 if (APPLE)
Index: llvm/tools/llvm-config/llvm-config.cpp
===
--- llvm/tools/llvm-config/llvm-config.cpp
+++ llvm/tools/llvm-config/llvm-config.cpp
@@ -357,10 +357,16 @@
 ("-I" + ActiveIncludeDir + " " + "-I" + ActiveObjRoot + "/include");
   } else {
 ActivePrefix = CurrentExecPrefix;
-ActiveIncludeDir = ActivePrefix + "/include";
-SmallString<256> path(StringRef(LLVM_TOOLS_INSTALL_DIR));
-sys::fs::make_absolute(ActivePrefix, path);
-ActiveBinDir = std::string(path.str());
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_INCLUDEDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);
+  ActiveIncludeDir = std::string(Path.str());
+}
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_BINDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);
+  ActiveBinDir = std::string(Path.str());
+}
 ActiveLibDir = ActivePrefix + "/lib" + LLVM_LIBDIR_SUFFIX;
 ActiveCMakeDir = ActiveLibDir + "/cmake/llvm";
 ActiveIncludeOption = "-I" + ActiveIncludeDir;
Index: llvm/tools/llvm-config/BuildVariables.inc.in
===
--- llvm/tools/llvm-config/BuildVariables.inc.in
+++ llvm/tools/llvm-config/BuildVariables.inc.in
@@ -23,6 +23,8 @@
 #define LLVM_CXXFLAGS "@LLVM_CXXFLAGS@"
 #define LLVM_BUILDMODE "@LLVM_BUILDMODE@"
 #define LLVM_LIBDIR_SUFFIX "@LLVM_LIBDIR_SUFFIX@"
+#define LLVM_INSTALL_BINDIR "@CMAKE_INSTALL_BINDIR@"
+#define LLVM_INSTALL_INCLUDEDIR "@CMAKE_INSTALL_INCLUDEDIR@"
 #define LLVM_TARGETS_BUILT "@LLVM_TARGETS_BUILT@"
 #define LLVM_SYSTEM_LIBS "@LLVM_SYSTEM_LIBS@"
 #define LLVM_BUILD_SYSTEM "@LLVM_BUILD_SY

[Lldb-commits] [PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[John Ericson via Phabricator via lldb-commits]

Ericson2314 updated this revision to Diff 360647.
Ericson2314 added a comment.

rebase, fixing conflicts


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

Files:
  clang/tools/scan-build/CMakeLists.txt
  libclc/CMakeLists.txt
  lldb/cmake/modules/FindLibEdit.cmake
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/cmake/modules/AddSphinxTarget.cmake
  llvm/cmake/modules/CMakeLists.txt
  llvm/cmake/modules/LLVMInstallSymlink.cmake
  llvm/docs/CMake.rst
  llvm/examples/Bye/CMakeLists.txt
  llvm/include/llvm/CMakeLists.txt
  llvm/tools/llvm-config/BuildVariables.inc.in
  llvm/tools/llvm-config/llvm-config.cpp
  llvm/tools/lto/CMakeLists.txt
  llvm/tools/opt-viewer/CMakeLists.txt
  llvm/tools/remarks-shlib/CMakeLists.txt
  openmp/runtime/src/CMakeLists.txt

Index: openmp/runtime/src/CMakeLists.txt
===
--- openmp/runtime/src/CMakeLists.txt
+++ openmp/runtime/src/CMakeLists.txt
@@ -323,7 +323,7 @@
 install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_LIB_FILE}\"
   \"${alias}${LIBOMP_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \${CMAKE_INSTALL_PREFIX}/bin)")
 install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E copy \"${LIBOMP_IMP_LIB_FILE}\"
-  \"${alias}${CMAKE_STATIC_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR})")
+  \"${alias}${CMAKE_STATIC_LIBRARY_SUFFIX}\" WORKING_DIRECTORY \"\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR}\")")
   endforeach()
 else()
 
@@ -335,7 +335,7 @@
 foreach(alias IN LISTS LIBOMP_ALIASES)
   install(CODE "execute_process(COMMAND \"\${CMAKE_COMMAND}\" -E create_symlink \"${LIBOMP_LIB_FILE}\"
 \"${alias}${LIBOMP_LIBRARY_SUFFIX}\" WORKING_DIRECTORY
-\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR})")
+\"\$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${OPENMP_INSTALL_LIBDIR}\")")
 endforeach()
   endif()
 endif()
Index: llvm/tools/remarks-shlib/CMakeLists.txt
===
--- llvm/tools/remarks-shlib/CMakeLists.txt
+++ llvm/tools/remarks-shlib/CMakeLists.txt
@@ -19,7 +19,7 @@
   endif()
   
   install(FILES ${LLVM_MAIN_INCLUDE_DIR}/llvm-c/Remarks.h
-DESTINATION include/llvm-c
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-c"
 COMPONENT Remarks)
 
   if (APPLE)
Index: llvm/tools/opt-viewer/CMakeLists.txt
===
--- llvm/tools/opt-viewer/CMakeLists.txt
+++ llvm/tools/opt-viewer/CMakeLists.txt
@@ -8,7 +8,7 @@
 
 foreach (file ${files})
   install(PROGRAMS ${file}
-DESTINATION share/opt-viewer
+DESTINATION "${CMAKE_INSTALL_DATADIR}/opt-viewer"
 COMPONENT opt-viewer)
 endforeach (file)
 
Index: llvm/tools/lto/CMakeLists.txt
===
--- llvm/tools/lto/CMakeLists.txt
+++ llvm/tools/lto/CMakeLists.txt
@@ -33,7 +33,7 @@
 ${SOURCES} DEPENDS intrinsics_gen)
 
 install(FILES ${LLVM_MAIN_INCLUDE_DIR}/llvm-c/lto.h
-  DESTINATION include/llvm-c
+  DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-c"
   COMPONENT LTO)
 
 if (APPLE)
Index: llvm/tools/llvm-config/llvm-config.cpp
===
--- llvm/tools/llvm-config/llvm-config.cpp
+++ llvm/tools/llvm-config/llvm-config.cpp
@@ -357,10 +357,16 @@
 ("-I" + ActiveIncludeDir + " " + "-I" + ActiveObjRoot + "/include");
   } else {
 ActivePrefix = CurrentExecPrefix;
-ActiveIncludeDir = ActivePrefix + "/include";
-SmallString<256> path(StringRef(LLVM_TOOLS_INSTALL_DIR));
-sys::fs::make_absolute(ActivePrefix, path);
-ActiveBinDir = std::string(path.str());
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_INCLUDEDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);
+  ActiveIncludeDir = std::string(Path.str());
+}
+{
+  SmallString<256> Path(StringRef(LLVM_INSTALL_BINDIR));
+  sys::fs::make_absolute(ActivePrefix, Path);
+  ActiveBinDir = std::string(Path.str());
+}
 ActiveLibDir = ActivePrefix + "/lib" + LLVM_LIBDIR_SUFFIX;
 ActiveCMakeDir = ActiveLibDir + "/cmake/llvm";
 ActiveIncludeOption = "-I" + ActiveIncludeDir;
Index: llvm/tools/llvm-config/BuildVariables.inc.in
===
--- llvm/tools/llvm-config/BuildVariables.inc.in
+++ llvm/tools/llvm-config/BuildVariables.inc.in
@@ -23,6 +23,8 @@
 #define LLVM_CXXFLAGS "@LLVM_CXXFLAGS@"
 #define LLVM_BUILDMODE "@LLVM_BUILDMODE@"
 #define LLVM_LIBDIR_SUFFIX "@LLVM_LIBDIR_SUFFIX@"
+#define LLVM_INSTALL_BINDIR "@CMAKE_INSTALL_BINDIR@"
+#define LLVM_INSTALL_INCLUDEDIR "@CMAKE_INSTALL_INCLUDEDIR@"
 #define LLVM_TARGETS_BUILT "@LLVM_TARGETS_BUILT@"
 #define LLVM_SYSTEM_LIBS "@LLVM_SYSTEM_LIBS@"
 #define LLVM_BUILD_SYSTEM "@LLVM_BUILD_SY

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Amy Huang via Phabricator via lldb-commits]

akhuang updated this revision to Diff 360621.
akhuang added a comment.
Herald added a subscriber: dang.

Add an opt out flag: fno-use-ctor-homing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/DebugInfoOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/debug-info-ctor-homing-flag.cpp
  clang/test/CodeGenCXX/debug-info-template-deduction-guide.cpp
  clang/test/Driver/cl-options.c
  clang/test/Driver/clang-g-opts.c
  clang/test/Driver/cuda-dwarf-2.cu
  clang/test/Driver/debug-options-as.c
  clang/test/Driver/debug-options.c
  clang/test/Driver/integrated-as.s
  clang/test/Driver/myriad-toolchain.c
  clang/test/Driver/openmp-offload-gpu.c
  clang/test/Driver/split-debug.c
  lldb/test/Shell/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp

Index: lldb/test/Shell/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp
===
--- lldb/test/Shell/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp
+++ lldb/test/Shell/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp
@@ -106,6 +106,9 @@
 int main() {
   MemberTest::Base B1;
   B1.Get();
+  // Create instance of C1 so that it has debug info (due to constructor
+  // homing).
+  MemberTest::Class C1;
   MemberTest::Class::StaticMemberFunc(1, 10, 2);
   return 0;
 }
Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -9,7 +9,7 @@
 
 // INLINE: "-fsplit-dwarf-inlining"
 // NOINLINE-NOT: "-fsplit-dwarf-inlining"
-// SPLIT:  "-debug-info-kind=limited"
+// SPLIT:  "-debug-info-kind=constructor"
 // SPLIT-SAME: "-ggnu-pubnames"
 // SPLIT-SAME: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
@@ -38,14 +38,14 @@
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf=single -g -gno-split-dwarf %s 2>&1 | FileCheck %s --check-prefix=NOSPLIT
 // RUN: %clang -### -c -target x86_64 -gno-split-dwarf -g -gsplit-dwarf %s 2>&1 | FileCheck %s --check-prefixes=NOINLINE,SPLIT
 
-// NOSPLIT: "-debug-info-kind=limited"
+// NOSPLIT: "-debug-info-kind=constructor"
 // NOSPLIT-NOT: "-ggnu-pubnames"
 // NOSPLIT-NOT: "-split-dwarf
 
 /// Test -gsplit-dwarf=single.
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf=single -g %s 2>&1 | FileCheck %s --check-prefix=SINGLE
 
-// SINGLE: "-debug-info-kind=limited"
+// SINGLE: "-debug-info-kind=constructor"
 // SINGLE: "-split-dwarf-file" "split-debug.o"
 // SINGLE-NOT: "-split-dwarf-output"
 
@@ -62,7 +62,7 @@
 
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf=split -g -gno-pubnames %s 2>&1 | FileCheck %s --check-prefixes=NOPUBNAMES
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf=split -g -gno-gnu-pubnames %s 2>&1 | FileCheck %s --check-prefixes=NOPUBNAMES
-// NOPUBNAMES:  "-debug-info-kind=limited"
+// NOPUBNAMES:  "-debug-info-kind=constructor"
 // NOPUBNAMES-NOT:  "-ggnu-pubnames"
 // NOPUBNAMES-SAME: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
Index: clang/test/Driver/openmp-offload-gpu.c
===
--- clang/test/Driver/openmp-offload-gpu.c
+++ clang/test/Driver/openmp-offload-gpu.c
@@ -248,7 +248,7 @@
 
 // HAS_DEBUG-NOT: warning: debug
 // HAS_DEBUG: "-triple" "nvptx64-nvidia-cuda"
-// HAS_DEBUG-SAME: "-debug-info-kind={{limited|line-tables-only}}"
+// HAS_DEBUG-SAME: "-debug-info-kind={{constructor|line-tables-only}}"
 // HAS_DEBUG-SAME: "-dwarf-version=2"
 // HAS_DEBUG-SAME: "-fopenmp-is-device"
 // HAS_DEBUG: ptxas
Index: clang/test/Driver/myriad-toolchain.c
===
--- clang/test/Driver/myriad-toolchain.c
+++ clang/test/Driver/myriad-toolchain.c
@@ -83,7 +83,7 @@
 // NOSTDLIB-NOT: "-lc"
 
 // RUN: %clang -### -c -g %s -target sparc-myriad 2>&1 | FileCheck -check-prefix=G_SPARC %s
-// G_SPARC: "-debug-info-kind=limited" "-dwarf-version=2"
+// G_SPARC: "-debug-info-kind=constructor" "-dwarf-version=2"
 
 // RUN: %clang -### -c %s -target sparc-myriad-rtems -fuse-init-array 2>&1 \
 // RUN: | FileCheck -check-prefix=USE-INIT-ARRAY %s
Index: clang/test/Driver/integrated-as.s
===
--- clang/test/Driver/integrated-as.s
+++ clang/test/Driver/integrated-as.s
@@ -27,19 +27,19 @@
 // XA_INCLUDE2: "-Ifoo_dir"
 
 // RUN: %clang -### -target x86_64--- -c -integrated-as %s -gdwarf-4 -gdwarf-2 2>&1 | FileCheck --check-prefix=DWARF2 %s
-// DWARF2: "-debug-info-kind=limited" "-dwarf-version=2"
+// DWARF2: "-debug-info-kind=constructor" "-dwarf-version=2"
 
 // RUN: %clang -### -target x86_64--- -c -integrated-as %s -gdwarf-3 2>&1 | FileCheck --check-prefix=DWARF3 %s

[Lldb-commits] [PATCH] D100810: Use `GNUInstallDirs` to support custom installation dirs. -- LLVM

[John Ericson via Phabricator via lldb-commits]

Ericson2314 added a subscriber: phosek.
Ericson2314 added a comment.

@phosek would you mind reviewing this one next?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100810

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Amy Huang via Phabricator via lldb-commits]

akhuang added a comment.

Realized it's probably a good idea to add an opt out flag (counterpart to 
fuse-ctor-homing). Also, maybe in a separate patch, maybe should make these 
clang flags instead of cc1 flags-


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106459: [LLDB][GUI] Check fields validity in actions

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 360778.
OmarEmaraDev added a comment.

- Merge main and fix conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106459

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1098,7 +1101,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1138,7 +1141,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1239,6 +1242,8 @@
 return eKeyNotHandled;
   }
 
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
+
   void FieldDelegateExitCallback() override {
 if (!IsSpecified() && m_required)
   SetError("This field is required!");
@@ -1246,8 +1251,6 @@
 
   bool IsSpecified() { return !m_content.empty(); }
 
-  bool HasError() { return !m_error.empty(); }
-
   void ClearError() { m_error.clear(); }
 
   const std::string &GetError() { return m_error; }
@@ -1892,6 +1895,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of 
an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldDelegate *AddTextField(const char *label, const char *content,
@@ -2500,6 +2516,10 @@
   void Attach(Window &window) {
 ClearError();
 
+bool all_fields_are_valid = CheckFieldsValidity();
+if (!all_fields_are_valid)
+  return;
+
 bool process_is_running = StopRunningProcess();
 if (process_is_running)
   return;


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1098,7 +1101,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1138,7 +1141,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1239,6 +1242,8 @@
 return eKeyNotHandled;
   }
 
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
+
   void FieldDelegateExitCallback() override {
 if (!IsSpecified() && m_required)
   SetError("This field is required!");
@@ -1246,8 +1251,6 @@
 
   bool IsSpecified() { return !m_content.empty(); }
 
-  bool HasError() { return !m_error.empty(); }
-
   void ClearError() { m_error.clear(); }
 
   const std::string &GetError() { return m_error; }
@@ -1892,6 +1895,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldD

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 360779.
mib added a comment.

Fix `ScriptedProcess::IsAlive` crash during process destruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/examples/python/scripted_process/my_scripted_process.py
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2974,7 +2974,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
 LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
   __FUNCTION

[Lldb-commits] [lldb] 312b43d - [lldb/Plugins] Add ScriptedProcess Process Plugin

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2021-07-22T14:47:33+02:00
New Revision: 312b43da05002bbe4a06de925e34b216252bc412

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

LOG: [lldb/Plugins] Add ScriptedProcess Process Plugin

This patch introduces Scripted Processes to lldb.

The goal, here, is to be able to attach in the debugger to fake processes
that are backed by script files (in Python, Lua, Swift, etc ...) and
inspect them statically.

Scripted Processes can be used in cooperative multithreading environments
like the XNU Kernel or other real-time operating systems, but it can
also help us improve the debugger testing infrastructure by writting
synthetic tests that simulates hard-to-reproduce process/thread states.

Although ScriptedProcess is not feature-complete at the moment, it has
basic execution capabilities and will improve in the following patches.

rdar://65508855

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

Signed-off-by: Med Ismail Bennani 

Added: 
lldb/source/Plugins/Process/scripted/CMakeLists.txt
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
lldb/source/Plugins/Process/scripted/ScriptedProcess.h

Modified: 
lldb/examples/python/scripted_process/my_scripted_process.py
lldb/examples/python/scripted_process/scripted_process.py
lldb/include/lldb/Interpreter/ScriptInterpreter.h
lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
lldb/include/lldb/Target/Process.h
lldb/source/Plugins/Process/CMakeLists.txt

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
lldb/source/Target/Target.cpp
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Removed: 




diff  --git a/lldb/examples/python/scripted_process/my_scripted_process.py 
b/lldb/examples/python/scripted_process/my_scripted_process.py
index d769e137d3f34..429531e80e1e0 100644
--- a/lldb/examples/python/scripted_process/my_scripted_process.py
+++ b/lldb/examples/python/scripted_process/my_scripted_process.py
@@ -29,6 +29,9 @@ def get_loaded_images(self):
 def get_process_id(self) -> int:
 return 42
 
+def should_stop(self) -> bool:
+return True
+
 def is_alive(self) -> bool:
 return True
 

diff  --git a/lldb/examples/python/scripted_process/scripted_process.py 
b/lldb/examples/python/scripted_process/scripted_process.py
index 354b20cfa7f90..72dce5c1e3bb0 100644
--- a/lldb/examples/python/scripted_process/scripted_process.py
+++ b/lldb/examples/python/scripted_process/scripted_process.py
@@ -137,6 +137,24 @@ def resume(self):
 """
 return lldb.SBError()
 
+@abstractmethod
+def should_stop(self):
+""" Check if the scripted process plugin should produce the stop event.
+
+Returns:
+bool: True if scripted process should broadcast a stop event.
+  False otherwise.
+"""
+pass
+
+def stop(self):
+""" Trigger the scripted process stop.
+
+Returns:
+lldb.SBError: An `lldb.SBError` with error code 0.
+"""
+return lldb.SBError()
+
 @abstractmethod
 def is_alive(self):
 """ Check if the scripted process is alive.

diff  --git a/lldb/include/lldb/Interpreter/ScriptInterpreter.h 
b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
index e98e1be049f98..80a054b32ce65 100644
--- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h
+++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h
@@ -144,7 +144,8 @@ class ScriptInterpreter : public PluginInterface {
 
   ScriptInterpreter(
   Debugger &debugger, lldb::ScriptLanguage script_lang,
-  lldb::ScriptedProcessInterfaceUP scripted_process_interface_up = {});
+  lldb::ScriptedProcessInterfaceUP scripted_process_interface_up =
+  std::make_unique());
 
   ~ScriptInterpreter() override = default;
 

diff  --git a/lldb/include/lldb/Interpreter/ScriptedProcessInterface.h 
b/lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
index 67fa8e3133cd5..223e89be87ee6 100644
--- a/lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
+++ b/lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
@@ -32,6 +32,10 @@ class ScriptedProcessInterface {
 
   virtual Status Resume() { return Status("ScriptedProcess did not resume"); }
 
+  virtual bool ShouldStop() { return true; }
+
+  virtual Status Stop() { return Status("ScriptedProcess did not stop"); }
+
   virtual lldb::MemoryRegionInfoSP
   GetMemoryRegionContainingAddress(lldb::addr_t address) {
 return nullptr;

diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index 1f671a17a04b4..2a993c498d302 100644
-

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani 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 rG312b43da0500: [lldb/Plugins] Add ScriptedProcess Process 
Plugin (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/examples/python/scripted_process/my_scripted_process.py
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2974,7 +2974,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()

[Lldb-commits] [lldb] 77440d6 - [lldb][NFC] Allow range-based for loops over DWARFDIE's children

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-07-22T15:03:30+02:00
New Revision: 77440d644b3ba26443c1d14d04a4046fab07d731

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

LOG: [lldb][NFC] Allow range-based for loops over DWARFDIE's children

This patch adds the ability to get a DWARFDIE's children as an LLVM range.

This way we can use for range loops to iterate over them and we can use LLVM's
algorithms like `llvm::all_of` to query all children.

The implementation has to do some small shenanigans as the iterator needs to
store a DWARFDIE, but a DWARFDIE container is also a DWARFDIE so it can't return
the iterator by value. I just made the `children` getter a templated function to
avoid the cyclic dependency.

Reviewed By: #lldb, werat, JDevlieghere

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

Added: 
lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/unittests/SymbolFile/DWARF/CMakeLists.txt

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 4cf4ea6ed0f4c..46015f7b43b1c 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -666,8 +666,7 @@ DWARFASTParserClang::ParseTypeModifier(const SymbolContext 
&sc,
 // Blocks have a __FuncPtr inside them which is a pointer to a
 // function of the proper type.
 
-for (DWARFDIE child_die = target_die.GetFirstChild();
- child_die.IsValid(); child_die = child_die.GetSibling()) {
+for (DWARFDIE child_die : target_die.children()) {
   if (!strcmp(child_die.GetAttributeValueAsString(DW_AT_name, ""),
   "__FuncPtr")) {
 DWARFDIE function_pointer_type =
@@ -1827,8 +1826,7 @@ bool DWARFASTParserClang::ParseTemplateDIE(
   case DW_TAG_GNU_template_parameter_pack: {
 template_param_infos.packed_args =
 std::make_unique();
-for (DWARFDIE child_die = die.GetFirstChild(); child_die.IsValid();
- child_die = child_die.GetSibling()) {
+for (DWARFDIE child_die : die.children()) {
   if (!ParseTemplateDIE(child_die, *template_param_infos.packed_args))
 return false;
 }
@@ -1933,8 +1931,7 @@ bool DWARFASTParserClang::ParseTemplateParameterInfos(
   if (!parent_die)
 return false;
 
-  for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid();
-   die = die.GetSibling()) {
+  for (DWARFDIE die : parent_die.children()) {
 const dw_tag_t tag = die.Tag();
 
 switch (tag) {
@@ -2108,8 +2105,7 @@ void 
DWARFASTParserClang::EnsureAllDIEsInDeclContextHaveBeenParsed(
   for (auto it = m_decl_ctx_to_die.find(opaque_decl_ctx);
it != m_decl_ctx_to_die.end() && it->first == opaque_decl_ctx;
it = m_decl_ctx_to_die.erase(it))
-for (DWARFDIE decl = it->second.GetFirstChild(); decl;
- decl = decl.GetSibling())
+for (DWARFDIE decl : it->second.children())
   GetClangDeclForDIE(decl);
 }
 
@@ -2145,8 +2141,7 @@ size_t DWARFASTParserClang::ParseChildEnumerators(
 
   size_t enumerators_added = 0;
 
-  for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid();
-   die = die.GetSibling()) {
+  for (DWARFDIE die : parent_die.children()) {
 const dw_tag_t tag = die.Tag();
 if (tag == DW_TAG_enumerator) {
   DWARFAttributes attributes;
@@ -2751,8 +2746,7 @@ bool DWARFASTParserClang::ParseChildMembers(
   if (ast == nullptr)
 return false;
 
-  for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid();
-   die = die.GetSibling()) {
+  for (DWARFDIE die : parent_die.children()) {
 dw_tag_t tag = die.Tag();
 
 switch (tag) {
@@ -2898,8 +2892,7 @@ size_t DWARFASTParserClang::ParseChildParameters(
 return 0;
 
   size_t arg_idx = 0;
-  for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid();
-   die = die.GetSibling()) {
+  for (DWARFDIE die : parent_die.children()) {
 const dw_tag_t tag = die.Tag();
 switch (tag) {
 case DW_TAG_formal_parameter: {
@@ -3018,8 +3011,7 @@ DWARFASTParser::ParseChildArrayInfo(const DWARFDIE 
&parent_die,
   if (!parent_die)
 return llvm::None;
 
-  for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid();
-   die = die.GetSibling()) {
+  for (DWARFDIE die : parent_die.children()) {
 const dw_tag_t tag = die.Tag();
 if (tag != DW_TAG_subrange_type)
   continue;
@@ -3321,8 +3313,7 @@ static DWARFDIE 
GetContainingFunctionWithAbstractOrigin(const DWA

[Lldb-commits] [PATCH] D103172: [lldb][NFC] Allow range-based for loops over DWARFDIE's children

[Raphael Isemann 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 rG77440d644b3b: [lldb][NFC] Allow range-based for loops over 
DWARFDIE's children (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103172

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
  lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp

Index: lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp
===
--- /dev/null
+++ lldb/unittests/SymbolFile/DWARF/DWARFDIETest.cpp
@@ -0,0 +1,105 @@
+//===-- DWARFDIETest.cpp --=---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Plugins/SymbolFile/DWARF/DWARFDIE.h"
+#include "TestingSupport/Symbol/YAMLModuleTester.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+TEST(DWARFDIETest, ChildIteration) {
+  // Tests DWARFDIE::child_iterator.
+
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_386
+DWARF:
+  debug_abbrev:
+- Table:
+- Code:0x0001
+  Tag: DW_TAG_compile_unit
+  Children:DW_CHILDREN_yes
+  Attributes:
+- Attribute:   DW_AT_language
+  Form:DW_FORM_data2
+- Code:0x0002
+  Tag: DW_TAG_base_type
+  Children:DW_CHILDREN_no
+  Attributes:
+- Attribute:   DW_AT_encoding
+  Form:DW_FORM_data1
+- Attribute:   DW_AT_byte_size
+  Form:DW_FORM_data1
+  debug_info:
+- Version: 4
+  AddrSize:8
+  Entries:
+- AbbrCode:0x0001
+  Values:
+- Value:   0x000C
+- AbbrCode:0x0002
+  Values:
+- Value:   0x0007 # DW_ATE_unsigned
+- Value:   0x0004
+- AbbrCode:0x0002
+  Values:
+- Value:   0x0007 # DW_ATE_unsigned
+- Value:   0x0008
+- AbbrCode:0x0002
+  Values:
+- Value:   0x0005 # DW_ATE_signed
+- Value:   0x0008
+- AbbrCode:0x
+)";
+
+  YAMLModuleTester t(yamldata);
+  ASSERT_TRUE((bool)t.GetDwarfUnit());
+
+  DWARFUnit *unit = t.GetDwarfUnit();
+  const DWARFDebugInfoEntry *die_first = unit->DIE().GetDIE();
+
+  // Create a DWARFDIE that has three DW_TAG_base_type children.
+  DWARFDIE top_die(unit, die_first);
+
+  // Create the iterator range that has the three tags as elements.
+  llvm::iterator_range children = top_die.children();
+
+  // Compare begin() to the first child DIE.
+  DWARFDIE::child_iterator child_iter = children.begin();
+  ASSERT_NE(child_iter, children.end());
+  const DWARFDebugInfoEntry *die_child0 = die_first->GetFirstChild();
+  EXPECT_EQ((*child_iter).GetDIE(), die_child0);
+
+  // Step to the second child DIE.
+  ++child_iter;
+  ASSERT_NE(child_iter, children.end());
+  const DWARFDebugInfoEntry *die_child1 = die_child0->GetSibling();
+  EXPECT_EQ((*child_iter).GetDIE(), die_child1);
+
+  // Step to the third child DIE.
+  ++child_iter;
+  ASSERT_NE(child_iter, children.end());
+  const DWARFDebugInfoEntry *die_child2 = die_child1->GetSibling();
+  EXPECT_EQ((*child_iter).GetDIE(), die_child2);
+
+  // Step to the end of the range.
+  ++child_iter;
+  EXPECT_EQ(child_iter, children.end());
+
+  // Take one of the DW_TAG_base_type DIEs (which has no children) and make
+  // sure the children range is now empty.
+  DWARFDIE no_children_die(unit, die_child0);
+  EXPECT_TRUE(no_children_die.children().empty());
+}
Index: lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
===
--- lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
+++ lldb/unittests/SymbolFile/DWARF/CMakeLists.txt
@@ -1,5 +1,6 @@
 add_lldb_unittest(SymbolFileDWARFTests
   DWARFASTParserClangTests.cpp
+  DWARFDIETest.cpp
   DWARFUnitTest.cpp
   S

[Lldb-commits] [lldb] eb61ffb - [lldb] Fix TestCompletion by using SIGPIPE instead of SIGINT as test signal

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-07-22T15:35:28+02:00
New Revision: eb61ffbcb277cfaeb459d6d38b34ba908d247f96

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

LOG: [lldb] Fix TestCompletion by using SIGPIPE instead of SIGINT as test signal

The test I added in commit 078003482e90ff5c7ba047a3d3152f0b0c392b31 was using
SIGINT for testing the tab completion. The idea is to have a signal that only
has one possible completion and I ended up picking SIGIN -> SIGINT for the test.
However on non-Linux systems there is SIGINFO which is a valid completion for
`SIGIN' and so the test fails there.

This replaces SIGIN -> SIGINT with SIGPIP -> SIGPIPE completion which according
to LLDB's signal list in Host.cpp is the only valid completion.

Added: 


Modified: 
lldb/test/API/functionalities/completion/TestCompletion.py

Removed: 




diff  --git a/lldb/test/API/functionalities/completion/TestCompletion.py 
b/lldb/test/API/functionalities/completion/TestCompletion.py
index 6a52d540184be..11f0e387245e0 100644
--- a/lldb/test/API/functionalities/completion/TestCompletion.py
+++ b/lldb/test/API/functionalities/completion/TestCompletion.py
@@ -164,8 +164,8 @@ def test_process_signal(self):
 
 self.complete_from_to('process signal ',
   'process signal SIG')
-self.complete_from_to('process signal SIGIN',
-  'process signal SIGINT')
+self.complete_from_to('process signal SIGPIP',
+  'process signal SIGPIPE')
 self.complete_from_to('process signal SIGA',
   ['SIGABRT',
'SIGALRM'])



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

[Lldb-commits] [lldb] 3d9a9fa - [lldb] Remove a wrong assert in TestStructTypes that checks that empty structs in C always have size 0

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-07-22T16:56:50+02:00
New Revision: 3d9a9fa6911a5228ce799a7c639e94d322678934

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

LOG: [lldb] Remove a wrong assert in TestStructTypes that checks that empty 
structs in C always have size 0

D105471 fixes the way we assign sizes to empty structs in C mode. Instead of
just giving them a size 0, we instead use the size we get from DWARF if 
possible.

After landing D105471 the TestStructTypes test started failing on Windows. The
tests checked that the size of an empty C struct is 0 while the size LLDB now
reports is 4 bytes. It turns out that 4 bytes are the actual size Clang is using
for C structs with the MicrosoftRecordLayoutBuilder. The commit that introduced
that behaviour is 00a061dccc6671c96412d7b28ab2012963208579.

This patch removes that specific check from TestStructTypes. Note that D105471
added a series of tests that already cover this case (and the added checks
automatically adjust to whatever size the target compiler chooses for empty
structs).

Added: 


Modified: 
lldb/test/API/lang/c/struct_types/main.c

Removed: 




diff  --git a/lldb/test/API/lang/c/struct_types/main.c 
b/lldb/test/API/lang/c/struct_types/main.c
index 08ef0a703f22b..e683c49108976 100644
--- a/lldb/test/API/lang/c/struct_types/main.c
+++ b/lldb/test/API/lang/c/struct_types/main.c
@@ -22,7 +22,6 @@ int main (int argc, char const *argv[])
 
 struct {} empty;
 //% self.expect("frame variable empty", substrs = ["empty = {}"])
-//% self.expect("expression -- sizeof(empty)", substrs = ["= 0"])
 
 struct rect_tag {
 struct point_tag bottom_left;



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

[Lldb-commits] [PATCH] D106553: [LLDB][GUI] Resolve paths in file/directory fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch resolves the paths in the file/directory fields before
performing checks. Those checks are applied on the file system if
m_need_to_exist is true, so remote files can set this to false to avoid
performing host-side file system checks. Additionally, methods to get
a resolved and a direct file specs were added to be used by client code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106553

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1297,8 +1297,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
   return;
 }
@@ -1308,7 +1311,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:
@@ -1327,8 +1340,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
   return;
 }
@@ -1338,7 +1354,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1297,8 +1297,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
   return;
 }
@@ -1308,7 +1311,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:
@@ -1327,8 +1340,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
   return;
 }
@@ -1338,7 +1354,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105471: [lldb] Generalize empty record size computation to avoid giving empty C++ structs a size of 0

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

I think this change broke the Windows LLDB bot. More specifically the 
TestStructTypes test:

https://lab.llvm.org/buildbot/#/builders/83/builds/8528


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105471

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

[Lldb-commits] [PATCH] D105471: [lldb] Generalize empty record size computation to avoid giving empty C++ structs a size of 0

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

In D105471#2896533 , 
@stella.stamenova wrote:

> I think this change broke the Windows LLDB bot. More specifically the 
> TestStructTypes test:
>
> https://lab.llvm.org/buildbot/#/builders/83/builds/8528

Looks like you fixed it already :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105471

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

[Lldb-commits] [PATCH] D105471: [lldb] Generalize empty record size computation to avoid giving empty C++ structs a size of 0

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D105471#2896544 , 
@stella.stamenova wrote:

> In D105471#2896533 , 
> @stella.stamenova wrote:
>
>> I think this change broke the Windows LLDB bot. More specifically the 
>> TestStructTypes test:
>>
>> https://lab.llvm.org/buildbot/#/builders/83/builds/8528
>
> Looks like you fixed it already :)

Lots of practice from breaking that bot far too often in the past :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105471

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

[Lldb-commits] [PATCH] D106564: [LLDB][GUI] Add Arch Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds an Arch field that inputs and validates an arch spec.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106564

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1345,6 +1345,25 @@
   bool m_need_to_exist;
 };
 
+class ArchFieldDelegate : public TextFieldDelegate {
+public:
+  ArchFieldDelegate(const char *label, const char *content, bool required)
+  : TextFieldDelegate(label, content, required) {}
+
+  void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
+if (!GetArchSpec().IsValid())
+  SetError("Not a valid arch!");
+  }
+
+  const std::string &GetArchString() { return m_content; }
+
+  ArchSpec GetArchSpec() { return ArchSpec(GetArchString()); }
+};
+
 class BooleanFieldDelegate : public FieldDelegate {
 public:
   BooleanFieldDelegate(const char *label, bool content)
@@ -1919,6 +1938,14 @@
 return delegate;
   }
 
+  ArchFieldDelegate *AddArchField(const char *label, const char *content,
+  bool required) {
+ArchFieldDelegate *delegate =
+new ArchFieldDelegate(label, content, required);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   IntegerFieldDelegate *AddIntegerField(const char *label, int content,
 bool required) {
 IntegerFieldDelegate *delegate =


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1345,6 +1345,25 @@
   bool m_need_to_exist;
 };
 
+class ArchFieldDelegate : public TextFieldDelegate {
+public:
+  ArchFieldDelegate(const char *label, const char *content, bool required)
+  : TextFieldDelegate(label, content, required) {}
+
+  void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
+if (!GetArchSpec().IsValid())
+  SetError("Not a valid arch!");
+  }
+
+  const std::string &GetArchString() { return m_content; }
+
+  ArchSpec GetArchSpec() { return ArchSpec(GetArchString()); }
+};
+
 class BooleanFieldDelegate : public FieldDelegate {
 public:
   BooleanFieldDelegate(const char *label, bool content)
@@ -1919,6 +1938,14 @@
 return delegate;
   }
 
+  ArchFieldDelegate *AddArchField(const char *label, const char *content,
+  bool required) {
+ArchFieldDelegate *delegate =
+new ArchFieldDelegate(label, content, required);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   IntegerFieldDelegate *AddIntegerField(const char *label, int content,
 bool required) {
 IntegerFieldDelegate *delegate =
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 360857.
mib edited the summary of this revision.
mib added a reviewer: teemperor.
mib added a project: LLDB.
mib added a comment.
Herald added a subscriber: JDevlieghere.

Conform `ScriptedProcessPythonInterface` to SWIG python types


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

Files:
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h


Index: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
===
--- 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
+++ 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
@@ -51,7 +51,7 @@
   bool IsAlive() override;
 
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  unsigned long long GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:
Index: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
===
--- 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
+++ 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
@@ -63,7 +63,7 @@
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  return static_cast(GetGenericInteger("shuold_stop"));
 }
 
 Status ScriptedProcessPythonInterface::Stop() {
@@ -134,21 +134,24 @@
   return Status("Returned object is null.");
 }
 
-size_t
+unsigned long long
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) 
{
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
  Locker::FreeLock);
 
+  unsigned long long invalid_address =
+  std::numeric_limits::max();
+
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return invalid_address;
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return invalid_address;
   PythonObject implementor(PyRefType::Borrowed,
(PyObject *)m_object_instance_sp->GetValue());
 
   if (!implementor.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return invalid_address;
 
   PythonObject pmeth(
   PyRefType::Owned,
@@ -158,12 +161,12 @@
 PyErr_Clear();
 
   if (!pmeth.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return invalid_address;
 
   if (PyCallable_Check(pmeth.get()) == 0) {
 if (PyErr_Occurred())
   PyErr_Clear();
-return LLDB_INVALID_ADDRESS;
+return invalid_address;
   }
 
   if (PyErr_Occurred())
@@ -181,9 +184,9 @@
 
   if (py_return.get()) {
 auto size = py_return.AsUnsignedLongLong();
-return (size) ? *size : LLDB_INVALID_ADDRESS;
+return (size) ? *size : invalid_address;
   }
-  return LLDB_INVALID_ADDRESS;
+  return invalid_address;
 }
 
 lldb::MemoryRegionInfoSP
@@ -280,7 +283,7 @@
 }
 
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
+  unsigned long long pid = GetGenericInteger("get_process_id");
 
   return (pid >= std::numeric_limits::max())
  ? LLDB_INVALID_PROCESS_ID
@@ -288,7 +291,7 @@
 }
 
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  return static_cast(GetGenericInteger("is_alive"));
 }
 
 #endif


Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
@@ -51,7 +51,7 @@
   bool IsAlive() override;
 
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  unsigned long long GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
@@ -63,7 +63,7 @@
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  return static_cast(GetGenericInteger("shuold_stop"));
 }
 
 Status ScriptedProcessPythonInterface::Stop() {
@@ -134,21 +134,24 @@
   return Status("Returned object is null.");
 }
 
-size_t
+unsigned long long
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
   Locker py_lock(&m_interpreter, Loc

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Some nits but otherwise this seems like the right direction. Thanks!




Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:66
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  return static_cast(GetGenericInteger("shuold_stop"));
 }

Unrelated to the change here, but that looks like a typo in the string.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:137
 
-size_t
+unsigned long long
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) 
{

Could we return an `llvm::Optional<...>` here? Then we don't need the 
`invalid_address` variable but instead just `return llvm::None` in the early 
exits.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:294
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  return static_cast(GetGenericInteger("is_alive"));
 }

I think this should return `false` when the underlying object is invalid? Bit 
out of scope for this patch, so feel free to fix this in another review.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h:54
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  unsigned long long GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);

Could you actually make a typedef for this? `typedef unsigned long long 
PythonInteger` or so. cpython really like to change their API between versions 
and this would make the potential fixups a bit easier if they decide to change 
the return value.

We might actually want to put this typedef into the `PythonObject` API int the 
future, but that's probably out-of-scope for this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

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

[Lldb-commits] [PATCH] D103172: [lldb][NFC] Allow range-based for loops over DWARFDIE's children

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h:97-105
+  template 
+  llvm::iterator_range children() const {
+return llvm::make_range(T(*this), T());
+  }
+};
+
+class DWARFDIE::child_iterator

rather than a temeplate, what if it were code something like this: 
https://godbolt.org/z/3abeK3EPx ?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103172

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

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 4 inline comments as done.
mib added inline comments.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:66
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  return static_cast(GetGenericInteger("shuold_stop"));
 }

teemperor wrote:
> Unrelated to the change here, but that looks like a typo in the string.
Thanks for catching that!



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:294
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  return static_cast(GetGenericInteger("is_alive"));
 }

teemperor wrote:
> I think this should return `false` when the underlying object is invalid? Bit 
> out of scope for this patch, so feel free to fix this in another review.
This is just the interface between Python and C++, the object sanity check is 
done above in `ScriptedProcess::IsAlive`.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h:54
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  unsigned long long GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);

teemperor wrote:
> Could you actually make a typedef for this? `typedef unsigned long long 
> PythonInteger` or so. cpython really like to change their API between 
> versions and this would make the potential fixups a bit easier if they decide 
> to change the return value.
> 
> We might actually want to put this typedef into the `PythonObject` API int 
> the future, but that's probably out-of-scope for this patch.
There is actually a `PythonInteger` class already but in order to extract the 
scalar, I have to make a proxy `StructuredData::Integer` to get the `uint64_t` 
value. Notice, in this case, the return type is not `unsigned long long` 
anymore ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

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

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 360925.
mib marked 3 inline comments as done.
mib added a comment.

Address @teemperor comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

Files:
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
@@ -51,7 +51,7 @@
   bool IsAlive() override;
 
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  llvm::Optional GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
@@ -63,7 +63,12 @@
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  auto should_stop = GetGenericInteger("should_stop");
+
+  if (!should_stop)
+return false;
+
+  return static_cast(*should_stop);
 }
 
 Status ScriptedProcessPythonInterface::Stop() {
@@ -134,21 +139,21 @@
   return Status("Returned object is null.");
 }
 
-size_t
+llvm::Optional
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
  Locker::FreeLock);
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   PythonObject implementor(PyRefType::Borrowed,
(PyObject *)m_object_instance_sp->GetValue());
 
   if (!implementor.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   PythonObject pmeth(
   PyRefType::Owned,
@@ -158,12 +163,12 @@
 PyErr_Clear();
 
   if (!pmeth.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (PyCallable_Check(pmeth.get()) == 0) {
 if (PyErr_Occurred())
   PyErr_Clear();
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   }
 
   if (PyErr_Occurred())
@@ -179,11 +184,22 @@
 PyErr_Clear();
   }
 
-  if (py_return.get()) {
-auto size = py_return.AsUnsignedLongLong();
-return (size) ? *size : LLDB_INVALID_ADDRESS;
+  if (!py_return.get())
+return llvm::None;
+
+  StructuredData::ObjectSP structured_object(
+  py_return.CreateStructuredObject());
+  if (!structured_object || !structured_object->IsValid())
+return llvm::None;
+
+  switch (structured_object->GetType()) {
+  case lldb::eStructuredDataTypeInteger:
+return structured_object->GetIntegerValue();
+  case lldb::eStructuredDataTypeBoolean:
+return structured_object->GetBooleanValue();
+  default:
+return llvm::None;
   }
-  return LLDB_INVALID_ADDRESS;
 }
 
 lldb::MemoryRegionInfoSP
@@ -280,15 +296,17 @@
 }
 
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
-
-  return (pid >= std::numeric_limits::max())
- ? LLDB_INVALID_PROCESS_ID
- : pid;
+  auto pid = GetGenericInteger("get_process_id");
+  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;
 }
 
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  auto is_alive = GetGenericInteger("is_alive");
+
+  if (!is_alive)
+return false;
+
+  return static_cast(*is_alive);
 }
 
 #endif
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106584: [lldb] Improve checking of file cache read eligibility for mach-O

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 created this revision.
augusto2112 added reviewers: jasonmolenda, aprantl.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

On mach-O, if an object file is eligible to be in the shared cache, it
cannot have data read from disk, since offsets in the process may be
fixed by the shared cache builder. This patch takes this into account by
checking if the mach-O file contains a LC_SEGMENT_SPLIT_INFO load
command, which indicates shared cache eligibility.

This patch disables the file cache optimization by default for other
object files, since there's a risk we were doing wrong reads elsewhere
as well, but makes it easy for them to implement their own logic to check
if a file cache read is safe by overriding IsSafeToReadFromFileCache.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106584

Files:
  lldb/include/lldb/Core/Section.h
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/source/Core/Section.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -1761,21 +1761,34 @@
   // Read from file cache if read-only section.
   if (!force_live_memory && resolved_addr.IsSectionOffset()) {
 SectionSP section_sp(resolved_addr.GetSection());
-if (section_sp) {
-  auto permissions = Flags(section_sp->GetPermissions());
-  bool is_readonly = !permissions.Test(ePermissionsWritable) &&
- permissions.Test(ePermissionsReadable);
-  if (is_readonly) {
-file_cache_bytes_read =
-ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error);
-if (file_cache_bytes_read == dst_len)
-  return file_cache_bytes_read;
-else if (file_cache_bytes_read > 0) {
-  file_cache_read_buffer =
-  std::make_unique(file_cache_bytes_read);
-  std::memcpy(file_cache_read_buffer.get(), dst, file_cache_bytes_read);
+ObjectFile *obj_file = section_sp->GetObjectFile();
+if (obj_file && obj_file->IsSafeToReadFromFileCache(resolved_addr)) {
+  file_cache_bytes_read =
+  ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error);
+
+#ifndef NDEBUG // Assert that file cache matches the process memory
+  if (ProcessIsValid() && file_cache_bytes_read == dst_len) {
+if (load_addr == LLDB_INVALID_ADDRESS)
+  load_addr = resolved_addr.GetLoadAddress(this);
+if (load_addr != LLDB_INVALID_ADDRESS) {
+  uint8_t *live_buf = (uint8_t *)malloc(dst_len);
+  bytes_read =
+  m_process_sp->ReadMemory(load_addr, live_buf, dst_len, error);
+  if (bytes_read == dst_len) {
+assert(memcmp(live_buf, dst, dst_len) == 0);
+  }
+  free(live_buf);
 }
   }
+#endif
+
+  if (file_cache_bytes_read == dst_len)
+return file_cache_bytes_read;
+  if (file_cache_bytes_read > 0) {
+file_cache_read_buffer =
+std::make_unique(file_cache_bytes_read);
+std::memcpy(file_cache_read_buffer.get(), dst, file_cache_bytes_read);
+  }
 }
   }
 
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -148,6 +148,11 @@
 
   uint32_t GetPluginVersion() override;
 
+  bool ContainsLoadCommand(uint32_t lc);
+  
+  bool IsSafeToReadFromFileCache(const lldb_private::Address &addr) override;
+
+
 protected:
   static lldb_private::UUID
   GetUUID(const llvm::MachO::mach_header &header,
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -7006,3 +7006,38 @@
   }
   return added_images;
 }
+
+bool ObjectFileMachO::ContainsLoadCommand(uint32_t lc) {
+  lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
+  llvm::MachO::load_command load_cmd;
+  for (uint32_t i = 0; i < m_header.ncmds; ++i) {
+const lldb::offset_t load_cmd_offset = offset;
+if (m_data.GetU32(&offset, &load_cmd, 2) == nullptr)
+  break;
+
+if (load_cmd.cmd == lc)
+  return true;
+offset = load_cmd_offset + load_cmd.cmdsize;
+  }
+  return false;
+}
+
+bool ObjectFileMachO::IsSafeToReadFromFileCache(
+const lldb_private::Address &addr) {
+  if (!addr.IsValid())
+return false;
+  SectionSP section_sp(addr.GetSection());
+  if (!section_sp)
+return false;
+
+  // A writable section is not safe to read from the file cache, as the contents
+  // between i

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Raphael Isemann via Phabricator via lldb-commits]

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

I think this is the last round of review so I'll just accept this modulo a few 
nits.




Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:190
+
+  StructuredData::ObjectSP structured_object(
+  py_return.CreateStructuredObject());

As discussed offline, I think with the API we got here I would much rather have 
the unhandled llvm::Expected from the old code than the strange StructuredData 
code that just silently hides errors. Let's revert this back to the old but 
with llvm::None and put a big FIXME there.

```
lang=c++
llvm::Expected size = py_return.AsUnsignedLongLong();
if (!size) {
  // FIXME: Handle error.
  return llvm::None;
}
return *size;
```



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:299
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
-
-  return (pid >= std::numeric_limits::max())
- ? LLDB_INVALID_PROCESS_ID
- : pid;
+  auto pid = GetGenericInteger("get_process_id");
+  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;

Could you spell the `auto` out here? `llvm::Optional` is I think 
simple enough.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:304
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  auto is_alive = GetGenericInteger("is_alive");
+

Same as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

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

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 360958.
mib added a comment.

Address last comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

Files:
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
@@ -51,7 +51,7 @@
   bool IsAlive() override;
 
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  llvm::Optional GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
@@ -63,7 +63,13 @@
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  llvm::Optional should_stop =
+  GetGenericInteger("should_stop");
+
+  if (!should_stop)
+return false;
+
+  return static_cast(*should_stop);
 }
 
 Status ScriptedProcessPythonInterface::Stop() {
@@ -134,21 +140,21 @@
   return Status("Returned object is null.");
 }
 
-size_t
+llvm::Optional
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
  Locker::FreeLock);
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   PythonObject implementor(PyRefType::Borrowed,
(PyObject *)m_object_instance_sp->GetValue());
 
   if (!implementor.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   PythonObject pmeth(
   PyRefType::Owned,
@@ -158,12 +164,12 @@
 PyErr_Clear();
 
   if (!pmeth.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (PyCallable_Check(pmeth.get()) == 0) {
 if (PyErr_Occurred())
   PyErr_Clear();
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   }
 
   if (PyErr_Occurred())
@@ -179,11 +185,15 @@
 PyErr_Clear();
   }
 
-  if (py_return.get()) {
-auto size = py_return.AsUnsignedLongLong();
-return (size) ? *size : LLDB_INVALID_ADDRESS;
-  }
-  return LLDB_INVALID_ADDRESS;
+  if (!py_return.get())
+return llvm::None;
+
+  llvm::Expected size = py_return.AsUnsignedLongLong();
+  // FIXME: Handle error.
+  if (!size)
+return llvm::None;
+
+  return *size;
 }
 
 lldb::MemoryRegionInfoSP
@@ -280,15 +290,17 @@
 }
 
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
-
-  return (pid >= std::numeric_limits::max())
- ? LLDB_INVALID_PROCESS_ID
- : pid;
+  llvm::Optional pid = GetGenericInteger("get_process_id");
+  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;
 }
 
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  llvm::Optional is_alive = GetGenericInteger("is_alive");
+
+  if (!is_alive)
+return false;
+
+  return static_cast(*is_alive);
 }
 
 #endif
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 3d4cadf - [lldb/Interpreter] Conform ScriptedProcessPythonInterface to SWIG python types

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2021-07-22T22:48:15+02:00
New Revision: 3d4cadfb26437bd686ca8177f5454a366fed59eb

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

LOG: [lldb/Interpreter] Conform ScriptedProcessPythonInterface to SWIG python 
types

This patch should address the compiler warnings due to mismatch type
comparaison.

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

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

Removed: 




diff  --git 
a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
 
b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
index 51168f8095f84..ce262c930f8b7 100644
--- 
a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
+++ 
b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
@@ -63,7 +63,13 @@ Status ScriptedProcessPythonInterface::Resume() {
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  llvm::Optional should_stop =
+  GetGenericInteger("should_stop");
+
+  if (!should_stop)
+return false;
+
+  return static_cast(*should_stop);
 }
 
 Status ScriptedProcessPythonInterface::Stop() {
@@ -134,21 +140,21 @@ Status 
ScriptedProcessPythonInterface::GetStatusFromMethod(
   return Status("Returned object is null.");
 }
 
-size_t
+llvm::Optional
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) 
{
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
  Locker::FreeLock);
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   PythonObject implementor(PyRefType::Borrowed,
(PyObject *)m_object_instance_sp->GetValue());
 
   if (!implementor.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   PythonObject pmeth(
   PyRefType::Owned,
@@ -158,12 +164,12 @@ 
ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
 PyErr_Clear();
 
   if (!pmeth.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (PyCallable_Check(pmeth.get()) == 0) {
 if (PyErr_Occurred())
   PyErr_Clear();
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   }
 
   if (PyErr_Occurred())
@@ -179,11 +185,15 @@ 
ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
 PyErr_Clear();
   }
 
-  if (py_return.get()) {
-auto size = py_return.AsUnsignedLongLong();
-return (size) ? *size : LLDB_INVALID_ADDRESS;
-  }
-  return LLDB_INVALID_ADDRESS;
+  if (!py_return.get())
+return llvm::None;
+
+  llvm::Expected size = py_return.AsUnsignedLongLong();
+  // FIXME: Handle error.
+  if (!size)
+return llvm::None;
+
+  return *size;
 }
 
 lldb::MemoryRegionInfoSP
@@ -280,15 +290,17 @@ StructuredData::DictionarySP 
ScriptedProcessPythonInterface::GetLoadedImages() {
 }
 
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
-
-  return (pid >= std::numeric_limits::max())
- ? LLDB_INVALID_PROCESS_ID
- : pid;
+  llvm::Optional pid = GetGenericInteger("get_process_id");
+  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;
 }
 
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  llvm::Optional is_alive = GetGenericInteger("is_alive");
+
+  if (!is_alive)
+return false;
+
+  return static_cast(*is_alive);
 }
 
 #endif

diff  --git 
a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h 
b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
index 5d53462df6f2f..1b5f347b97182 100644
--- 
a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
+++ 
b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
@@ -51,7 +51,7 @@ class ScriptedProcessPythonInterface : public 
ScriptedProcessInterface {
   bool IsAlive() override;
 
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  llvm::Optional GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:



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

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Med Ismail Bennani 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 rG3d4cadfb2643: [lldb/Interpreter] Conform 
ScriptedProcessPythonInterface to SWIG python types (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

Files:
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
@@ -51,7 +51,7 @@
   bool IsAlive() override;
 
 protected:
-  size_t GetGenericInteger(llvm::StringRef method_name);
+  llvm::Optional GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
@@ -63,7 +63,13 @@
 }
 
 bool ScriptedProcessPythonInterface::ShouldStop() {
-  return GetGenericInteger("shuold_stop");
+  llvm::Optional should_stop =
+  GetGenericInteger("should_stop");
+
+  if (!should_stop)
+return false;
+
+  return static_cast(*should_stop);
 }
 
 Status ScriptedProcessPythonInterface::Stop() {
@@ -134,21 +140,21 @@
   return Status("Returned object is null.");
 }
 
-size_t
+llvm::Optional
 ScriptedProcessPythonInterface::GetGenericInteger(llvm::StringRef method_name) {
   Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
  Locker::FreeLock);
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (!m_object_instance_sp)
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   PythonObject implementor(PyRefType::Borrowed,
(PyObject *)m_object_instance_sp->GetValue());
 
   if (!implementor.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   PythonObject pmeth(
   PyRefType::Owned,
@@ -158,12 +164,12 @@
 PyErr_Clear();
 
   if (!pmeth.IsAllocated())
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
 
   if (PyCallable_Check(pmeth.get()) == 0) {
 if (PyErr_Occurred())
   PyErr_Clear();
-return LLDB_INVALID_ADDRESS;
+return llvm::None;
   }
 
   if (PyErr_Occurred())
@@ -179,11 +185,15 @@
 PyErr_Clear();
   }
 
-  if (py_return.get()) {
-auto size = py_return.AsUnsignedLongLong();
-return (size) ? *size : LLDB_INVALID_ADDRESS;
-  }
-  return LLDB_INVALID_ADDRESS;
+  if (!py_return.get())
+return llvm::None;
+
+  llvm::Expected size = py_return.AsUnsignedLongLong();
+  // FIXME: Handle error.
+  if (!size)
+return llvm::None;
+
+  return *size;
 }
 
 lldb::MemoryRegionInfoSP
@@ -280,15 +290,17 @@
 }
 
 lldb::pid_t ScriptedProcessPythonInterface::GetProcessID() {
-  size_t pid = GetGenericInteger("get_process_id");
-
-  return (pid >= std::numeric_limits::max())
- ? LLDB_INVALID_PROCESS_ID
- : pid;
+  llvm::Optional pid = GetGenericInteger("get_process_id");
+  return (!pid) ? LLDB_INVALID_PROCESS_ID : *pid;
 }
 
 bool ScriptedProcessPythonInterface::IsAlive() {
-  return GetGenericInteger("is_alive");
+  llvm::Optional is_alive = GetGenericInteger("is_alive");
+
+  if (!is_alive)
+return false;
+
+  return static_cast(*is_alive);
 }
 
 #endif
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 254c4d1 - [lldb] Fix build failure introduced by 3d4cadfb26437bd686ca8177f5454a366fed59eb

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2021-07-22T20:54:27Z
New Revision: 254c4d174ea3bd3601818b003fc169cdedf24fb9

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

LOG: [lldb] Fix build failure introduced by 
3d4cadfb26437bd686ca8177f5454a366fed59eb

This patch updates the `ScriptedProcess::GetGenericInteger` return type
to `llvm::Optional` to match implementation.

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

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

Removed: 




diff  --git 
a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h 
b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
index 1b5f347b9718..30cb5a882af2 100644
--- 
a/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
+++ 
b/lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
@@ -51,7 +51,8 @@ class ScriptedProcessPythonInterface : public 
ScriptedProcessInterface {
   bool IsAlive() override;
 
 protected:
-  llvm::Optional GetGenericInteger(llvm::StringRef method_name);
+  llvm::Optional
+  GetGenericInteger(llvm::StringRef method_name);
   Status GetStatusFromMethod(llvm::StringRef method_name);
 
 private:



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

[Lldb-commits] [PATCH] D105788: [LLDB] Silence warnings from ScriptedProcessPythonInterface.cpp

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

Hi @omjavaid , please let me know if you're still seeing the warnings. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105788

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

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 marked 39 inline comments as done.
jj10306 added inline comments.



Comment at: lldb/source/Target/TraceHTR.cpp:53-57
+std::vector const &TraceHTR::GetBlockLayers() const {
+  return m_block_layers;
+}
+
+HTRInstructionLayer const &TraceHTR::GetInstructionLayer() const {

wallace wrote:
> wallace wrote:
> > as these layers could change because of merges and what not, better remove 
> > the consts out of these methods
> remove a llvm::ArrayRef
Layers are not mutated after being constructed by a pass, so there is currently 
no reason to expose mutable references to these structures.



Comment at: lldb/source/Target/TraceHTR.cpp:151
+  while (valid_cursor) {
+// TODO: how should we handle cursor errors in this loop?
+lldb::addr_t current_instruction_load_address = cursor->GetLoadAddress();

wallace wrote:
> jj10306 wrote:
> > wallace wrote:
> > > you need to enhance the Instruction object, to support errors. You can 
> > > store the error string in it using a char * pointer from a ConstString. 
> > > Errors are not frequent, so that should be fine. You need to display the 
> > > errors in the export data as well, as hiding them could be misleading to 
> > > the user
> > What Instruction object are you referring to?
> > Given the way the InstructionLayer currently works, vector of load 
> > addresses and a map for metadata, how would you suggest incorporating the 
> > error information?
> > My first thought was to store a special value (such as 0) in the vector of 
> > load addresses for each error, but this seems a bit hacky and doesn't allow 
> > a way to map a specific error back to its error message. 
> > What are your thoughts?
> If in CTR you can embed error messages, it would be nice to have that in the 
> metadata. It's also valid to have an instruction address of 0 in the case of 
> errors. However, you have end up having abnormal blocks like
>insn1 insn2 error_kind1 insn1 insn2 error_kind2
> 
> so if you just look at the addresses, [insn1, insn2, 0] is a block that 
> appears twice.
> 
> Now the question is: if your users will want to see the specific errors, then 
> you'll have to handle it accordingly in the metadata. But if your users don't 
> case what kind of errors are displayed in the chrome trace view, then 
> replacing an error with address 0 makes sense. You could leave a TODO here in 
> that case. Let's sync up offline anyway
Discussed offline - decided to replace errors with an address of 0 for the time 
being. If decoding errors are common/users want to know the specifics of the 
error, we can add logic to store the error's message in a future diff.


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

https://reviews.llvm.org/D105741

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

[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 updated this revision to Diff 360965.
jj10306 marked 2 inline comments as done.
jj10306 added a comment.

Address comments:

- use unique_ptr to prevent unnecessary copying
- add support for trace decoding errors


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

https://reviews.llvm.org/D105741

Files:
  lldb/docs/htr.rst
  lldb/docs/media/basic_super_block_pass.png
  lldb/docs/media/htr_example.png
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceHTR.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceHTR.cpp
  lldb/test/API/commands/trace/TestTraceExport.py

Index: lldb/test/API/commands/trace/TestTraceExport.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceExport.py
@@ -0,0 +1,60 @@
+import lldb
+from intelpt_testcase import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+import os
+
+class TestTraceExport(TraceIntelPTTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def testErrorMessages(self):
+ctf_test_file = self.getBuildArtifact("ctf-test.json")
+# We first check the output when there are no targets
+self.expect(f"thread trace export --format ctf --outfile {ctf_test_file}",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect(f"thread trace export --format ctf --outfile {ctf_test_file}",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect(f"thread trace export --format ctf --outfile {ctf_test_file}",
+substrs=["error: Process is not being traced"],
+error=True)
+
+def testExportChromeTraceFormat(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+ctf_test_file = self.getBuildArtifact("ctf-test.json")
+
+# file name, no count
+if os.path.exists(ctf_test_file):
+remove_file(ctf_test_file)
+self.expect(f"thread trace export --format ctf --outfile {ctf_test_file}",
+substrs=["Success", f"{ctf_test_file}"])
+self.assertTrue(os.path.exists(ctf_test_file))
+
+def testInvalidExportFormat(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+ctf_test_file = self.getBuildArtifact("ctf-test.json")
+unknown_format = "DEADBEEF"
+
+self.expect(f"thread trace export --format {unknown_format} --outfile {ctf_test_file}",
+substrs=[f"error: unknown format '{unknown_format}' specified."],
+error=True)
+
Index: lldb/source/Target/TraceHTR.cpp
===
--- /dev/null
+++ lldb/source/Target/TraceHTR.cpp
@@ -0,0 +1,485 @@
+//===-- TraceHTR.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/TraceHTR.h"
+
+#include "lldb/Symbol/Function.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+#include "llvm/Support/JSON.h"
+#include 
+#include 
+
+using namespace lldb_private;
+using namespace lldb;
+
+size_t HTRBlockMetadata::GetNumInstructions() const {
+  return m_num_instructions;
+}
+
+llvm::Optional
+HTRBlockMetadata::GetMostFrequentlyCalledFunction() const {
+  size_t max_ncalls = 0;
+  llvm::Optional max_name = llvm::None;
+  for (const auto &[name, ncalls] : m_func_calls) {
+if (ncalls > max_ncalls) {
+  max_ncalls = ncalls;
+  max_name = name.AsCString();
+}
+  }
+  return max_name;
+}
+
+llvm::DenseMap const &
+HTRBlockMetadata::GetFunctionCalls() const {
+  return m_func_calls;
+}
+
+lldb::addr_t HTRBlockMetadata::GetFirstInstructionLoadAddress() const {
+  return m_first_instruction_load_address;
+}
+
+size_t HTRBlock::GetOffset() const { return m_offset; }
+
+size_t HTRBlock::GetSize() const { return m_size; }
+
+HTRBlockMetadata const &HTR

[Lldb-commits] [lldb] bcce8e0 - Fix the logic so stop-hooks get run after a breakpoint that ran an expression

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2021-07-22T15:06:41-07:00
New Revision: bcce8e0fccc1d6c2f18ade0ffa7039fb705bade2

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

LOG: Fix the logic so stop-hooks get run after a breakpoint that ran an 
expression

Code was added to Target::RunStopHook to make sure that we don't run stop hooks 
when
you stop after an expression evaluation. But the way it was done was to check 
that we
hadn't run an expression since the last natural stop. That failed in the case 
where you
stopped for a breakpoint which had run an expression, because the stop-hooks 
get run
after the breakpoint actions, and so by the time we got to running the 
stop-hooks,
we had already run a user expression.

I fixed this by adding a target ivar tracking the last natural stop ID at which 
we had
run a stop-hook. Then we keep track of this and make sure we run the stop-hooks 
only
once per natural stop.

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

Added: 


Modified: 
lldb/include/lldb/Target/Target.h
lldb/source/Target/Target.cpp
lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
lldb/test/API/commands/target/stop-hooks/TestStopHooks.py
lldb/test/API/commands/target/stop-hooks/main.c

Removed: 




diff  --git a/lldb/include/lldb/Target/Target.h 
b/lldb/include/lldb/Target/Target.h
index 0db5209cd1f35..ac8d002b09a12 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1429,8 +1429,10 @@ class Target : public 
std::enable_shared_from_this,
   typedef std::map StopHookCollection;
   StopHookCollection m_stop_hooks;
   lldb::user_id_t m_stop_hook_next_id;
+  uint32_t m_latest_stop_hook_id; /// This records the last natural stop at
+  /// which we ran a stop-hook.
   bool m_valid;
-  bool m_suppress_stop_hooks;
+  bool m_suppress_stop_hooks; /// Used to not run stop hooks for expressions
   bool m_is_dummy_target;
   unsigned m_next_persistent_variable_index = 0;
   /// An optional \a lldb_private::Trace object containing processor trace

diff  --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 396f77dfedb83..1f8e8c54fa9e1 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -95,6 +95,7 @@ Target::Target(Debugger &debugger, const ArchSpec 
&target_arch,
   m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
   m_image_search_paths(ImageSearchPathsChanged, this),
   m_source_manager_up(), m_stop_hooks(), m_stop_hook_next_id(0),
+  m_latest_stop_hook_id(0),
   m_valid(true), m_suppress_stop_hooks(false),
   m_is_dummy_target(is_dummy_target),
   m_frame_recognizer_manager_up(
@@ -181,6 +182,7 @@ void Target::CleanupProcess() {
   DisableAllWatchpoints(false);
   ClearAllWatchpointHitCounts();
   ClearAllWatchpointHistoricValues();
+  m_latest_stop_hook_id = 0;
 }
 
 void Target::DeleteCurrentProcess() {
@@ -2609,12 +2611,6 @@ bool Target::RunStopHooks() {
   if (m_process_sp->GetState() != eStateStopped)
 return false;
 
-  //  make sure we check that we are not stopped
-  // because of us running a user expression since in that case we do not want
-  // to run the stop-hooks
-  if (m_process_sp->GetModIDRef().IsLastResumeForUserExpression())
-return false;
-
   if (m_stop_hooks.empty())
 return false;
 
@@ -2629,6 +2625,18 @@ bool Target::RunStopHooks() {
   if (!any_active_hooks)
 return false;
 
+  //  make sure we check that we are not stopped
+  // because of us running a user expression since in that case we do not want
+  // to run the stop-hooks.  Note, you can't just check whether the last stop
+  // was for a User Expression, because breakpoint commands get run before
+  // stop hooks, and one of them might have run an expression.  You have
+  // to ensure you run the stop hooks once per natural stop.
+  uint32_t last_natural_stop = 
m_process_sp->GetModIDRef().GetLastNaturalStopID();
+  if (last_natural_stop != 0 && m_latest_stop_hook_id == last_natural_stop)
+return false;
+
+  m_latest_stop_hook_id = last_natural_stop;
+
   std::vector exc_ctx_with_reasons;
 
   ThreadList &cur_threadlist = m_process_sp->GetThreadList();

diff  --git a/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py 
b/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
index 7ef5a72b9f6fa..53c2c2e07a332 100644
--- a/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
+++ b/lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
@@ -96,12 +96,12 @@ def do_test_auto_continue(self, return_true):
 # Now set the breakpoint on step_out_of_me, and make sure we run the
 # expression, then continue back to main.
 bkpt = target

[Lldb-commits] [PATCH] D106514: Fix the logic so stop-hooks get run after a breakpoint that ran an expression

[Jim Ingham 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 rGbcce8e0fccc1: Fix the logic so stop-hooks get run after a 
breakpoint that ran an expression (authored by jingham).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D106514?vs=360661&id=360990#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106514

Files:
  lldb/include/lldb/Target/Target.h
  lldb/source/Target/Target.cpp
  lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
  lldb/test/API/commands/target/stop-hooks/TestStopHooks.py
  lldb/test/API/commands/target/stop-hooks/main.c

Index: lldb/test/API/commands/target/stop-hooks/main.c
===
--- lldb/test/API/commands/target/stop-hooks/main.c
+++ lldb/test/API/commands/target/stop-hooks/main.c
@@ -7,9 +7,15 @@
   return g_var; // Set a breakpoint here and step out.
 }
 
+void
+increment_gvar() {
+  g_var++;
+}
+
 int
 main()
 {
   int result = step_out_of_me(); // Stop here first
+  increment_gvar(); // Continue to here
   return result;
 }
Index: lldb/test/API/commands/target/stop-hooks/TestStopHooks.py
===
--- lldb/test/API/commands/target/stop-hooks/TestStopHooks.py
+++ lldb/test/API/commands/target/stop-hooks/TestStopHooks.py
@@ -29,6 +29,11 @@
 """Test that stop hooks fire on step-out."""
 self.step_out_test()
 
+def test_stop_hooks_after_expr(self):
+"""Test that a stop hook fires when hitting a breakpoint
+   that runs an expression"""
+self.after_expr_test()
+
 def step_out_test(self):
 (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
"Set a breakpoint here", self.main_source_file)
@@ -42,3 +47,44 @@
 self.assertTrue(var.IsValid())
 self.assertEqual(var.GetValueAsUnsigned(), 1, "Updated g_var")
 
+def after_expr_test(self):
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+interp.HandleCommand("target stop-hook add -o 'expr g_var++'", result)
+self.assertTrue(result.Succeeded, "Set the target stop hook")
+
+(target, process, thread, first_bkpt) = lldbutil.run_to_source_breakpoint(self,
+   "Set a breakpoint here", self.main_source_file)
+
+var = target.FindFirstGlobalVariable("g_var")
+self.assertTrue(var.IsValid())
+self.assertEqual(var.GetValueAsUnsigned(), 1, "Updated g_var")
+
+bkpt = target.BreakpointCreateBySourceRegex("Continue to here", self.main_source_file)
+self.assertNotEqual(bkpt.GetNumLocations(), 0, "Set the second breakpoint")
+commands = lldb.SBStringList()
+commands.AppendString("expr increment_gvar()")
+bkpt.SetCommandLineCommands(commands);
+
+threads = lldbutil.continue_to_breakpoint(process, bkpt)
+self.assertEqual(len(threads), 1, "Hit my breakpoint")
+
+self.assertTrue(var.IsValid())
+self.assertEqual(var.GetValueAsUnsigned(), 3, "Updated g_var")
+
+# Make sure running an expression does NOT run the stop hook.
+# Our expression will increment it by one, but the stop shouldn't
+# have gotten it to 5.
+threads[0].frames[0].EvaluateExpression("increment_gvar()")
+self.assertTrue(var.IsValid())
+self.assertEqual(var.GetValueAsUnsigned(), 4, "Updated g_var")
+
+
+# Make sure a rerun doesn't upset the state we've set up:
+process.Kill()
+lldbutil.run_to_breakpoint_do_run(self, target, first_bkpt)
+var = target.FindFirstGlobalVariable("g_var")
+self.assertTrue(var.IsValid())
+self.assertEqual(var.GetValueAsUnsigned(), 1, "Updated g_var")
+
+
Index: lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
===
--- lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
+++ lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
@@ -96,12 +96,12 @@
 # Now set the breakpoint on step_out_of_me, and make sure we run the
 # expression, then continue back to main.
 bkpt = target.BreakpointCreateBySourceRegex("Set a breakpoint here and step out", self.main_source_file)
-self.assertTrue(bkpt.GetNumLocations() > 0, "Got breakpoints in step_out_of_me")
+self.assertNotEqual(bkpt.GetNumLocations(), 0, "Got breakpoints in step_out_of_me")
 process.Continue()
 
 var = target.FindFirstGlobalVariable("g_var")
 self.assertTrue(var.IsValid())
-self.assertEqual(var.GetValueAsUnsigned

[Lldb-commits] [PATCH] D106584: [lldb] Improve checking of file cache read eligibility for mach-O

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Hm there's no phab action which says "reject approach" :) the perf impact of 
forcing all reads to be out of memory would be dramatically bad for a remote 
system debug session, this can't be done.  If there is a specific caller that 
must use live memory even for a section marked "read-only", it needs to use 
force_live_memory=true in its call to ReadMemory.  This change would make app 
launch time for iOS apps etc probably take 30-60 seconds longer than they do 
right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106584

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

[Lldb-commits] [PATCH] D106584: [lldb] Improve checking of file cache read eligibility for mach-O

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I should add — I also don’t think this will fix the bug you’re working on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106584

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

[Lldb-commits] [PATCH] D106355: [DWARF5] Only fallback to manual index if no entry was found

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D106355#2896117 , @kimanh wrote:

> In our case we have some third-party libraries that were not built by us, and 
> therefore they don't have any name index. Our main focus, is however, not to 
> debug those third party libraries necessarily, but only our main code that we 
> are compiling. Given that the manual index is taking some time to be 
> generated, we could be lazy about generating it only if we need it.

I have a feeling during debugging you will hit the `ManualDWARFIndex::Index` 
soon during some other access anyway.

One possibility would be to disable loading DWARF for that file. There is 
`settings set symbols.enable-external-lookup false` (use by 
`Symbols::LocateExecutableSymbolFile`) but that works only for separate debug 
info in system directories like `/usr/lib/debug/`.

If you do not want to debug those libraries cannot you just strip debug info 
from them (`llvm-strip -g`)?

Wouldn't be best to generate `.debug_names` for those 3rd party libraries? GDB 
has `gdb-add-index` but its `.gdb_index` has not enough information to be 
useful for LLDB. Still LLDB could generate `.debug_names` out of its internal 
representation in `ManualDWARFIndex`. That would be useful also for debugging 
binaries from non-clang compilers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106355

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

[Lldb-commits] [PATCH] D106584: [lldb] Improve checking of file cache read eligibility for mach-O

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I do like the debug-lldb check that the file cache read matches the live memory 
part.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106584

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

[Lldb-commits] [PATCH] D106501: [trace] Add the definition of a TraceExporter plugin

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 361013.
wallace added a comment.
Herald added a subscriber: dang.

I've added a basic implementation of a CTF exporter plugin that can be used by 
https://reviews.llvm.org/D105741

  (lldb) help thread trace export ctf
  Export a given thread's trace to Chrome Trace Format
  
  Syntax: thread trace export ctf []
  
  Command Options Usage:
thread trace export ctf [-t ]
  
 -t  ( --tid  )
  Export the trace for the specified thread index. Otherwise, the 
currently selected thread
  will be used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106501

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/TraceExporter.h
  lldb/include/lldb/lldb-forward.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Core/PluginManager.cpp
  lldb/source/Plugins/CMakeLists.txt
  lldb/source/Plugins/TraceExporter/CMakeLists.txt
  lldb/source/Plugins/TraceExporter/ctf/CMakeLists.txt
  lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.cpp
  lldb/source/Plugins/TraceExporter/ctf/CommandObjectThreadTraceExportCTF.h
  lldb/source/Plugins/TraceExporter/ctf/TraceExporterCTF.cpp
  lldb/source/Plugins/TraceExporter/ctf/TraceExporterCTF.h
  lldb/source/Plugins/TraceExporter/ctf/TraceExporterCTFOptions.td
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/TraceExporter.cpp

Index: lldb/source/Target/TraceExporter.cpp
===
--- /dev/null
+++ lldb/source/Target/TraceExporter.cpp
@@ -0,0 +1,32 @@
+//===-- TraceExporter.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/TraceExporter.h"
+
+#include "lldb/Core/PluginManager.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace llvm;
+
+static Error createInvalidPlugInError(StringRef plugin_name) {
+  return createStringError(
+  std::errc::invalid_argument,
+  "no trace expoter plug-in matches the specified type: \"%s\"",
+  plugin_name.data());
+}
+
+Expected
+TraceExporter::FindPlugin(llvm::StringRef plugin_name) {
+  ConstString name(plugin_name);
+  if (auto create_callback =
+  PluginManager::GetTraceExporterCreateCallback(name))
+return create_callback();
+
+  return createInvalidPlugInError(plugin_name);
+}
Index: lldb/source/Target/CMakeLists.txt
===
--- lldb/source/Target/CMakeLists.txt
+++ lldb/source/Target/CMakeLists.txt
@@ -68,6 +68,7 @@
   ThreadSpec.cpp
   Trace.cpp
   TraceCursor.cpp
+  TraceExporter.cpp
   TraceInstructionDumper.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
Index: lldb/source/Plugins/TraceExporter/ctf/TraceExporterCTFOptions.td
===
--- /dev/null
+++ lldb/source/Plugins/TraceExporter/ctf/TraceExporterCTFOptions.td
@@ -0,0 +1,9 @@
+include "../../../../source/Commands/OptionsBase.td"
+
+let Command = "thread trace export ctf" in {
+  def thread_trace_export_ctf: Option<"tid", "t">,
+Group<1>,
+Arg<"ThreadIndex">,
+Desc<"Export the trace for the specified thread index. Otherwise, the "
+ "currently selected thread will be used.">;
+}
Index: lldb/source/Plugins/TraceExporter/ctf/TraceExporterCTF.h
===
--- /dev/null
+++ lldb/source/Plugins/TraceExporter/ctf/TraceExporterCTF.h
@@ -0,0 +1,42 @@
+//===-- TraceExporterCTF.h --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_SOURCE_PLUGINS_TRACE_EXPORTER_CTF_H
+#define LLDB_SOURCE_PLUGINS_TRACE_EXPORTER_CTF_H
+
+#include "lldb/Target/TraceExporter.h"
+
+namespace lldb_private {
+namespace ctf {
+
+/// Trace Exporter Plugin that can produce traces in Chrome Trace Format.
+/// Still in development.
+class TraceExporterCTF : public TraceExporter {
+public:
+  ~TraceExporterCTF() override = default;
+
+  /// PluginInterface protocol
+  /// \{
+  static llvm::Expected CreateInstance();
+
+  ConstString GetPluginName() override;
+
+  static void Initialize();
+
+  static void Terminate();
+
+  static ConstString GetPluginNameStatic();
+
+  uint32_t GetPluginVersion() override;
+  /// \}
+};
+
+} // namespace ctf
+} // na

[Lldb-commits] [PATCH] D106623: Fix a logic error in "break delete --disabled" when no "protected" breakpoints are specified.

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added a reviewer: JDevlieghere.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Fix "break delete --disabled" with no arguments.

  

The code that figured out which breakpoints to delete was supposed
to set the result status if it found breakpoints, and then the code
that actually deleted them checked that the result's status was set.

  

The code for "break delete --disabled" failed to set the status if
no "protected" breakpoints were provided.  This was a confusing way
to implement this, so I reworked it with early returns so it was less
error prone, and added a test case for the no arguments case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106623

Files:
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py

Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -313,3 +313,22 @@
 
 bp_3 = target.FindBreakpointByID(bp_id_3)
 self.assertTrue(bp_3.IsValid(), "DeleteMeNot didn't protect disabled breakpoint 3")
+
+# Reset the first breakpoint, disable it, and do this again with no protected name:
+bp_1 = target.BreakpointCreateByName("main")
+
+bp_1.SetEnabled(False)
+
+bp_id_1 = bp_1.GetID()
+
+self.runCmd("breakpoint delete --disabled")
+
+bp_1 = target.FindBreakpointByID(bp_id_1)
+self.assertFalse(bp_1.IsValid(), "Didn't delete disabled breakpoint 1")
+
+bp_2 = target.FindBreakpointByID(bp_id_2)
+self.assertTrue(bp_2.IsValid(), "Deleted enabled breakpoint 2")
+
+bp_3 = target.FindBreakpointByID(bp_id_3)
+self.assertFalse(bp_3.IsValid(), "Didn't delete disabled breakpoint 3")
+
Index: lldb/source/Commands/CommandObjectBreakpoint.cpp
===
--- lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -1458,6 +1458,7 @@
   return false;
 }
 
+// Handle the delete all breakpoints case:
 if (command.empty() && !m_options.m_delete_disabled) {
   if (!m_options.m_force &&
   !m_interpreter.Confirm(
@@ -1471,67 +1472,73 @@
 (uint64_t)num_breakpoints, num_breakpoints > 1 ? "s" : "");
   }
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
-} else {
-  // Particular breakpoint selected; disable that breakpoint.
-  BreakpointIDList valid_bp_ids;
-  
-  if (m_options.m_delete_disabled) {
-BreakpointIDList excluded_bp_ids;
+  return result.Succeeded();
+}
+ 
+// Either we have some kind of breakpoint specification(s),
+// or we are handling "break disable --deleted".  Gather the list
+// of breakpoints to delete here, the we'll delete them below.
+BreakpointIDList valid_bp_ids;
+
+if (m_options.m_delete_disabled) {
+  BreakpointIDList excluded_bp_ids;
 
-if (!command.empty()) {
-  CommandObjectMultiwordBreakpoint::VerifyBreakpointOrLocationIDs(
-  command, &target, result, &excluded_bp_ids,
-  BreakpointName::Permissions::PermissionKinds::deletePerm);
-}
-for (auto breakpoint_sp : breakpoints.Breakpoints()) {
-  if (!breakpoint_sp->IsEnabled() && breakpoint_sp->AllowDelete()) {
-BreakpointID bp_id(breakpoint_sp->GetID());
-size_t pos = 0;
-if (!excluded_bp_ids.FindBreakpointID(bp_id, &pos))
-  valid_bp_ids.AddBreakpointID(breakpoint_sp->GetID());
-  }
-}
-if (valid_bp_ids.GetSize() == 0) {
-  result.AppendError("No disabled breakpoints.");
-  return false;
-}
-  } else {
+  if (!command.empty()) {
 CommandObjectMultiwordBreakpoint::VerifyBreakpointOrLocationIDs(
-command, &target, result, &valid_bp_ids,
+command, &target, result, &excluded_bp_ids,
 BreakpointName::Permissions::PermissionKinds::deletePerm);
+if (!result.Succeeded())
+  return false;
   }
-  
-  if (result.Succeeded()) {
-int delete_count = 0;
-int disable_count = 0;
-const size_t count = valid_bp_ids.GetSize();
-for (size_t i = 0; i < count; ++i) {
-  BreakpointID cur_bp_id = valid_bp_ids.GetBreakpointIDAtIndex(i);
 
-  if (cur_bp_id.GetBreakpointID() != LLDB_INVALID_BREAK_ID) {
-if (cur_bp_id.GetLocationID() != LLDB_INVALID_BREAK_ID) {
-  Breakpoint *breakpoint =
-  target.GetBreakpointB