[Lldb-commits] [lldb] 160a504 - [lldb][NFC] Add 'breakpoint command list' test

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2019-12-02T11:57:55+01:00
New Revision: 160a5045c699ac523eac3c7a1984705c3e86720e

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

LOG: [lldb][NFC] Add 'breakpoint command list' test

The command has zero test coverage and I'll have to touch the
code formatting the output commands, so let's start by adding a
test for it.

Added: 

lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/TestBreakpointCommandList.py
lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/a.yaml

Modified: 


Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/TestBreakpointCommandList.py
 
b/lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/TestBreakpointCommandList.py
new file mode 100644
index ..f1a8656a73b5
--- /dev/null
+++ 
b/lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/TestBreakpointCommandList.py
@@ -0,0 +1,44 @@
+"""
+Test 'breakpoint command list'.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test_list_commands(self):
+src_dir = self.getSourceDir()
+yaml_path = os.path.join(src_dir, "a.yaml")
+yaml_base, ext = os.path.splitext(yaml_path)
+obj_path = self.getBuildArtifact("main.o")
+self.yaml2obj(yaml_path, obj_path)
+
+# Create a target with the object file we just created from YAML
+target = self.dbg.CreateTarget(obj_path)
+self.assertTrue(target, VALID_TARGET)
+
+# Test without any breakpoints.
+self.expect("breakpoint command list 1", error=True, substrs=["error: 
No breakpoints exist for which to list commands"])
+
+# Set a breakpoint
+self.runCmd("b foo")
+
+# Check list breakpoint commands for breakpoints that have no commands.
+self.expect("breakpoint command list 1", startstr="Breakpoint 1 does 
not have an associated command.")
+
+# Add a breakpoint command.
+self.runCmd("breakpoint command add -o 'source list' 1")
+
+# List breakpoint command that we just created.
+self.expect("breakpoint command list 1", startstr="""Breakpoint 1:
+Breakpoint commands:
+  source list
+""")
+
+# List breakpoint command with invalid breakpoint ID.
+self.expect("breakpoint command list 2", error=True, startstr="error: 
'2' is not a currently valid breakpoint ID.")

diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/a.yaml 
b/lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/a.yaml
new file mode 100644
index ..1007f60c19ee
--- /dev/null
+++ 
b/lldb/packages/Python/lldbsuite/test/commands/breakpoint/command/list/a.yaml
@@ -0,0 +1,18 @@
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:ELFDATA2LSB
+  Type:ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:.text
+Type:SHT_PROGBITS
+Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+AddressAlign:0x0010
+Content: 554889E5897DFC5DC3
+Symbols:
+  - Name:foo
+Type:STT_FUNC
+Section: .text
+Size:0x0009
+...



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

[Lldb-commits] [lldb] f8fb372 - [lldb][NFC] Make Stream's IndentLevel an unsigned integers.

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2019-12-02T13:01:26+01:00
New Revision: f8fb3729e9d794f174aa737351235f76e6ac46db

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

LOG: [lldb][NFC] Make Stream's IndentLevel an unsigned integers.

We expect it to be always positive values and LLVM/Clang's IndentLevel
values are already unsigned integers, so we should do the same.

Added: 


Modified: 
lldb/include/lldb/Utility/Stream.h
lldb/source/Target/Target.cpp
lldb/source/Utility/Stream.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Stream.h 
b/lldb/include/lldb/Utility/Stream.h
index 88cdb88d77ad..9982d8236a65 100644
--- a/lldb/include/lldb/Utility/Stream.h
+++ b/lldb/include/lldb/Utility/Stream.h
@@ -338,8 +338,8 @@ class Stream {
   /// Get the current indentation level.
   ///
   /// \return
-  /// The current indentation level as an integer.
-  int GetIndentLevel() const;
+  /// The current indentation level.
+  unsigned GetIndentLevel() const;
 
   /// Indent the current line in the stream.
   ///
@@ -353,10 +353,10 @@ class Stream {
   size_t Indent(llvm::StringRef s);
 
   /// Decrement the current indentation level.
-  void IndentLess(int amount = 2);
+  void IndentLess(unsigned amount = 2);
 
   /// Increment the current indentation level.
-  void IndentMore(int amount = 2);
+  void IndentMore(unsigned amount = 2);
 
   /// Output an offset value.
   ///
@@ -411,7 +411,7 @@ class Stream {
   ///
   /// \param[in] level
   /// The new indentation level.
-  void SetIndentLevel(int level);
+  void SetIndentLevel(unsigned level);
 
   /// Output a SLEB128 number to the stream.
   ///
@@ -442,7 +442,7 @@ class Stream {
   uint32_t m_addr_size; ///< Size of an address in bytes.
   lldb::ByteOrder
   m_byte_order;   ///< Byte order to use when encoding scalar types.
-  int m_indent_level; ///< Indention level.
+  unsigned m_indent_level; ///< Indention level.
   std::size_t m_bytes_written = 0; ///< Number of bytes written so far.
 
   void _PutHex8(uint8_t uvalue, bool add_prefix);

diff  --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 4b9a1b77ad16..aeb77b7cf676 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -3177,7 +3177,7 @@ void Target::StopHook::SetThreadSpecifier(ThreadSpec 
*specifier) {
 
 void Target::StopHook::GetDescription(Stream *s,
   lldb::DescriptionLevel level) const {
-  int indent_level = s->GetIndentLevel();
+  unsigned indent_level = s->GetIndentLevel();
 
   s->SetIndentLevel(indent_level + 2);
 

diff  --git a/lldb/source/Utility/Stream.cpp b/lldb/source/Utility/Stream.cpp
index 991f7e924d8d..119d1e0f7964 100644
--- a/lldb/source/Utility/Stream.cpp
+++ b/lldb/source/Utility/Stream.cpp
@@ -185,16 +185,18 @@ Stream &Stream::operator<<(int64_t sval) {
 }
 
 // Get the current indentation level
-int Stream::GetIndentLevel() const { return m_indent_level; }
+unsigned Stream::GetIndentLevel() const { return m_indent_level; }
 
 // Set the current indentation level
-void Stream::SetIndentLevel(int indent_level) { m_indent_level = indent_level; 
}
+void Stream::SetIndentLevel(unsigned indent_level) {
+  m_indent_level = indent_level;
+}
 
 // Increment the current indentation level
-void Stream::IndentMore(int amount) { m_indent_level += amount; }
+void Stream::IndentMore(unsigned amount) { m_indent_level += amount; }
 
 // Decrement the current indentation level
-void Stream::IndentLess(int amount) {
+void Stream::IndentLess(unsigned amount) {
   if (m_indent_level >= amount)
 m_indent_level -= amount;
   else



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

[Lldb-commits] [lldb] 4f728bf - [lldb][NFC] Use raw_ostream instead of Stream in Baton::GetDescription

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2019-12-02T13:27:21+01:00
New Revision: 4f728bfc13c45bc744bfdbfc3086bed74a8cbb4c

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

LOG: [lldb][NFC] Use raw_ostream instead of Stream in Baton::GetDescription

Removing raw_ostream here is getting us closer to removing LLDB's Stream
class.

Added: 


Modified: 
lldb/include/lldb/Breakpoint/BreakpointOptions.h
lldb/include/lldb/Breakpoint/WatchpointOptions.h
lldb/include/lldb/Utility/Baton.h
lldb/source/Breakpoint/BreakpointOptions.cpp
lldb/source/Breakpoint/WatchpointOptions.cpp
lldb/source/Commands/CommandObjectBreakpointCommand.cpp
lldb/source/Commands/CommandObjectWatchpointCommand.cpp
lldb/source/Utility/Baton.cpp

Removed: 




diff  --git a/lldb/include/lldb/Breakpoint/BreakpointOptions.h 
b/lldb/include/lldb/Breakpoint/BreakpointOptions.h
index 9e02afff5227..2c52170eb9f6 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointOptions.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointOptions.h
@@ -88,7 +88,8 @@ friend class Breakpoint;
 explicit CommandBaton(std::unique_ptr Data)
 : TypedBaton(std::move(Data)) {}
 
-void GetDescription(Stream *s, lldb::DescriptionLevel level) const 
override;
+void GetDescription(llvm::raw_ostream &s, lldb::DescriptionLevel level,
+unsigned indentation) const override;
   };
 
   typedef std::shared_ptr CommandBatonSP;

diff  --git a/lldb/include/lldb/Breakpoint/WatchpointOptions.h 
b/lldb/include/lldb/Breakpoint/WatchpointOptions.h
index b395dde21901..0dc34d4ebef7 100644
--- a/lldb/include/lldb/Breakpoint/WatchpointOptions.h
+++ b/lldb/include/lldb/Breakpoint/WatchpointOptions.h
@@ -180,7 +180,8 @@ class WatchpointOptions {
 CommandBaton(std::unique_ptr Data)
 : TypedBaton(std::move(Data)) {}
 
-void GetDescription(Stream *s, lldb::DescriptionLevel level) const 
override;
+void GetDescription(llvm::raw_ostream &s, lldb::DescriptionLevel level,
+unsigned indentation) const override;
   };
 
 protected:

diff  --git a/lldb/include/lldb/Utility/Baton.h 
b/lldb/include/lldb/Utility/Baton.h
index 4050f2af2bf0..c42867489c65 100644
--- a/lldb/include/lldb/Utility/Baton.h
+++ b/lldb/include/lldb/Utility/Baton.h
@@ -12,6 +12,8 @@
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-public.h"
 
+#include "llvm/Support/raw_ostream.h"
+
 #include 
 
 namespace lldb_private {
@@ -37,8 +39,9 @@ class Baton {
 
   virtual void *data() = 0;
 
-  virtual void GetDescription(Stream *s,
-  lldb::DescriptionLevel level) const = 0;
+  virtual void GetDescription(llvm::raw_ostream &s,
+  lldb::DescriptionLevel level,
+  unsigned indentation) const = 0;
 };
 
 class UntypedBaton : public Baton {
@@ -50,7 +53,8 @@ class UntypedBaton : public Baton {
   }
 
   void *data() override { return m_data; }
-  void GetDescription(Stream *s, lldb::DescriptionLevel level) const override;
+  void GetDescription(llvm::raw_ostream &s, lldb::DescriptionLevel level,
+  unsigned indentation) const override;
 
   void *m_data; // Leave baton public for easy access
 };
@@ -63,7 +67,8 @@ template  class TypedBaton : public Baton {
   const T *getItem() const { return Item.get(); }
 
   void *data() override { return Item.get(); }
-  void GetDescription(Stream *s, lldb::DescriptionLevel level) const override 
{}
+  void GetDescription(llvm::raw_ostream &s, lldb::DescriptionLevel level,
+  unsigned indentation) const override {}
 
 protected:
   std::unique_ptr Item;

diff  --git a/lldb/source/Breakpoint/BreakpointOptions.cpp 
b/lldb/source/Breakpoint/BreakpointOptions.cpp
index 0d4c6173c3c5..8fd16f420c04 100644
--- a/lldb/source/Breakpoint/BreakpointOptions.cpp
+++ b/lldb/source/Breakpoint/BreakpointOptions.cpp
@@ -566,7 +566,8 @@ void BreakpointOptions::GetDescription(Stream *s,
   if (m_callback_baton_sp.get()) {
 if (level != eDescriptionLevelBrief) {
   s->EOL();
-  m_callback_baton_sp->GetDescription(s, level);
+  m_callback_baton_sp->GetDescription(s->AsRawOstream(), level,
+  s->GetIndentLevel());
 }
   }
   if (!m_condition_text.empty()) {
@@ -578,35 +579,33 @@ void BreakpointOptions::GetDescription(Stream *s,
 }
 
 void BreakpointOptions::CommandBaton::GetDescription(
-Stream *s, lldb::DescriptionLevel level) const {
+llvm::raw_ostream &s, lldb::DescriptionLevel level,
+unsigned indentation) const {
   const CommandData *data = getItem();
 
   if (level == eDescriptionLevelBrief) {
-s->Printf(", commands = %s",
-  (data && data->user_source.GetSize() > 0) ? "yes" : "no");
+  

[Lldb-commits] [lldb] d62026e - [lldb][NFC] Don't calculate member indices in DWARFASTParserClang::ParseChildMembers

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2019-12-02T14:43:40+01:00
New Revision: d62026e2dde1d27c7d1c702f11b0464e1d470d4f

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

LOG: [lldb][NFC] Don't calculate member indices in 
DWARFASTParserClang::ParseChildMembers

We keep counting members and then don't do anything with the computed result.

Added: 


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 43030c62cb40..ca1db03b02fa 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2465,7 +2465,6 @@ bool DWARFASTParserClang::ParseChildMembers(
   const uint64_t parent_bit_size =
   parent_byte_size == UINT64_MAX ? UINT64_MAX : parent_byte_size * 8;
 
-  uint32_t member_idx = 0;
   BitfieldInfo last_field_info;
 
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
@@ -2935,7 +2934,6 @@ bool DWARFASTParserClang::ParseChildMembers(
   }
 }
   }
-  ++member_idx;
 } break;
 
 case DW_TAG_subprogram:



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Pavel Labath via Phabricator via lldb-commits]

labath added reviewers: JDevlieghere, aprantl.
labath added a subscriber: dblaikie.
labath added a comment.

Throwing some additional dwarf people in here...

In D70840#1763750 , @mstorsjo wrote:

> In D70840#1763705 , @mstorsjo wrote:
>
> > In D70840#1763639 , @labath wrote:
> >
> > > Yeah, this is going to be tricky... I don't really know what's the right 
> > > way to do this, but here are the thoughts I have on this so far:
> > >
> > > - The new DWARFDebugInfoEntry member is going to substantially increase 
> > > our memory footprint -- that's a non-starter
> >
> >
> > Ok, I feared that class was one where the size is critical yeah...
> >
> > > - I don't like the inconsistency where all addresses are demangled in the 
> > > DWARF code, except the line tables, which are in the "generic" code
> >
> > Yup. The reason for doing it like this was as I tried to find the most 
> > narrow interface where I could catch all of them.
>
>
> Is there any corresponding place in generic code where one could do the same 
> operation on the addresses that comes from pc_lo/pc_hi ranges from 
> .debug_info and .debug_aranges (not familiar with this section and when it's 
> generated etc), if that would end up touching fewer places? The existing 
> predecent in DWARFCallFrameInfo::GetFDEIndex is dwarf specific, but in 
> generic code outside of the dwarf symbolfile plugin.


debug_aranges is a sort of a lookup table for speeding up address->compile unit 
searches. llvm does not generate it by default since, and I think the reason is 
that you can usually get the same kind of data from the DW_AT_ranges attribute 
of the compile unit. I don't think you would be able to handle that in generic 
code, as debug_aranges does not make it's way into generic code.

Overall, I think this needs to be handled in DWARF code. That may even make 
sense, since PDB may not suffer from the same problem (does it?)

TBH, the `m_addr_mask` member issue is not that hard to resolve -- all 
DWARFDebugInfoEntry functions get a DWARFUnit argument (otherwise they wouldn't 
be able to do anything). Theoretically, we could store the mask there.

However, the question on my mind is what does that say about the llvm<->lldb 
dwarf parser convergence (one of our long term goals is to delete the lldb 
dwarf parser altogether, leaving just the high level lldb-specific stuff). 
Adding mask handling code low into the dwarf parser would go against that goal, 
as there is no equivalent llvm functionality.

One possible solution would be to try to add equivalent llvm functionality -- 
it sounds like something like this is needed there too, as all llvm tools (I 
don't know if there's anything besides llvm-symbolizer) will probably not work 
without it. (If they do, it would be interesting to figure out how they manage 
that.)

Another possibility might be to implement this in the higher levels of the 
dwarf code (the parts that are likely to stay in lldb forever)...


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [PATCH] D69273: ValueObject: Fix a crash related to children address type computation

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This seems to have fizzled out without any sort of a conclusion.. Did you guys 
do anything about the crashes you were seeing on the swift side?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69273



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

[Lldb-commits] [PATCH] D69273: ValueObject: Fix a crash related to children address type computation

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

We've been off all the past week. I'll circle back with Jim about this once I 
get to the office.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69273



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a subscriber: rnk.
dblaikie added a comment.

In D70840#1765219 , @labath wrote:

> Throwing some additional dwarf people in here...
>
> In D70840#1763750 , @mstorsjo wrote:
>
> > In D70840#1763705 , @mstorsjo 
> > wrote:
> >
> > > In D70840#1763639 , @labath 
> > > wrote:
> > >
> > > > Yeah, this is going to be tricky... I don't really know what's the 
> > > > right way to do this, but here are the thoughts I have on this so far:
> > > >
> > > > - The new DWARFDebugInfoEntry member is going to substantially increase 
> > > > our memory footprint -- that's a non-starter
> > >
> > >
> > > Ok, I feared that class was one where the size is critical yeah...
> > >
> > > > - I don't like the inconsistency where all addresses are demangled in 
> > > > the DWARF code, except the line tables, which are in the "generic" code
> > >
> > > Yup. The reason for doing it like this was as I tried to find the most 
> > > narrow interface where I could catch all of them.
> >
> >
> > Is there any corresponding place in generic code where one could do the 
> > same operation on the addresses that comes from pc_lo/pc_hi ranges from 
> > .debug_info and .debug_aranges (not familiar with this section and when 
> > it's generated etc), if that would end up touching fewer places? The 
> > existing predecent in DWARFCallFrameInfo::GetFDEIndex is dwarf specific, 
> > but in generic code outside of the dwarf symbolfile plugin.
>
>
> debug_aranges is a sort of a lookup table for speeding up address->compile 
> unit searches. llvm does not generate it by default since, and I think the 
> reason is that you can usually get the same kind of data from the 
> DW_AT_ranges attribute of the compile unit. I don't think you would be able 
> to handle that in generic code, as debug_aranges does not make it's way into 
> generic code.
>
> Overall, I think this needs to be handled in DWARF code. That may even make 
> sense, since PDB may not suffer from the same problem (does it?)
>
> TBH, the `m_addr_mask` member issue is not that hard to resolve -- all 
> DWARFDebugInfoEntry functions get a DWARFUnit argument (otherwise they 
> wouldn't be able to do anything). Theoretically, we could store the mask 
> there.
>
> However, the question on my mind is what does that say about the llvm<->lldb 
> dwarf parser convergence (one of our long term goals is to delete the lldb 
> dwarf parser altogether, leaving just the high level lldb-specific stuff). 
> Adding mask handling code low into the dwarf parser would go against that 
> goal, as there is no equivalent llvm functionality.
>
> One possible solution would be to try to add equivalent llvm functionality -- 
> it sounds like something like this is needed there too, as all llvm tools (I 
> don't know if there's anything besides llvm-symbolizer) will probably not 
> work without it. (If they do, it would be interesting to figure out how they 
> manage that.)


Yeah, I don't know much about the ARM tagged pointer stuff, but if the tags 
don't appear in the return addresses in a stack trace & thus the addresses 
you'd pass to llvm-symbolizer then I think it'd make sense to implement it 
somewhere in LLVM's libDebugInfoDWARF (& yeah, don't know about PDB either, 
perhaps @rnk does). If there is no common code between debug_aranges and other 
address parsing -perhaps there should be? a filter of some kind that could be 
used for all addresses being read?


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [PATCH] D70906: [lldb] Move register info "augmentation" from gdb-remote into ABI

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: jasonmolenda, clayborg, tatyana-krasnukha.
Herald added a subscriber: aprantl.
Herald added a project: LLDB.

Previously the ABI plugin exposed some "register infos" and the
gdb-remote code used those to fill in the missing bits. Now, the
"filling in" code is in the ABI plugin itself, and the gdb-remote code
just invokes that.

The motivation for this is two-fold:
a) the "augmentation" logic is useful outside of process gdb-remote. For

  instance, it would allow us to avoid repeating the register number
  definitions in minidump code.

b) It gives more implementation freedom to the ABI classes. Now that

  these "register infos" are essentially implementation details, classes
  can use other methods to obtain dwarf/eh_frame register numbers -- for
  instance they can consult llvm MC layer.

Since the augmentation code was not currently tested anywhere, I took
the opportunity to create a simple test for it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70906

Files:
  lldb/include/lldb/Target/ABI.h
  
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestTargetXMLArch.py
  
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/basic_eh_frame.yaml
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Target/ABI.cpp

Index: lldb/source/Target/ABI.cpp
===
--- lldb/source/Target/ABI.cpp
+++ lldb/source/Target/ABI.cpp
@@ -63,24 +63,6 @@
   return false;
 }
 
-bool ABI::GetRegisterInfoByKind(RegisterKind reg_kind, uint32_t reg_num,
-RegisterInfo &info) {
-  if (reg_kind < eRegisterKindEHFrame || reg_kind >= kNumRegisterKinds)
-return false;
-
-  uint32_t count = 0;
-  const RegisterInfo *register_info_array = GetRegisterInfoArray(count);
-  if (register_info_array) {
-for (uint32_t i = 0; i < count; ++i) {
-  if (register_info_array[i].kinds[reg_kind] == reg_num) {
-info = register_info_array[i];
-return true;
-  }
-}
-  }
-  return false;
-}
-
 ValueObjectSP ABI::GetReturnValueObject(Thread &thread, CompilerType &ast_type,
 bool persistent) const {
   if (!ast_type.IsValid())
@@ -229,3 +211,20 @@
   assert(info_up);
   return info_up;
 }
+
+void ABI::AugmentRegisterInfo(RegisterInfo &info) {
+  if (info.kinds[eRegisterKindEHFrame] != LLDB_INVALID_REGNUM &&
+  info.kinds[eRegisterKindDWARF] != LLDB_INVALID_REGNUM)
+return;
+
+  RegisterInfo abi_info;
+  if (!GetRegisterInfoByName(ConstString(info.name), abi_info))
+return;
+
+  if (info.kinds[eRegisterKindEHFrame] == LLDB_INVALID_REGNUM)
+info.kinds[eRegisterKindEHFrame] = abi_info.kinds[eRegisterKindEHFrame];
+  if (info.kinds[eRegisterKindDWARF] == LLDB_INVALID_REGNUM)
+info.kinds[eRegisterKindDWARF] = abi_info.kinds[eRegisterKindDWARF];
+  if (info.kinds[eRegisterKindGeneric] == LLDB_INVALID_REGNUM)
+info.kinds[eRegisterKindGeneric] = abi_info.kinds[eRegisterKindGeneric];
+}
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -388,36 +388,6 @@
   return false;
 }
 
-// If the remote stub didn't give us eh_frame or DWARF register numbers for a
-// register, see if the ABI can provide them.
-// DWARF and eh_frame register numbers are defined as a part of the ABI.
-static void AugmentRegisterInfoViaABI(RegisterInfo ®_info,
-  ConstString reg_name, ABISP abi_sp) {
-  if (reg_info.kinds[eRegisterKindEHFrame] == LLDB_INVALID_REGNUM ||
-  reg_info.kinds[eRegisterKindDWARF] == LLDB_INVALID_REGNUM) {
-if (abi_sp) {
-  RegisterInfo abi_reg_info;
-  if (abi_sp->GetRegisterInfoByName(reg_name, abi_reg_info)) {
-if (reg_info.kinds[eRegisterKindEHFrame] == LLDB_INVALID_REGNUM &&
-abi_reg_info.kinds[eRegisterKindEHFrame] != LLDB_INVALID_REGNUM) {
-  reg_info.kinds[eRegisterKindEHFrame] =
-  abi_reg_info.kinds[eRegisterKindEHFrame];
-}
-if (reg_info.kinds[eRegisterKindDWARF] == LLDB_INVALID_REGNUM &&
-abi_reg_info.kinds[eRegisterKindDWARF] != LLDB_INVALID_REGNUM) {
-  reg_info.kinds[eRegisterKindDWARF] =
-  abi_reg_info.kinds[eRegisterKindDWARF];
-}
-if (reg_info.kinds[eRegisterKindGeneric] == LLDB_INVALID_REGNUM &&
-abi_reg_info.kinds[eRegisterKindGeneric] != LLDB_INVALID_REGNUM) {
-  reg_info.kinds[eRegisterKindGeneric] =
-  abi_reg_info.kinds[eRegisterKindGeneric];
-}
-  }
-}
-  }
-}
-
 static size_t SplitCommaSeparatedRegisterNumberString(
 const llvm::StringRef &comma_separated_regiter_numbers,
 std::vector ®nums, int base) {
@@ 

[Lldb-commits] [PATCH] D70907: Change Target::FindBreakpointsByName to use a vector

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Using a BreakpointList corrupts the breakpoints' IDs because
BreakpointList::Add sets the ID, so use a vector instead.

Note that, despite the similar name, SBTarget::FindBreakpointsByName
doesn't suffer the same problem, because it uses a SBBreakpointList,
which is more like a BreakpointIDList than a BreakpointList under the
covers.

Add a check to TestBreakpointNames that, without this fix, notices the
ID getting mutated and fails.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70907

Files:
  lldb/include/lldb/Breakpoint/BreakpointList.h
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
  lldb/source/API/SBTarget.cpp
  lldb/source/Breakpoint/BreakpointList.cpp
  lldb/source/Target/Target.cpp


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -728,11 +728,11 @@
 }
 
 void Target::ApplyNameToBreakpoints(BreakpointName &bp_name) {
-  BreakpointList bkpts_with_name(false);
+  std::vector bkpts_with_name;
   m_breakpoint_list.FindBreakpointsByName(bp_name.GetName().AsCString(),
   bkpts_with_name);
 
-  for (auto bp_sp : bkpts_with_name.Breakpoints())
+  for (auto bp_sp : bkpts_with_name)
 bp_name.ConfigureBreakpoint(bp_sp);
 }
 
Index: lldb/source/Breakpoint/BreakpointList.cpp
===
--- lldb/source/Breakpoint/BreakpointList.cpp
+++ lldb/source/Breakpoint/BreakpointList.cpp
@@ -129,7 +129,7 @@
 }
 
 bool BreakpointList::FindBreakpointsByName(const char *name,
-   BreakpointList &matching_bps) {
+   std::vector 
&matching_bps) {
   Status error;
   if (!name)
 return false;
@@ -139,7 +139,7 @@
 
   for (BreakpointSP bkpt_sp : Breakpoints()) {
 if (bkpt_sp->MatchesName(name)) {
-  matching_bps.Add(bkpt_sp, false);
+  matching_bps.push_back(bkpt_sp);
 }
   }
 
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -1176,12 +1176,12 @@
   TargetSP target_sp(GetSP());
   if (target_sp) {
 std::lock_guard guard(target_sp->GetAPIMutex());
-BreakpointList bkpt_list(false);
+std::vector bkpt_list;
 bool is_valid =
 target_sp->GetBreakpointList().FindBreakpointsByName(name, bkpt_list);
 if (!is_valid)
   return false;
-for (BreakpointSP bkpt_sp : bkpt_list.Breakpoints()) {
+for (BreakpointSP bkpt_sp : bkpt_list) {
   bkpts.AppendByID(bkpt_sp->GetID());
 }
   }
Index: 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
===
--- 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
+++ 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_names/TestBreakpointNames.py
@@ -155,8 +155,13 @@
 def do_check_using_names(self):
 """Use Python APIs to check names work in place of breakpoint ID's."""
 
+# Create a dummy breakpoint to use up ID 1
+_ = self.target.BreakpointCreateByLocation(self.main_file_spec, 30)
+
+# Create a breakpiont to test with
 bkpt = self.target.BreakpointCreateByLocation(self.main_file_spec, 10)
 bkpt_name = "ABreakpoint"
+bkpt_id = bkpt.GetID()
 other_bkpt_name= "_AnotherBreakpoint"
 
 # Add a name and make sure we match it:
@@ -169,6 +174,7 @@
 self.assertTrue(bkpts.GetSize() == 1, "One breakpoint matched.")
 found_bkpt = bkpts.GetBreakpointAtIndex(0)
 self.assertTrue(bkpt.GetID() == found_bkpt.GetID(),"The right 
breakpoint.")
+self.assertTrue(bkpt.GetID() == bkpt_id,"With the same ID as before.")
 
 retval = lldb.SBCommandReturnObject()
 self.dbg.GetCommandInterpreter().HandleCommand("break disable 
%s"%(bkpt_name), retval)
Index: lldb/include/lldb/Breakpoint/BreakpointList.h
===
--- lldb/include/lldb/Breakpoint/BreakpointList.h
+++ lldb/include/lldb/Breakpoint/BreakpointList.h
@@ -68,7 +68,7 @@
   ///
   /// \result
   ///   \bfalse if the input name was not a legal breakpoint name.
-  bool FindBreakpointsByName(const char *name, BreakpointList &matching_bps);
+  bool FindBreakpointsByName(const char *name, std::vector 
&matching_bps);
 
   /// Returns the number of elements in this breakpoint list.
   ///


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source

[Lldb-commits] [PATCH] D70906: [lldb] Move register info "augmentation" from gdb-remote into ABI

[Greg Clayton via Phabricator via lldb-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70906



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

[Lldb-commits] [PATCH] D70622: [cmake][lit] Follow symlinks when looking for lit tests & reconfigure lldb's tests.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Personally I'd prefer moving the test cases, but maybe there's a good 
historical reason for the current layout. I'll ask around during out team 
meeting later today and report back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70622



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So the main goal here is to let the Architecture plug-in handle the address 
masking in all places and make sure there is no code like:

  if (ArchSpec arch = m_objfile_sp->GetArchitecture()) {
if (arch.GetTriple().getArch() == llvm::Triple::arm ||
arch.GetTriple().getArch() == llvm::Triple::thumb)
  return ~1ull;
  }

anywhere in the codebase. Otherwise when we add a new architecture that 
requires bit stripping/adding, we end up having to change many locations in the 
code (2 of which were added with this patch). So use the Architecture plug-in 
to do this and we just need to edit the Architecture plug-in code for each 
architecture.


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So some background on how address masks are handled in LLDB:

Typically the way we have tried to take care of the extra Thumb bit for ARM 
using:

  lldb::addr_t Address::GetCallableLoadAddress(Target *target, bool is_indirect 
= false) const;
  lldb::addr_t GetOpcodeLoadAddress(Target *target, AddressClass addr_class = 
AddressClass::eInvalid) const;

The first will add the extra bit to an address if needed. The latter will strip 
the bit if needed. This does require a target though and the target uses the 
"Architecture" class for ARM to do the work of using the mask. Not sure if we 
want to try to get an architecture class and use that here for stripping the 
bit instead of using an address mask?

Also, any lldb_private::Address can be asked for its address class:

  AddressClass Address::GetAddressClass() const;

This will return "eCode" for ARM code and "eCodeAlternateISA" for Thumb code. 
This is resolved by the ObjectFile::GetAddressClass:

  /// Get the address type given a file address in an object file.
  ///
  /// Many binary file formats know what kinds This is primarily for ARM
  /// binaries, though it can be applied to any executable file format that
  /// supports different opcode types within the same binary. ARM binaries
  /// support having both ARM and Thumb within the same executable container.
  /// We need to be able to get \return
  /// The size of an address in bytes for the currently selected
  /// architecture (and object for archives). Returns zero if no
  /// architecture or object has been selected.
  virtual AddressClass GetAddressClass(lldb::addr_t file_addr);

So currently no code in LLDB tries to undo the address masks that might be in 
the object file or debug info, and we take care of it after the fact. Early in 
the ARM days there used to be extra symbols that were added to the symbol table 
with names like "$a" for ARM, "$t" for Thumb and "$d" for data. There would be 
multiple of these symbols in an ordered vector of symbols that would create a 
CPU map. This was early in the ARM days before the branch instruction would 
watch for bit zero. Later ARM architectures started using bit zero to indicate 
which mode to put the processor in instead of using an explicit "branch to arm" 
or "branch to thumb" instructions. When this new stuff came out bit zero 
started showing up in symbol tables. So the current code allows for either the 
old style (CPU map in symbol table with $a $t and $d symbols) and the new style 
(bit zero set and no CPU map).




Comment at: lldb/include/lldb/Symbol/LineTable.h:333
+
+  bool m_clear_address_zeroth_bit = false;
 };

Might be nice to let the line table parse itself first, and then in a post 
production step clean up all the addresses? Maybe

```
void LineTable::Finalize(Architecture *arch);
```

Then we let the architecture plug-in handle any stripping using:

```
lldb::addr_t Architecture::GetOpcodeLoadAddress(lldb::addr_t addr, AddressClass 
addr_class) const;
```

The ArchitectureArm plugin does this:

```
addr_t ArchitectureArm::GetOpcodeLoadAddress(addr_t opcode_addr,
 AddressClass addr_class) const {
  switch (addr_class) {
  case AddressClass::eData:
  case AddressClass::eDebug:
return LLDB_INVALID_ADDRESS;
  default: break;
  }
  return opcode_addr & ~(1ull);
}
```





Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.cpp:19-20
 // Constructor
-DWARFDebugAranges::DWARFDebugAranges() : m_aranges() {}
+DWARFDebugAranges::DWARFDebugAranges(dw_addr_t addr_mask)
+: m_aranges(), m_addr_mask(addr_mask) {}
 

Use Architecture plug-in instead of hard coded mask.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAranges.h:53
   RangeToDIE m_aranges;
+  dw_addr_t m_addr_mask;
 };

See comment in LineTable.h above.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:41-42
 
-  m_cu_aranges_up = std::make_unique();
+  m_cu_aranges_up =
+  std::make_unique(m_dwarf.GetAddressMask());
   const DWARFDataExtractor &debug_aranges_data =

Use Architecture plug-in instead of hard coded mask.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:258
 case DW_AT_low_pc:
-  lo_pc = form_value.Address();
+  lo_pc = form_value.Address() & m_addr_mask;
 

Use Architecture plug-in instead of hard coded mask.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:273
   form_value.Form() == DW_FORM_GNU_addr_index) {
-hi_pc = form_value.Address();
+hi_pc = form_value.Address() & m_addr_mask;
   } else {

ditto...



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h:44-45
 
+  void SetAd

[Lldb-commits] [PATCH] D70887: [lldb] Use realpath to avoid issues with symlinks in source paths

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Might be a better idea to realpath this once in the setup code instead of 
calling realpath thousands and thousands of times? Maybe we just santize 
"LLDB_TEST" and "LLDB_BUILD" one time with top level code like:

  os.environ["LLDB_TEST"] = os.path.realpath(os.environ["LLDB_TEST"])
  os.environ["LLDB_BUILD"] = os.path.realpath(os.environ["LLDB_BUILD"])




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70887



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

[Lldb-commits] [PATCH] D70885: [lldb] Use explicit lldb commands on tests

[Greg Clayton via Phabricator via lldb-commits]

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

We shouldn't be running the test suite that allows your ~/.lldbinit file to be 
run. This can really hose up many things, so we should change the lldb-vscode 
test suite to not allow your ~/.lldbinit file to be loaded instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70885



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

[Lldb-commits] [PATCH] D70884: [lldb] Fix TestFormattersSBAPI test

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added inline comments.
This revision is now accepted and ready to land.



Comment at: 
lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py:71
 
-format.format = lldb.eFormatOctal
+format.SetFormat(lldb.eFormatOctal)
 category.AddTypeFormat(lldb.SBTypeNameSpecifier("int"), format)

both "format.format = lldb.eFormatOctal" and this are the same right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70884



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Greg Clayton via Phabricator via lldb-commits]

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

Would be good to write a test for this where the init file does "script 
print('hello')" which would hose up the connection


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70883: [vscode.py] Make read_packet only return None when EOF

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So what is this fix actually doing? Allowing random STDOUT to be ignored?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70883



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1207-1210
+  auto arguments = request.getObject("arguments");
+  const auto skipInitFiles = GetBoolean(arguments, "skipInitFiles", false);
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skipInitFiles /*source_init_files*/);

Are we allows to add extra "initialize" arguments here? How and who will ever 
pass this to lldb-vscode? Is there a way to make Microsoft VS Code pass this? 
If not, then just use an environment variable instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70882: Add skipInitFiles option to lldb-vscode initialize

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

If MS VS Code IDE itself will never pass this to the "initialize" packet, then 
we might just want to use an env var instead? You should probably get rid of 
https://reviews.llvm.org/D70886 and just modify this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70882



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Abandon this one and modify https://reviews.llvm.org/D70882?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70907: Change Target::FindBreakpointsByName to use a vector

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Breakpoint/BreakpointList.h:71
   ///   \bfalse if the input name was not a legal breakpoint name.
-  bool FindBreakpointsByName(const char *name, BreakpointList &matching_bps);
+  bool FindBreakpointsByName(const char *name, std::vector 
&matching_bps);
 

I think the API would look nicer if we returned an 
`llvm::Optional>` where `None` means an invalid breakpoint name 
and an empty list no matches. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70907



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

[Lldb-commits] [PATCH] D70851: [lldb] s/FileSpec::Equal/FileSpec::Match

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM assuming the answer to my inline comment is negative :-)




Comment at: lldb/include/lldb/Utility/FileSpec.h:196
 
   static bool Equal(const FileSpec &a, const FileSpec &b, bool full);
 

Why do we still need the `Equal` method? Are there cases where `full` is only 
decided at runtime? Would it be worth to update the call sites to use `==` or 
`::Match` directly? I think having both `Match` and `Equal` with these 
semantics is confusing and will likely reintroduce the things you just cleaned 
up. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70851



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

[Lldb-commits] [PATCH] D70827: [lldb] Remove FileSpec->CompileUnit inheritance

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Awesome, thank you! This has been on my todo-list for awhile :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70827



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

[Lldb-commits] [PATCH] D70830: [LLDB] Disable MSVC warning C4190

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM


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

https://reviews.llvm.org/D70830



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

[Lldb-commits] [PATCH] D70907: Change Target::FindBreakpointsByName to use a vector

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet marked an inline comment as done.
JosephTremoulet added inline comments.



Comment at: lldb/include/lldb/Breakpoint/BreakpointList.h:71
   ///   \bfalse if the input name was not a legal breakpoint name.
-  bool FindBreakpointsByName(const char *name, BreakpointList &matching_bps);
+  bool FindBreakpointsByName(const char *name, std::vector 
&matching_bps);
 

JDevlieghere wrote:
> I think the API would look nicer if we returned an 
> `llvm::Optional>` where `None` means an invalid breakpoint name 
> and an empty list no matches. What do you think?
I think I'd go with `Expected<>` over `Optional<>`, since the `false` return 
indicates invalid input.

I actually originally considered different signatures for this change.  My 
first inclination was to switch the populated list from a `BreakpointList` to a 
`BreakpointIDList`, but it seemed that would be inefficient for the call from 
`Target::ApplyNameToBreakpoints` that needs the actual breakpoints.  So then I 
went down the route of `Expected>`, but it 
was quickly becoming more code (and more convoluted) than seemed warranted.  So 
I looked around, saw `std::vector`s being populated by e.g. 
`Breakpoint::GetNames` and `SourceManager::FindLinesMatchingRegex`, and decided 
to follow suit.

Which is a long way to say:  populating a `std::vector` seems to "fit in" with 
surrounding code better, but aside from that, yes I think returning 
`Expected` would be a more natural fit.  I don't know which of 
those concerns to prefer in this code; LMK and I'm happy to switch it if that 
seems best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70907



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

[Lldb-commits] [PATCH] D70851: [lldb] s/FileSpec::Equal/FileSpec::Match

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/Utility/FileSpec.h:196
 
   static bool Equal(const FileSpec &a, const FileSpec &b, bool full);
 

JDevlieghere wrote:
> Why do we still need the `Equal` method? Are there cases where `full` is only 
> decided at runtime? Would it be worth to update the call sites to use `==` or 
> `::Match` directly? I think having both `Match` and `Equal` with these 
> semantics is confusing and will likely reintroduce the things you just 
> cleaned up. 
I agree with JDev here.



Comment at: lldb/include/lldb/Utility/FileSpec.h:202
+  /// pattern matches everything.
+  static bool Match(const FileSpec &pattern, const FileSpec &file);
+

Maybe rename to "Matches"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70851



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

[Lldb-commits] [PATCH] D70882: Add skipInitFiles option to lldb-vscode initialize

[António Afonso via Phabricator via lldb-commits]

aadsm planned changes to this revision.
aadsm added a comment.

Yes, I'll change this to env it makes more sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70882



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

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

The way phabricator shows the diff is tricky, this change has nothing to do 
with D70882  and stands by itself. The 
important part here is making the `g_vsc.debugger.SetOutputFileHandle(out, 
true); g_vsc.debugger.SetErrorFileHandle(out, false);` happen before the 
debugger creation. Not sure how to create a test for this though since there's 
no mechanism to give lldb-vscode an initial file to load...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70886



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

[Lldb-commits] [PATCH] D70884: [lldb] Fix TestFormattersSBAPI test

[António Afonso via Phabricator via lldb-commits]

aadsm marked an inline comment as done.
aadsm added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py:71
 
-format.format = lldb.eFormatOctal
+format.SetFormat(lldb.eFormatOctal)
 category.AddTypeFormat(lldb.SBTypeNameSpecifier("int"), format)

clayborg wrote:
> both "format.format = lldb.eFormatOctal" and this are the same right?
Exactly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70884



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

[Lldb-commits] [PATCH] D70885: [lldb] Use explicit lldb commands on tests

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

Yeah, that's what I do in D70882 . I still 
think tests can be more robust by using explicit lldb command names but don't 
really care much :). I'm happy to abandon it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70885



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

[Lldb-commits] [PATCH] D70883: [vscode.py] Make read_packet only return None when EOF

[António Afonso via Phabricator via lldb-commits]

aadsm added a comment.

Yes. This happens when lldb-vscode is run in stdin/stdout mode (which is what 
happens in tests and I've also seen some IDEs doing the same) and installed 
commands are doing "print". In this case the client gets data that does not 
start with `Content-Length`.
This fixes the tests but overall is still a problem for IDEs (but that's 
another discussion).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70883



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

[Lldb-commits] [PATCH] D70887: [lldb] Use realpath to avoid issues with symlinks in source paths

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 231740.
aadsm added a comment.

Solve this at the failing test level. We can't be sure of the filename will end 
up in the debug info so we just add both keys to the mapping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70887

Files:
  
lldb/packages/Python/lldbsuite/test/macosx/DBGSourcePathRemapping/TestDSYMSourcePathRemapping.py


Index: 
lldb/packages/Python/lldbsuite/test/macosx/DBGSourcePathRemapping/TestDSYMSourcePathRemapping.py
===
--- 
lldb/packages/Python/lldbsuite/test/macosx/DBGSourcePathRemapping/TestDSYMSourcePathRemapping.py
+++ 
lldb/packages/Python/lldbsuite/test/macosx/DBGSourcePathRemapping/TestDSYMSourcePathRemapping.py
@@ -35,6 +35,7 @@
 import re
 self.assertTrue(re.match(r'[0-9a-fA-F-]+', uuid))
 plist = os.path.join(dsym, 'Contents', 'Resources', uuid + '.plist')
+botdir_realpath = os.path.realpath(botdir)
 with open(plist, 'w') as f:
 f.write('\n')
 f.write('http://www.apple.com/DTDs/PropertyList-1.0.dtd";>\n')
@@ -44,6 +45,9 @@
 f.write('  \n')
 f.write('' + botdir + '\n')
 f.write('' + userdir + '\n')
+if botdir_realpath != botdir:
+f.write('' + botdir_realpath + '\n')
+f.write('' + userdir + '\n')
 f.write('  \n')
 f.write('\n')
 f.write('\n')


Index: lldb/packages/Python/lldbsuite/test/macosx/DBGSourcePathRemapping/TestDSYMSourcePathRemapping.py
===
--- lldb/packages/Python/lldbsuite/test/macosx/DBGSourcePathRemapping/TestDSYMSourcePathRemapping.py
+++ lldb/packages/Python/lldbsuite/test/macosx/DBGSourcePathRemapping/TestDSYMSourcePathRemapping.py
@@ -35,6 +35,7 @@
 import re
 self.assertTrue(re.match(r'[0-9a-fA-F-]+', uuid))
 plist = os.path.join(dsym, 'Contents', 'Resources', uuid + '.plist')
+botdir_realpath = os.path.realpath(botdir)
 with open(plist, 'w') as f:
 f.write('\n')
 f.write('http://www.apple.com/DTDs/PropertyList-1.0.dtd";>\n')
@@ -44,6 +45,9 @@
 f.write('  \n')
 f.write('' + botdir + '\n')
 f.write('' + userdir + '\n')
+if botdir_realpath != botdir:
+f.write('' + botdir_realpath + '\n')
+f.write('' + userdir + '\n')
 f.write('  \n')
 f.write('\n')
 f.write('\n')
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70907: Change Target::FindBreakpointsByName to use a vector

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Breakpoint/BreakpointList.h:71
   ///   \bfalse if the input name was not a legal breakpoint name.
-  bool FindBreakpointsByName(const char *name, BreakpointList &matching_bps);
+  bool FindBreakpointsByName(const char *name, std::vector 
&matching_bps);
 

JosephTremoulet wrote:
> JDevlieghere wrote:
> > I think the API would look nicer if we returned an 
> > `llvm::Optional>` where `None` means an invalid breakpoint 
> > name and an empty list no matches. What do you think?
> I think I'd go with `Expected<>` over `Optional<>`, since the `false` return 
> indicates invalid input.
> 
> I actually originally considered different signatures for this change.  My 
> first inclination was to switch the populated list from a `BreakpointList` to 
> a `BreakpointIDList`, but it seemed that would be inefficient for the call 
> from `Target::ApplyNameToBreakpoints` that needs the actual breakpoints.  So 
> then I went down the route of `Expected iterator>>`, but it was quickly becoming more code (and more convoluted) than 
> seemed warranted.  So I looked around, saw `std::vector`s being populated by 
> e.g. `Breakpoint::GetNames` and `SourceManager::FindLinesMatchingRegex`, and 
> decided to follow suit.
> 
> Which is a long way to say:  populating a `std::vector` seems to "fit in" 
> with surrounding code better, but aside from that, yes I think returning 
> `Expected` would be a more natural fit.  I don't know which of 
> those concerns to prefer in this code; LMK and I'm happy to switch it if that 
> seems best.
I think an `Expected` is perfect. I proposed `Optional` because the way things 
are, the call sites would have to discard the error anyway. However, given that 
`FindBreakpointsByName` takes a `Status`, I think it would be good to propagate 
that up. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70907



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

[Lldb-commits] [PATCH] D70847: [lldb-vscode] Ensure that target matches the executable file

[Greg Clayton via Phabricator via lldb-commits]

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

I think the fix is better done to fix the root cause issue instead of working 
around it. I have suggested a fix in inline comments.




Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1358
+// Create a target that matches the file.
+g_vsc.target = g_vsc.debugger.CreateTarget(program.data());
 

You will lose any state that was set in the original target by 
"g_vsc.RunInitCommands();" if you do this. Not sure if this matters since users 
can use "RunPreRunCommands"...

Also the original g_vsc._target had tried to listen to events from this target. 
From the "initialize" code:

```
void request_initialize(const llvm::json::Object &request) {
  g_vsc.debugger = lldb::SBDebugger::Create(true /*source_init_files*/);
  // Create an empty target right away since we might get breakpoint requests
  // before we are given an executable to launch in a "launch" request, or a
  // executable when attaching to a process by process ID in a "attach"
  // request.
  ...
  g_vsc.target = g_vsc.debugger.CreateTarget(nullptr);
  lldb::SBListener listener = g_vsc.debugger.GetListener();
  listener.StartListeningForEvents(
  g_vsc.target.GetBroadcaster(),
  lldb::SBTarget::eBroadcastBitBreakpointChanged);
  listener.StartListeningForEvents(g_vsc.broadcaster,
   eBroadcastBitStopEventThread);
```
Though if this is patch works for you this must not be needed?

Do breakpoints work fine for you with this patch when you set them before you 
run? This comment might be out of date as I think I fixed when I sent the 
packet:

```
{"event":"initialized","seq":0,"type":"event"}
```

Until after the process was launched.

Where things seems to go wrong is in "Target::Launch(...)":

```
  // 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()) {
```

The platform could be incorrect if the executable inside the launch info 
doesn't match the platform, or even if it works for the platform, it might not 
be the platform that exists. 

A better fix might be to modify Target::GetOrCreateModule(...) (which is called 
by SBTarget::AddModule()) as follows:

```
if (module_sp) {
  bool isExecutable = false; // NEW CODE
  ObjectFile *objfile = module_sp->GetObjectFile();
  if (objfile) {
switch (objfile->GetType()) {
  case ObjectFile::eTypeExecutable:/// A normal executable
isExecutable = true;  // NEW CODE (and moved)
LLVM_FALLTHROUGH;  // NEW CODE
  case ObjectFile::eTypeCoreFile: /// A core file that has a checkpoint 
of
/// a program's execution state
case ObjectFile::eTypeDynamicLinker: /// The platform's dynamic linker
 /// executable
case ObjectFile::eTypeObjectFile:/// An intermediate object file
case ObjectFile::eTypeSharedLibrary: /// A shared library that can be
 /// used during execution
  break;
case ObjectFile::eTypeDebugInfo: /// An object file that contains only
 /// debug information
  if (error_ptr)
error_ptr->SetErrorString("debug info files aren't valid target "
  "modules, please specify an executable");
  return ModuleSP();
case ObjectFile::eTypeStubLibrary: /// A library that can be linked
   /// against but not used for
   /// execution
  if (error_ptr)
error_ptr->SetErrorString("stub libraries aren't valid target "
  "modules, please specify an executable");
  return ModuleSP();
default:
  if (error_ptr)
error_ptr->SetErrorString(
"unsupported file type, please specify an executable");
  return ModuleSP();
}
// GetSharedModule is not guaranteed to find the old shared module, for
// instance in the common case where you pass in the UUID, it is only
// going to find the one module matching the UUID.  In fact, it has no
// good way to know what the "old module" relevant to this target is,
// since there might be many copies of a module with this file spec in
// various running debug sessions, but only one of them will belong to
// this target. So let's remove the UUID from the module list, and look
// in the target's module list. Only do this if there is SOMETHING else
// in the module spec...
if (!old_module_sp) {
  if

[Lldb-commits] [PATCH] D70622: [cmake][lit] Follow symlinks when looking for lit tests & reconfigure lldb's tests.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

No objections to moving the test. Jordan, let me know if you're up for it, 
otherwise I'm happy to take care of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70622



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

[Lldb-commits] [PATCH] D70847: [lldb-vscode] Ensure that target matches the executable file

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The other fix would be to add a new LLDB API function:

  lldb::SBModule lldb::SBTarget::SetExecutable(const char *path, const char 
*triple, const char *symfile);

Where we would pass in "nullptr" for "triple" and "symfile" from lldb-vscode 
and we call that. But I do think fix SBTarget::AddModule to "do the right 
thing" is a good fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70847



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth resigned from this revision.
amccarth added a comment.

I'm following along, but I don't think I have enough domain knowledge to 
qualify as a reviewer.


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [PATCH] D70882: Add environment var LLDBVSCODE_SKIP_INIT_FILES to lldb-vscode

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 231759.
aadsm added a comment.

Use an env var instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70882

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -61,6 +61,8 @@
 
 #endif
 
+const bool skip_init_files = getenv("LLDBVSCODE_SKIP_INIT_FILES") != NULL;
+
 using namespace lldb_vscode;
 
 namespace {
@@ -1194,7 +1196,8 @@
 //   }]
 // }
 void request_initialize(const llvm::json::Object &request) {
-  g_vsc.debugger = lldb::SBDebugger::Create(true /*source_init_files*/);
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skip_init_files /*source_init_files*/);
   // Create an empty target right away since we might get breakpoint requests
   // before we are given an executable to launch in a "launch" request, or a
   // executable when attaching to a process by process ID in a "attach"
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -839,13 +839,19 @@
 
 
 class DebugAdaptor(DebugCommunication):
-def __init__(self, executable=None, port=None, init_commands=[]):
+def __init__(
+self, executable=None, port=None, init_commands=[], 
skip_init_files=True
+):
 self.process = None
 if executable is not None:
+env = os.environ.copy()
+if skip_init_files:
+env['LLDBVSCODE_SKIP_INIT_FILES'] = "1"
 self.process = subprocess.Popen([executable],
 stdin=subprocess.PIPE,
 stdout=subprocess.PIPE,
-stderr=subprocess.PIPE)
+stderr=subprocess.PIPE,
+env=env)
 DebugCommunication.__init__(self, self.process.stdout,
 self.process.stdin, init_commands)
 elif port is not None:


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -61,6 +61,8 @@
 
 #endif
 
+const bool skip_init_files = getenv("LLDBVSCODE_SKIP_INIT_FILES") != NULL;
+
 using namespace lldb_vscode;
 
 namespace {
@@ -1194,7 +1196,8 @@
 //   }]
 // }
 void request_initialize(const llvm::json::Object &request) {
-  g_vsc.debugger = lldb::SBDebugger::Create(true /*source_init_files*/);
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skip_init_files /*source_init_files*/);
   // Create an empty target right away since we might get breakpoint requests
   // before we are given an executable to launch in a "launch" request, or a
   // executable when attaching to a process by process ID in a "attach"
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -839,13 +839,19 @@
 
 
 class DebugAdaptor(DebugCommunication):
-def __init__(self, executable=None, port=None, init_commands=[]):
+def __init__(
+self, executable=None, port=None, init_commands=[], skip_init_files=True
+):
 self.process = None
 if executable is not None:
+env = os.environ.copy()
+if skip_init_files:
+env['LLDBVSCODE_SKIP_INIT_FILES'] = "1"
 self.process = subprocess.Popen([executable],
 stdin=subprocess.PIPE,
 stdout=subprocess.PIPE,
-stderr=subprocess.PIPE)
+stderr=subprocess.PIPE,
+env=env)
 DebugCommunication.__init__(self, self.process.stdout,
 self.process.stdin, init_commands)
 elif port is not None:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70884: [lldb] Fix TestFormattersSBAPI test

[António Afonso via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGafd5d912812e: [lldb] Fix TestFormattersSBAPI test (authored 
by aadsm, committed by António Afonso ).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70884

Files:
  
lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py


Index: 
lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py
===
--- 
lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py
+++ 
lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py
@@ -68,17 +68,17 @@
 self.expect("frame variable foo.E",
 substrs=['b8cca70a'])
 
-format.format = lldb.eFormatOctal
+format.SetFormat(lldb.eFormatOctal)
 category.AddTypeFormat(lldb.SBTypeNameSpecifier("int"), format)
 self.expect("frame variable foo.A",
-substrs=['01'])
+substrs=[' 01'])
 self.expect("frame variable foo.E",
 substrs=['b8cca70a'])
 
 category.DeleteTypeFormat(lldb.SBTypeNameSpecifier("int"))
 category.DeleteTypeFormat(lldb.SBTypeNameSpecifier("long"))
 self.expect("frame variable foo.A", matching=False,
-substrs=['01'])
+substrs=[' 01'])
 self.expect("frame variable foo.E", matching=False,
 substrs=['b8cca70a'])
 
@@ -90,10 +90,13 @@
 new_category.IsValid(),
 "getting a non-existing category worked")
 new_category = self.dbg.CreateCategory("foobar")
-new_category.enabled = True
+new_category.SetEnabled(True)
 new_category.AddTypeSummary(
 lldb.SBTypeNameSpecifier(
-"^.*t$", True), summary)
+"^.*t$",
+True,  # is_regexp
+), summary)
+
 self.expect("frame variable foo.A",
 substrs=['hello world'])
 self.expect("frame variable foo.E", matching=False,
@@ -102,7 +105,7 @@
 substrs=['hello world'])
 self.expect("frame variable foo.F",
 substrs=['hello world'])
-new_category.enabled = False
+new_category.SetEnabled(False)
 self.expect("frame variable foo.A", matching=False,
 substrs=['hello world'])
 self.expect("frame variable foo.E", matching=False,
@@ -379,7 +382,7 @@
 lldb.SBTypeSummary.CreateWithScriptCode("return 'hello scripted 
world';"))
 self.expect("frame variable foo", matching=False,
 substrs=['hello scripted world'])
-new_category.enabled = True
+new_category.SetEnabled(True)
 self.expect("frame variable foo", matching=True,
 substrs=['hello scripted world'])
 


Index: lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py
===
--- lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py
+++ lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py
@@ -68,17 +68,17 @@
 self.expect("frame variable foo.E",
 substrs=['b8cca70a'])
 
-format.format = lldb.eFormatOctal
+format.SetFormat(lldb.eFormatOctal)
 category.AddTypeFormat(lldb.SBTypeNameSpecifier("int"), format)
 self.expect("frame variable foo.A",
-substrs=['01'])
+substrs=[' 01'])
 self.expect("frame variable foo.E",
 substrs=['b8cca70a'])
 
 category.DeleteTypeFormat(lldb.SBTypeNameSpecifier("int"))
 category.DeleteTypeFormat(lldb.SBTypeNameSpecifier("long"))
 self.expect("frame variable foo.A", matching=False,
-substrs=['01'])
+substrs=[' 01'])
 self.expect("frame variable foo.E", matching=False,
 substrs=['b8cca70a'])
 
@@ -90,10 +90,13 @@
 new_category.IsValid(),
 "getting a non-existing category worked")
 new_category = self.dbg.CreateCategory("foobar")
-new_category.enabled = True
+new_category.SetEnabled(True)
 new_category.AddTypeSummary(
 lldb.SBTypeNameSpecifier(
-"^.*t$", True), summary)
+"^.*t$",
+True,  # is_regexp
+), summary)
+
 self.expect("frame variable foo.A",
 substrs=['hello world'])
 self.expect("frame variable foo.E", matching=False,
@@ -102,7 +105,7 @@
 substrs=['hello world'])
 self.expect("frame variable foo.F",
 substrs=['hello wor

[Lldb-commits] [PATCH] D70848: [LLDB] Set the right address size on output DataExtractors from ObjectFile

[Martin Storsjö via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7d019d1a3be2: [LLDB] Set the right address size on output 
DataExtractors from ObjectFile (authored by mstorsjo).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70848

Files:
  lldb/source/Symbol/ObjectFile.cpp
  lldb/test/Shell/SymbolFile/DWARF/win-i386-line-table.s


Index: lldb/test/Shell/SymbolFile/DWARF/win-i386-line-table.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/win-i386-line-table.s
@@ -0,0 +1,55 @@
+# Test that lldb can read a line table for an architecture with a different
+# address size than the one that of the host.
+
+# REQUIRES: lld, x86
+
+# RUN: llvm-mc -triple i686-windows-gnu %s -filetype=obj > %t.o
+# RUN: lld-link %t.o -out:%t.exe -debug:dwarf -entry:entry -subsystem:console 
-lldmingw
+# RUN: %lldb %t.exe -o "image dump line-table -v win-i386-line-table.c" -b | 
FileCheck %s
+
+# CHECK: Line table for win-i386-line-table.c in `win-i386-line-table.s.tmp.exe
+# CHECK: 0x00401000: /path/to/src/win-i386-line-table.c:2:1
+# CHECK: 0x00401001: /path/to/src/win-i386-line-table.c:2:1
+
+.text
+.file   "win-i386-line-table.c"
+.globl  _entry  # -- Begin function entry
+_entry: # @entry
+.file   1 "/path/to/src" "win-i386-line-table.c"
+.loc1 1 0   # win-i386-line-table.c:1:0
+.cfi_sections .debug_frame
+.cfi_startproc
+.loc1 2 1 prologue_end  # win-i386-line-table.c:2:1
+retl
+.cfi_endproc
+# -- End function
+.section.debug_str,"dr"
+Linfo_string1:
+.asciz  "win-i386-line-table.c"
+.section.debug_abbrev,"dr"
+Lsection_abbrev:
+.byte   1   # Abbreviation Code
+.byte   17  # DW_TAG_compile_unit
+.byte   1   # DW_CHILDREN_yes
+.byte   3   # DW_AT_name
+.byte   14  # DW_FORM_strp
+.byte   16  # DW_AT_stmt_list
+.byte   23  # DW_FORM_sec_offset
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   0   # EOM(3)
+.section.debug_info,"dr"
+Lsection_info:
+Lcu_begin0:
+.long   Ldebug_info_end0-Ldebug_info_start0 # Length of Unit
+Ldebug_info_start0:
+.short  4   # DWARF version number
+.secrel32   Lsection_abbrev # Offset Into Abbrev. Section
+.byte   4   # Address Size (in bytes)
+.byte   1   # Abbrev [1] 0xb:0x2d 
DW_TAG_compile_unit
+.secrel32   Linfo_string1   # DW_AT_name
+.secrel32   Lline_table_start0 # DW_AT_stmt_list
+.byte   0   # End Of Children Mark
+Ldebug_info_end0:
+.section.debug_line,"dr"
+Lline_table_start0:
Index: lldb/source/Symbol/ObjectFile.cpp
===
--- lldb/source/Symbol/ObjectFile.cpp
+++ lldb/source/Symbol/ObjectFile.cpp
@@ -477,7 +477,13 @@
DataExtractor &data) const {
   // The entire file has already been mmap'ed into m_data, so just copy from
   // there as the back mmap buffer will be shared with shared pointers.
-  return data.SetData(m_data, offset, length);
+  size_t ret = data.SetData(m_data, offset, length);
+  // DataExtractor::SetData copies the address byte size from m_data, but
+  // m_data's address byte size is only set from sizeof(void*), and we can't
+  // access subclasses GetAddressByteSize() when setting up m_data in the
+  // constructor.
+  data.SetAddressByteSize(GetAddressByteSize());
+  return ret;
 }
 
 size_t ObjectFile::CopyData(lldb::offset_t offset, size_t length,


Index: lldb/test/Shell/SymbolFile/DWARF/win-i386-line-table.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/win-i386-line-table.s
@@ -0,0 +1,55 @@
+# Test that lldb can read a line table for an architecture with a different
+# address size than the one that of the host.
+
+# REQUIRES: lld, x86
+
+# RUN: llvm-mc -triple i686-windows-gnu %s -filetype=obj > %t.o
+# RUN: lld-link %t.o -out:%t.exe -debug:dwarf -entry:entry -subsystem:console -lldmingw
+# RUN: %lldb %t.exe -o "image dump line-table -v win-i386-line-table.c" -b | FileCheck %s
+
+# CHECK: Line table for win-i386-line-table.c in `win-i386-line-table.s.tmp.exe
+# CHECK: 0x00401000: /path/to/src/win-i386-line-table.c:2:1
+# CHECK: 0x00401001: /path/to/src/win-i386-line-table.c:2:1
+
+.text
+.file   "win-i386-lin

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Martin Storsjö via Phabricator via lldb-commits]

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

In D70840#1765219 , @labath wrote:

> debug_aranges is a sort of a lookup table for speeding up address->compile 
> unit searches. llvm does not generate it by default since, and I think the 
> reason is that you can usually get the same kind of data from the 
> DW_AT_ranges attribute of the compile unit.


Ok, thanks for the explanation!

On the topic of debug_aranges (but unrelated to this thread), I've been 
notified about a project, drmingw, a postmortem debugger for the mingw 
environment (I think), which stumbles on code built by llvm exactly due to the 
lack of debug_aranges: 
https://github.com/jrfonseca/drmingw/issues/42#issuecomment-526850588 They 
speculate that it almost even would be mandated by the dwarf spec.

> Overall, I think this needs to be handled in DWARF code. That may even make 
> sense, since PDB may not suffer from the same problem (does it?)

Not sure actually, I'd have to check. (FWIW, llvm doesn't support generating 
codeview debug info for PDB for ARM, only x86 and aarch64, but I could check 
with MSVC tools.)




Comment at: lldb/include/lldb/Symbol/LineTable.h:333
+
+  bool m_clear_address_zeroth_bit = false;
 };

clayborg wrote:
> Might be nice to let the line table parse itself first, and then in a post 
> production step clean up all the addresses? Maybe
> 
> ```
> void LineTable::Finalize(Architecture *arch);
> ```
> 
> Then we let the architecture plug-in handle any stripping using:
> 
> ```
> lldb::addr_t Architecture::GetOpcodeLoadAddress(lldb::addr_t addr, 
> AddressClass addr_class) const;
> ```
> 
> The ArchitectureArm plugin does this:
> 
> ```
> addr_t ArchitectureArm::GetOpcodeLoadAddress(addr_t opcode_addr,
>  AddressClass addr_class) const {
>   switch (addr_class) {
>   case AddressClass::eData:
>   case AddressClass::eDebug:
> return LLDB_INVALID_ADDRESS;
>   default: break;
>   }
>   return opcode_addr & ~(1ull);
> }
> ```
> 
> 
Thanks for all the pointers and the explanations! I'll try to have a look into 
all of this in a few days.


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [lldb] e5290a0 - [lldb/CMake] Simplify logic for adding example Python packages (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2019-12-02T13:03:24-08:00
New Revision: e5290a06d6c23bd222543cb9b689a199343021a7

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

LOG: [lldb/CMake] Simplify logic for adding example Python packages (NFC)

This simplifies the CMake logic for adding the Python examples to the
Python package. It unifies the use of create_python_package by adding
the NOINIT option and removes the `target` argument, which is always
`finish_swig`.

Added: 


Modified: 
lldb/CMakeLists.txt

Removed: 




diff  --git a/lldb/CMakeLists.txt b/lldb/CMakeLists.txt
index 81d7dd8123bd..6b5f7640b336 100644
--- a/lldb/CMakeLists.txt
+++ b/lldb/CMakeLists.txt
@@ -119,26 +119,20 @@ if (NOT LLDB_DISABLE_PYTHON)
   "${lldb_scripts_dir}/lldb.py"
   "${lldb_python_build_path}/__init__.py")
 
-  if(APPLE)
-SET(lldb_python_heap_dir "${lldb_python_build_path}/macosx/heap")
-add_custom_command(TARGET finish_swig POST_BUILD VERBATIM
-  COMMAND ${CMAKE_COMMAND} -E make_directory ${lldb_python_heap_dir}
-  COMMAND ${CMAKE_COMMAND} -E copy
-"${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap/heap_find.cpp"
-"${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap/Makefile"
-${lldb_python_heap_dir})
-  endif()
-
-  function(create_python_package target pkg_dir)
-cmake_parse_arguments(ARG "" "" "FILES" ${ARGN})
+  function(create_python_package pkg_dir)
+cmake_parse_arguments(ARG "NOINIT" "" "FILES" ${ARGN})
 if(ARG_FILES)
   set(copy_cmd COMMAND ${CMAKE_COMMAND} -E copy ${ARG_FILES} ${pkg_dir})
 endif()
-add_custom_command(TARGET ${target} POST_BUILD VERBATIM
+if(NOT ARG_NOINIT)
+  set(init_cmd COMMAND ${PYTHON_EXECUTABLE}
+  "${LLDB_SOURCE_DIR}/scripts/Python/createPythonInit.py"
+  "${pkg_dir}" ${ARG_FILES})
+endif()
+add_custom_command(TARGET finish_swig POST_BUILD VERBATIM
   COMMAND ${CMAKE_COMMAND} -E make_directory ${pkg_dir}
   ${copy_cmd}
-  COMMAND ${PYTHON_EXECUTABLE} 
"${LLDB_SOURCE_DIR}/scripts/Python/createPythonInit.py"
-${pkg_dir} ${ARG_FILES}
+  ${init_cmd}
   WORKING_DIRECTORY ${lldb_python_build_path})
   endfunction()
 
@@ -146,28 +140,32 @@ if (NOT LLDB_DISABLE_PYTHON)
 COMMAND ${CMAKE_COMMAND} -E copy
   "${LLDB_SOURCE_DIR}/source/Interpreter/embedded_interpreter.py" 
${lldb_python_build_path})
 
-  create_python_package(finish_swig "formatters/cpp"
+  # Distribute the examples as python packages.
+  create_python_package("formatters/cpp"
 FILES "${LLDB_SOURCE_DIR}/examples/synthetic/gnu_libstdcpp.py"
   "${LLDB_SOURCE_DIR}/examples/synthetic/libcxx.py")
-  # Make an empty __init__.py in lldb/runtime as this is required for
-  # Python to recognize lldb.runtime as a valid package (and hence,
-  # lldb.runtime.objc as a valid contained package)
-  create_python_package(finish_swig "runtime")
-  # Having these files copied here ensure that lldb/formatters is a
-  # valid package itself
-  create_python_package(finish_swig "formatters"
+
+  create_python_package("formatters"
 FILES "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/cache.py"
   "${LLDB_SOURCE_DIR}/examples/summaries/synth.py"
   "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/metrics.py"
   "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/attrib_fromdict.py"
   "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/Logger.py")
-  create_python_package(finish_swig "utils"
+
+  create_python_package("utils"
 FILES "${LLDB_SOURCE_DIR}/examples/python/symbolication.py")
+
   if(APPLE)
-create_python_package(finish_swig "macosx"
+create_python_package("macosx"
   FILES "${LLDB_SOURCE_DIR}/examples/python/crashlog.py"
 "${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap.py")
-create_python_package(finish_swig "diagnose"
+
+create_python_package("macosx/heap"
+  FILES "${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap/heap_find.cpp"
+"${LLDB_SOURCE_DIR}/examples/darwin/heap_find/heap/Makefile"
+NOINIT)
+
+create_python_package("diagnose"
   FILES "${LLDB_SOURCE_DIR}/examples/python/diagnose_unwind.py"
 "${LLDB_SOURCE_DIR}/examples/python/diagnose_nsstring.py")
   endif()



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

[Lldb-commits] [lldb] 8f2c100 - [lldb/CMake] Add in_call_stack to the utilities package

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2019-12-02T13:03:24-08:00
New Revision: 8f2c100f6fa5250b987d11e53d24119bc1f8850e

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

LOG: [lldb/CMake] Add in_call_stack to the utilities package

A subset of the examples are shipped as python packages. Include the
in_call_stack utility.

Added: 


Modified: 
lldb/CMakeLists.txt

Removed: 




diff  --git a/lldb/CMakeLists.txt b/lldb/CMakeLists.txt
index 6b5f7640b336..e66fa49a5114 100644
--- a/lldb/CMakeLists.txt
+++ b/lldb/CMakeLists.txt
@@ -153,7 +153,8 @@ if (NOT LLDB_DISABLE_PYTHON)
   "${LLDB_SOURCE_DIR}/examples/summaries/cocoa/Logger.py")
 
   create_python_package("utils"
-FILES "${LLDB_SOURCE_DIR}/examples/python/symbolication.py")
+FILES "${LLDB_SOURCE_DIR}/examples/python/in_call_stack.py"
+  "${LLDB_SOURCE_DIR}/examples/python/symbolication.py")
 
   if(APPLE)
 create_python_package("macosx"



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

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added subscribers: clayborg, shafik.
shafik added a comment.

In D70846#1763802 , @teemperor wrote:

> This LGTM, but the TODO directly above the change in 
> ClangExpressionDeclMap.cpp worries me a bit. I am not sure how good our test 
> coverage is for calling functions when there are instance methods in scope 
> (e.g., we are in instance method and try to call another instance method).


The comment was added by this change 


I am concerned about regressions since we are not sure how well this is covered.




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1213
 
-target->GetImages().FindFunctions(name, eFunctionNameTypeFull,
+target->GetImages().FindFunctions(name, eFunctionNameTypeFull | 
eFunctionNameTypeBase,
   include_symbols, include_inlines,

I did some archeology here and this was changed from `eFunctionNameTypeBase` to 
`eFunctionNameTypeFull` by [this 
change](https://github.com/llvm/llvm-project/commit/43fe217b119998264be04a773c4770592663c306)
 so maybe @clayborg can chime in here. Although it was a while ago.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70846



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

[Lldb-commits] [PATCH] D70848: [LLDB] Set the right address size on output DataExtractors from ObjectFile

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Symbol/ObjectFile.cpp:480-486
+  size_t ret = data.SetData(m_data, offset, length);
+  // DataExtractor::SetData copies the address byte size from m_data, but
+  // m_data's address byte size is only set from sizeof(void*), and we can't
+  // access subclasses GetAddressByteSize() when setting up m_data in the
+  // constructor.
+  data.SetAddressByteSize(GetAddressByteSize());
+  return ret;

I would vote to make this happen within DataExtractor::SetData(const 
DataExtractor &...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70848



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

[Lldb-commits] [PATCH] D70882: Add environment var LLDBVSCODE_SKIP_INIT_FILES to lldb-vscode

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:64
 
+const bool skip_init_files = getenv("LLDBVSCODE_SKIP_INIT_FILES") != NULL;
+

make singleton function:

```
bool GetSkipInitFiles() {
  static const bool skip_init = getenv("LLDBVSCODE_SKIP_INIT_FILES") != NULL;
  return skip_init;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70882



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

[Lldb-commits] [PATCH] D70907: Change Target::FindBreakpointsByName to use a vector

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In other places, I've used  "XList" to mean the container that manages the 
things contained, and "XCollection" to be a random possibly unrelated 
collection of items.  It doesn't make any sense to have a collection of 
breakpoints from more than one target, so the collection class could ensure 
that, whereas a std::vector is just a random assortment.  But I'm not sure that 
adds enough value to warrant adding a collection container here.  std::vector 
is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70907



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

[Lldb-commits] [PATCH] D70906: [lldb] Move register info "augmentation" from gdb-remote into ABI

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.

Good change, lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70906



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

[Lldb-commits] [PATCH] D70932: [EditLine] Fix RecallHistory that makes it move in the opposite direction.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: jingham, labath, teemperor, clayborg.
Herald added a subscriber: abidh.
Herald added a project: LLDB.

The naming used by editline for the history operations is counter
intuitive to how it's used in lldb for the REPL.

- The H_PREV operation returns the previous element in the history, which is 
**newer** than the current one.
- The H_NEXT operation returns the next element in the history, which is 
**older** than the current one.

This exposed itself as a bug in the REPL where the behavior of up- and
down-arrow was inverted. This wasn't immediately obvious because of how we save
the current "live" entry.

This patch fixes the bug and introduces and enum to wrap the editline
operations that  match the semantics of lldb.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D70932

Files:
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp

Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -97,6 +97,32 @@
   return true;
 }
 
+static int GetOperation(HistoryOperation op) {
+  // The naming used by editline for the history operations is counter
+  // intuitive to how it's used here.
+  //
+  //  - The H_PREV operation returns the previous element in the history, which
+  //is newer than the current one.
+  //
+  //  - The H_NEXT operation returns the next element in the history, which is
+  //older than the current one.
+  //
+  // The naming of the enum entries match the semantic meaning.
+  switch(op) {
+case HistoryOperation::Oldest:
+  return H_FIRST;
+case HistoryOperation::Older:
+  return H_NEXT;
+case HistoryOperation::Current:
+  return H_CURR;
+case HistoryOperation::Newer:
+  return H_PREV;
+case HistoryOperation::Newest:
+  return H_LAST;
+  }
+}
+
+
 EditLineStringType CombineLines(const std::vector &lines) {
   EditLineStringStreamType combined_stream;
   for (EditLineStringType line : lines) {
@@ -423,7 +449,8 @@
   return lines;
 }
 
-unsigned char Editline::RecallHistory(bool earlier) {
+unsigned char Editline::RecallHistory(HistoryOperation op) {
+  assert(op == HistoryOperation::Older || op == HistoryOperation::Newer);
   if (!m_history_sp || !m_history_sp->IsValid())
 return CC_ERROR;
 
@@ -433,27 +460,38 @@
 
   // Treat moving from the "live" entry differently
   if (!m_in_history) {
-if (!earlier)
+switch (op) {
+case HistoryOperation::Newer:
   return CC_ERROR; // Can't go newer than the "live" entry
-if (history_w(pHistory, &history_event, H_FIRST) == -1)
-  return CC_ERROR;
-
-// Save any edits to the "live" entry in case we return by moving forward
-// in history (it would be more bash-like to save over any current entry,
-// but libedit doesn't offer the ability to add entries anywhere except the
-// end.)
-SaveEditedLine();
-m_live_history_lines = m_input_lines;
-m_in_history = true;
+case HistoryOperation::Older: {
+  if (history_w(pHistory, &history_event,
+GetOperation(HistoryOperation::Newest)) == -1)
+return CC_ERROR;
+  // Save any edits to the "live" entry in case we return by moving forward
+  // in history (it would be more bash-like to save over any current entry,
+  // but libedit doesn't offer the ability to add entries anywhere except
+  // the end.)
+  SaveEditedLine();
+  m_live_history_lines = m_input_lines;
+  m_in_history = true;
+} break;
+default:
+  llvm_unreachable("unsupported history direction");
+}
   } else {
-if (history_w(pHistory, &history_event, earlier ? H_PREV : H_NEXT) == -1) {
-  // Can't move earlier than the earliest entry
-  if (earlier)
+if (history_w(pHistory, &history_event, GetOperation(op)) == -1) {
+  switch (op) {
+  case HistoryOperation::Older:
+// Can't move earlier than the earliest entry.
 return CC_ERROR;
-
-  // ... but moving to newer than the newest yields the "live" entry
-  new_input_lines = m_live_history_lines;
-  m_in_history = false;
+  case HistoryOperation::Newer:
+// Moving to newer-than-the-newest entry yields the "live" entry.
+new_input_lines = m_live_history_lines;
+m_in_history = false;
+break;
+  default:
+llvm_unreachable("unsupported history direction");
+  }
 }
   }
 
@@ -468,8 +506,17 @@
 
   // Prepare to edit the last line when moving to previous entry, or the first
   // line when moving to next entry
-  SetCurrentLine(m_current_line_index =
- earlier ? (int)m_input_lines.size() - 1 : 0);
+  switch (op) {
+  case HistoryOperation::Older:
+m_current_line_index = (int)m_input_lines.size() - 1;
+break;
+  case HistoryOperation::Newer:
+m_current_line_index = 0;

[Lldb-commits] [PATCH] D70932: [EditLine] Fix RecallHistory that makes it move in the opposite direction.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

(No test because this code is only used by the REPL)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70932



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

[Lldb-commits] [PATCH] D70885: [lldb] Use explicit lldb commands on tests

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I don't think it is wise to use shortened command names like "br" in a test if 
you aren't testing shortest command string.  We don't guarantee that we will 
keep all currently unique shortened command names unique in perpetuity...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70885



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

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

In D70846#1763731 , @labath wrote:

> There's `lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp`, which 
> probably didn't get run for you as you didn't have lld enabled (cmake 
> -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the 
> new behavior, but other than that, I think this is a good change.


So with this change for the `find-basic-function.cpp` test I no longer see any 
results for the `full` case so we should at least generate a case that has 
results for the `full` case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70846



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

[Lldb-commits] [PATCH] D70722: [lldb/IRExecutionUnit] Stop searching based on demangled names

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Seems reasonable. @teemperor wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70722



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I don't want to add noise to this, but there's a related change that I'd like 
to upstream soon -- ARMv8.3 pointer authentication bits in addresses.  With the 
ARMv8.3 ISA, higher bits that aren't used for addressing can be used to sign & 
authenticate pointers that are saved in memory, where they may be overwritten 
via buffer over-runs et al, to verify that they have not been modified before 
they are used.  I hope to put together a patchset for upstreaming the changes I 
made to lldb to support this soon.

This is similar to the 0th bit set for the lr on Aarch32 code, to indicate that 
the caller function is thumb; when lldb reads the lr value, it has to strip the 
0th bit before it does a symbol lookup to find the caller.  With PAC bits, lldb 
needs to do the same -- clear or set the top bits of the address before symbol 
lookup.

However, the PAC bits give us two complications that the thumb bit does not 
have:  First, the number of bits used for ptrauth are variable - depending on 
how many page tables the operating system is using for processes, we need to 
strip more/fewer bits.  So we need access to a Process object to determine this 
at runtime.  Second, people may need to debug issues with their PAC bits, so we 
want to print the real unadulterated pointer value, and if it resolves to a 
symbol address, we print the stripped value and the symbol.  For instance, 
where we might show the lr value like this for register read:

  lr = 0x0001bfe03b48  libdispatch.dylib`dispatch_mig_server + 272

once authentication bits have been added, we'll show:

  lr = 0x4051f781bfe03b48 (0x0001bfe03b48) 
libdispatch.dylib`dispatch_mig_server + 272

I'm not sure we should try to handle both the thumb bit and pac bits in the 
same mechanism, but it's close enough that I wanted to bring it up.

I currently have my method in the ABI (my method name is poorly chosen, but I'd 
go with StripAuthenticationBits right now if I were putting it in the ABI) 
because I have a weak pointer to the Process from there.

I also added a SBValue::GetValueAsAddress API similar to GetValueAsUnsigned, 
which runs the value through the strip method.  At the API level, script 
writers may want to show the pointer value with PAC bits and/or the actual 
pointer value, depending on what they are doing.

I have calls to this ABI method scattered across lldb, although unlike the 
thumb bit, it won't show up in the DWARF at all.

I haven't thought about moving this StripAuthenticationBits method into 
Architecture - the callers would all need to provide a Process*, but that's how 
everyone gets an ABI already, so it might be straightforward to do that.


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D70840#1765876 , @mstorsjo wrote:

> In D70840#1765219 , @labath wrote:
>
> > debug_aranges is a sort of a lookup table for speeding up address->compile 
> > unit searches. llvm does not generate it by default since, and I think the 
> > reason is that you can usually get the same kind of data from the 
> > DW_AT_ranges attribute of the compile unit.
>
>
> Ok, thanks for the explanation!
>
> On the topic of debug_aranges (but unrelated to this thread), I've been 
> notified about a project, drmingw, a postmortem debugger for the mingw 
> environment (I think), which stumbles on code built by llvm exactly due to 
> the lack of debug_aranges: 
> https://github.com/jrfonseca/drmingw/issues/42#issuecomment-526850588 They 
> speculate that it almost even would be mandated by the dwarf spec.


Yeah, I've had some conversations with folks on a couple of different things 
that have been curious about Clang/LLVM's lack of debug_aranges - my general 
take on it (as one of the people involved in turning off debug_aranges by 
default in LLVM's output - it can still be enabled by -gdwarf-aranges) is that 
they add a significant amount of object size for little benefit compared to the 
ranges on CU DIEs themselves. Clients who read input without aranges for a 
given CU should look in the CU DIE to see if there are ranges there - it should 
add very little overhead to consumers & save some significant debug info size 
(especially for split-dwarf in object sizes (where the relocations still exist 
& take up a fair bit of space - especially with compression enabled, where 
relocations are not compressed)).


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [PATCH] D70934: [Reproducer] Add version check

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: labath.
Herald added subscribers: teemperor, abidh.
Herald added a project: LLDB.

To ensure a reproducer works correctly, the version of LLDB used for capture 
and replay must match. Right now the reproducer already contains the LLDB 
version. However, this is purely informative. LLDB will happily replay a 
reproducer generated with a different version of LLDB, which can cause subtle 
differences.

This patch adds a version check which compares the current LLDB version with 
the one in the reproducer. If the version doesn't match, LLDB will refuse to 
replay. It also adds an escape hatch to make it possible to still replay the 
reproducer without having to mess with the recorded version. This might prove 
useful when you know two versions of LLDB match, even though the version string 
doesn't. This behavior is triggered by passing a new flag 
`-reproducer-skip-version-check` to the lldb driver.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D70934

Files:
  lldb/include/lldb/API/SBReproducer.h
  lldb/source/API/SBReproducer.cpp
  lldb/test/Shell/Reproducer/TestVersionCheck.test
  lldb/tools/driver/Driver.cpp
  lldb/tools/driver/Options.td

Index: lldb/tools/driver/Options.td
===
--- lldb/tools/driver/Options.td
+++ lldb/tools/driver/Options.td
@@ -232,5 +232,7 @@
 def replay: Separate<["--", "-"], "replay">,
   MetaVarName<"">,
   HelpText<"Tells the debugger to replay a reproducer from .">;
+def skip_version_check: F<"reproducer-skip-version-check">,
+  HelpText<"Skip the reproducer version check.">;
 
 def REM : R<["--"], "">;
Index: lldb/tools/driver/Driver.cpp
===
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -797,7 +797,9 @@
 
 llvm::Optional InitializeReproducer(opt::InputArgList &input_args) {
   if (auto *replay_path = input_args.getLastArg(OPT_replay)) {
-if (const char *error = SBReproducer::Replay(replay_path->getValue())) {
+const bool skip_version_check = input_args.hasArg(OPT_skip_version_check);
+if (const char *error =
+SBReproducer::Replay(replay_path->getValue(), skip_version_check)) {
   WithColor::error() << "reproducer replay failed: " << error << '\n';
   return 1;
 }
Index: lldb/test/Shell/Reproducer/TestVersionCheck.test
===
--- /dev/null
+++ lldb/test/Shell/Reproducer/TestVersionCheck.test
@@ -0,0 +1,29 @@
+# REQUIRES: system-darwin
+
+# This tests the reproducer version check.
+
+# RUN: rm -rf %t.repro
+# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out
+# RUN: %lldb -x -b -s %S/Inputs/FileCapture.in --capture --capture-path %t.repro %t.out | FileCheck %s --check-prefix CHECK --check-prefix CAPTURE
+
+# Make sure that replay works.
+# RUN: %lldb --replay %t.repro | FileCheck %s --check-prefix CHECK --check-prefix REPLAY
+
+# Change the reproducer version.
+# RUN: echo "bogus" >> %t.repro/version.txt
+
+# Make sure that replay works.
+# RUN: not %lldb --replay %t.repro 2>&1 | FileCheck %s --check-prefix ERROR
+
+# Make sure that we can circumvent the version check with -reproducer-skip-version-check.
+# RUN: %lldb --replay %t.repro -reproducer-skip-version-check | FileCheck %s --check-prefix CHECK --check-prefix REPLAY
+
+# CAPTURE: testing
+# REPLAY-NOT: testing
+
+# CHECK: Process {{.*}} exited
+
+# CAPTURE: Reproducer is in capture mode.
+# CAPTURE: Reproducer written
+
+# ERROR: error: reproducer replay failed: reproducer capture and replay version don't match
Index: lldb/source/API/SBReproducer.cpp
===
--- lldb/source/API/SBReproducer.cpp
+++ lldb/source/API/SBReproducer.cpp
@@ -22,8 +22,8 @@
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBHostOS.h"
 #include "lldb/API/SBReproducer.h"
-
 #include "lldb/Host/FileSystem.h"
+#include "lldb/lldb-private.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -124,7 +124,7 @@
   return nullptr;
 }
 
-const char *SBReproducer::Replay(const char *path) {
+const char *SBReproducer::Replay(const char *path, bool skip_version_check) {
   static std::string error;
   if (auto e = Reproducer::Initialize(ReproducerMode::Replay, FileSpec(path))) {
 error = llvm::toString(std::move(e));
@@ -137,6 +137,22 @@
 return error.c_str();
   }
 
+  if (!skip_version_check) {
+llvm::Expected version = loader->LoadBuffer();
+if (!version) {
+  error = llvm::toString(version.takeError());
+  return error.c_str();
+}
+if (lldb_private::GetVersion() != llvm::StringRef(*version).rtrim()) {
+  error = "reproducer capture and replay version don't match:\n";
+  error.append("reproducer captured with:\n");
+  error.append(*version);
+  error.append("reproducer replayed with:\n");
+  error.append(lldb_private::GetV

[Lldb-commits] [PATCH] D70882: Add environment var LLDBVSCODE_SKIP_INIT_FILES to lldb-vscode

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 231807.
aadsm added a comment.

Put the logic into a function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70882

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -454,6 +454,11 @@
   }
 }
 
+const bool GetSkipInitFiles() {
+  static const bool skip_init_files = getenv("LLDBVSCODE_SKIP_INIT_FILES") != 
NULL;
+  return skip_init_files;
+}
+
 // "AttachRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 // "type": "object",
@@ -1194,7 +1199,9 @@
 //   }]
 // }
 void request_initialize(const llvm::json::Object &request) {
-  g_vsc.debugger = lldb::SBDebugger::Create(true /*source_init_files*/);
+  const bool skip_init_files = GetSkipInitFiles();
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skip_init_files /*source_init_files*/);
   // Create an empty target right away since we might get breakpoint requests
   // before we are given an executable to launch in a "launch" request, or a
   // executable when attaching to a process by process ID in a "attach"
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -839,13 +839,19 @@
 
 
 class DebugAdaptor(DebugCommunication):
-def __init__(self, executable=None, port=None, init_commands=[]):
+def __init__(
+self, executable=None, port=None, init_commands=[], 
skip_init_files=True
+):
 self.process = None
 if executable is not None:
+env = os.environ.copy()
+if skip_init_files:
+env['LLDBVSCODE_SKIP_INIT_FILES'] = "1"
 self.process = subprocess.Popen([executable],
 stdin=subprocess.PIPE,
 stdout=subprocess.PIPE,
-stderr=subprocess.PIPE)
+stderr=subprocess.PIPE,
+env=env)
 DebugCommunication.__init__(self, self.process.stdout,
 self.process.stdin, init_commands)
 elif port is not None:


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -454,6 +454,11 @@
   }
 }
 
+const bool GetSkipInitFiles() {
+  static const bool skip_init_files = getenv("LLDBVSCODE_SKIP_INIT_FILES") != NULL;
+  return skip_init_files;
+}
+
 // "AttachRequest": {
 //   "allOf": [ { "$ref": "#/definitions/Request" }, {
 // "type": "object",
@@ -1194,7 +1199,9 @@
 //   }]
 // }
 void request_initialize(const llvm::json::Object &request) {
-  g_vsc.debugger = lldb::SBDebugger::Create(true /*source_init_files*/);
+  const bool skip_init_files = GetSkipInitFiles();
+  g_vsc.debugger =
+  lldb::SBDebugger::Create(!skip_init_files /*source_init_files*/);
   // Create an empty target right away since we might get breakpoint requests
   // before we are given an executable to launch in a "launch" request, or a
   // executable when attaching to a process by process ID in a "attach"
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -839,13 +839,19 @@
 
 
 class DebugAdaptor(DebugCommunication):
-def __init__(self, executable=None, port=None, init_commands=[]):
+def __init__(
+self, executable=None, port=None, init_commands=[], skip_init_files=True
+):
 self.process = None
 if executable is not None:
+env = os.environ.copy()
+if skip_init_files:
+env['LLDBVSCODE_SKIP_INIT_FILES'] = "1"
 self.process = subprocess.Popen([executable],
 stdin=subprocess.PIPE,
 stdout=subprocess.PIPE,
-stderr=subprocess.PIPE)
+stderr=subprocess.PIPE,
+env=env)
 DebugCommunication.__init__(self, self.process.stdout,
 self.process.stdin, init_commands)
 elif port is not None:
_

[Lldb-commits] [PATCH] D70848: [LLDB] Set the right address size on output DataExtractors from ObjectFile

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.



Comment at: lldb/source/Symbol/ObjectFile.cpp:480-486
+  size_t ret = data.SetData(m_data, offset, length);
+  // DataExtractor::SetData copies the address byte size from m_data, but
+  // m_data's address byte size is only set from sizeof(void*), and we can't
+  // access subclasses GetAddressByteSize() when setting up m_data in the
+  // constructor.
+  data.SetAddressByteSize(GetAddressByteSize());
+  return ret;

clayborg wrote:
> I would vote to make this happen within DataExtractor::SetData(const 
> DataExtractor &...)
Do you mean that we'd extend `DataExtractor::SetData(const DataExtractor &...)` 
to take a byte address size parameter, or that we'd update `m_data`'s byte 
address size before doing `data.SetData()` here?

Ideally I'd set the right byte address size in `m_data` as soon as it is known 
and available. It's not possible to do this in `ObjectFile`'s constructor, as 
that is called before the subclass is constructed and its virtual methods are 
available, but is there a better point in the lifetime where we could update it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70848



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