[Lldb-commits] [lldb] 0157a74 - [lldb] Fix an asan error from 27df2d9f556c

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-01-22T10:14:47+01:00
New Revision: 0157a74bec3d2ef1fac5b874327b97d2ae8e95c8

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

LOG: [lldb] Fix an asan error from 27df2d9f556c

This error is caused by a combination of a couple of factors:
- the test accidentally creating a list with a single (empty) FileSpec
  instead of an empty list
- lldb overzeleously converting empty strings into nullptrs
- asan overzeleously validating symlink(2) arguments (the real symlink
  call would just fail with EFAULT)

I fix this by using FileSpec::GetPath instead of GetCString. This avoids
the nullptr and also avoids inserting the path into the global string
pool.

I also enhance the test case to test both empty paths and empty lists.

Added: 


Modified: 

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py
lldb/source/Host/posix/FileSystemPosix.cpp

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py
 
b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py
index 006c0bff0838..7735bdaf459c 100644
--- 
a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py
+++ 
b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py
@@ -46,6 +46,18 @@ def test_symlink_paths_set_procselfcwd(self):
 
 @skipIf(hostoslist=["windows"])
 def test_symlink_paths_unset(self):
+pwd_symlink = self.create_src_symlink()
+self.doBuild(pwd_symlink, None)
+src_path = self.getBuildArtifact(_SRC_FILE)
+self.assertRaises(
+AssertionError,
+lldbutil.run_break_set_by_file_and_line,
+self,
+src_path,
+self.line)
+
+@skipIf(hostoslist=["windows"])
+def test_symlink_paths_empty(self):
 pwd_symlink = self.create_src_symlink()
 self.doBuild(pwd_symlink, "")
 src_path = self.getBuildArtifact(_SRC_FILE)
@@ -67,9 +79,11 @@ def create_src_symlink(self):
 def doBuild(self, pwd_symlink, setting_value):
 self.build(None, None, {'PWD': pwd_symlink})
 
-self.runCmd(
-"settings set %s '%s'" %
-(_COMP_DIR_SYM_LINK_PROP, setting_value))
+if setting_value:
+cmd = "settings set %s '%s'" % (_COMP_DIR_SYM_LINK_PROP, 
setting_value)
+else:
+cmd = "settings clear %s"%_COMP_DIR_SYM_LINK_PROP
+self.runCmd(cmd)
 
 exe = self.getBuildArtifact(_EXE_NAME)
 self.runCmd('file ' + exe, CURRENT_EXECUTABLE_SET)

diff  --git a/lldb/source/Host/posix/FileSystemPosix.cpp 
b/lldb/source/Host/posix/FileSystemPosix.cpp
index 32fae68abb4d..30bf9a51eb67 100644
--- a/lldb/source/Host/posix/FileSystemPosix.cpp
+++ b/lldb/source/Host/posix/FileSystemPosix.cpp
@@ -43,7 +43,7 @@ Status FileSystem::Symlink(const FileSpec &src, const 
FileSpec &dst) {
 Status FileSystem::Readlink(const FileSpec &src, FileSpec &dst) {
   Status error;
   char buf[PATH_MAX];
-  ssize_t count = ::readlink(src.GetCString(), buf, sizeof(buf) - 1);
+  ssize_t count = ::readlink(src.GetPath().c_str(), buf, sizeof(buf) - 1);
   if (count < 0)
 error.SetErrorToErrno();
   else {



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

[Lldb-commits] [PATCH] D71770: [lldb] Don't process symlinks deep inside DWARFUnit

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks. I believe this should be fixed by 0157a74be 
. I'll 
check the bot to confirm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71770



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

[Lldb-commits] [lldb] 889a4f5 - [lldb] s/lldb/%lldb in two tests

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-01-22T11:02:29+01:00
New Revision: 889a4f55c9100d55f9c120b8408c16491d73c7b5

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

LOG: [lldb] s/lldb/%lldb in two tests

%lldb is the proper substitution. Using "lldb" can cause us to execute
the system lldb instead of the one we are testing. This happens at least
in standalone builds.

Added: 


Modified: 
lldb/test/Shell/ObjectFile/ELF/PT_LOAD-overlap-PT_TLS.yaml
lldb/test/Shell/ObjectFile/ELF/PT_TLS-overlap-PT_LOAD.yaml

Removed: 




diff  --git a/lldb/test/Shell/ObjectFile/ELF/PT_LOAD-overlap-PT_TLS.yaml 
b/lldb/test/Shell/ObjectFile/ELF/PT_LOAD-overlap-PT_TLS.yaml
index d68446668047..e3261d870396 100644
--- a/lldb/test/Shell/ObjectFile/ELF/PT_LOAD-overlap-PT_TLS.yaml
+++ b/lldb/test/Shell/ObjectFile/ELF/PT_LOAD-overlap-PT_TLS.yaml
@@ -2,7 +2,7 @@
 
 # RUN: yaml2obj %s > %t
 # RUN: lldb-test object-file %t | FileCheck %s
-# RUN: lldb %t -o "image lookup -a 0x1000" -b | FileCheck 
--check-prefix=LOOKUP %s
+# RUN: %lldb %t -o "image lookup -a 0x1000" -b | FileCheck 
--check-prefix=LOOKUP %s
 
 # CHECK:Index: 0
 # CHECK-NEXT:   ID: 0x

diff  --git a/lldb/test/Shell/ObjectFile/ELF/PT_TLS-overlap-PT_LOAD.yaml 
b/lldb/test/Shell/ObjectFile/ELF/PT_TLS-overlap-PT_LOAD.yaml
index cd3939e05415..1cacf55d5374 100644
--- a/lldb/test/Shell/ObjectFile/ELF/PT_TLS-overlap-PT_LOAD.yaml
+++ b/lldb/test/Shell/ObjectFile/ELF/PT_TLS-overlap-PT_LOAD.yaml
@@ -2,7 +2,7 @@
 
 # RUN: yaml2obj %s > %t
 # RUN: lldb-test object-file %t | FileCheck %s
-# RUN: lldb %t -o "image lookup -a 0x1000" -b | FileCheck 
--check-prefix=LOOKUP %s
+# RUN: %lldb %t -o "image lookup -a 0x1000" -b | FileCheck 
--check-prefix=LOOKUP %s
 
 # CHECK:Index: 0
 # CHECK-NEXT:   ID: 0x



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

[Lldb-commits] [PATCH] D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It took me a while, but I tracked this down to the lack of `%` in front of lldb 
in the RUN: commands. We don't have an "lldb" substitution, so this ends up 
running whatever it finds on the path. Normally this does not matter because we 
add the build dir to the lit path, but for some reason this is not happening in 
a standalone build (wild guess: probably we just add the "llvm" build dir).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65282



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

[Lldb-commits] [PATCH] D73119: [lldb/Initializers] Rename plugins to match their entry points

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D73119#1832707 , @JDevlieghere 
wrote:

> I think the remaining discrepancies between the plugin name and the directory 
> make sense. For example, I don't really see the benefit of renaming 
> `AppleObjCRuntime` to `LanguageRuntimeAppleObjeC`. The `ClangASTContext` is 
> the exception, but I really don't want to rename that class :-)


The choice of ClangASTContext seems particularly unfortunate, as:

- the class with that name is not even inside that folder
- that class is about to get renamed to ClangTypeSystem (in D72684 
 -- I'm not sure what is that patch waiting 
for)

Is there even an `Initialize` function in `ExpressionParser/Clang` ? I don't 
see any.. It seems this is one of those not-really-plugins. And if that's the 
case, then you should be able to ignore it for now. Once this becomes a "real" 
plugin, it will get an Initialize function, and we can create an 
"ExpressionParserClang" class to house that function...

As for `AppleObjCRuntime`, I'm not insisting on changing that, though I am 
wondering if that won't get it your way when autogenerating the initalizers. 
I'm not fully sure what are your plans for that. If you're going to generate 
the `#include` lines then it looks like this discrepancy will matter. If you're 
going the "extern" route, then generating `#include` is not needed and you 
headers can be called anything. With the global constructor approach (my 
favourite :P) we wouldn't need to autogenerate anything at all...


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

https://reviews.llvm.org/D73119



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

[Lldb-commits] [PATCH] D73125: [lldb/Plugins] Move entry points out of plugin namespace

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Some of these plugins are very simple (a single class) and so having the 
namespace there is not that useful. However, for the not-so-simple plugins, it 
seems strange to have one part of it be in the namespace, and one part outside. 
E.g., it's unfortunate that that ProcessGDBRemote now has to qualify all 
references to ThreadGDBRemote, as they are both coupled closely together.

Technically, the ProcessGDBRemote class is not really the "entry point" into 
"gdb" plugin -- only its "initialize" function is. Nothing else should access 
the ProcessGDBRemote class directly (except through its base class).

In this sense, it was a mistake to have the "initalize" function be a (static) 
method. It makes it impossible to forward-declare it, and that means that 
anything which includes this file gets exposed to whatever ProcessGDBRemote.h 
includes. IIUC, this was the main reason we created the 
ScriptInterpreterPythonImpl class (which means that now 
ScriptInterpreterPython::Initialize references 
ScriptInterpreterPythonImpl::CreateInstance). Apart from python there are other 
plugins that could potentially cause problems due to 3rd party dependencies and 
their prolific uses of macros, and which might benefit from additional 
compartmentalization.

It seems to me like this would be a good opportunity to solve both of these 
issues and move the Initialize functions out of their classes (or just create a 
function that forwards to these). Those functions can be in the lldb_private 
namespace and have fully predictable names. They are also easily 
forward-declarable, so we don't need to generate the `#include` directives. And 
that in turn means we don't have to rename everything to make things 
super-consistent. And the added benefits are that the plugins are fully 
self-contained and don't have to do second-level pimpling just to avoid 
namespace polution.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D73125



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

[Lldb-commits] [PATCH] D73125: [lldb/Plugins] Move entry points out of plugin namespace

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

A weaker for of that would be to do something like `using ProcessGDBRemote = 
process_gdb_remote::ProcessGDBRemote`, which would require less changes to 
existing plugins, but it would still require us to include all the headers...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D73125



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

[Lldb-commits] [lldb] 3d7177a - [lldb/DWARF] Remove one more auto-dwo method

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-01-22T13:03:57+01:00
New Revision: 3d7177acd751704d42278ea78e5353943187045d

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

LOG: [lldb/DWARF] Remove one more auto-dwo method

Summary:
Our DWARFUnit was automatically forwarding the requests to the split
unit when looking for a DIE by offset. llvm::DWARFUnit does not do that,
and is not likely to start doing it any time soon.

This patch deletes the this logic and updates the callers to request the
correct unit instead. While doing that, I've found a bit of duplicated
code for lookup up a function and block by address, so I've extracted
that into a helper function.

Reviewers: JDevlieghere, aprantl, clayborg, jdoerfert

Subscribers: lldb-commits

Tags: #lldb

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
index 1e04baca2c58..c8014c6231cd 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
@@ -191,7 +191,7 @@ DWARFDIE
 DWARFDebugInfo::GetDIE(const DIERef &die_ref) {
   DWARFUnit *cu = GetUnit(die_ref);
   if (cu)
-return cu->GetDIE(die_ref.die_offset());
+return cu->GetNonSkeletonUnit().GetDIE(die_ref.die_offset());
   return DWARFDIE(); // Not found
 }
 

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
index 4ac07515a21c..ee189f9331e5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -534,9 +534,6 @@ static bool CompareDIEOffset(const DWARFDebugInfoEntry &die,
 DWARFDIE
 DWARFUnit::GetDIE(dw_offset_t die_offset) {
   if (die_offset != DW_INVALID_OFFSET) {
-if (GetDwoSymbolFile())
-  return GetDwoSymbolFile()->GetCompileUnit()->GetDIE(die_offset);
-
 if (ContainsDIEOffset(die_offset)) {
   ExtractDIEsIfNeeded();
   DWARFDebugInfoEntry::const_iterator end = m_die_array.cend();

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 19e15b527903..f025a5794f0c 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1771,6 +1771,32 @@ SymbolFileDWARF::GlobalVariableMap 
&SymbolFileDWARF::GetGlobalAranges() {
   return *m_global_aranges_up;
 }
 
+void SymbolFileDWARF::ResolveFunctionAndBlock(lldb::addr_t file_vm_addr,
+  bool lookup_block,
+  SymbolContext &sc) {
+  assert(sc.comp_unit);
+  DWARFUnit &cu = GetDWARFCompileUnit(sc.comp_unit)->GetNonSkeletonUnit();
+  DWARFDIE function_die = cu.LookupAddress(file_vm_addr);
+  DWARFDIE block_die;
+  if (function_die) {
+sc.function = sc.comp_unit->FindFunctionByUID(function_die.GetID()).get();
+if (sc.function == nullptr)
+  sc.function = ParseFunction(*sc.comp_unit, function_die);
+
+if (sc.function && lookup_block)
+  block_die = function_die.LookupDeepestBlock(file_vm_addr);
+  }
+
+  if (!sc.function || ! lookup_block)
+return;
+
+  Block &block = sc.function->GetBlock(true);
+  if (block_die)
+sc.block = block.FindBlockByID(block_die.GetID());
+  else
+sc.block = block.FindBlockByID(function_die.GetID());
+}
+
 uint32_t SymbolFileDWARF::ResolveSymbolContext(const Address &so_addr,
SymbolContextItem resolve_scope,
SymbolContext &sc) {
@@ -1834,17 +1860,11 @@ uint32_t SymbolFileDWARF::ResolveSymbolContext(const 
Address &so_addr,
 bool force_check_line_table = false;
 if (resolve_scope &
 (eSymbolContextFunction | eSymbolContextBlock)) {
-  DWARFDIE function_die = dwarf_cu->LookupAddress(file_vm_addr);
-  DWARFDIE block_die;
-  if (function_die) {
-sc.function =
-
sc.comp_unit->FindFunctionByUID(function_die.GetID()).get();
-if (sc.function == nullptr)
-  sc.function = ParseFunction(*sc.comp_unit, function_die);
-
-if (sc.function && (resolve_scope & eSymbolContextBlock))
-  block_die = function_die.LookupDeepestBlock(file_vm_addr);
-  } else {
+   

[Lldb-commits] [PATCH] D73112: [lldb/DWARF] Remove one more auto-dwo method

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3d7177acd751: [lldb/DWARF] Remove one more auto-dwo method 
(authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73112

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -381,6 +381,13 @@
   bool ResolveFunction(const DWARFDIE &die, bool include_inlines,
lldb_private::SymbolContextList &sc_list);
 
+  /// Resolve functions and (possibly) blocks for the given file address and a
+  /// compile unit. The compile unit comes from the sc argument and it must be
+  /// set. The results of the lookup (if any) are written back to the symbol
+  /// context.
+  void ResolveFunctionAndBlock(lldb::addr_t file_vm_addr, bool lookup_block,
+   lldb_private::SymbolContext &sc);
+
   virtual lldb::TypeSP
   FindDefinitionTypeForDWARFDeclContext(const DWARFDeclContext &die_decl_ctx);
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1771,6 +1771,32 @@
   return *m_global_aranges_up;
 }
 
+void SymbolFileDWARF::ResolveFunctionAndBlock(lldb::addr_t file_vm_addr,
+  bool lookup_block,
+  SymbolContext &sc) {
+  assert(sc.comp_unit);
+  DWARFUnit &cu = GetDWARFCompileUnit(sc.comp_unit)->GetNonSkeletonUnit();
+  DWARFDIE function_die = cu.LookupAddress(file_vm_addr);
+  DWARFDIE block_die;
+  if (function_die) {
+sc.function = sc.comp_unit->FindFunctionByUID(function_die.GetID()).get();
+if (sc.function == nullptr)
+  sc.function = ParseFunction(*sc.comp_unit, function_die);
+
+if (sc.function && lookup_block)
+  block_die = function_die.LookupDeepestBlock(file_vm_addr);
+  }
+
+  if (!sc.function || ! lookup_block)
+return;
+
+  Block &block = sc.function->GetBlock(true);
+  if (block_die)
+sc.block = block.FindBlockByID(block_die.GetID());
+  else
+sc.block = block.FindBlockByID(function_die.GetID());
+}
+
 uint32_t SymbolFileDWARF::ResolveSymbolContext(const Address &so_addr,
SymbolContextItem resolve_scope,
SymbolContext &sc) {
@@ -1834,17 +1860,11 @@
 bool force_check_line_table = false;
 if (resolve_scope &
 (eSymbolContextFunction | eSymbolContextBlock)) {
-  DWARFDIE function_die = dwarf_cu->LookupAddress(file_vm_addr);
-  DWARFDIE block_die;
-  if (function_die) {
-sc.function =
-sc.comp_unit->FindFunctionByUID(function_die.GetID()).get();
-if (sc.function == nullptr)
-  sc.function = ParseFunction(*sc.comp_unit, function_die);
-
-if (sc.function && (resolve_scope & eSymbolContextBlock))
-  block_die = function_die.LookupDeepestBlock(file_vm_addr);
-  } else {
+  ResolveFunctionAndBlock(file_vm_addr,
+  resolve_scope & eSymbolContextBlock, sc);
+  if (sc.function)
+resolved |= eSymbolContextFunction;
+  else {
 // We might have had a compile unit that had discontiguous
 // address ranges where the gaps are symbols that don't have
 // any debug info. Discontiguous compile unit address ranges
@@ -1853,21 +1873,8 @@
 // of the aranges down.
 force_check_line_table = true;
   }
-
-  if (sc.function != nullptr) {
-resolved |= eSymbolContextFunction;
-
-if (resolve_scope & eSymbolContextBlock) {
-  Block &block = sc.function->GetBlock(true);
-
-  if (block_die)
-sc.block = block.FindBlockByID(block_die.GetID());
-  else
-sc.block = block.FindBlockByID(function_die.GetID());
-  if (sc.block)
-resolved |= eSymbolContextBlock;
-}
-  }
+  if (sc.block)
+resolved |= eSymbolContextBlock;
 }
 
 if ((resolve_scope & eSymbolContextLineEntry) ||

[Lldb-commits] [PATCH] D70646: Move non-DWARF code: `DWARFUnit` -> `SymbolFileDWARF`

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 239571.
jankratochvil retitled this revision from "RFC 2/3: Move non-DWARF code: 
`DWARFUnit` -> `SymbolFileDWARF`" to "Move non-DWARF code: `DWARFUnit` -> 
`SymbolFileDWARF`".
jankratochvil edited the summary of this revision.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70646

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -305,6 +305,27 @@
 
   lldb_private::FileSpec GetFile(DWARFUnit &unit, size_t file_idx);
 
+  static llvm::Expected
+  GetTypeSystem(DWARFUnit &unit);
+
+  static DWARFASTParser *GetDWARFParser(DWARFUnit &unit);
+
+  // CompilerDecl related functions
+
+  static lldb_private::CompilerDecl GetDecl(const DWARFDIE &die);
+
+  static lldb_private::CompilerDeclContext GetDeclContext(const DWARFDIE &die);
+
+  static lldb_private::CompilerDeclContext
+  GetContainingDeclContext(const DWARFDIE &die);
+
+  static void GetDWARFDeclContext(const DWARFDIE &die,
+  DWARFDeclContext &dwarf_decl_ctx);
+
+  static lldb::LanguageType LanguageTypeFromDWARF(uint64_t val);
+
+  static lldb::LanguageType GetLanguage(DWARFUnit &unit);
+
 protected:
   typedef llvm::DenseMap
   DIEToTypePtr;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -688,7 +688,7 @@
   cu_file_spec.SetFile(remapped_file, FileSpec::Style::native);
   }
 
-  LanguageType cu_language = DWARFUnit::LanguageTypeFromDWARF(
+  LanguageType cu_language = SymbolFileDWARF::LanguageTypeFromDWARF(
   cu_die.GetAttributeValueAsUnsigned(DW_AT_language, 0));
 
   bool is_optimized = dwarf_cu.GetNonSkeletonUnit().GetIsOptimized();
@@ -766,8 +766,7 @@
   if (!die.IsValid())
 return nullptr;
 
-  auto type_system_or_err =
-  GetTypeSystemForLanguage(die.GetCU()->GetLanguageType());
+  auto type_system_or_err = GetTypeSystemForLanguage(GetLanguage(*die.GetCU()));
   if (auto err = type_system_or_err.takeError()) {
 LLDB_LOG_ERROR(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_SYMBOLS),
std::move(err), "Unable to parse function");
@@ -799,7 +798,7 @@
   std::lock_guard guard(GetModuleMutex());
   DWARFUnit *dwarf_cu = GetDWARFCompileUnit(&comp_unit);
   if (dwarf_cu)
-return dwarf_cu->GetLanguageType();
+return GetLanguage(*dwarf_cu);
   else
 return eLanguageTypeUnknown;
 }
@@ -1287,7 +1286,7 @@
   // SymbolFileDWARF::GetDIE(). See comments inside the
   // SymbolFileDWARF::GetDIE() for details.
   if (DWARFDIE die = GetDIE(type_uid))
-return die.GetDecl();
+return GetDecl(die);
   return CompilerDecl();
 }
 
@@ -1300,7 +1299,7 @@
   // SymbolFileDWARF::GetDIE(). See comments inside the
   // SymbolFileDWARF::GetDIE() for details.
   if (DWARFDIE die = GetDIE(type_uid))
-return die.GetDeclContext();
+return GetDeclContext(die);
   return CompilerDeclContext();
 }
 
@@ -1311,7 +1310,7 @@
   // SymbolFileDWARF::GetDIE(). See comments inside the
   // SymbolFileDWARF::GetDIE() for details.
   if (DWARFDIE die = GetDIE(type_uid))
-return die.GetContainingDeclContext();
+return GetContainingDeclContext(die);
   return CompilerDeclContext();
 }
 
@@ -1442,7 +1441,7 @@
   dwarf_die.GetID(), dwarf_die.GetTagAsCString(),
   type->GetName().AsCString());
 assert(compiler_type);
-DWARFASTParser *dwarf_ast = dwarf_die.GetDWARFParser();
+DWARFASTParser *dwarf_ast = GetDWARFParser(*dwarf_die.GetCU());
 if (dwarf_ast)
   return dwarf_ast->CompleteTypeFromDWARF(dwarf_die, type, compiler_type);
   }
@@ -2114,7 +2113,7 @@
   sc.comp_unit = GetCompUnitForDWARFCompUnit(*dwarf_cu);
 
   if (parent_decl_ctx) {
-DWARFASTParser *dwarf_ast = die.GetDWARFParser();
+DWARFASTParser *dwarf_ast = GetDWARFParser(*die.GetCU());
 if (dwarf_ast) {
   CompilerDeclContext actual_parent_decl_ctx =
   dwarf_ast->GetDeclContextConta

[Lldb-commits] [PATCH] D72953: Fix the handling of unnamed bit-fields when parsing DWARF

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I just went back to the radar we had about the CG crash and this *does* fix 
that issue (rdar://53932023). I could also not reproduce the CG crash again so 
I assume that was something else that fiddled around in that area. So LGTM 
minus the thing with the `last_bitfield_info` `last_field_info` duality which I 
would prefer we could get rid of before landing (but it's not a must and can be 
done as a follow-up commit).


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

https://reviews.llvm.org/D72953



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

[Lldb-commits] [PATCH] D70645: RFC 1/3: Unify src<->dst DWARFASTParser for CopyUniqueClassMethodTypes()

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil abandoned this revision.
jankratochvil added a comment.

This refactorization is no longer needed as I am abandoning D70647 
 to choose a different way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70645



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

[Lldb-commits] [PATCH] D73191: Ignore methods in full-name function lookup (with accelerators)

[Jaroslav Sevcik via Phabricator via lldb-commits]

jarin created this revision.
jarin added a reviewer: labath.
jarin added a project: LLDB.
Herald added subscribers: lldb-commits, arphaman, aprantl.

In the spirit of https://reviews.llvm.org/D70846, we only return non-methods in 
Apple/DebugNamesDWARFIndex::GetFunction if eFunctionNameTypeFull is requested.

This speeds up lookup in the presence of large amount of methods of the same 
name (a typical examples would be constructors of templates with many 
instantiations or overloaded operators).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73191

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp


Index: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
@@ -58,14 +58,11 @@
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbaz::foo()", mangled = 
"_ZZ5ffbarvEN4sbaz3fooEv"
 
-// FULL-INDEXED: Found 7 functions:
+// FULL-INDEXED: Found 4 functions:
 // FULL-INDEXED-DAG: name = "foo()", mangled = "_Z3foov"
 // FULL-INDEXED-DAG: name = "foo(int)", mangled = "_Z3fooi"
 // FULL-INDEXED-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
 // FULL-INDEXED-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
-// FULL-INDEXED-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
-// FULL-INDEXED-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
-// FULL-INDEXED-DAG: name = "ffbar()::sbaz::foo()", mangled = 
"_ZZ5ffbarvEN4sbaz3fooEv"
 
 // FULL: Found 0 functions:
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
@@ -39,8 +39,8 @@
   if (!SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
 return;
 
-  // In case of a full match, we just insert everything we find.
-  if (name_type_mask & eFunctionNameTypeFull) {
+  // In case of a full match, we insert all non-methods we find.
+  if (name_type_mask & eFunctionNameTypeFull && !die.IsMethod()) {
 dies.push_back(die);
 return;
   }


Index: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
===
--- lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
+++ lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp
@@ -58,14 +58,11 @@
 // METHOD-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
 // METHOD-DAG: name = "ffbar()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
 
-// FULL-INDEXED: Found 7 functions:
+// FULL-INDEXED: Found 4 functions:
 // FULL-INDEXED-DAG: name = "foo()", mangled = "_Z3foov"
 // FULL-INDEXED-DAG: name = "foo(int)", mangled = "_Z3fooi"
 // FULL-INDEXED-DAG: name = "bar::foo()", mangled = "_ZN3bar3fooEv"
 // FULL-INDEXED-DAG: name = "bar::baz::foo()", mangled = "_ZN3bar3baz3fooEv"
-// FULL-INDEXED-DAG: name = "sbar::foo()", mangled = "_ZN4sbar3fooEv"
-// FULL-INDEXED-DAG: name = "sbar::foo(int)", mangled = "_ZN4sbar3fooEi"
-// FULL-INDEXED-DAG: name = "ffbar()::sbaz::foo()", mangled = "_ZZ5ffbarvEN4sbaz3fooEv"
 
 // FULL: Found 0 functions:
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
@@ -39,8 +39,8 @@
   if (!SymbolFileDWARF::DIEInDeclContext(&parent_decl_ctx, die))
 return;
 
-  // In case of a full match, we just insert everything we find.
-  if (name_type_mask & eFunctionNameTypeFull) {
+  // In case of a full match, we insert all non-methods we find.
+  if (name_type_mask & eFunctionNameTypeFull && !die.IsMethod()) {
 dies.push_back(die);
 return;
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D70647: RFC 3/3: Remove DWARFDIE dependency from functions moved by D70646

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil abandoned this revision.
jankratochvil marked an inline comment as done.
jankratochvil added a comment.

I tried to use `DIERef` instead of `DWARFDIE` everywhere as @labath does not 
like to increase `DWARFDIE` size from 16 bytes to 24 bytes.  But that is not 
really feasible. For `DIERef` one needs to also carry `SymbolFileDWARF *` along 
and also resolving `DIERef` into `DWARFDIE` is slow as it has to bisect CUs 
from the DIE offset. I will post a different proposal how to implement DWZ.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70647



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

[Lldb-commits] [lldb] 9dc9f7c - [lldb/Target] Sort CMakeLists (NFC)

[Med Ismail Bennani via lldb-commits]

Author: Med Ismail Bennani
Date: 2020-01-22T16:38:37+01:00
New Revision: 9dc9f7ca145e7fd5fafbdf071a2e5b5914918c04

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

LOG: [lldb/Target] Sort CMakeLists (NFC)

Signed-off-by: Med Ismail Bennani 

Added: 


Modified: 
lldb/source/Target/CMakeLists.txt

Removed: 




diff  --git a/lldb/source/Target/CMakeLists.txt 
b/lldb/source/Target/CMakeLists.txt
index a92951e6a730..12ae2a9dd9da 100644
--- a/lldb/source/Target/CMakeLists.txt
+++ b/lldb/source/Target/CMakeLists.txt
@@ -9,10 +9,10 @@ lldb_tablegen(TargetPropertiesEnum.inc 
-gen-lldb-property-enum-defs
 add_lldb_library(lldbTarget
   ABI.cpp
   ExecutionContext.cpp
-  JITLoader.cpp
-  JITLoaderList.cpp
   InstrumentationRuntime.cpp
   InstrumentationRuntimeStopInfo.cpp
+  JITLoader.cpp
+  JITLoaderList.cpp
   Language.cpp
   LanguageRuntime.cpp
   Memory.cpp



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

[Lldb-commits] [PATCH] D73206: `DWARFASTParserClang::m_decl_ctx_to_die` `DWARFDIE`->`DIERef` for DWZ

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: labath, clayborg.
jankratochvil added a project: LLDB.
Herald added a subscriber: aprantl.
Herald added a reviewer: shafik.

I think this patch is not good. I wrote it to as a solution to @labath's 
request that `DWARFDIE` should not increase in size from 16 bytes to 24 bytes 
(for a new 'main CU' pointer needed for DWZ).
I have found `DWARFDIE` is mostly used only for parameters and autovariables. I 
found (by `grep -r '<.*DWARF\(Base\|\)DIE' lldb`) only two cases of `DWARFDIE` 
in permanent structures:

- `DWARFASTParserClang::m_decl_ctx_to_die` refactored in this patch
- `ElaboratingDIEIterator::m_worklist` but I think it will never grow to any 
more `DWARFDIE`s than a few so it is worse to refactor it to `DIERef` which is 
slower and due to few entries it does not save any memory space.

I think the testcase improvement could be checked-in - after this patch the 
testcase was crashing trying to dereference original `(DWARFDebugInfoEntry 
*)1LL`.
As a replacement of `D70647` and as a solution to implement DWZ support (D40474 
) proposing multiple possibilities how to 
satisfy DWZ's need for `DWARFDIE::m_main_cu`, could you choose one?

1. `DWARFDIE` 16B->24B does not matter as `DWARFDIE` is not stored anywhere - 
maybe one could use this patch in such case.
2. PointerUnion for DWARFDIE::m_cu so that DWARFDIE remains 16B without DWZ.

  class DWARFBaseDIE {
llvm::PointerUnion m_cu;
DWARFDebugInfoEntry *m_die;
  };
  class DWARFUnitPair {`
DWARFUnit *m_cu;
DWARFCompileUnit *m_main_cu;
  };

3. maybe: Compress `DWARFDIE::m_die` into 24-bit index of `DWARFDebugInfoEntry` 
in `DWARFUnit::m_die_array`. Chosen index instead of DIE offset as that should 
more easily fit into 24 bits even for largest CUs. The gained free 8 bits could 
be used for `DW_LANG_*` which is what MainCU is needed in DWZ. This assumes 
32-bit LLDB host platform as they are still supported by LLDB. I would have to 
verify `DW_LANG_*` is all LLDB really needs from the MainCU, it is difficult to 
find it out from my current patchset.

Thanks for a feedback.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73206

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/unittests/Expression/DWARFExpressionTest.cpp
  lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
  lldb/unittests/TestingSupport/YAMLModuleTester.h

Index: lldb/unittests/TestingSupport/YAMLModuleTester.h
===
--- /dev/null
+++ lldb/unittests/TestingSupport/YAMLModuleTester.h
@@ -0,0 +1,146 @@
+//===- YAMLModuleTester.h ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLDB_UNITTESTS_TESTINGSUPPORT_YAMLMODULETESTER_H
+#define LLDB_UNITTESTS_TESTINGSUPPORT_YAMLMODULETESTER_H
+
+#include "Plugins/SymbolFile/DWARF/DWARFUnit.h"
+#include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "lldb/Core/Module.h"
+#include "lldb/Core/Section.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Symbol/ClangASTContext.h"
+#include "llvm/ObjectYAML/DWARFEmitter.h"
+
+namespace lldb_private {
+
+/// A mock module holding an object file parsed from YAML.
+class YAMLModule : public lldb_private::Module {
+public:
+  YAMLModule(ArchSpec &arch) : Module(FileSpec("test"), arch) {}
+  void SetObjectFile(lldb::ObjectFileSP obj_file) { m_objfile_sp = obj_file; }
+  ObjectFile *GetObjectFile() override { return m_objfile_sp.get(); }
+};
+
+/// A mock object file that can be parsed from YAML.
+class YAMLObjectFile : public lldb_private::ObjectFile {
+  const lldb::ModuleSP m_module_sp;
+  llvm::StringMap> &m_section_map;
+  /// Because there is only one DataExtractor in the ObjectFile
+  /// interface, all sections are copied into a contiguous buffer.
+  std::vector m_buffer;
+
+public:
+  YAMLObjectFile(const lldb::ModuleSP &module_sp,
+ llvm::StringMap> &map)
+  : ObjectFile(module_sp, &module_sp->GetFileSpec(), /*file_offset*/ 0,
+   /*length*/ 0, /*data_sp*/ nullptr, /*data_offset*/ 0),
+m_module_sp(module_sp), m_section_map(map) {}
+
+  /// Callback for initializing the module's list of sections.
+  void CreateSections(SectionList &unified_section_list) override {
+lldb::offset_t total_bytes = 0;
+for (auto &entry : m_section_map)
+  total_bytes += entry.getValue()->getBufferSize();
+m_buffer.reserve(total_bytes);
+m_data =
+DataExtractor(m_buffer.data(), total_bytes, lldb::eByteOrderLittle

[Lldb-commits] [PATCH] D73119: [lldb/Initializers] Rename plugins to match their entry points

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D73119#1833232 , @labath wrote:

> As for `AppleObjCRuntime`, I'm not insisting on changing that, though I am 
> wondering if that won't get it your way when autogenerating the initalizers. 
> I'm not fully sure what are your plans for that. If you're going to generate 
> the `#include` lines then it looks like this discrepancy will matter. If 
> you're going the "extern" route, then generating `#include` is not needed and 
> you headers can be called anything. With the global constructor approach (my 
> favourite :P) we wouldn't need to autogenerate anything at all...


As much as I personally like the idea of the global constructors, I don't see 
how it could work. There's no guarantee in initialization order, so you might 
end up initializing some of the plugins before the plugin manager itself is 
initialized. Although definitely a bug, it has come up in the past that the 
initialization order amongst the plugins matters as well. Finally, it doesn't 
offer a solution to terminating them again. Maybe you've already thought about 
all this?


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

https://reviews.llvm.org/D73119



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

[Lldb-commits] [PATCH] D73119: [lldb/Initializers] Rename plugins to match their entry points

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D73119#1834279 , @JDevlieghere 
wrote:

> In D73119#1833232 , @labath wrote:
>
> > As for `AppleObjCRuntime`, I'm not insisting on changing that, though I am 
> > wondering if that won't get it your way when autogenerating the 
> > initalizers. I'm not fully sure what are your plans for that. If you're 
> > going to generate the `#include` lines then it looks like this discrepancy 
> > will matter. If you're going the "extern" route, then generating `#include` 
> > is not needed and you headers can be called anything. With the global 
> > constructor approach (my favourite :P) we wouldn't need to autogenerate 
> > anything at all...
>
>
> As much as I personally like the idea of the global constructors, I don't see 
> how it could work. There's no guarantee in initialization order, so you might 
> end up initializing some of the plugins before the plugin manager itself is 
> initialized. Although definitely a bug, it has come up in the past that the 
> initialization order amongst the plugins matters as well. Finally, it doesn't 
> offer a solution to terminating them again. Maybe you've already thought 
> about all this?


Registering a plugin consists of putting some pointers into a vector. We just 
need to make sure the vector is initiailized on first use (which I think it 
already is) instead of being a global variable. If we wanted to make plugin 
order explicit we could make each plugin provide some sort of a priority for 
itself, and sort the vector based on that (but I don't think we really need to 
do that).

As for termination, that can be solved by passing one additional function 
pointer in the initialization routine -- the Terminate function. The plugin 
manager can then automatically call this for all registered plugins. (This 
would be a nice cleanup independently of everything else.)

That said, I have now found one snag with this approach. The way that the build 
is setup right now, each plugin first becomes an archive file, which then gets 
included into liblldb. If liblldb does not require any symbol from the archive 
file (global constructors don't count), the linker will just ignore it. That is 
fixable, but it requires either some platform-specific flags 
(-Wl,--whole-archive for elf), or playing with cmake OBJECT libraries, which 
look like they could support this, but I don't fully understand how they work. 
Both of these things make this approach less appealing than I originally 
thought...


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

https://reviews.llvm.org/D73119



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

[Lldb-commits] [lldb] 89c8866 - Convert AssertTrue( A == B) to AssertEqual(A, B) in TestObjCStepping.py.

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2020-01-22T13:20:15-08:00
New Revision: 89c8866c0417a415ab546aa870569308f15b0ec8

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

LOG: Convert AssertTrue( A == B) to AssertEqual(A, B) in TestObjCStepping.py.

Added: 


Modified: 

lldb/packages/Python/lldbsuite/test/lang/objc/objc-stepping/TestObjCStepping.py

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/lang/objc/objc-stepping/TestObjCStepping.py
 
b/lldb/packages/Python/lldbsuite/test/lang/objc/objc-stepping/TestObjCStepping.py
index bf80995b109d..8fb485847b44 100644
--- 
a/lldb/packages/Python/lldbsuite/test/lang/objc/objc-stepping/TestObjCStepping.py
+++ 
b/lldb/packages/Python/lldbsuite/test/lang/objc/objc-stepping/TestObjCStepping.py
@@ -106,21 +106,21 @@ def test_with_python_api(self):
 # Now step in, that should leave us in the Source randomMethod:
 thread.StepInto()
 line_number = thread.GetFrameAtIndex(0).GetLineEntry().GetLine()
-self.assertTrue(
-line_number == self.source_randomMethod_line,
+self.assertEqual(
+line_number, self.source_randomMethod_line,
 "Stepped into Source randomMethod.")
 
 # Now step in again, through the super call, and that should leave us
 # in the SourceBase randomMethod:
 thread.StepInto()
 line_number = thread.GetFrameAtIndex(0).GetLineEntry().GetLine()
-self.assertTrue(
-line_number == self.sourceBase_randomMethod_line,
+self.assertEqual(
+line_number, self.sourceBase_randomMethod_line,
 "Stepped through super into SourceBase randomMethod.")
 
 threads = lldbutil.continue_to_breakpoint(process, break2)
-self.assertTrue(
-len(threads) == 1,
+self.assertEqual(
+len(threads), 1,
 "Continued to second breakpoint in main.")
 
 # Again, step in twice gets us to a stret method and a stret super
@@ -128,21 +128,21 @@ def test_with_python_api(self):
 thread = threads[0]
 thread.StepInto()
 line_number = thread.GetFrameAtIndex(0).GetLineEntry().GetLine()
-self.assertTrue(
-line_number == self.source_returnsStruct_start_line,
+self.assertEqual(
+line_number, self.source_returnsStruct_start_line,
 "Stepped into Source returnsStruct.")
 
 threads = lldbutil.continue_to_breakpoint(
 process, break_returnStruct_call_super)
-self.assertTrue(
-len(threads) == 1,
+self.assertEqual(
+len(threads), 1,
 "Stepped to the call super line in Source returnsStruct.")
 thread = threads[0]
 
 thread.StepInto()
 line_number = thread.GetFrameAtIndex(0).GetLineEntry().GetLine()
-self.assertTrue(
-line_number == self.sourceBase_returnsStruct_start_line,
+self.assertEqual(
+line_number, self.sourceBase_returnsStruct_start_line,
 "Stepped through super into SourceBase returnsStruct.")
 
 # Cool now continue to get past the call that initializes the 
Observer, and then do our steps in again to see that
@@ -150,8 +150,8 @@ def test_with_python_api(self):
 # object.
 
 threads = lldbutil.continue_to_breakpoint(process, break3)
-self.assertTrue(
-len(threads) == 1,
+self.assertEqual(
+len(threads), 1,
 "Continued to third breakpoint in main, our object should now be 
swizzled.")
 
 newClassName = mySource_isa.GetSummary()
@@ -168,21 +168,21 @@ def test_with_python_api(self):
 thread = threads[0]
 thread.StepInto()
 line_number = thread.GetFrameAtIndex(0).GetLineEntry().GetLine()
-self.assertTrue(
-line_number == self.source_randomMethod_line,
+self.assertEqual(
+line_number, self.source_randomMethod_line,
 "Stepped into Source randomMethod in swizzled object.")
 
 # Now step in again, through the super call, and that should leave us
 # in the SourceBase randomMethod:
 thread.StepInto()
 line_number = thread.GetFrameAtIndex(0).GetLineEntry().GetLine()
-self.assertTrue(
-line_number == self.sourceBase_randomMethod_line,
+self.assertEqual(
+line_number, self.sourceBase_randomMethod_line,
 "Stepped through super into SourceBase randomMethod in swizzled 
object.")
 
 threads = lldbutil.continue_to_breakpoint(process, break4)
-self.assertTrue(
-len(threads) == 1,
+self.assertEqual(
+len(threads), 1,
 "Continued to fourth breakpoint in main.")
  

[Lldb-commits] [lldb] 536612d - [lldb/Test] Use lit's capabilities to skip lldb-repro tests.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T13:24:12-08:00
New Revision: 536612df4b499c7338719ab8a31973f086bff590

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

LOG: [lldb/Test] Use lit's capabilities to skip lldb-repro tests.

This allows us to skip the reproducer tests themselves as a whole as
well as individual tests with the UNSUPPORTED keyword.

Added: 


Modified: 
lldb/test/Shell/Process/TestEnvironment.test
lldb/test/Shell/Quit/TestQuitExitCode-30.test
lldb/test/Shell/Quit/TestQuitExitCode30.test
lldb/test/Shell/Quit/TestQuitExitCodeHexA.test
lldb/test/Shell/Reproducer/lit.local.cfg
lldb/test/Shell/lit.cfg.py

Removed: 




diff  --git a/lldb/test/Shell/Process/TestEnvironment.test 
b/lldb/test/Shell/Process/TestEnvironment.test
index a9c624b8a4ec..e6d6e56fc920 100644
--- a/lldb/test/Shell/Process/TestEnvironment.test
+++ b/lldb/test/Shell/Process/TestEnvironment.test
@@ -1,4 +1,5 @@
 UNSUPPORTED: system-windows
+UNSUPPORTED: lldb-repro
 
 The double quotes around "BAR" ensure we don't match the command.
 

diff  --git a/lldb/test/Shell/Quit/TestQuitExitCode-30.test 
b/lldb/test/Shell/Quit/TestQuitExitCode-30.test
index 2f15398c7614..b0b02bdf7004 100644
--- a/lldb/test/Shell/Quit/TestQuitExitCode-30.test
+++ b/lldb/test/Shell/Quit/TestQuitExitCode-30.test
@@ -1,3 +1,4 @@
 # UNSUPPORTED: system-windows
+# UNSUPPORTED: lldb-repro
 # RUN: %python %S/expect_exit_code.py 226 %lldb -b -s %s
 q -30

diff  --git a/lldb/test/Shell/Quit/TestQuitExitCode30.test 
b/lldb/test/Shell/Quit/TestQuitExitCode30.test
index e5ff634e7136..92ad3c6d1fe4 100644
--- a/lldb/test/Shell/Quit/TestQuitExitCode30.test
+++ b/lldb/test/Shell/Quit/TestQuitExitCode30.test
@@ -1,3 +1,4 @@
 # UNSUPPORTED: system-windows
+# UNSUPPORTED: lldb-repro
 # RUN: %python %S/expect_exit_code.py 30  %lldb -b -s %s
 q 30

diff  --git a/lldb/test/Shell/Quit/TestQuitExitCodeHexA.test 
b/lldb/test/Shell/Quit/TestQuitExitCodeHexA.test
index ca0e2d5acc3b..59b7103ad086 100644
--- a/lldb/test/Shell/Quit/TestQuitExitCodeHexA.test
+++ b/lldb/test/Shell/Quit/TestQuitExitCodeHexA.test
@@ -1,3 +1,4 @@
 # UNSUPPORTED: system-windows
+# UNSUPPORTED: lldb-repro
 # RUN: %python %S/expect_exit_code.py 10 %lldb -b -s %s
 q 0xA

diff  --git a/lldb/test/Shell/Reproducer/lit.local.cfg 
b/lldb/test/Shell/Reproducer/lit.local.cfg
index f40d81c38099..7f4022768c87 100644
--- a/lldb/test/Shell/Reproducer/lit.local.cfg
+++ b/lldb/test/Shell/Reproducer/lit.local.cfg
@@ -6,5 +6,5 @@ if 'LLVM_DISABLE_CRASH_REPORT' in config.environment:
 if 'LLDB_CAPTURE_REPRODUCER' in config.environment:
   del config.environment['LLDB_CAPTURE_REPRODUCER']
 
-if hasattr(config, 'skip_reproducer_test') and config.skip_reproducer_test:
+if 'lldb-repro' in config.available_features:
   config.unsupported = True

diff  --git a/lldb/test/Shell/lit.cfg.py b/lldb/test/Shell/lit.cfg.py
index 271ca7b2a138..3bda378060af 100644
--- a/lldb/test/Shell/lit.cfg.py
+++ b/lldb/test/Shell/lit.cfg.py
@@ -48,7 +48,7 @@
 # just captured reproducer.
 lldb_repro_mode = lit_config.params.get('lldb-run-with-repro', None)
 if lldb_repro_mode:
-  config.skip_reproducer_test = True
+  config.available_features.add('lldb-repro')
   lit_config.note("Running Shell test with lldb-repo in {} 
mode.".format(lldb_repro_mode))
   toolchain.use_lldb_repro_substitutions(config, lldb_repro_mode)
 



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

[Lldb-commits] [lldb] 31662e6 - [lldb/Util] Fix lldb-repro now it doesn't take a path to lldb

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T13:24:12-08:00
New Revision: 31662e67e089264dabc9d1f915aa1d7b4d51c0a3

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

LOG: [lldb/Util] Fix lldb-repro now it doesn't take a path to lldb

The indices into the arguments array were off because we no longer pass
the path to lldb as the first argument.

Added: 


Modified: 
lldb/utils/lldb-repro/lldb-repro.py

Removed: 




diff  --git a/lldb/utils/lldb-repro/lldb-repro.py 
b/lldb/utils/lldb-repro/lldb-repro.py
index c925c474619e..f7579e8d14d3 100755
--- a/lldb/utils/lldb-repro/lldb-repro.py
+++ b/lldb/utils/lldb-repro/lldb-repro.py
@@ -21,17 +21,17 @@
 
 
 def help():
-print("usage: {} capture|replay [args]".fmt(sys.argv[0]))
+print("usage: {} capture|replay [args]".format(sys.argv[0]))
 
 
 def main():
-if len(sys.argv) < 3:
+if len(sys.argv) < 2:
 help()
 return 1
 
 # Compute a hash based on the input arguments and the current working
 # directory.
-args = ' '.join(sys.argv[3:])
+args = ' '.join(sys.argv[2:])
 cwd = os.getcwd()
 input_hash = str(hash((cwd, args)))
 
@@ -40,15 +40,15 @@ def main():
 
 # Create a new lldb invocation with capture or replay enabled.
 lldb = os.path.join(os.path.dirname(sys.argv[0]), 'lldb')
-new_args = [sys.argv[1]]
-if sys.argv[2] == "replay":
+new_args = [lldb]
+if sys.argv[1] == "replay":
 new_args.extend(['--replay', reproducer_path])
-elif sys.argv[2] == "capture":
+elif sys.argv[1] == "capture":
 new_args.extend([
 '--capture', '--capture-path', reproducer_path,
 '--reproducer-auto-generate'
 ])
-new_args.extend(sys.argv[1:])
+new_args.extend(sys.argv[2:])
 else:
 help()
 return 1



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

[Lldb-commits] [PATCH] D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: friss, JDevlieghere.
Herald added subscribers: lldb-commits, aprantl.
Herald added a project: LLDB.

Clang added a new feature to the ObjC compiler that will translate method
calls to commonly un-overridden methods into a function that checks whether
the method is overridden anywhere and if not directly dispatches to the
NSObject implementation.

  

That means if you do override any of these methods, "step-in" will not step
into your code, since we hit the wrapper function, which has no debug info,
and immediately step out again.

  

Add code to recognize these functions as "trampolines" and a thread plan that
will get us from the function to the user code, if overridden.

Note, this is not the same feature as the objc_direct attribute, whereby an 
ObjC developer can state that a certain method will not be overridden so that 
the compiler will issue a direct call to the implementation function.  That 
actually shouldn't need debugger support for stepping, since we will step 
directly into the implementation function, not into some ObjC runtime shim.  So 
we'll either stop or not correctly based on whether the implementation function 
has debug info or not.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73225

Files:
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanStepInRange.h
  lldb/packages/Python/lldbsuite/test/lang/objc/direct-dispatch-step/Makefile
  
lldb/packages/Python/lldbsuite/test/lang/objc/direct-dispatch-step/TestObjCDirectDispatchStepping.py
  
lldb/packages/Python/lldbsuite/test/lang/objc/direct-dispatch-step/stepping-tests.m
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.h
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h

Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
@@ -12,6 +12,9 @@
 #include "AppleObjCTrampolineHandler.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Target/ThreadPlan.h"
+#include "lldb/Target/ThreadPlanStepInRange.h"
+#include "lldb/Target/ThreadPlanStepOut.h"
+#include "lldb/Target/ThreadPlanShouldStopHere.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-types.h"
 
@@ -20,7 +23,7 @@
 class AppleThreadPlanStepThroughObjCTrampoline : public ThreadPlan {
 public:
   AppleThreadPlanStepThroughObjCTrampoline(
-  Thread &thread, AppleObjCTrampolineHandler *trampoline_handler,
+  Thread &thread, AppleObjCTrampolineHandler &trampoline_handler,
   ValueList &values, lldb::addr_t isa_addr, lldb::addr_t sel_addr,
   bool stop_others);
 
@@ -52,25 +55,63 @@
 private:
   bool InitializeFunctionCaller();
 
-  AppleObjCTrampolineHandler *m_trampoline_handler; // FIXME - ensure this
+  AppleObjCTrampolineHandler &m_trampoline_handler; // FIXME - ensure this
 // doesn't go away on us?
 // SP maybe?
   lldb::addr_t m_args_addr; // Stores the address for our step through function
 // result structure.
-  // lldb::addr_t m_object_addr;  // This is only for Description.
   ValueList m_input_values;
   lldb::addr_t m_isa_addr; // isa_addr and sel_addr are the keys we will use to
// cache the implementation.
   lldb::addr_t m_sel_addr;
   lldb::ThreadPlanSP m_func_sp; // This is the function call plan.  We fill it
-// at start, then set it
-  // to NULL when this plan is done.  That way we know to go to:
+// at start, then set it to NULL when this plan
+// is done.  That way we know to go on to:
   lldb::ThreadPlanSP m_run_to_sp;  // The plan that runs to the target.
   FunctionCaller *m_impl_function; // This is a pointer to a impl function that
-  // is owned by the client that pushes this plan.
+   // is owned by the client that pushes this 
+   // plan.
   bool m_stop_others;
 };
 
+class AppleThreadPlanStepThroughDirectDispatch 
+: public ThreadPlanStepOut {
+public:
+  AppleThreadPlanStepThroughDirectDispatch(
+  Thread &thread, AppleObjCTrampolineHandler &handler,
+  llvm::StringRef dispatch_func_name, bool stop_others,
+  LazyBool step_in_avoids_code_without_debug_info);
+
+  ~AppleThreadPlan

[Lldb-commits] [lldb] f55b033 - [TestStdModuleSysroot] Only run locally.

[Davide Italiano via lldb-commits]

Author: Davide Italiano
Date: 2020-01-22T13:56:51-08:00
New Revision: f55b033c028019653fed8fc685b1d33bf529b92b

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

LOG: [TestStdModuleSysroot] Only run locally.

Added: 


Modified: 

lldb/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
 
b/lldb/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
index 684e287a69ed..452e8d697f2d 100644
--- 
a/lldb/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
+++ 
b/lldb/packages/Python/lldbsuite/test/commands/expression/import-std-module/sysroot/TestStdModuleSysroot.py
@@ -16,6 +16,7 @@ class ImportStdModule(TestBase):
 # test configurations where libc++ is actually supposed to be tested.
 @add_test_categories(["libc++"])
 @skipIf(compiler=no_match("clang"))
+@skipIfRemote # This test messes with the platform, can't be run remotely.
 def test(self):
 self.build()
 



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

[Lldb-commits] [PATCH] D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:786
+
+  m_opt_dispatch_map.insert(std::pair(sym_addr, i));
+}

You could simplify this by using `.emplace(sym_addr, i)`. 



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:889
+  pos = m_msgSend_map.find(addr);
+  if (pos != m_msgSend_map.end()) {
+return_ptr = &g_dispatch_functions[(*pos).second];

If you rewrite this you can get rid of the return_ptr temporary. 

```
if (pos != m_msgSend_map.end()) {
return &g_dispatch_functions[(*pos).second];
}
return nullptr;
```



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:918
 
-  MsgsendMap::iterator pos;
-  pos = m_msgSend_map.find(curr_pc);
-  if (pos != m_msgSend_map.end()) {
-this_dispatch = g_dispatch_functions[(*pos).second];
-found_it = true;
-  }
-
+  this_dispatch = FindDispatchFunction(curr_pc);
+  

Can you initialize this on line 910?



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:1200
+  if (!this_dispatch && !ret_plan_sp) {
+const char *opt_name = nullptr;
+MsgsendMap::iterator pos;

Same here, why not do the assignment when you declare the variables?



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:202
+
+// objc uses optimized dispatch functions for some common and seldom overridden
+// methods.  For instance 

`/// Objective-C ...`



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:317
+  else
+stop_reason = stop_info_sp->GetStopReason();
+

```
StopReason stop_reason = stop_info_sp ? stop_info_sp->GetStopReason() : 
eStopReasonNone;
```



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:320
+  // See if this is one of our msgSend breakpoints:
+  if (stop_reason == eStopReasonBreakpoint) {
+ProcessSP process_sp = GetThread().GetProcess();

I'd invert the condition and make this an early return with the comment below. 



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:358
+bool AppleThreadPlanStepThroughDirectDispatch::ShouldStop(Event *event_ptr) {
+
+  // If step out plan finished, that means we didn't find our way into a 
method 

Spurious empty line



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:364
+  bool step_out_should_stop = ThreadPlanStepOut::ShouldStop(event_ptr);
+  if (step_out_should_stop) {
+SetPlanComplete(true);

Looks like `step_out_should_stop ` isn't used anywhere else? 

```if(ThreadPlanStepOut::ShouldStop(event_ptr)) {```



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:67
   lldb::addr_t m_sel_addr;
   lldb::ThreadPlanSP m_func_sp; // This is the function call plan.  We fill it
+// at start, then set it to NULL when this plan

These should be Doxygen comments before the variable if you're already touching 
them...

```
/// This is the function call plan.  We fill it at the start, then ...
lldb::ThreadPlanSP m_func_sp;
```



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:77
 
+class AppleThreadPlanStepThroughDirectDispatch 
+: public ThreadPlanStepOut {

Can you please clang-format the patch, this should fit on one line.



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:93
+
+  // The base class MischiefManaged does some cleanup - so you have to call it
+  // in your MischiefManaged derived class.

`///` 



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:110
+  std::vector m_msgSend_bkpts;
+  bool m_at_msg_send = false;
+  bool m_stop_others;

Let's initialize this in the ctor as well, the inconsistency with the next line 
leaves me wondering if this is a bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73225



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-b

[Lldb-commits] [lldb] 4c2b0a6 - [lldb/Utility] Don't forward directories to the file collector

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T15:01:53-08:00
New Revision: 4c2b0a63661576c1849862bad3978050fc6a2ff7

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

LOG: [lldb/Utility] Don't forward directories to the file collector

The VFS mapping writer assumes that all the paths it gets are files.
When passed a directory, it ends up as a file in the VFS mapping twice,
once as a file and once as a directory.

  {
'type': 'file',
'name': "Output",
'external-contents': "/root/path/to/Output"
  },
  {
'type': 'directory',
'name': "Output",
'contents': [ ... ]
  }

Added: 


Modified: 
lldb/include/lldb/Host/FileSystem.h
lldb/source/Host/common/FileSystem.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/FileSystem.h 
b/lldb/include/lldb/Host/FileSystem.h
index 528c43519a32..af1bbbd1c397 100644
--- a/lldb/include/lldb/Host/FileSystem.h
+++ b/lldb/include/lldb/Host/FileSystem.h
@@ -187,6 +187,7 @@ class FileSystem {
   }
 
 private:
+  void AddFile(const llvm::Twine &file);
   static llvm::Optional &InstanceImpl();
   llvm::IntrusiveRefCntPtr m_fs;
   std::shared_ptr m_collector;

diff  --git a/lldb/source/Host/common/FileSystem.cpp 
b/lldb/source/Host/common/FileSystem.cpp
index 2db5bff3207f..7f15a6bdb094 100644
--- a/lldb/source/Host/common/FileSystem.cpp
+++ b/lldb/source/Host/common/FileSystem.cpp
@@ -279,8 +279,7 @@ void FileSystem::Resolve(FileSpec &file_spec) {
 std::shared_ptr
 FileSystem::CreateDataBuffer(const llvm::Twine &path, uint64_t size,
  uint64_t offset) {
-  if (m_collector)
-m_collector->addFile(path);
+  AddFile(path);
 
   const bool is_volatile = !IsLocal(path);
   const ErrorOr external_path = GetExternalPath(path);
@@ -418,8 +417,7 @@ static mode_t GetOpenMode(uint32_t permissions) {
 Expected FileSystem::Open(const FileSpec &file_spec,
   File::OpenOptions options,
   uint32_t permissions, bool should_close_fd) {
-  if (m_collector)
-m_collector->addFile(file_spec.GetPath());
+  AddFile(file_spec.GetPath());
 
   const int open_flags = GetOpenFlags(options);
   const mode_t open_mode =
@@ -466,3 +464,9 @@ ErrorOr FileSystem::GetExternalPath(const 
llvm::Twine &path) {
 ErrorOr FileSystem::GetExternalPath(const FileSpec &file_spec) {
   return GetExternalPath(file_spec.GetPath());
 }
+
+void FileSystem::AddFile(const llvm::Twine &file) {
+  if (m_collector && !llvm::sys::fs::is_directory(file)) {
+m_collector->addFile(file);
+  }
+}



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

[Lldb-commits] [PATCH] D73148: [lldb/Value] Report size of Value as size of underlying data buffer

[Vedant Kumar via Phabricator via lldb-commits]

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

In D73148#1832897 , @clayborg wrote:

> Actually it would be nice to have a test that will trigger on at least one 
> build bot that runs ASAN?


I'll add an end-to-end test for DW_OP_piece, though I worry it might be brittle.

As I was testing this more, I realized that CommandObjectFrameVariable may 
construct an empty Value and call GetValueByteSize on it. This patch breaks 
that code path because it makes the reported size 0 (instead of whatever 
GetCompilerType().GetByteSize() would be). I'm not sure what the best fix is. 
Some options:

1. Special-case 0-sized eValueTypeHostAddress Values, and fall back to the 
compiler type in this case.
2. Just make DW_OP_piece produce a type-sized buffer (by padding with zeroes 
when needed), as Greg suggested earlier.

Any advice appreciated. I'm leaning towards (1) as it's a simpler fix (and I've 
tested it out on an end-to-end example).


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

https://reviews.llvm.org/D73148



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

[Lldb-commits] [lldb] 6ae61f7 - [lldb/Test] Skip script interpreter tests reading from stdin for lldb-repro

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T15:22:25-08:00
New Revision: 6ae61f7675d65e767662873e87de088b0ae2dc3e

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

LOG: [lldb/Test] Skip script interpreter tests reading from stdin for lldb-repro

The reproducers currently only shadow the command interpreter. It would
be possible to make it work for the Lua interpreter which uses the
IOHandlerEditline under the hood, but the Python one runs a REPL in
Python itself so there's no (straightforward) way to shadow that.

Given that we already capture any API calls, this isn't super high on my
list of priorities.

Added: 


Modified: 
lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
lldb/test/Shell/ScriptInterpreter/Python/crashlog.test
lldb/test/Shell/ScriptInterpreter/Python/scripted_breakpoint.test

Removed: 




diff  --git a/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
index aeafbecaa83f..db0df1694e9e 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/bindings.test
@@ -1,4 +1,6 @@
 # REQUIRES: lua
+# UNSUPPORTED: lldb-repro
+#
 # RUN: cat %s | %lldb --script-language lua 2>&1 | FileCheck %s
 script
 debugger = lldb.SBDebugger.Create()

diff  --git a/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test 
b/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
index e962fc7f5ec7..1b26bd0e2ead 100644
--- a/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
+++ b/lldb/test/Shell/ScriptInterpreter/Lua/convenience_variables.test
@@ -1,4 +1,5 @@
 # REQUIRES: lua
+# UNSUPPORTED: lldb-repro
 #
 # This tests that the convenience variables are not nil. Given that there is no
 # target we only expect the debugger to be valid.

diff  --git a/lldb/test/Shell/ScriptInterpreter/Python/crashlog.test 
b/lldb/test/Shell/ScriptInterpreter/Python/crashlog.test
index 047cb80bf2f7..293d34514fdc 100644
--- a/lldb/test/Shell/ScriptInterpreter/Python/crashlog.test
+++ b/lldb/test/Shell/ScriptInterpreter/Python/crashlog.test
@@ -1,5 +1,6 @@
 # -*- python 
-*-
 # REQUIRES: system-darwin
+# UNSUPPORTED: lldb-repro
 # DEBUG: cd %S/../../../../examples/python && cat %s | %lldb && false
 # RUN: cd %S/../../../../examples/python && cat %s | %lldb | FileCheck %s
 # CHECK-LABEL: {{S}}KIP BEYOND CHECKS

diff  --git a/lldb/test/Shell/ScriptInterpreter/Python/scripted_breakpoint.test 
b/lldb/test/Shell/ScriptInterpreter/Python/scripted_breakpoint.test
index c49124a22bf0..12fccfe16eff 100644
--- a/lldb/test/Shell/ScriptInterpreter/Python/scripted_breakpoint.test
+++ b/lldb/test/Shell/ScriptInterpreter/Python/scripted_breakpoint.test
@@ -1,4 +1,5 @@
 # REQUIRES: python
+# UNSUPPORTED: lldb-repro
 #
 # Test that the scripting language argument to "breakpoint command" is honored
 # even if the global scripting language is 
diff erent.



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

[Lldb-commits] [PATCH] D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 239722.
jingham added a comment.

Addressed Jonas' review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73225

Files:
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/include/lldb/Target/ThreadPlanStepInRange.h
  lldb/packages/Python/lldbsuite/test/lang/objc/direct-dispatch-step/Makefile
  
lldb/packages/Python/lldbsuite/test/lang/objc/direct-dispatch-step/TestObjCDirectDispatchStepping.py
  
lldb/packages/Python/lldbsuite/test/lang/objc/direct-dispatch-step/stepping-tests.m
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.h
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h

Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
===
--- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
+++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
@@ -12,6 +12,9 @@
 #include "AppleObjCTrampolineHandler.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Target/ThreadPlan.h"
+#include "lldb/Target/ThreadPlanStepInRange.h"
+#include "lldb/Target/ThreadPlanStepOut.h"
+#include "lldb/Target/ThreadPlanShouldStopHere.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-types.h"
 
@@ -20,7 +23,7 @@
 class AppleThreadPlanStepThroughObjCTrampoline : public ThreadPlan {
 public:
   AppleThreadPlanStepThroughObjCTrampoline(
-  Thread &thread, AppleObjCTrampolineHandler *trampoline_handler,
+  Thread &thread, AppleObjCTrampolineHandler &trampoline_handler,
   ValueList &values, lldb::addr_t isa_addr, lldb::addr_t sel_addr,
   bool stop_others);
 
@@ -52,23 +55,60 @@
 private:
   bool InitializeFunctionCaller();
 
-  AppleObjCTrampolineHandler *m_trampoline_handler; // FIXME - ensure this
-// doesn't go away on us?
-// SP maybe?
-  lldb::addr_t m_args_addr; // Stores the address for our step through function
-// result structure.
-  // lldb::addr_t m_object_addr;  // This is only for Description.
+  AppleObjCTrampolineHandler &m_trampoline_handler; /// The handler itself.
+  lldb::addr_t m_args_addr; /// Stores the address for our step through function
+/// result structure.
   ValueList m_input_values;
-  lldb::addr_t m_isa_addr; // isa_addr and sel_addr are the keys we will use to
-   // cache the implementation.
+  lldb::addr_t m_isa_addr; /// isa_addr and sel_addr are the keys we will use to
+   /// cache the implementation.
   lldb::addr_t m_sel_addr;
-  lldb::ThreadPlanSP m_func_sp; // This is the function call plan.  We fill it
-// at start, then set it
-  // to NULL when this plan is done.  That way we know to go to:
-  lldb::ThreadPlanSP m_run_to_sp;  // The plan that runs to the target.
-  FunctionCaller *m_impl_function; // This is a pointer to a impl function that
-  // is owned by the client that pushes this plan.
-  bool m_stop_others;
+  lldb::ThreadPlanSP m_func_sp; /// This is the function call plan.  We fill it
+/// at start, then set it to NULL when this plan
+/// is done.  That way we know to go on to:
+  lldb::ThreadPlanSP m_run_to_sp;  /// The plan that runs to the target.
+  FunctionCaller *m_impl_function; /// This is a pointer to a impl function that
+   /// is owned by the client that pushes this
+   /// plan.
+  bool m_stop_others;  /// Whether we should stop other threads.
+};
+
+class AppleThreadPlanStepThroughDirectDispatch: public ThreadPlanStepOut {
+public:
+  AppleThreadPlanStepThroughDirectDispatch(
+  Thread &thread, AppleObjCTrampolineHandler &handler,
+  llvm::StringRef dispatch_func_name, bool stop_others,
+  LazyBool step_in_avoids_code_without_debug_info);
+
+  ~AppleThreadPlanStepThroughDirectDispatch() override;
+
+  void GetDescription(Stream *s, lldb::DescriptionLevel level) override;
+
+  bool ShouldStop(Event *event_ptr) override;
+
+  bool StopOthers() override { return m_stop_others; }
+
+  bool MischiefManaged() override;
+
+  bool DoWillResume(lldb::StateType resume_state, bool current_plan) override;
+
+  void SetFlagsToDefault() override {
+  GetFlags().Set(ThreadPlanStepInRange::GetDefaultFlagsValue());
+  }
+
+protected:
+  bool DoPlanExplain

[Lldb-commits] [PATCH] D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 13 inline comments as done.
jingham added a comment.

Thanks for the suggestions!  A couple were not to my taste, but I fixed all the 
others.




Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:317
+  else
+stop_reason = stop_info_sp->GetStopReason();
+

JDevlieghere wrote:
> ```
> StopReason stop_reason = stop_info_sp ? stop_info_sp->GetStopReason() : 
> eStopReasonNone;
> ```
I don't like trigraphs like this.  The don't make things clearer to my eye and 
if you ever wanted to stop on one or the other branches, you are just sad.



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:320
+  // See if this is one of our msgSend breakpoints:
+  if (stop_reason == eStopReasonBreakpoint) {
+ProcessSP process_sp = GetThread().GetProcess();

JDevlieghere wrote:
> I'd invert the condition and make this an early return with the comment 
> below. 
The logic in this computation is generally goes "if A do X, else if B do Y"...  
So I don't like to write these:

if (!The One Case I've Had To Handle So Far)
  return;

DoStuff();

It isn't really that much clearer, and it means you have to redo it to handle 
another case.

So I'm happy with it the way it is.



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:364
+  bool step_out_should_stop = ThreadPlanStepOut::ShouldStop(event_ptr);
+  if (step_out_should_stop) {
+SetPlanComplete(true);

JDevlieghere wrote:
> Looks like `step_out_should_stop ` isn't used anywhere else? 
> 
> ```if(ThreadPlanStepOut::ShouldStop(event_ptr)) {```
I don't see the benefit.  What's there is easy to read, and more 
self-documenting.  Having important bits of code in an if statement always 
makes them harder to read for me.  I'm happy the way this is.



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:93
+
+  // The base class MischiefManaged does some cleanup - so you have to call it
+  // in your MischiefManaged derived class.

JDevlieghere wrote:
> `///` 
This wasn't doc, it was a reminder to me, but it wasn't necessary so I removed 
it...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73225



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

[Lldb-commits] [lldb] 1f45914 - Embed a zero-length /dev/null in darwin-debug for the special section.

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2020-01-22T15:50:33-08:00
New Revision: 1f45914b4289db7e5ec8d5759707c16e865f02e5

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

LOG: Embed a zero-length /dev/null in darwin-debug for the special section.

Fred suggested that instead of embedded CMakeLists.txt in the binary as
the contents of a special section, see if /dev/null would work.  It
does.

Added: 


Modified: 
lldb/tools/darwin-debug/CMakeLists.txt

Removed: 




diff  --git a/lldb/tools/darwin-debug/CMakeLists.txt 
b/lldb/tools/darwin-debug/CMakeLists.txt
index ab3dbf0060ab..9039aa3423b4 100644
--- a/lldb/tools/darwin-debug/CMakeLists.txt
+++ b/lldb/tools/darwin-debug/CMakeLists.txt
@@ -4,7 +4,7 @@
 # process and that process will start suspended, so debugserver will
 # need to double-resume it to make it run.  A random file is copied
 # into the segment.
-set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} 
-Wl,-sectcreate,ExecExtraSuspend,ExecExtraSuspend,${CMAKE_CURRENT_SOURCE_DIR}/CMakeLists.txt")
+set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} 
-Wl,-sectcreate,ExecExtraSuspend,ExecExtraSuspend,/dev/null")
 
 add_lldb_tool(darwin-debug ADD_TO_FRAMEWORK
   darwin-debug.cpp



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

[Lldb-commits] [lldb] c4144ca - [lldb/Reproducer] Disable buffering of stdout during replay

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T16:17:08-08:00
New Revision: c4144caf9b865a2064e49afcdfff474426fc5d47

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

LOG: [lldb/Reproducer] Disable buffering of stdout during replay

Different buffering behavior during capture and replay caused some of
the shell tests to fail when run from a reproducer. By disabling stdout
buffering we get a better approximation of how things get flushed during
an regular debug session. There is a performance impact but since this
only affects replay this is acceptable.

Added: 


Modified: 
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index 473786ef4d3e..0b74acaacb7e 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -8,6 +8,7 @@
 
 #include "lldb/Utility/ReproducerInstrumentation.h"
 #include "lldb/Utility/Reproducer.h"
+#include 
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
@@ -47,6 +48,10 @@ bool Registry::Replay(llvm::StringRef buffer) {
   Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_API);
 #endif
 
+  // Disable buffering stdout so that we approximate the way things get flushed
+  // during an interactive session.
+  setvbuf(stdout, nullptr, _IONBF, 0);
+
   Deserializer deserializer(buffer);
   while (deserializer.HasData(1)) {
 unsigned id = deserializer.Deserialize();



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

[Lldb-commits] [PATCH] D72953: Fix the handling of unnamed bit-fields when parsing DWARF

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 239732.
shafik marked 14 inline comments as done.
shafik added a comment.

Realized that the assert that I was hitting in some cases only reproduced using 
C++ for example one case we needed to use a class w/ private members.  So 
created a new bit-field test which is C++ specific.

Addressed comments:

- Removed magic numbers from `FieldInfo`
- Removed the need for `last_field_info` and `last_bitfield_info`
- Refactored away more code


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

https://reviews.llvm.org/D72953

Files:
  lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/Makefile
  lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py
  lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -170,33 +170,20 @@
   lldb::ModuleSP GetModuleForType(const DWARFDIE &die);
 
 private:
-  struct BitfieldInfo {
-uint64_t bit_size;
-uint64_t bit_offset;
+  struct FieldInfo {
+uint64_t bit_size = 0;
+uint64_t bit_offset = 0;
+bool is_bitfield = false;
 
-BitfieldInfo()
-: bit_size(LLDB_INVALID_ADDRESS), bit_offset(LLDB_INVALID_ADDRESS) {}
+FieldInfo() = default;
 
-void Clear() {
-  bit_size = LLDB_INVALID_ADDRESS;
-  bit_offset = LLDB_INVALID_ADDRESS;
-}
-
-bool IsValid() const {
-  return (bit_size != LLDB_INVALID_ADDRESS) &&
- (bit_offset != LLDB_INVALID_ADDRESS);
-}
+void SetIsBitfield(bool flag) { is_bitfield = flag; }
+bool IsBitfield() { return is_bitfield; }
 
 bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const {
-  if (IsValid()) {
-// This bitfield info is valid, so any subsequent bitfields must not
-// overlap and must be at a higher bit offset than any previous bitfield
-// + size.
-return (bit_size + bit_offset) <= next_bit_offset;
-  } else {
-// If the this BitfieldInfo is not valid, then any offset isOK
-return true;
-  }
+  // Any subsequent bitfields must not overlap and must be at a higher
+  // bit offset than any previous bitfield + size.
+  return (bit_size + bit_offset) <= next_bit_offset;
 }
   };
 
@@ -208,7 +195,7 @@
 lldb::AccessType &default_accessibility,
 DelayedPropertyList &delayed_properties,
 lldb_private::ClangASTImporter::LayoutInfo &layout_info,
-BitfieldInfo &last_field_info);
+FieldInfo &last_field_info);
 
   bool CompleteRecordType(const DWARFDIE &die, lldb_private::Type *type,
   lldb_private::CompilerType &clang_type);
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -85,35 +85,6 @@
   return false;
 }
 
-struct BitfieldInfo {
-  uint64_t bit_size;
-  uint64_t bit_offset;
-
-  BitfieldInfo()
-  : bit_size(LLDB_INVALID_ADDRESS), bit_offset(LLDB_INVALID_ADDRESS) {}
-
-  void Clear() {
-bit_size = LLDB_INVALID_ADDRESS;
-bit_offset = LLDB_INVALID_ADDRESS;
-  }
-
-  bool IsValid() const {
-return (bit_size != LLDB_INVALID_ADDRESS) &&
-   (bit_offset != LLDB_INVALID_ADDRESS);
-  }
-
-  bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const {
-if (IsValid()) {
-  // This bitfield info is valid, so any subsequent bitfields must not
-  // overlap and must be at a higher bit offset than any previous bitfield
-  // + size.
-  return (bit_size + bit_offset) <= next_bit_offset;
-} else {
-  // If the this BitfieldInfo is not valid, then any offset isOK
-  return true;
-}
-  }
-};
 
 ClangASTImporter &DWARFASTParserClang::GetClangASTImporter() {
   if (!m_clang_ast_importer_up) {
@@ -2419,7 +2390,7 @@
 lldb::AccessType &default_accessibility,
 DelayedPropertyList &delayed_properties,
 lldb_private::ClangASTImporter::LayoutInfo &layout_info,
-BitfieldInfo &last_field_info) {
+FieldInfo &last_field_info) {
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
   const dw_tag_t tag = die.Tag();
   // Get the parent byte size so we can verify any members will fit
@@ -2453,6 +2424,14 @@
   const dw_attr_t attr = attributes.AttributeAtIndex(i);
   DWARFFormValue form_value;
   if (attributes.ExtractFormValueAtIndex(i, form_value)) {
+// DW_AT_data_member_location indicates

[Lldb-commits] [PATCH] D72953: Fix the handling of unnamed bit-fields when parsing DWARF

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2393
 lldb_private::ClangASTImporter::LayoutInfo &layout_info,
-BitfieldInfo &last_field_info) {
+FieldInfo &last_bitfield_info, FieldInfo &last_field_info) {
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();

teemperor wrote:
> IIUC then those two bitfield info variables are mutually exclusive? I.e., 
> either the last field was bitfield or a normal field? If yes, would it make 
> sense to model this as one FieldInfo variable and have a bool value for 
> differentiating between them? That way we don't have the possibility that we 
> forget to clear one when we set the other.
It was not obvious during the initial refactoring wave but yes, I removed one 
of them.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2674
 
-  if (anon_field_info.IsValid()) {
+  if (unnamed_field_info.IsValid()) {
+if (data_bit_offset != UINT64_MAX)

aprantl wrote:
> Can you please add comments explaining what each of these cases handle?
I removed a lot of in the next wave of refactoring.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h:174
+  struct FieldInfo {
 uint64_t bit_size;
 uint64_t bit_offset;

aprantl wrote:
> Not you fault, but: This data structure is a disaster waiting to happen. 
> Instead of having magic sentinel values, should we use an Optional 
> that is always valid if it exists? Or should the members be Optionals?
This is a good point, it took a bit more refactoring of the code using this but 
I removed the magic numbers and used an `Optional` for the 
`unnamed_field_info` case.


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

https://reviews.llvm.org/D72953



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

[Lldb-commits] [lldb] 83a093b - [lldb/Reproducer] Mark some driver tests as unsupported for lldb-repro

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T17:18:06-08:00
New Revision: 83a093b8ecc8a8e3a4420dc5385dca57e8016109

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

LOG: [lldb/Reproducer] Mark some driver tests  as unsupported for lldb-repro

These test are checking for diagnostics printed by the driver. During
replay we only replay the SB API calls made by the driver, so it's
expected that these messages aren't displayed.

Added: 


Modified: 
lldb/test/Shell/Driver/LocalLLDBInit.test
lldb/test/Shell/Driver/TestCore.test
lldb/test/Shell/Driver/TestFile.test
lldb/test/Shell/Driver/TestRepl.test

Removed: 




diff  --git a/lldb/test/Shell/Driver/LocalLLDBInit.test 
b/lldb/test/Shell/Driver/LocalLLDBInit.test
index 69b7781415a3..e1b66a099844 100644
--- a/lldb/test/Shell/Driver/LocalLLDBInit.test
+++ b/lldb/test/Shell/Driver/LocalLLDBInit.test
@@ -1,4 +1,6 @@
 # REQUIRES: python
+# UNSUPPORTED: lldb-repro
+#
 # RUN: mkdir -p %t.root
 # RUN: mkdir -p %t.home
 # RUN: cp %S/Inputs/.lldbinit %t.root

diff  --git a/lldb/test/Shell/Driver/TestCore.test 
b/lldb/test/Shell/Driver/TestCore.test
index cca8171da631..247261723512 100644
--- a/lldb/test/Shell/Driver/TestCore.test
+++ b/lldb/test/Shell/Driver/TestCore.test
@@ -1,2 +1,4 @@
+# UNSUPPORTED: lldb-repro
+#
 # RUN: not %lldb -c /bogus/path 2>&1 | FileCheck %s
 # CHECK: error: file specified in --core (-c) option doesn't exist

diff  --git a/lldb/test/Shell/Driver/TestFile.test 
b/lldb/test/Shell/Driver/TestFile.test
index 0e80594aeb1b..776baf8ba0c5 100644
--- a/lldb/test/Shell/Driver/TestFile.test
+++ b/lldb/test/Shell/Driver/TestFile.test
@@ -1,2 +1,4 @@
+# UNSUPPORTED: lldb-repro
+#
 # RUN: not %lldb -f /bogus/path 2>&1 | FileCheck %s
 # CHECK: error: file specified in --file (-f) option doesn't exist

diff  --git a/lldb/test/Shell/Driver/TestRepl.test 
b/lldb/test/Shell/Driver/TestRepl.test
index 083863985a14..a0bf8c26fd57 100644
--- a/lldb/test/Shell/Driver/TestRepl.test
+++ b/lldb/test/Shell/Driver/TestRepl.test
@@ -1,3 +1,5 @@
+# UNSUPPORTED: lldb-repro
+#
 # RUN: echo ':quit' | %lldb -x --repl -O 'expr 42' -S %S/Inputs/Print2.in -o 
'expr 99' -s %s 2>&1 | FileCheck %s
 # CHECK: {{w}}arning: commands specified to run after file load (via -o or -s) 
are ignored in REPL mode
 # CHECK: (int) $0 = 42



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

[Lldb-commits] [lldb] 9be5c13 - [lldb/Test] Add check-lldb-repro target

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T17:35:45-08:00
New Revision: 9be5c13538898c7632c2de7300de9479688a2460

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

LOG: [lldb/Test] Add check-lldb-repro target

This adds a new target check-lldb-repro which runs the shell tests with
the lldb-repo utility. It runs the shell tests twice, once while
capturing a reproducer and then again by replaying that reproducer.

Added: 


Modified: 
lldb/test/Shell/CMakeLists.txt

Removed: 




diff  --git a/lldb/test/Shell/CMakeLists.txt b/lldb/test/Shell/CMakeLists.txt
index d203f1e093c7..a843f11f3a06 100644
--- a/lldb/test/Shell/CMakeLists.txt
+++ b/lldb/test/Shell/CMakeLists.txt
@@ -15,3 +15,17 @@ if (CMAKE_GENERATOR STREQUAL "Xcode")
 ${CMAKE_CURRENT_BINARY_DIR}
 DEPENDS lldb-test-deps)
 endif()
+
+# Add a lit test suite that runs the shell test while capturing a reproducer.
+add_lit_testsuite(check-lldb-repro-capture
+  "Running lldb shell test suite with reproducer capture"
+  ${CMAKE_CURRENT_BINARY_DIR}
+  PARAMS "lldb-run-with-repro=capture"
+  DEPENDS lldb-test-deps)
+
+# Add a lit test suite that runs the shell test by replaying a reproducer.
+add_lit_testsuite(check-lldb
+  "Running lldb shell test suite with reproducer replay"
+  ${CMAKE_CURRENT_BINARY_DIR}
+  PARAMS "lldb-run-with-repro=capture"
+  DEPENDS lldb-test-deps check-lldb-repro-capture)



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

[Lldb-commits] [lldb] 9b5a9f2 - [lldb/Test] Fix type in add_lit_testsuite

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T17:51:58-08:00
New Revision: 9b5a9f2fab17d52debce2cde26e94610deeb034c

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

LOG: [lldb/Test] Fix type in add_lit_testsuite

The new test suite should be called check-lldb-repro rather than
check-lldb.

Added: 


Modified: 
lldb/test/Shell/CMakeLists.txt

Removed: 




diff  --git a/lldb/test/Shell/CMakeLists.txt b/lldb/test/Shell/CMakeLists.txt
index a843f11f3a06..48c02fed2c86 100644
--- a/lldb/test/Shell/CMakeLists.txt
+++ b/lldb/test/Shell/CMakeLists.txt
@@ -24,7 +24,7 @@ add_lit_testsuite(check-lldb-repro-capture
   DEPENDS lldb-test-deps)
 
 # Add a lit test suite that runs the shell test by replaying a reproducer.
-add_lit_testsuite(check-lldb
+add_lit_testsuite(check-lldb-repro
   "Running lldb shell test suite with reproducer replay"
   ${CMAKE_CURRENT_BINARY_DIR}
   PARAMS "lldb-run-with-repro=capture"



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

[Lldb-commits] [PATCH] D73148: [lldb/Value] Report size of Value as size of underlying data buffer

[Vedant Kumar via Phabricator via lldb-commits]

vsk updated this revision to Diff 239753.
vsk edited the summary of this revision.
This revision is now accepted and ready to land.

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

https://reviews.llvm.org/D73148

Files:
  lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/Makefile
  
lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/TestDW_OP_piece.py
  lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/main.cpp
  lldb/source/Core/Value.cpp
  lldb/unittests/Expression/DWARFExpressionTest.cpp

Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -360,4 +360,27 @@
   // Note that the "00" should really be "undef", but we can't
   // represent that yet.
   llvm::HasValue(GetScalar(16, 0xff00, true)));
+
+  for (unsigned char ByteSize = 1; ByteSize <= 8; ++ByteSize) {
+llvm::Expected empty = Evaluate({DW_OP_piece, ByteSize});
+// Note that the "0" should really be "undef", but we can't
+// represent that yet.
+EXPECT_THAT_EXPECTED(empty,
+ llvm::HasValue(GetScalar(ByteSize * 8, 0, true)));
+
+Value pieces;
+ASSERT_EQ(pieces.AppendDataToHostBuffer(empty.get()), ByteSize);
+ASSERT_EQ(pieces.GetValueByteSize(nullptr, nullptr), ByteSize);
+  }
+
+  // When CommandObjectFrameVariable constructs an empty Value, check that
+  // we do not successfully report its size as 0. A CompilerType is needed to
+  // get the size in this special case.
+  {
+Value empty;
+empty.ResizeData(0);
+Status error;
+ASSERT_EQ(empty.GetValueByteSize(&error, nullptr), 0);
+ASSERT_TRUE(error.Fail());
+  }
 }
Index: lldb/source/Core/Value.cpp
===
--- lldb/source/Core/Value.cpp
+++ lldb/source/Core/Value.cpp
@@ -222,6 +222,12 @@
   case eContextTypeLLDBType: // Type *
   case eContextTypeVariable: // Variable *
   {
+// The size of this Value may be less than the size of the type of its
+// source variable due to truncating operations such as DW_OP_piece.
+if (m_value_type == eValueTypeHostAddress)
+  if (uint64_t buffer_size = GetBuffer().GetByteSize())
+return buffer_size;
+
 auto *scope = exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr;
 if (llvm::Optional size = GetCompilerType().GetByteSize(scope)) {
   if (error_ptr)
Index: lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/main.cpp
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/functionalities/dw_op_piece/main.cpp
@@ -0,0 +1,62 @@
+// The point of this test is to exercise lldb's DW_OP_piece evaluation logic.
+// The CHECK lines in the test depend on aspects of the optimizer's behavior
+// that aren't guaranteed. If the test fails after a compiler change, run it in
+// trace mode (./bin/lldb-dotest -t ...): this should print out the new location
+// expressions.
+
+__attribute__((noinline, optnone)) int use(int x) { return x; }
+
+volatile int sink;
+
+struct S1 {
+  int f1;
+  int *f2;
+};
+
+struct S2 {
+  char a, b;
+  int pad;
+  S2(int x) {
+a = x & 0xff;
+b = x & 0xff00;
+  }
+};
+
+int main() {
+  S1 v1;
+  v1.f1 = sink;
+  v1.f2 = nullptr;
+
+  // 1) Check that "v1" is described by 3 pieces (one each for "f1" and "f2"
+  // (in registers), and a third for padding (a literal)).
+  //
+  //% self.filecheck("image lookup -va $pc", "main.cpp", "-check-prefix=INFO-V1")
+  // INFO-V1: name = "v1", type = "S1", location = DW_OP_reg0 RAX, DW_OP_piece 0x4, DW_OP_piece 0x4, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x8, decl
+  sink++;
+
+  // 2) Check that lldb displays "v1" correctly.
+  //
+  //% self.filecheck("frame variable v1", "main.cpp", "-check-prefix=SHOW-V1")
+  // SHOW-V1:  (S1) v1 = {
+  // SHOW-V1-NEXT:   f1 = 0
+  // SHOW-V1-NEXT:   f2 = 0x{{0+$}}
+  // SHOW-V1-NEXT: }
+
+  S2 v2(v1.f1);
+
+  // 3) Check that "v2" is described by 2 pieces (one each for "a" and "b").
+  // Note that a piece for "pad" is left out: this is crucial, as it means that
+  // DWARFExpression::Evaluate() will produce a 2-byte Value. We want to test
+  // that lldb can handle the Value without smashing memory (this is a
+  // regression test for a bug found by ASan).
+  sink += use(v2.a);
+  //% self.filecheck("image lookup -va $pc", "main.cpp", "-check-prefix=INFO-V2")
+  // INFO-V2: name = "v2", type = "S2", location = DW_OP_piece 0x1, DW_OP_lit0, DW_OP_stack_value, DW_OP_piece 0x1, decl
+
+  // 4) Check that lldb displays "v2" correctly.
+  sink += use(v2.pad);
+  //% self.filecheck("frame variable v2", "main.cpp", "-check-prefix=SHOW-V2")
+  // SHOW-V2: (S2) v2 = (a = '\0', b = '\0', pad = {{.*}})
+
+  return 0;
+}
Index: lldb/packages/Python/lldbsuite/test/function

[Lldb-commits] [lldb] 48490e3 - [lldb/Docs] Document testing strategies for the reproducers

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-22T18:02:36-08:00
New Revision: 48490e3247af93eaf576a7bf1c1f6b7450fe6d54

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

LOG: [lldb/Docs] Document testing strategies for the reproducers

Document the different ways we test the reproducers. This is mostly to
describe the new check-lldb-repro target.

Added: 


Modified: 
lldb/docs/resources/reproducers.rst

Removed: 




diff  --git a/lldb/docs/resources/reproducers.rst 
b/lldb/docs/resources/reproducers.rst
index d28cade45215..c20c29892978 100644
--- a/lldb/docs/resources/reproducers.rst
+++ b/lldb/docs/resources/reproducers.rst
@@ -93,6 +93,31 @@ Design
 
 Coming soon.
 
+Testing
+---
+
+Reproducers are tested in the following ways:
+
+ - Unit tests to cover the generic reproducer infrastructure.
+ - Focused test cases in the ``test/Shell/Reproducer`` directory. These tests
+   serve as integration tests for the reproducers infrastructure, as well as
+   doing some sanity checking for basic debugger functionality.
+
+Additional testing is possible:
+
+ - It's possible to unconditionally capture reproducers while running the
+   entire test suite by setting the ``LLDB_CAPTURE_REPRODUCER`` environment
+   variable. Assuming the reproducers behave correctly, this can also help to
+   reproduce test failures.
+ - It's possible to run the shell tests from a reproducer replay. The
+   ``check-lldb-repro`` target will run the shell test suite twice. First it
+   runs the test suite and captures a reproducer for every lldb invocation and
+   saves it to a known location based off lldb's arguments and  working
+   directory. Then it runs the test suite again, this time replaying the
+   reproducers. Certain tests do not fit this paradigm (for example test that
+   check the output of the binary being debugged) and can be skipped by setting
+   ``UNSUPPORTED: lldb-repro`` at the top of the test.
+
 Knows Issues
 
 



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

[Lldb-commits] [PATCH] D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:317
+  else
+stop_reason = stop_info_sp->GetStopReason();
+

jingham wrote:
> JDevlieghere wrote:
> > ```
> > StopReason stop_reason = stop_info_sp ? stop_info_sp->GetStopReason() : 
> > eStopReasonNone;
> > ```
> I don't like trigraphs like this.  The don't make things clearer to my eye 
> and if you ever wanted to stop on one or the other branches, you are just sad.
What about 
```
 StopReason stop_reason = eStopReasonNone;
  if (stop_info_sp)
stop_reason = stop_info_sp->GetStopReason();
```



Comment at: 
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:364
+  bool step_out_should_stop = ThreadPlanStepOut::ShouldStop(event_ptr);
+  if (step_out_should_stop) {
+SetPlanComplete(true);

jingham wrote:
> JDevlieghere wrote:
> > Looks like `step_out_should_stop ` isn't used anywhere else? 
> > 
> > ```if(ThreadPlanStepOut::ShouldStop(event_ptr)) {```
> I don't see the benefit.  What's there is easy to read, and more 
> self-documenting.  Having important bits of code in an if statement always 
> makes them harder to read for me.  I'm happy the way this is.
Can we make it at least `const` then? :-) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73225



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

[Lldb-commits] [PATCH] D73148: [lldb/Value] Report size of Value as size of underlying data buffer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D73148#1834955 , @vsk wrote:

> In D73148#1832897 , @clayborg wrote:
>
> > Actually it would be nice to have a test that will trigger on at least one 
> > build bot that runs ASAN?
>
>
> I'll add an end-to-end test for DW_OP_piece, though I worry it might be 
> brittle.


I don't think that's a good way to write a test like this. I suggest checking 
out `test/Shell/SymbolFile/DWARF/DW_OP_piece-struct.s` for an  dwarf expression 
test that does not rely on guessing the DW_OP opcodes used by the compiler. 
Maybe you can just take that test and tweak it to do what you need?


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

https://reviews.llvm.org/D73148



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

[Lldb-commits] [PATCH] D72751: [LLDB] Add DynamicLoaderWasmDYLD plugin for WebAssembly debugging

[Paolo Severini via Phabricator via lldb-commits]

paolosev updated this revision to Diff 239784.
paolosev added a comment.

I have verified the logic of the dynamic loader quite carefully, but there are 
a couple of things to clarify.

A Wasm module is loaded at a 64 bit address, where the upper 32 bits are used 
as module identifier. Let’s say that we have a module with Id==4, so it will be 
loaded at address 0x0004`.  Each section is loaded at its relative 
file offset. Therefore if the code section starts at file offset 0x4d in the 
Wasm module, we call:
Target::SetSectionLoadAddress(section_sp, 0x4004d).

The module can also contain embedded DWARF sections, which will also be loaded 
at their relative file offset in the same way. And since there cannot be 
duplicated sections in a module, there is no overlapping, we can always convert 
a load address back into a section.

However, there are two complications.

The first is that we need to call `Target::SetSectionLoadAddress()` twice, from 
two different places. First we need to call `Target::SetSectionLoadAddress()` 
in `ObjectFileWasm::SetLoadAddress()`, and then again in 
`DynamicLoaderWasmDYLD::DidAttach()`. The reason for this seems to originate in 
the sequence of function calls:

In `DynamicLoaderWasmDYLD::DidAttach()` we call 
`ProcessGDBRemote::LoadModules()` to get list of loaded modules from the remote 
(Wasm engine).
`ProcessGDBRemote::LoadModules()` calls, first:

- `DynamicLoaderWasmDYLD::LoadModuleAtAddress()` and from there:
  1. `DynamicLoader::UpdateLoadedSections() -> ObjectFileWasm::SetLoadAddress()`
  2. `Target::GetImages()::AppendIfNeeded(module) -> 
ProcessGDBRemote::ModulesDidLoad() -> JITLoaderList::ModulesDidLoad() -> 
Module::GetSymbolFile() -> SymbolFileDWARF::CalculateAbilities()`. Here we 
initialize the symbols for the module, and set `m_did_load_symfile`, but for 
this to work we need to have already set the load address for each section, in 
the previous `ObjectFileWasm::SetLoadAddress()`.

then:

- `Target::SetExecutableModule() -> Target::ClearModules() -> 
SectionLoadList::Clear()`

So, at the end of `LoadModules()` in `DynamicLoaderWasmDYLD::DidAttach()` the 
SectionLoadList is empty, and we need to set it again by calling 
`Target::.SetSectionLoadAddress()` again. 
This works but the duplication is ugly; is there a way to improve this?
_

The second problem is that the Code Section needs to be initialized (in 
`ObjectFileWasm::CreateSections()`) with `m_file_addr = m_file_offset = 0`, and 
not with the actual file offset of the Code section in the Wasm file. If we set 
`Section::m_file_addr` and `Section::m_file_offset` to the actual code offset, 
the DWARF info does not work correctly.

I have some doubts regarding the DWARF data generated by Clang for a Wasm 
target. Looking at an example, for a Wasm module that has the Code section at 
offset 0x57, I see this DWARF data:

  0x000b: DW_TAG_compile_unit
[…]
DW_AT_low_pc (0x)
DW_AT_ranges (0x
   [0x0002, 0x000e)
   [0x000f, 0x001a)
   [0x001b, 0x0099)
   [0x009b, 0x011c))

The documentation  says 
that //“Wherever a code address is used in DWARF for WebAssembly, it must be 
the offset of an instruction relative within the Code section of the 
WebAssembly file.”// 
But is this correct? Shouldn't maybe code addresses be offset-ed by the file 
address of the Code section?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72751

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestWasm.py
  
lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/test_wasm.yaml
  lldb/source/API/SystemInitializerFull.cpp
  lldb/source/Plugins/DynamicLoader/CMakeLists.txt
  lldb/source/Plugins/DynamicLoader/wasm-DYLD/CMakeLists.txt
  lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
  lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp
  lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.h
  lldb/tools/lldb-test/SystemInitializerTest.cpp

Index: lldb/tools/lldb-test/SystemInitializerTest.cpp
===
--- lldb/tools/lldb-test/SystemInitializerTest.cpp
+++ lldb/tools/lldb-test/SystemInitializerTest.cpp
@@ -37,6 +37,7 @@
 #include "Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h"
 #include "Plugins/DynamicLoader/Static/DynamicLoaderStatic.h"
 #include "Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h"
+#include "Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h"
 #include "Plugins/Instruction/ARM64/EmulateInstructionARM64.h"
 #include "Plugins/Instruction/PPC64/EmulateInstructionPPC64.h"
 #include 

Re: [Lldb-commits] [lldb] 9be5c13 - [lldb/Test] Add check-lldb-repro target

[Martin Storsjö via lldb-commits]

On Wed, 22 Jan 2020, Jonas Devlieghere via lldb-commits wrote:



Author: Jonas Devlieghere
Date: 2020-01-22T17:35:45-08:00
New Revision: 9be5c13538898c7632c2de7300de9479688a2460

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

LOG: [lldb/Test] Add check-lldb-repro target

This adds a new target check-lldb-repro which runs the shell tests with
the lldb-repo utility. It runs the shell tests twice, once while
capturing a reproducer and then again by replaying that reproducer.

Added:


Modified:
   lldb/test/Shell/CMakeLists.txt

Removed:




diff  --git a/lldb/test/Shell/CMakeLists.txt b/lldb/test/Shell/CMakeLists.txt
index d203f1e093c7..a843f11f3a06 100644
--- a/lldb/test/Shell/CMakeLists.txt
+++ b/lldb/test/Shell/CMakeLists.txt
@@ -15,3 +15,17 @@ if (CMAKE_GENERATOR STREQUAL "Xcode")
${CMAKE_CURRENT_BINARY_DIR}
DEPENDS lldb-test-deps)
endif()
+
+# Add a lit test suite that runs the shell test while capturing a reproducer.
+add_lit_testsuite(check-lldb-repro-capture
+  "Running lldb shell test suite with reproducer capture"
+  ${CMAKE_CURRENT_BINARY_DIR}
+  PARAMS "lldb-run-with-repro=capture"
+  DEPENDS lldb-test-deps)
+
+# Add a lit test suite that runs the shell test by replaying a reproducer.
+add_lit_testsuite(check-lldb
+  "Running lldb shell test suite with reproducer replay"
+  ${CMAKE_CURRENT_BINARY_DIR}
+  PARAMS "lldb-run-with-repro=capture"
+  DEPENDS lldb-test-deps check-lldb-repro-capture)


FYI (the commit mails from my commits don't seem to make it to the list), 
I made a small tweak to this in 
https://github.com/llvm/llvm-project/commit/1db1b8b8b35727a01387c1bc0bbf25701ad05d3f 
as the above construct with add_lit_testsuite(... DEPENDS check-*) will 
cause that check-* testsuite to be run when just building test dependncies 
with "ninja test-depends".


// Martin

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