[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[Pavel Labath via Phabricator via lldb-commits]

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

Actually I think this is fine. We would want to squeeze as much information as 
possible from these kinds of line tables.

I don't think fully preserving the existing behavior would be that easy, 
actually. We have another call to the llvm line table parser in 
`ParseLLVMLineTable` (line 154), but this one calls 
`DWARFDebugLine::LineTable::getOrParseLineTable` There, we already ignore (log) 
recoverable errors, and it'd be hard to tell the "new" kinds of errors from the 
"old" ones...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D70840#1845176 , @clayborg wrote:

> Speaking to having the LLVM layer strip things out, this should be an option 
> if it does get added there. If we have a stripped binary and we have no 
> symbols or any other CPU map, and DWARF is the only way to tell if a function 
> was ARM or Thumb by looking at bit zero, then we need to somehow be able to 
> get this raw information.


I would expect there will still be some way to get the raw data, because the 
dumping tools would want to get it.

However, if I understand things correctly, this is would not be a very useful 
source of information for us -- on windows arm, all debug info addresses will 
have the thumb bit set (because everything is thumb), and everywhere else the 
debug info will have the bit 0 cleared, regardless of whether the function is 
thumb or not...


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

https://reviews.llvm.org/D70840



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

[Lldb-commits] [lldb] a5fb2e3 - [lldb] Complete return types of CXXMethodDecls to prevent crashing due to covariant return types

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-01-29T09:08:35+01:00
New Revision: a5fb2e371ec2b585ca56cbc1a116912aabe347d3

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

LOG: [lldb] Complete return types of CXXMethodDecls to prevent crashing due to 
covariant return types

Summary:
Currently we crash in Clang's CodeGen when we call functions with covariant 
return types with this assert:
```
Assertion failed: (DD && "queried property of class with no definition"), 
function data, file clang/include/clang/AST/DeclCXX.h, line 433.
```
when calling `clang::CXXRecordDecl::isDerivedFrom` from the 
`ItaniumVTableBuilder`.

Clang seems to assume that the underlying record decls of covariant return 
types are already completed.
This is true during a normal Clang invocation as there the type checker will 
complete both decls when
checking if the overloaded function is valid (i.e., the return types are 
covariant).

When we minimally import our AST into the expression in LLDB we don't do this 
type checking (which
would complete the record decls) and we end up trying to access the invalid 
record decls from CodeGen
which makes us trigger the assert.

This patch just completes the underlying types of ptr/ref return types of 
virtual function so that the
underlying records are complete and we behave as Clang expects us to do.

Fixes rdar://38048657

Reviewers: lhames, shafik

Reviewed By: shafik

Subscribers: abidh, JDevlieghere, lldb-commits

Tags: #lldb

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

Added: 
lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/Makefile

lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/main.cpp

Modified: 
lldb/source/Symbol/ClangASTImporter.cpp

Removed: 




diff  --git 
a/lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/Makefile 
b/lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/Makefile
new file mode 100644
index ..8b20bcb0
--- /dev/null
+++ 
b/lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff  --git 
a/lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
 
b/lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
new file mode 100644
index ..86a6ddd6e47b
--- /dev/null
+++ 
b/lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
@@ -0,0 +1,40 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self,"// break here", 
lldb.SBFileSpec("main.cpp"))
+
+# Test covariant return types for pointers to class that contains the 
called function.
+self.expect_expr("derived.getPtr()", result_type="Derived *")
+self.expect_expr("base_ptr_to_derived->getPtr()", result_type="Base *")
+self.expect_expr("base.getPtr()", result_type="Base *")
+# The same tests with reference types. LLDB drops the reference when 
it turns the
+# result into a SBValue so check for the the underlying type of the 
result.
+self.expect_expr("derived.getRef()", result_type="Derived")
+self.expect_expr("base_ptr_to_derived->getRef()", result_type="Base")
+self.expect_expr("base.getRef()", result_type="Base")
+
+# Test covariant return types for pointers to class that does *not* 
contain the called function.
+self.expect_expr("derived.getOtherPtr()", result_type="OtherDerived *")
+self.expect_expr("base_ptr_to_derived->getOtherPtr()", 
result_type="OtherBase *")
+self.expect_expr("base.getOtherPtr()", result_type="OtherBase *")
+# The same tests with reference types. LLDB drops the reference when 
it turns the
+# result into a SBValue so check for the the underlying type of the 
result.
+self.expect_expr("derived.getOtherRef()", result_type="OtherDerived")
+self.expect_expr("base_ptr_to_derived->getOtherRef()", 
result_type="OtherBase")
+self.expect_expr("base.getOtherRef()", result_type="OtherBase")
+
+# Test that we call the right function and get the right value back.
+self.expect_expr("derived.getOtherPtr()->value()", 
result_summary='"derived"')
+self.expect_expr("base_ptr_to_derived->getOtherPtr()->value()", 
result_summary='"derived"')
+self.expect_exp

[Lldb-commits] [PATCH] D73517: [lldb] Delete ValueObject::GetBaseClassPath

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Core/ValueObject.cpp:2041
 
 void ValueObject::GetExpressionPath(Stream &s, bool qualify_cxx_base_classes,
 GetExpressionPathFormat epformat) {

Should we remove the `qualify_cxx_base_classes` argument as well, given that it 
no longer does anything ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73517



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

[Lldb-commits] [PATCH] D73024: [lldb] Complete return types of CXXMethodDecls to prevent crashing due to covariant return types

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa5fb2e371ec2: [lldb] Complete return types of CXXMethodDecls 
to prevent crashing due to… (authored by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73024

Files:
  lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/Makefile
  
lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
  lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/main.cpp
  lldb/source/Symbol/ClangASTImporter.cpp

Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -976,6 +976,25 @@
   }
 }
 
+/// Takes a CXXMethodDecl and completes the return type if necessary. This
+/// is currently only necessary for virtual functions with covariant return
+/// types where Clang's CodeGen expects that the underlying records are already
+/// completed.
+static void MaybeCompleteReturnType(ClangASTImporter &importer,
+CXXMethodDecl *to_method) {
+  if (!to_method->isVirtual())
+return;
+  QualType return_type = to_method->getReturnType();
+  if (!return_type->isPointerType() && !return_type->isReferenceType())
+return;
+
+  clang::RecordDecl *rd = return_type->getPointeeType()->getAsRecordDecl();
+  if (!rd)
+return;
+
+  importer.CompleteTagDecl(rd);
+}
+
 void ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl *from,
  clang::Decl *to) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
@@ -1121,6 +1140,9 @@
   }
 }
   }
+
+  if (clang::CXXMethodDecl *to_method = dyn_cast(to))
+MaybeCompleteReturnType(m_master, to_method);
 }
 
 clang::Decl *
Index: lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/main.cpp
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/main.cpp
@@ -0,0 +1,40 @@
+struct OtherBase {
+  // Allow checking actual type from the test by giving
+  // this class and the subclass unique values here.
+  virtual const char *value() { return "base"; }
+};
+struct OtherDerived : public OtherBase {
+  const char *value() override { return "derived"; }
+};
+
+// Those have to be globals as they would be completed if they
+// are members (which would make this test always pass).
+OtherBase other_base;
+OtherDerived other_derived;
+
+struct Base {
+  // Function with covariant return type that is same class.
+  virtual Base* getPtr() { return this; }
+  virtual Base& getRef() { return *this; }
+  // Function with covariant return type that is a different class.
+  virtual OtherBase* getOtherPtr() { return &other_base; }
+  virtual OtherBase& getOtherRef() { return other_base; }
+};
+
+struct Derived : public Base {
+  Derived* getPtr() override { return this; }
+  Derived& getRef() override { return *this; }
+  OtherDerived* getOtherPtr() override { return &other_derived; }
+  OtherDerived& getOtherRef() override { return other_derived; }
+};
+
+int main() {
+  Derived derived;
+  Base base;
+  Base *base_ptr_to_derived = &derived;
+  (void)base_ptr_to_derived->getPtr();
+  (void)base_ptr_to_derived->getRef();
+  (void)base_ptr_to_derived->getOtherPtr();
+  (void)base_ptr_to_derived->getOtherRef();
+  return 0; // break here
+}
Index: lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/lang/cpp/covariant-return-types/TestCovariantReturnTypes.py
@@ -0,0 +1,40 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self,"// break here", lldb.SBFileSpec("main.cpp"))
+
+# Test covariant return types for pointers to class that contains the called function.
+self.expect_expr("derived.getPtr()", result_type="Derived *")
+self.expect_expr("base_ptr_to_derived->getPtr()", result_type="Base *")
+self.expect_expr("base.getPtr()", result_type="Base *")
+# The same tests with reference types. LLDB drops the reference when it turns the
+# result into a SBValue so check for the the underlying type of the result.
+self.expect_expr("derived.getRef()", result_type="Derived")
+self.expect_expr("base_ptr_to_derived->getRef()", result_type="Base")
+self.expect_expr("base.getRef()", result_type="Base")
+
+#

[Lldb-commits] [lldb] ab8b22d - [lldb] Don't create duplicate declarations when completing a forward declaration with a definition from another source

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-01-29T09:20:47+01:00
New Revision: ab8b22d1c2d97b1e50c73b8640c3acb192652059

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

LOG: [lldb] Don't create duplicate declarations when completing a forward 
declaration with a definition from another source

Summary:
I noticed this strange line in `ASTImporterDelegate::ImportDefinitionTo` which 
doesn't make a lot of sense:
```
to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
```

It forcibly sets the imported TagDecl to be defined if the source TagDecl was 
defined. This doesn't make any
sense as in this code we already forced the ASTImporter to import the 
definition so this should always be
a no-op.

Turns out this is hiding two bugs:
1. The way we handle forward declarations in the debug info that might be 
completed later is that we
  import them and then mark them as having external lexical storage. This makes 
Clang ask for the definition
  later when it needs it (at which point we hopefully have the definition 
around and can complete it). However,
  this is currently not completing the forward decls with external storage but 
instead creates a duplicated decl
  in the target AST which is then defined. The forward decl is kept incomplete 
after the import and we just
  forcibly make it a definition of the record without any content with our 
workaround. The TestSharedLib* tests
  is only passing because of this.
2. Minimal import of lambdas is broken and never imports the definition it 
seems. That appears to be a bug
  in the ASTImporter which gives the definition of lambda's some special 
treatment. TestLambdas.py is actually
  broken but is passing because of this workaround.

This patch fixes the first bug by forcing the ASTImporter to import to the 
target forward declaration. We can't
delete the workaround as the second bug is still around but that will be a 
follow up review for the ASTImporter.
However it will get rid of all the duplicated RecordDecls that are in our 
expression AST that are strangely defined
but don't have any of the fields they are supposed to have.

Reviewers: shafik, labath

Reviewed By: shafik

Subscribers: aprantl, abidh, JDevlieghere, lldb-commits

Tags: #lldb

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

Added: 


Modified: 
lldb/source/Symbol/ClangASTImporter.cpp
lldb/unittests/Symbol/TestClangASTImporter.cpp

Removed: 




diff  --git a/lldb/source/Symbol/ClangASTImporter.cpp 
b/lldb/source/Symbol/ClangASTImporter.cpp
index d031ab721573..847fa5cb091d 100644
--- a/lldb/source/Symbol/ClangASTImporter.cpp
+++ b/lldb/source/Symbol/ClangASTImporter.cpp
@@ -882,6 +882,14 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl 
*From) {
 
 void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
 clang::Decl *to, clang::Decl *from) {
+  // We might have a forward declaration from a shared library that we
+  // gave external lexical storage so that Clang asks us about the full
+  // definition when it needs it. In this case the ASTImporter isn't aware
+  // that the forward decl from the shared library is the actual import
+  // target but would create a second declaration that would then be defined.
+  // We want that 'to' is actually complete after this function so let's
+  // tell the ASTImporter that 'to' was imported from 'from'.
+  MapImported(from, to);
   ASTImporter::Imported(from, to);
 
   /*

diff  --git a/lldb/unittests/Symbol/TestClangASTImporter.cpp 
b/lldb/unittests/Symbol/TestClangASTImporter.cpp
index c6eb7dd185dc..1340ef3ea62f 100644
--- a/lldb/unittests/Symbol/TestClangASTImporter.cpp
+++ b/lldb/unittests/Symbol/TestClangASTImporter.cpp
@@ -88,6 +88,33 @@ TEST_F(TestClangASTImporter, CopyTypeTagDecl) {
   EXPECT_EQ(origin.decl, source.record_decl);
 }
 
+TEST_F(TestClangASTImporter, CompleteFwdDeclWithOtherOrigin) {
+  // Create an AST with a full type that is defined.
+  clang_utils::SourceASTWithRecord source_with_definition;
+
+  // Create an AST with a type thst is only a forward declaration with the
+  // same name as the one in the the other source.
+  std::unique_ptr fwd_decl_source = clang_utils::createAST();
+  CompilerType fwd_decl_type = clang_utils::createRecord(
+  *fwd_decl_source, source_with_definition.record_decl->getName());
+
+  // Create a target and import the forward decl.
+  std::unique_ptr target = clang_utils::createAST();
+  ClangASTImporter importer;
+  CompilerType imported = importer.CopyType(*target, fwd_decl_type);
+  ASSERT_TRUE(imported.IsValid());
+  EXPECT_FALSE(imported.IsDefined());
+
+  // Now complete the forward decl with the definition from the other source.
+  // This should define the decl and give it the fields of the other origin.
+  clang::TagDecl *impor

[Lldb-commits] [PATCH] D73345: [lldb] Don't create duplicate declarations when completing a forward declaration with a definition from another source

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGab8b22d1c2d9: [lldb] Don't create duplicate 
declarations when completing a forward… (authored by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73345

Files:
  lldb/source/Symbol/ClangASTImporter.cpp
  lldb/unittests/Symbol/TestClangASTImporter.cpp


Index: lldb/unittests/Symbol/TestClangASTImporter.cpp
===
--- lldb/unittests/Symbol/TestClangASTImporter.cpp
+++ lldb/unittests/Symbol/TestClangASTImporter.cpp
@@ -88,6 +88,33 @@
   EXPECT_EQ(origin.decl, source.record_decl);
 }
 
+TEST_F(TestClangASTImporter, CompleteFwdDeclWithOtherOrigin) {
+  // Create an AST with a full type that is defined.
+  clang_utils::SourceASTWithRecord source_with_definition;
+
+  // Create an AST with a type thst is only a forward declaration with the
+  // same name as the one in the the other source.
+  std::unique_ptr fwd_decl_source = clang_utils::createAST();
+  CompilerType fwd_decl_type = clang_utils::createRecord(
+  *fwd_decl_source, source_with_definition.record_decl->getName());
+
+  // Create a target and import the forward decl.
+  std::unique_ptr target = clang_utils::createAST();
+  ClangASTImporter importer;
+  CompilerType imported = importer.CopyType(*target, fwd_decl_type);
+  ASSERT_TRUE(imported.IsValid());
+  EXPECT_FALSE(imported.IsDefined());
+
+  // Now complete the forward decl with the definition from the other source.
+  // This should define the decl and give it the fields of the other origin.
+  clang::TagDecl *imported_tag_decl = ClangUtil::GetAsTagDecl(imported);
+  importer.CompleteTagDeclWithOrigin(imported_tag_decl,
+ source_with_definition.record_decl);
+  ASSERT_TRUE(imported.IsValid());
+  EXPECT_TRUE(imported.IsDefined());
+  EXPECT_EQ(1U, imported.GetNumFields());
+}
+
 TEST_F(TestClangASTImporter, DeportDeclTagDecl) {
   // Tests that the ClangASTImporter::DeportDecl completely copies TagDecls.
   clang_utils::SourceASTWithRecord source;
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
+++ lldb/source/Symbol/ClangASTImporter.cpp
@@ -882,6 +882,14 @@
 
 void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
 clang::Decl *to, clang::Decl *from) {
+  // We might have a forward declaration from a shared library that we
+  // gave external lexical storage so that Clang asks us about the full
+  // definition when it needs it. In this case the ASTImporter isn't aware
+  // that the forward decl from the shared library is the actual import
+  // target but would create a second declaration that would then be defined.
+  // We want that 'to' is actually complete after this function so let's
+  // tell the ASTImporter that 'to' was imported from 'from'.
+  MapImported(from, to);
   ASTImporter::Imported(from, to);
 
   /*


Index: lldb/unittests/Symbol/TestClangASTImporter.cpp
===
--- lldb/unittests/Symbol/TestClangASTImporter.cpp
+++ lldb/unittests/Symbol/TestClangASTImporter.cpp
@@ -88,6 +88,33 @@
   EXPECT_EQ(origin.decl, source.record_decl);
 }
 
+TEST_F(TestClangASTImporter, CompleteFwdDeclWithOtherOrigin) {
+  // Create an AST with a full type that is defined.
+  clang_utils::SourceASTWithRecord source_with_definition;
+
+  // Create an AST with a type thst is only a forward declaration with the
+  // same name as the one in the the other source.
+  std::unique_ptr fwd_decl_source = clang_utils::createAST();
+  CompilerType fwd_decl_type = clang_utils::createRecord(
+  *fwd_decl_source, source_with_definition.record_decl->getName());
+
+  // Create a target and import the forward decl.
+  std::unique_ptr target = clang_utils::createAST();
+  ClangASTImporter importer;
+  CompilerType imported = importer.CopyType(*target, fwd_decl_type);
+  ASSERT_TRUE(imported.IsValid());
+  EXPECT_FALSE(imported.IsDefined());
+
+  // Now complete the forward decl with the definition from the other source.
+  // This should define the decl and give it the fields of the other origin.
+  clang::TagDecl *imported_tag_decl = ClangUtil::GetAsTagDecl(imported);
+  importer.CompleteTagDeclWithOrigin(imported_tag_decl,
+ source_with_definition.record_decl);
+  ASSERT_TRUE(imported.IsValid());
+  EXPECT_TRUE(imported.IsDefined());
+  EXPECT_EQ(1U, imported.GetNumFields());
+}
+
 TEST_F(TestClangASTImporter, DeportDeclTagDecl) {
   // Tests that the ClangASTImporter::DeportDecl completely copies TagDecls.
   clang_utils::SourceASTWithRecord source;
Index: lldb/source/Symbol/ClangASTImporter.cpp
===
--- lldb/source/Symbol/ClangASTImporter.cpp
++

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4103
+// this point.
+// TODO:  Is this part worthwhile?  `foo.exe` will never match `foo.pdb`
+if (matching_modules.IsEmpty())

This is not unreasonable in the non-pdb world. You can have a stripped version 
of a file somewhere prepared for deployment to some device (or downloaded from 
the device), and then you'll also have an unstripped version of that file (with 
the same name) in some build folder.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149
+
+lldbassert(matching_modules.GetSize() == 1);
+ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));

This should be a regular assert according to 
.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175-4178
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "
+   "spec and the symbol file spec\n");
+}

This part is not good. Everywhere else except pdbs this means that we were in 
fact unable to load the symbol file. What happens is that if we cannot load the 
object file representing the symbol file (at [[ 
https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
 | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile using the 
object file of the original module (line 52).

The effect of that is that the symbol file creation will always succeed, the 
previous checks are more-or-less useless, and the only way to check if we are 
really using the symbols from the file the user specified is to compare the 
file specs.

Now, PDB symbol files are abusing this fallback, and reading the symbols from 
the pdb files even though they were in fact asked to read them from the 
executable file. This is why this may sound like a "discrepancy" coming from 
the pdb world, but everywhere else this just means that the symbol file was not 
actually loaded.


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

Thanks for the review comments! I'll go ahead and land it like this, assuming 
my local test run passes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



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

[Lldb-commits] [PATCH] D73303: [lldb/Target] Add Assert StackFrame Recognizer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Since the failure reason was not very obvious from the bot message, I took a 
quick look, and to save you the trouble of reproducing this, here's what I 
found:

The reason the new test failed was because the symbols you are searching for 
(`__GI_raise`, `__GI___assert_fail`) are private libc symbols which are only 
available if you happen to have debug info for the system libc installed. If 
you don't, these will show up as `raise` and `__assert_fail`, respectively.

Also the TestExitDuringStep was failing because of some apparent memory 
corruption in the stop reason  (I am getting output like `* thread #2, name = 
'a.out', stop reason = P�4��`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73303



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

[Lldb-commits] [lldb] e06444d - [lldb] Fix windows build for the StringRef conversion operator change

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-01-29T10:08:40+01:00
New Revision: e06444d982f031ed2de20b8d5d3de2dfadb09e96

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

LOG: [lldb] Fix windows build for the StringRef conversion operator change

"operator std::string()" is now explicit.

Added: 


Modified: 
lldb/source/Host/windows/PipeWindows.cpp

Removed: 




diff  --git a/lldb/source/Host/windows/PipeWindows.cpp 
b/lldb/source/Host/windows/PipeWindows.cpp
index 38e490482ed4..873eb51dc682 100644
--- a/lldb/source/Host/windows/PipeWindows.cpp
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -105,7 +105,7 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
 return Status(ERROR_ALREADY_EXISTS, eErrorTypeWin32);
 
   std::string pipe_path = g_pipe_name_prefix;
-  pipe_path.append(name);
+  pipe_path.append(name.str());
 
   // Always open for overlapped i/o.  We implement blocking manually in Read
   // and Write.
@@ -183,7 +183,7 @@ Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
   attributes.bInheritHandle = child_process_inherit;
 
   std::string pipe_path = g_pipe_name_prefix;
-  pipe_path.append(name);
+  pipe_path.append(name.str());
 
   if (is_read) {
 m_read = ::CreateFileA(pipe_path.c_str(), GENERIC_READ, 0, &attributes,



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

[Lldb-commits] [PATCH] D73539: [AVR] Basic support for remote debugging

[Pavel Labath via Phabricator via lldb-commits]

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

Thanks. This looks fine but could you also change the title of the patch 
please? This patch doesn't really have anything to do with remote debugging 
(though it's fine to mention that this is enough to make it work). What you're 
really just doing is ensuring the "avr" architecture is properly recognised by 
the ArchSpec class.

Also, do you have commit access?


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

https://reviews.llvm.org/D73539



___
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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath added a subscriber: jingham.
labath added a comment.

Thanks. My hopefully final question is not really for you but more like for 
other lldb developers (@jingham, @clayborg, etc.).

Given that this plugin is now consisting of boiler plate only, I am wondering 
if we should not instead make it possible for this use case to work without any 
special plugins needed. A couple of options that come to mind are:

- make the base DynamicLoader class instantiatable, and use it whenever we fail 
to find a specialized plugin
- same as above, but only do that for ProcessGDBRemote instances
- make ProcessGDBRemote call `LoadModules()` itself if no dynamic loader 
instance is available

WDYT?

In D72751#1843458 , @paolosev wrote:

> Yes, this seems to be the case with the current implementation of 
> ObjectFileWASM. It creates the section list in 
> `ObjectFileWasm::SetLoadAddress` which calls `Target::SetSectionLoadAddress` 
> but the sections don't need to be fully loaded, and during 
> `SymbolFileDWARF::CalculateAbilities(...)` `ObjectFile::ReadSectionData` is 
> called to load the necessary data.


This is correct, but I want to point out that the "load" in SetLoadAddress and 
in ReadSectionData have two very different meanings. The first one records the 
address of a section in the process memory, while the second one "load" the 
contents of a section into lldb memory (from whereever). The second one should 
work regardless of whether the first one was called. This is why you are able 
to inspect the debug info of an executable before actually running it.

> File addresses can uniquely identify a single section, there is no problem 
> with this, and there is always a single code section per module. The only 
> "weirdness" is that since DWARF code addresses for Wasm are calculated from 
> the beginning of the Code section, not the beginning of the file, for the 
> Code section, `Section::m_file_offset` can normally be the file offset, but 
> `Section::m_file_addr` needs to be zero. This seems to make all DWARF-related 
> code work, but, as Pavel said, maybe there could be places where LLDB expects 
> the "load bias" to be the same for each section, which could cause problems?

The basic section loading machinery can handle sections which are "shuffled" 
around, but this is not true of everything (because this is not how typical 
object file formats work). Given that you only have one code section (no debug 
info or symbols should point into the debug sections) I think you should be 
mostly fine.

In fact it would be possible to organize things such that the "load bias" is a 
constant, if we create an additional pseudo-section for the file header (like 
we do for COFF) with a negative file address. The layout would them look 
something like this

  /\
  |   header   |  file_addr = -sizeof(header)
  ||
  |   code |  file_addr = 0
  ||
  | debug_info |  file_addr = offsetof(debug_info) - sizeof(header)
  \/

This would keep the code section at address zero, and after applying a load 
bias of `module_id | sizeof(header)`, everything would land in the right place. 
The reason I haven't proposed that is because that gets a bit messy, and so it 
seems acceptable to just do what you do now, provided it ends up working.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72751



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

[Lldb-commits] [lldb] 7a6ebb5 - [lldb] More windows StringRef fixes

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-01-29T11:15:20+01:00
New Revision: 7a6ebb5ba3cefef1865a2e0c5f9196101cbd2733

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

LOG: [lldb] More windows StringRef fixes

I don't have a windows build around, so I am just going by the buildbot
messages.

Added: 


Modified: 
lldb/source/Host/windows/ConnectionGenericFileWindows.cpp
lldb/source/Host/windows/PipeWindows.cpp

Removed: 




diff  --git a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp 
b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp
index 9f38422d7e3d..817663d6212e 100644
--- a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp
+++ b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp
@@ -127,7 +127,7 @@ lldb::ConnectionStatus 
ConnectionGenericFile::Connect(llvm::StringRef path,
   }
 
   m_owns_file = true;
-  m_uri.assign(path);
+  m_uri = path.str();
   return eConnectionStatusSuccess;
 }
 

diff  --git a/lldb/source/Host/windows/PipeWindows.cpp 
b/lldb/source/Host/windows/PipeWindows.cpp
index 873eb51dc682..b63e58e53e7f 100644
--- a/lldb/source/Host/windows/PipeWindows.cpp
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -104,7 +104,7 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
   if (CanRead() || CanWrite())
 return Status(ERROR_ALREADY_EXISTS, eErrorTypeWin32);
 
-  std::string pipe_path = g_pipe_name_prefix;
+  std::string pipe_path = g_pipe_name_prefix.str();
   pipe_path.append(name.str());
 
   // Always open for overlapped i/o.  We implement blocking manually in Read
@@ -182,7 +182,7 @@ Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
   SECURITY_ATTRIBUTES attributes = {};
   attributes.bInheritHandle = child_process_inherit;
 
-  std::string pipe_path = g_pipe_name_prefix;
+  std::string pipe_path = g_pipe_name_prefix.str();
   pipe_path.append(name.str());
 
   if (is_read) {



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

[Lldb-commits] [lldb] 7116e43 - [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via lldb-commits]

Author: James Henderson
Date: 2020-01-29T10:23:41Z
New Revision: 7116e431c0ab4194907bbaf73482ac05d923787f

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

LOG: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

Many of the debug line prologue errors are not inherently fatal. In most
cases, we can make reasonable assumptions and carry on. This patch does
exactly that. In the case of length problems, the approach of "assume
stated length is correct" is taken which means the offset might need
adjusting.

This is a relanding of b94191fe, fixing an LLD test and the LLDB build.

Reviewed by: dblaikie, labath

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

Added: 


Modified: 
lld/test/ELF/Inputs/undef-bad-debug.s
lld/test/ELF/undef.s
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

Removed: 




diff  --git a/lld/test/ELF/Inputs/undef-bad-debug.s 
b/lld/test/ELF/Inputs/undef-bad-debug.s
index d52710b2efd8..bf517f3ea1cd 100644
--- a/lld/test/ELF/Inputs/undef-bad-debug.s
+++ b/lld/test/ELF/Inputs/undef-bad-debug.s
@@ -5,6 +5,8 @@ sym2:
 .quad zed6b
 sym3:
 .quad zed7
+sym4:
+.quad zed8
 
 .section .debug_line,"",@progbits
 .Lunit:
@@ -47,6 +49,12 @@ sym3:
 .Lunit2:
 .long .Lunit2_end - .Lunit2_start # unit length
 .Lunit2_start:
+.short 1# version
+.Lunit2_end:
+
+.Lunit3:
+.long .Lunit3_end - .Lunit3_start # unit length
+.Lunit3_start:
 .short 4# version
 .long .Lprologue2_end - .Lprologue2_start # prologue length
 .Lprologue2_start:
@@ -64,7 +72,7 @@ sym3:
 .byte 0
 .Lprologue2_end:
 .byte 0, 9, 2   # DW_LNE_set_address
-.quad sym3
+.quad sym4
 .byte 3 # DW_LNS_advance_line
 .byte 10
 .byte 1 # DW_LNS_copy
@@ -79,7 +87,7 @@ sym3:
 .byte 99# DW_LNS_advance_pc
 .byte 119
 # Missing end of sequence.
-.Lunit2_end:
+.Lunit3_end:
 
 .section .debug_info,"",@progbits
 .long   .Lcu_end - .Lcu_start   # Length of Unit
@@ -117,6 +125,21 @@ sym3:
 .byte   0   # End Of Children Mark
 .Lcu2_end:
 
+.long   .Lcu3_end - .Lcu3_start # Length of Unit
+.Lcu3_start:
+.short  4   # DWARF version number
+.long   .Lsection_abbrev# Offset Into Abbrev. Section
+.byte   8   # Address Size (in bytes)
+.byte   1   # Abbrev [1] 0xb:0x79 DW_TAG_compile_unit
+.long   .Lunit3 # DW_AT_stmt_list
+.byte   2   # Abbrev [2] 0x2a:0x15 DW_TAG_variable
+.long   .Linfo3_string  # DW_AT_name
+# DW_AT_external
+.byte   1   # DW_AT_decl_file
+.byte   3   # DW_AT_decl_line
+.byte   0   # End Of Children Mark
+.Lcu3_end:
+
 .section .debug_abbrev,"",@progbits
 .Lsection_abbrev:
 .byte   1   # Abbreviation Code
@@ -148,3 +171,5 @@ sym3:
 .asciz "sym2"
 .Linfo2_string:
 .asciz "sym3"
+.Linfo3_string:
+.asciz "sym4"

diff  --git a/lld/test/ELF/undef.s b/lld/test/ELF/undef.s
index 2ca733a26fd7..b8df0458539d 100644
--- a/lld/test/ELF/undef.s
+++ b/lld/test/ELF/undef.s
@@ -22,7 +22,7 @@
 # CHECK-NEXT: >>> referenced by undef.s
 # CHECK-NEXT: >>>   {{.*}}:(.text+0x10)
 
-# CHECK:  error: undefined symbol: vtable for Foo
+# CHECK:  error: undefined symbol: vtable for Foo 
 # CHECK-NEXT: >>> referenced by undef.s
 # CHECK-NEXT: >>>   {{.*}}:(.text+0x15)
 # CHECK-NEXT: the vtable symbol may be undefined because the class is missing 
its key function (see https://lld.llvm.org/missingkeyfunction)
@@ -52,17 +52,25 @@
 # is requested, even if that particular part of the line information is not 
currently required.
 # Also show that the warnings are only printed once.
 # CHECK:  warning: parsing line table prologue at 0x should have 
ended at 0x0038 but it ended at 0x0037
-# CHECK-NEXT: warning: last sequence in debug line table at offset 0x005b 
is not terminated
+# CHECK-NEXT: warning: parsing line table prologue at offset 0x005b found 
unsupported version 0x01
+# CHECK-NEXT: warning: last sequence in debug line table at offset 0x0061 
is not terminated
 # CHECK:  error: undefi

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7116e431c0ab: [DebugInfo] Make most debug line prologue 
errors non-fatal to parsing (authored by jhenderson).

Changed prior to commit:
  https://reviews.llvm.org/D72158?vs=240852&id=241076#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158

Files:
  lld/test/ELF/Inputs/undef-bad-debug.s
  lld/test/ELF/undef.s
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
  llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
  llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
  llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -359,10 +359,15 @@
 
   generate();
 
-  checkGetOrParseLineTableEmitsFatalError(
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  EXPECT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+
+  checkError(
   {"parsing line table prologue at 0x found an invalid directory "
"or file table description at 0x0014",
-   "failed to parse entry content descriptions because no path was found"});
+   "failed to parse entry content descriptions because no path was found"},
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooLargePrologueLength) {
@@ -379,14 +384,24 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  --Result.Prologue.PrologueLength;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength + 1 + Prologue.sizeofTotalLength();
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd - 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
@@ -408,16 +423,29 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  if (Version < 5)
+Result.Prologue.PrologueLength += 2;
+  else
+Result.Prologue.PrologueLength += 1;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength - 1 + Prologue.sizeofTotalLength();
   if (Version < 5)
 --ExpectedEnd;
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd + 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 INSTANTIATE_TEST_CASE_P(
@@ -636,14 +664,15 @@
   EXPECT_EQ(Parser.getOffset(), 0u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 62u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 136u);
   EXPECT_TRUE(Parser.done());
 
+  EXPECT_FALSE(Recoverable);
   EXPECT_FALSE(Unrecoverable);
 }
 
@@ -688,10 +717,11 @@
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_EQ(Parser.getOffset(), 4u);
   EXPECT_TRUE(Parser.done());
+  EXPECT_FALSE(Recoverable);
 
   checkError("parsing line table prologue at offset 0x unsupported "
  "reserved unit length found of value 0xfff0",
@@ -767,11 +797,12 @@
   generate();
 
   DWARFDebugLine::SectionParser Pa

[Lldb-commits] [PATCH] D73539: [AVR] Recognize the AVR architecture in lldb

[Ayke via Phabricator via lldb-commits]

aykevl added a comment.

Ok, I've updated the title and the commit message (text until the separator). 
Does that look good?

I have commit access so I can merge this myself.


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

https://reviews.llvm.org/D73539



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

[Lldb-commits] [lldb] 33fa672 - [lldb/Reproducers] Add logging to the string template specialization

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T09:16:43-08:00
New Revision: 33fa6727b7ce1c3aad6ef4bb24dff2ce3e3b2c7a

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

LOG: [lldb/Reproducers] Add logging to the string template specialization

Only the templated function had logging for deserialization. The string
deserializer is implemented as a specialization and now prints to the
log as well.

Added: 


Modified: 
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index 10e4e3a14e60..46def9f59d7b 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -32,6 +32,10 @@ template <> const char *Deserializer::Deserialize() {
 return nullptr;
   const char *str = m_buffer.data();
   m_buffer = m_buffer.drop_front(pos + 1);
+#ifdef LLDB_REPRO_INSTR_TRACE
+llvm::errs() << "Deserializing with " << LLVM_PRETTY_FUNCTION << " -> \""
+ << str << "\"\n";
+#endif
   return str;
 }
 



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

[Lldb-commits] [lldb] ce07cde - [lldb/Host] Fix implicit StringRef to std::string conversion

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T10:36:04-08:00
New Revision: ce07cdea336619c308c3130e936944c67774549d

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

LOG: [lldb/Host] Fix implicit StringRef to std::string conversion

lldb\source\Host\windows\Host.cpp(228): error C2440: 'initializing':
cannot convert from 'llvm::StringRef' to
'std::basic_string,std::allocator>'

Added: 


Modified: 
lldb/source/Host/windows/Host.cpp

Removed: 




diff  --git a/lldb/source/Host/windows/Host.cpp 
b/lldb/source/Host/windows/Host.cpp
index 88245675b373..599d94802812 100644
--- a/lldb/source/Host/windows/Host.cpp
+++ b/lldb/source/Host/windows/Host.cpp
@@ -225,7 +225,7 @@ Status Host::ShellExpandArguments(ProcessLaunchInfo 
&launch_info) {
 
 int status;
 std::string output;
-std::string command = expand_command.GetString();
+std::string command = expand_command.GetString().str();
 Status e =
 RunShellCommand(command.c_str(), launch_info.GetWorkingDirectory(),
 &status, nullptr, &output, std::chrono::seconds(10));



___
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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D72751#1846385 , @labath wrote:

> Thanks. My hopefully final question is not really for you but more like for 
> other lldb developers (@jingham, @clayborg, etc.).
>
> Given that this plugin is now consisting of boiler plate only, I am wondering 
> if we should not instead make it possible for this use case to work without 
> any special plugins needed. A couple of options that come to mind are:
>
> - make the base DynamicLoader class instantiatable, and use it whenever we 
> fail to find a specialized plugin
> - same as above, but only do that for ProcessGDBRemote instances
> - make ProcessGDBRemote call `LoadModules()` itself if no dynamic loader 
> instance is available WDYT?


I am fine with 1 as long as we document the DynamicLoader class to say that it 
will call Process::LoadModules() and will be used if no specialized loader is 
needed for your platform. I would like to a see a solution that will work for 
any process plug-in and not just ProcessGDBRemote. If we change solution 3 
above to say "Make lldb_private::Process call `LoadModules()` itself if no 
dynamic loader instance is available" then solution 3 is also fine.

> 
> 
> In D72751#1843458 , @paolosev wrote:
> 
>> Yes, this seems to be the case with the current implementation of 
>> ObjectFileWASM. It creates the section list in 
>> `ObjectFileWasm::SetLoadAddress` which calls `Target::SetSectionLoadAddress` 
>> but the sections don't need to be fully loaded, and during 
>> `SymbolFileDWARF::CalculateAbilities(...)` `ObjectFile::ReadSectionData` is 
>> called to load the necessary data.
> 
> 
> This is correct, but I want to point out that the "load" in SetLoadAddress 
> and in ReadSectionData have two very different meanings. The first one 
> records the address of a section in the process memory, while the second one 
> "load" the contents of a section into lldb memory (from whereever). The 
> second one should work regardless of whether the first one was called. This 
> is why you are able to inspect the debug info of an executable before 
> actually running it.
> 
>> File addresses can uniquely identify a single section, there is no problem 
>> with this, and there is always a single code section per module. The only 
>> "weirdness" is that since DWARF code addresses for Wasm are calculated from 
>> the beginning of the Code section, not the beginning of the file, for the 
>> Code section, `Section::m_file_offset` can normally be the file offset, but 
>> `Section::m_file_addr` needs to be zero. This seems to make all 
>> DWARF-related code work, but, as Pavel said, maybe there could be places 
>> where LLDB expects the "load bias" to be the same for each section, which 
>> could cause problems?
> 
> The basic section loading machinery can handle sections which are "shuffled" 
> around, but this is not true of everything (because this is not how typical 
> object file formats work). Given that you only have one code section (no 
> debug info or symbols should point into the debug sections) I think you 
> should be mostly fine.
> 
> In fact it would be possible to organize things such that the "load bias" is 
> a constant, if we create an additional pseudo-section for the file header 
> (like we do for COFF) with a negative file address. The layout would them 
> look something like this
> 
>   /\
>   |   header   |  file_addr = -sizeof(header)
>   ||
>   |   code |  file_addr = 0
>   ||
>   | debug_info |  file_addr = offsetof(debug_info) - sizeof(header)
>   \/
> 
> 
> This would keep the code section at address zero, and after applying a load 
> bias of `module_id | sizeof(header)`, everything would land in the right 
> place. The reason I haven't proposed that is because that gets a bit messy, 
> and so it seems acceptable to just do what you do now, provided it ends up 
> working.

Yes the current approach allows anyone to load any section at any address. On 
Darwin systems, the DYLD shared cache will move __TEXT, __DATA, and other 
sections around such that all __TEXT sections from all shared libraries in the 
shared cache are all in the one contiguous range. The slide is different for 
each section, so we have some nice flexibility with being able to set the 
section load address individually. They will even invert the memory order 
sometimes where in the file we have __TEXT followed by __DATA, but in the 
shared cache __DATA appears at a lower address than __TEXT. We currently don't 
have the ability to load the same section at multiple addresses. This can 
happen when a shared library is loaded multiple times in memory, which we have 
seen on Android where a vendor will have a file that is the same as the base 
system, and the same exact file in loaded, albeit from different paths.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.ll

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149
+
+lldbassert(matching_modules.GetSize() == 1);
+ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));

labath wrote:
> This should be a regular assert according to 
> .
This assert will never trigger with the above code already checking for empty 
on line 4123 and then checking if size > 1 on line 4140. Remove it?



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175
+
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "

labath wrote:
> This part is not good. Everywhere else except pdbs this means that we were in 
> fact unable to load the symbol file. What happens is that if we cannot load 
> the object file representing the symbol file (at [[ 
> https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
>  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile using the 
> object file of the original module (line 52).
> 
> The effect of that is that the symbol file creation will always succeed, the 
> previous checks are more-or-less useless, and the only way to check if we are 
> really using the symbols from the file the user specified is to compare the 
> file specs.
> 
> Now, PDB symbol files are abusing this fallback, and reading the symbols from 
> the pdb files even though they were in fact asked to read them from the 
> executable file. This is why this may sound like a "discrepancy" coming from 
> the pdb world, but everywhere else this just means that the symbol file was 
> not actually loaded.
This could also fail if the symbol file spec was a bundle which got resolved 
when the module was loaded. If the user specified "/path/to/foo.dSYM" and the 
underlying code was able to resolve the symbol file in the dSYM bundle to 
"/path/to/foo.dSYM/Contents/Resources/DWARF/foo".


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth marked 3 inline comments as done.
amccarth added a comment.

Thanks for the feedback.  Obviously I'm confused about how LLDB handles split 
debug info, so I need more clarification about how to proceed.




Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4103
+// this point.
+// TODO:  Is this part worthwhile?  `foo.exe` will never match `foo.pdb`
+if (matching_modules.IsEmpty())

labath wrote:
> This is not unreasonable in the non-pdb world. You can have a stripped 
> version of a file somewhere prepared for deployment to some device (or 
> downloaded from the device), and then you'll also have an unstripped version 
> of that file (with the same name) in some build folder.
Sure, if you've got two files with the same name in two directories that's fine.

But the loop tries peeling the extension off of the supplied file name and 
comparing it to the target's file name.  I guess it's trying to match something 
like `foo.unstripped` to `foo`.  Is that a typical thing?



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175-4178
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "
+   "spec and the symbol file spec\n");
+}

clayborg wrote:
> labath wrote:
> > This part is not good. Everywhere else except pdbs this means that we were 
> > in fact unable to load the symbol file. What happens is that if we cannot 
> > load the object file representing the symbol file (at [[ 
> > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
> >  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile using the 
> > object file of the original module (line 52).
> > 
> > The effect of that is that the symbol file creation will always succeed, 
> > the previous checks are more-or-less useless, and the only way to check if 
> > we are really using the symbols from the file the user specified is to 
> > compare the file specs.
> > 
> > Now, PDB symbol files are abusing this fallback, and reading the symbols 
> > from the pdb files even though they were in fact asked to read them from 
> > the executable file. This is why this may sound like a "discrepancy" coming 
> > from the pdb world, but everywhere else this just means that the symbol 
> > file was not actually loaded.
> This could also fail if the symbol file spec was a bundle which got resolved 
> when the module was loaded. If the user specified "/path/to/foo.dSYM" and the 
> underlying code was able to resolve the symbol file in the dSYM bundle to 
> "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".
Interesting.  I did not expect that fallback behavior from the API.

> PDB symbol files are abusing this fallback

I'm not sure there's abuse going on here.  I'm not understanding something 
about how lldb models this situation.  Consider the case where the user 
explicitly asks lldb to load symbols from a PDB:

(lldb) target add symbols D:\my_symbols\foo.pdb

Then `symbol_file` is the PDB and `object_file` is the executable or DLL.  I 
would expect those file specs to match only in the case where the symbols are 
in the binary (in other words, when symbol file and object file are the same 
file).  Even ignoring the PDB case, it seems this wouldn't even work in the 
case you mentioned above where the object file is stripped and the symbol file 
is an unstripped copy of the object file (possibly with a different name).  You 
can also get here if the symbols were matched by UUID rather than file spec.  
Or when the symbols were matched by the basename minus some extension.

Given that dsyms work, I must be misunderstanding something here.  Can you help 
me understand?

What's the right thing to do here?  If I just undo this refactor, then we're 
back to silent failures when the symbols exist in a different file than the 
object.


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

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



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149
+
+lldbassert(matching_modules.GetSize() == 1);
+ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));

clayborg wrote:
> labath wrote:
> > This should be a regular assert according to 
> > .
> This assert will never trigger with the above code already checking for empty 
> on line 4123 and then checking if size > 1 on line 4140. Remove it?
It's true that this assert won't happen given the early returns above, so I 
will remove it if you like.

But I did it intentionally:  (1) to document the requirement and (2) to protect 
against future changes accidentally breaking the assumptions.  If the function 
were much shorter, then it might be obvious enough as is.  But early returns, 
especially halfway down a long function, can be easy to miss.


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149
+
+lldbassert(matching_modules.GetSize() == 1);
+ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));

amccarth wrote:
> clayborg wrote:
> > labath wrote:
> > > This should be a regular assert according to 
> > > .
> > This assert will never trigger with the above code already checking for 
> > empty on line 4123 and then checking if size > 1 on line 4140. Remove it?
> It's true that this assert won't happen given the early returns above, so I 
> will remove it if you like.
> 
> But I did it intentionally:  (1) to document the requirement and (2) to 
> protect against future changes accidentally breaking the assumptions.  If the 
> function were much shorter, then it might be obvious enough as is.  But early 
> returns, especially halfway down a long function, can be easy to miss.
Gotcha. It is fine to leave it in, just switch to assert() as Pavel mentioned.


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [lldb] a111ffb - [lldb] Fix build break in ProcessDebugger due to StringRef usage changes

[Stella Stamenova via lldb-commits]

Author: Stella Stamenova
Date: 2020-01-29T13:19:04-08:00
New Revision: a111ffbb03f7a9e61bfb2dc29689234887e30014

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

LOG: [lldb] Fix build break in ProcessDebugger due to StringRef usage changes

Added: 


Modified: 
lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp 
b/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
index 2cf384ec1e08..8a85c8ba6f4e 100644
--- a/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
+++ b/lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
@@ -127,7 +127,7 @@ Status ProcessDebugger::LaunchProcess(ProcessLaunchInfo 
&launch_info,
 stream.Printf("ProcessDebugger unable to launch '%s'.  ProcessDebugger can 
"
   "only be used for debug launches.",
   launch_info.GetExecutableFile().GetPath().c_str());
-std::string message = stream.GetString();
+std::string message = stream.GetString().str();
 result.SetErrorString(message.c_str());
 
 LLDB_LOG(log, "error: {0}", message);



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth marked 2 inline comments as done.
amccarth added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175
+
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "

amccarth wrote:
> clayborg wrote:
> > labath wrote:
> > > This part is not good. Everywhere else except pdbs this means that we 
> > > were in fact unable to load the symbol file. What happens is that if we 
> > > cannot load the object file representing the symbol file (at [[ 
> > > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
> > >  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile using 
> > > the object file of the original module (line 52).
> > > 
> > > The effect of that is that the symbol file creation will always succeed, 
> > > the previous checks are more-or-less useless, and the only way to check 
> > > if we are really using the symbols from the file the user specified is to 
> > > compare the file specs.
> > > 
> > > Now, PDB symbol files are abusing this fallback, and reading the symbols 
> > > from the pdb files even though they were in fact asked to read them from 
> > > the executable file. This is why this may sound like a "discrepancy" 
> > > coming from the pdb world, but everywhere else this just means that the 
> > > symbol file was not actually loaded.
> > This could also fail if the symbol file spec was a bundle which got 
> > resolved when the module was loaded. If the user specified 
> > "/path/to/foo.dSYM" and the underlying code was able to resolve the symbol 
> > file in the dSYM bundle to "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".
> Interesting.  I did not expect that fallback behavior from the API.
> 
> > PDB symbol files are abusing this fallback
> 
> I'm not sure there's abuse going on here.  I'm not understanding something 
> about how lldb models this situation.  Consider the case where the user 
> explicitly asks lldb to load symbols from a PDB:
> 
> (lldb) target add symbols D:\my_symbols\foo.pdb
> 
> Then `symbol_file` is the PDB and `object_file` is the executable or DLL.  I 
> would expect those file specs to match only in the case where the symbols are 
> in the binary (in other words, when symbol file and object file are the same 
> file).  Even ignoring the PDB case, it seems this wouldn't even work in the 
> case you mentioned above where the object file is stripped and the symbol 
> file is an unstripped copy of the object file (possibly with a different 
> name).  You can also get here if the symbols were matched by UUID rather than 
> file spec.  Or when the symbols were matched by the basename minus some 
> extension.
> 
> Given that dsyms work, I must be misunderstanding something here.  Can you 
> help me understand?
> 
> What's the right thing to do here?  If I just undo this refactor, then we're 
> back to silent failures when the symbols exist in a different file than the 
> object.
In your example, the file specs do not match.

1. The old code would therefore reject the symbol file and return an error.
2. The new code would therefore emit a warning but proceed with the selected 
symbol file and return success.

Do you want either of these behaviors or something else?


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175
+
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "

amccarth wrote:
> amccarth wrote:
> > clayborg wrote:
> > > labath wrote:
> > > > This part is not good. Everywhere else except pdbs this means that we 
> > > > were in fact unable to load the symbol file. What happens is that if we 
> > > > cannot load the object file representing the symbol file (at [[ 
> > > > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
> > > >  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile using 
> > > > the object file of the original module (line 52).
> > > > 
> > > > The effect of that is that the symbol file creation will always 
> > > > succeed, the previous checks are more-or-less useless, and the only way 
> > > > to check if we are really using the symbols from the file the user 
> > > > specified is to compare the file specs.
> > > > 
> > > > Now, PDB symbol files are abusing this fallback, and reading the 
> > > > symbols from the pdb files even though they were in fact asked to read 
> > > > them from the executable file. This is why this may sound like a 
> > > > "discrepancy" coming from the pdb world, but everywhere else this just 
> > > > means that the symbol file was not actually loaded.
> > > This could also fail if the symbol file spec was a bundle which got 
> > > resolved when the module was loaded. If the user specified 
> > > "/path/to/foo.dSYM" and the underlying code was able to resolve the 
> > > symbol file in the dSYM bundle to 
> > > "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".
> > Interesting.  I did not expect that fallback behavior from the API.
> > 
> > > PDB symbol files are abusing this fallback
> > 
> > I'm not sure there's abuse going on here.  I'm not understanding something 
> > about how lldb models this situation.  Consider the case where the user 
> > explicitly asks lldb to load symbols from a PDB:
> > 
> > (lldb) target add symbols D:\my_symbols\foo.pdb
> > 
> > Then `symbol_file` is the PDB and `object_file` is the executable or DLL.  
> > I would expect those file specs to match only in the case where the symbols 
> > are in the binary (in other words, when symbol file and object file are the 
> > same file).  Even ignoring the PDB case, it seems this wouldn't even work 
> > in the case you mentioned above where the object file is stripped and the 
> > symbol file is an unstripped copy of the object file (possibly with a 
> > different name).  You can also get here if the symbols were matched by UUID 
> > rather than file spec.  Or when the symbols were matched by the basename 
> > minus some extension.
> > 
> > Given that dsyms work, I must be misunderstanding something here.  Can you 
> > help me understand?
> > 
> > What's the right thing to do here?  If I just undo this refactor, then 
> > we're back to silent failures when the symbols exist in a different file 
> > than the object.
> In your example, the file specs do not match.
> 
> 1. The old code would therefore reject the symbol file and return an error.
> 2. The new code would therefore emit a warning but proceed with the selected 
> symbol file and return success.
> 
> Do you want either of these behaviors or something else?
all I know is I can and have typed:
```
(lldb) target symbols add /path/to/foo.dSYM
```
And it would successfully associate it with the binary, probably because the 
code on 4071 would see a UUID and accept it before it gets to this point. 

What is the point of the warning? When would this happen?


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

https://reviews.llvm.org/D73594



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

[Lldb-commits] [lldb] 91aa67b - [lldb/Reproducers] Add (de)serialization overload for char**

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T14:08:21-08:00
New Revision: 91aa67bf290bc7f877b1b90128284863bc31aa43

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

LOG: [lldb/Reproducers] Add (de)serialization overload for char**

This patch adds an overload to serialize and deserialize char** types.
This is necessary for things like the SBLaunchInfo ctor. We serialize
the array length followed by each individual item.

Added: 


Modified: 
lldb/include/lldb/Utility/ReproducerInstrumentation.h
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/ReproducerInstrumentation.h 
b/lldb/include/lldb/Utility/ReproducerInstrumentation.h
index 2698338a38bb..ca92f9f2f6e6 100644
--- a/lldb/include/lldb/Utility/ReproducerInstrumentation.h
+++ b/lldb/include/lldb/Utility/ReproducerInstrumentation.h
@@ -375,6 +375,7 @@ class Deserializer {
 /// Partial specialization for C-style strings. We read the string value
 /// instead of treating it as pointer.
 template <> const char *Deserializer::Deserialize();
+template <> const char **Deserializer::Deserialize();
 template <> char *Deserializer::Deserialize();
 
 /// Helpers to auto-synthesize function replay code. It deserializes the replay
@@ -604,6 +605,22 @@ class Serializer {
 m_stream.write(0x0);
   }
 
+  void Serialize(const char **t) {
+// Compute the size of the array.
+const char *const *temp = t;
+size_t size = 0;
+while (*temp++)
+  size++;
+Serialize(size);
+
+// Serialize the content of the array.
+while (*t) {
+  m_stream << *t;
+  m_stream.write(0x0);
+  ++t;
+}
+  }
+
   /// Serialization stream.
   llvm::raw_ostream &m_stream;
 

diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index 46def9f59d7b..fb39fec41d43 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -9,6 +9,7 @@
 #include "lldb/Utility/ReproducerInstrumentation.h"
 #include "lldb/Utility/Reproducer.h"
 #include 
+#include 
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
@@ -39,6 +40,15 @@ template <> const char *Deserializer::Deserialize() {
   return str;
 }
 
+template <> const char **Deserializer::Deserialize() {
+  size_t size = Deserialize();
+  const char **r =
+  reinterpret_cast(calloc(size + 1, sizeof(char *)));
+  for (size_t i = 0; i < size; ++i)
+r[i] = Deserialize();
+  return r;
+}
+
 bool Registry::Replay(const FileSpec &file) {
   auto error_or_file = llvm::MemoryBuffer::getFile(file.GetPath());
   if (auto err = error_or_file.getError())



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

[Lldb-commits] [PATCH] D73665: Stop emitting a breakpoint for each location in a breakpoint when responding to breakpoint commands.

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added a reviewer: labath.
Herald added a project: LLDB.

The VS Code DAP expects on response for each breakpoint that was requested. If 
we responsd with multiple entries for one breakpoint the VS Code UI gets out of 
date. Currently the VS code DAP doesn't handle one breakpoint with multiple 
locations. If this ever gets fixed we can modify our code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73665

Files:
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/LLDBUtils.cpp
  lldb/tools/lldb-vscode/LLDBUtils.h

Index: lldb/tools/lldb-vscode/LLDBUtils.h
===
--- lldb/tools/lldb-vscode/LLDBUtils.h
+++ lldb/tools/lldb-vscode/LLDBUtils.h
@@ -106,46 +106,6 @@
 /// The LLDB frame index ID.
 uint32_t GetLLDBFrameID(uint64_t dap_frame_id);
 
-/// Given a LLDB breakpoint, make a breakpoint ID that is unique to a
-/// specific breakpoint and breakpoint location.
-///
-/// VSCode requires a Breakpoint "id" to be unique, so we use the
-/// breakpoint ID in the lower BREAKPOINT_ID_SHIFT bits and the
-/// breakpoint location ID in the upper BREAKPOINT_ID_SHIFT bits.
-///
-/// \param[in] bp_loc
-/// The LLDB break point location.
-///
-/// \return
-/// A unique integer that allows us to easily find the right
-/// stack frame within a thread on subsequent VS code requests.
-int64_t MakeVSCodeBreakpointID(lldb::SBBreakpointLocation &bp_loc);
-
-/// Given a VSCode breakpoint ID, convert to a LLDB breakpoint ID.
-///
-/// VSCode requires a Breakpoint "id" to be unique, so we use the
-/// breakpoint ID in the lower BREAKPOINT_ID_SHIFT bits and the
-/// breakpoint location ID in the upper BREAKPOINT_ID_SHIFT bits.
-///
-/// \param[in] dap_breakpoint_id
-/// The VSCode breakpoint ID to convert to an LLDB breakpoint ID.
-///
-/// \return
-/// The LLDB breakpoint ID.
-uint32_t GetLLDBBreakpointID(uint64_t dap_breakpoint_id);
-
-/// Given a VSCode breakpoint ID, convert to a LLDB breakpoint location ID.
-///
-/// VSCode requires a Breakpoint "id" to be unique, so we use the
-/// breakpoint ID in the lower BREAKPOINT_ID_SHIFT bits and the
-/// breakpoint location ID in the upper BREAKPOINT_ID_SHIFT bits.
-///
-/// \param[in] dap_breakpoint_id
-/// The VSCode frame ID to convert to a breakpoint location ID.
-///
-/// \return
-/// The LLDB breakpoint location ID.
-uint32_t GetLLDBBreakpointLocationID(uint64_t dap_breakpoint_id);
 } // namespace lldb_vscode
 
 #endif
Index: lldb/tools/lldb-vscode/LLDBUtils.cpp
===
--- lldb/tools/lldb-vscode/LLDBUtils.cpp
+++ lldb/tools/lldb-vscode/LLDBUtils.cpp
@@ -79,19 +79,4 @@
frame.GetFrameID());
 }
 
-static uint32_t constexpr BREAKPOINT_ID_SHIFT = 22;
-
-uint32_t GetLLDBBreakpointID(uint64_t dap_breakpoint_id) {
-  return dap_breakpoint_id >> BREAKPOINT_ID_SHIFT;
-}
-
-uint32_t GetLLDBBreakpointLocationID(uint64_t dap_breakpoint_id) {
-  return dap_breakpoint_id & ((1u << BREAKPOINT_ID_SHIFT) - 1);
-}
-
-int64_t MakeVSCodeBreakpointID(lldb::SBBreakpointLocation &bp_loc) {
-  return (int64_t)(bp_loc.GetBreakpoint().GetID() << BREAKPOINT_ID_SHIFT |
-   bp_loc.GetID());
-}
-
 } // namespace lldb_vscode
Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -289,7 +289,7 @@
 return llvm::json::Value(std::move(object));
 
   object.try_emplace("verified", true);
-  const auto vs_id = MakeVSCodeBreakpointID(bp_loc);
+  const auto vs_id = bp_loc.GetBreakpoint().GetID();
   object.try_emplace("id", vs_id);
   auto bp_addr = bp_loc.GetAddress();
   if (bp_addr.IsValid()) {
@@ -308,10 +308,12 @@
   const auto num_locations = bp.GetNumLocations();
   if (num_locations == 0)
 return;
-  for (size_t i = 0; i < num_locations; ++i) {
-auto bp_loc = bp.GetLocationAtIndex(i);
-breakpoints.emplace_back(CreateBreakpoint(bp_loc));
-  }
+  // VS Code DAP doesn't currently allow one breakpoint to have multiple
+  // locations so we just report the first one. If we report all locations
+  // then the IDE starts showing the wrong line numbers and locations for
+  // other source file and line breakpoints in the same file.
+  auto bp_loc = bp.GetLocationAtIndex(0);
+  breakpoints.emplace_back(CreateBreakpoint(bp_loc));
 }
 
 // "Event": {
@@ -870,4 +872,3 @@
 }
 
 } // namespace lldb_vscode
-
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

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



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175
+
+if (object_file->GetFileSpec() != symbol_fspec) {
+  result.AppendWarning("there is a discrepancy between the module file "

clayborg wrote:
> amccarth wrote:
> > amccarth wrote:
> > > clayborg wrote:
> > > > labath wrote:
> > > > > This part is not good. Everywhere else except pdbs this means that we 
> > > > > were in fact unable to load the symbol file. What happens is that if 
> > > > > we cannot load the object file representing the symbol file (at [[ 
> > > > > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48
> > > > >  | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile 
> > > > > using the object file of the original module (line 52).
> > > > > 
> > > > > The effect of that is that the symbol file creation will always 
> > > > > succeed, the previous checks are more-or-less useless, and the only 
> > > > > way to check if we are really using the symbols from the file the 
> > > > > user specified is to compare the file specs.
> > > > > 
> > > > > Now, PDB symbol files are abusing this fallback, and reading the 
> > > > > symbols from the pdb files even though they were in fact asked to 
> > > > > read them from the executable file. This is why this may sound like a 
> > > > > "discrepancy" coming from the pdb world, but everywhere else this 
> > > > > just means that the symbol file was not actually loaded.
> > > > This could also fail if the symbol file spec was a bundle which got 
> > > > resolved when the module was loaded. If the user specified 
> > > > "/path/to/foo.dSYM" and the underlying code was able to resolve the 
> > > > symbol file in the dSYM bundle to 
> > > > "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".
> > > Interesting.  I did not expect that fallback behavior from the API.
> > > 
> > > > PDB symbol files are abusing this fallback
> > > 
> > > I'm not sure there's abuse going on here.  I'm not understanding 
> > > something about how lldb models this situation.  Consider the case where 
> > > the user explicitly asks lldb to load symbols from a PDB:
> > > 
> > > (lldb) target add symbols D:\my_symbols\foo.pdb
> > > 
> > > Then `symbol_file` is the PDB and `object_file` is the executable or DLL. 
> > >  I would expect those file specs to match only in the case where the 
> > > symbols are in the binary (in other words, when symbol file and object 
> > > file are the same file).  Even ignoring the PDB case, it seems this 
> > > wouldn't even work in the case you mentioned above where the object file 
> > > is stripped and the symbol file is an unstripped copy of the object file 
> > > (possibly with a different name).  You can also get here if the symbols 
> > > were matched by UUID rather than file spec.  Or when the symbols were 
> > > matched by the basename minus some extension.
> > > 
> > > Given that dsyms work, I must be misunderstanding something here.  Can 
> > > you help me understand?
> > > 
> > > What's the right thing to do here?  If I just undo this refactor, then 
> > > we're back to silent failures when the symbols exist in a different file 
> > > than the object.
> > In your example, the file specs do not match.
> > 
> > 1. The old code would therefore reject the symbol file and return an error.
> > 2. The new code would therefore emit a warning but proceed with the 
> > selected symbol file and return success.
> > 
> > Do you want either of these behaviors or something else?
> all I know is I can and have typed:
> ```
> (lldb) target symbols add /path/to/foo.dSYM
> ```
> And it would successfully associate it with the binary, probably because the 
> code on 4071 would see a UUID and accept it before it gets to this point. 
> 
> What is the point of the warning? When would this happen?
There are examples in the thread with Pavel just below this thread.  On 
Windows, your symbols may come from a file called `foo.pdb` for an executable 
called `foo.exe`.  The old code would successfully load the PDB file, then 
decide it was no good because the file specs don't match.  (Heck, at this 
point, we've just gone through a bunch of gyrations to allow matching of 
differing file specs.)  So you'd get a mostly silent failure for code that 
actually worked.

My intent was to simply warn about such cases without actually forcing a 
failure, and to do so with some explicit feedback so that the user understands.

I'll wait to hear what Pavel says.  If necessary, I'll just roll back this part 
of the change (so that this remains just a refactor), and I'll figure out a way 
around the problem in a later patch.


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

https://reviews.llvm.org/D73594



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

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

[Paolo Severini via Phabricator via lldb-commits]

paolosev added a comment.

Regarding:

>> - make the base DynamicLoader class instantiatable, and use it whenever we 
>> fail to find a specialized plugin
>> - same as above, but only do that for ProcessGDBRemote instances
>> - make ProcessGDBRemote call LoadModules() itself if no dynamic loader 
>> instance is available WDYT?
> 
> I am fine with 1 as long as we document the DynamicLoader class to say that 
> it will call Process::LoadModules() and will be used if no specialized loader 
> is needed for your platform. I would like to a see a solution that will work 
> for any process plug-in and not just ProcessGDBRemote. If we change solution 
> 3 above to say "Make lldb_private::Process call LoadModules() itself if no 
> dynamic loader instance is available" then solution 3 is also fine.

there is a problem: if I remove DynamicLoaderWasmDYLD what happens is that 
`DynamicLoaderStatic` is found as a valid loader for a triple like 
**"wasm32-unknown-unknown-wasm"** because the Triple::OS is 
llvm::Triple::UnknownOS (I found out the hard way when I was registering 
DynamicLoaderWasmDYLD after DynamicLoaderStatic :-)).
There is an explicit check for UnknownOS:

  DynamicLoader *DynamicLoaderStatic::CreateInstance(Process *process,
 bool force) {
bool create = force;
if (!create) {
  const llvm::Triple &triple_ref =
  process->GetTarget().GetArchitecture().GetTriple();
  const llvm::Triple::OSType os_type = triple_ref.getOS();
  if ((os_type == llvm::Triple::UnknownOS))
create = true;
}
...
  
  call stack:
  DynamicLoaderStatic::CreateInstance(lldb_private::Process * process, bool 
force) Line 29
  DynamicLoader::FindPlugin(lldb_private::Process * process, const char * 
plugin_name) Line 52
  lldb_private::process_gdb_remote::ProcessGDBRemote::GetDynamicLoader() Line 
3993
  lldb_private::Process::CompleteAttach() Line 2931
  lldb_private::Process::ConnectRemote(lldb_private::Stream * strm, 
llvm::StringRef remote_url) Line 3022

Could ProcessGDBRemote::GetDynamicLoader behave differently just when the 
architecture is wasm32, maybe? But then it is probably cleaner to add this 
plugin class, what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72751



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

[Lldb-commits] [lldb] 446e4e4 - [lldb/Reproducers] Account for char** being a nullptr

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T16:32:48-08:00
New Revision: 446e4e4cf6d1126b4dc2fe816d4bb01b184b2e64

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

LOG: [lldb/Reproducers] Account for char** being a nullptr

Added: 


Modified: 
lldb/include/lldb/Utility/ReproducerInstrumentation.h
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/ReproducerInstrumentation.h 
b/lldb/include/lldb/Utility/ReproducerInstrumentation.h
index ca92f9f2f6e6..9dbe8dc547f5 100644
--- a/lldb/include/lldb/Utility/ReproducerInstrumentation.h
+++ b/lldb/include/lldb/Utility/ReproducerInstrumentation.h
@@ -606,9 +606,14 @@ class Serializer {
   }
 
   void Serialize(const char **t) {
+size_t size = 0;
+if (!t) {
+  Serialize(size);
+  return;
+}
+
 // Compute the size of the array.
 const char *const *temp = t;
-size_t size = 0;
 while (*temp++)
   size++;
 Serialize(size);

diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index fb39fec41d43..4109f73bfef3 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -42,6 +42,8 @@ template <> const char *Deserializer::Deserialize() {
 
 template <> const char **Deserializer::Deserialize() {
   size_t size = Deserialize();
+  if (size == 0)
+return nullptr;
   const char **r =
   reinterpret_cast(calloc(size + 1, sizeof(char *)));
   for (size_t i = 0; i < size; ++i)



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

[Lldb-commits] [lldb] 91f863b - [lldb/Reproducers] Add unittest for char** (de)serializer

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T17:16:31-08:00
New Revision: 91f863be4f04337da19c26c4fbda4ce10bfc0668

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

LOG: [lldb/Reproducers] Add unittest for char** (de)serializer

Added: 


Modified: 
lldb/unittests/Utility/ReproducerInstrumentationTest.cpp

Removed: 




diff  --git a/lldb/unittests/Utility/ReproducerInstrumentationTest.cpp 
b/lldb/unittests/Utility/ReproducerInstrumentationTest.cpp
index 4baf9a79330b..e87c49f7bc00 100644
--- a/lldb/unittests/Utility/ReproducerInstrumentationTest.cpp
+++ b/lldb/unittests/Utility/ReproducerInstrumentationTest.cpp
@@ -393,6 +393,57 @@ TEST(SerializationRountripTest, 
SerializeDeserializeCString) {
   EXPECT_STREQ(cstr, deserializer.Deserialize());
 }
 
+TEST(SerializationRountripTest, SerializeDeserializeCStringArray) {
+  const char *foo = "foo";
+  const char *bar = "bar";
+  const char *baz = "baz";
+  const char *arr[4] = {foo, bar, baz, nullptr};
+
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(static_cast(arr));
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  const char **deserialized = deserializer.Deserialize();
+  EXPECT_STREQ("foo", deserialized[0]);
+  EXPECT_STREQ("bar", deserialized[1]);
+  EXPECT_STREQ("baz", deserialized[2]);
+}
+
+TEST(SerializationRountripTest, SerializeDeserializeCStringArrayNullptrElem) {
+  const char *arr[1] = {nullptr};
+
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(static_cast(arr));
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  const char **deserialized = deserializer.Deserialize();
+  EXPECT_EQ(nullptr, deserialized);
+}
+
+TEST(SerializationRountripTest, SerializeDeserializeCStringArrayNullptr) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(static_cast(nullptr));
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  const char **deserialized = deserializer.Deserialize();
+  EXPECT_EQ(nullptr, deserialized);
+}
+
 TEST(SerializationRountripTest, SerializeDeserializeObjectPointer) {
   Foo foo;
   Bar bar;



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

[Lldb-commits] [lldb] e28d8f9 - [lldb] Replace SmallStr.str().str() with std::string conversion operator.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T21:34:19-08:00
New Revision: e28d8f9069b92f5c20416c575f49727daf5adb1a

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

LOG: [lldb] Replace SmallStr.str().str() with std::string conversion operator.

Use the std::string conversion operator introduced in
d7049213d0fcda691c9e79f9b41e357198d99738. The SmallString in the log
statement doesn't require conversion at all when using the variadic log
macro.

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
lldb/unittests/Expression/CppModuleConfigurationTest.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
index 26fcb81de8de..8abb7e420575 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -67,10 +67,10 @@ static bool DefaultComputeClangResourceDirectory(FileSpec 
&lldb_shlib_spec,
 llvm::sys::path::native(relative_path);
 llvm::sys::path::append(clang_dir, relative_path);
 if (!verify || VerifyClangPath(clang_dir)) {
-  LLDB_LOGF(log,
-"DefaultComputeClangResourceDir: Setting ClangResourceDir "
-"to \"%s\", verify = %s",
-clang_dir.str().str().c_str(), verify ? "true" : "false");
+  LLDB_LOG(log,
+   "DefaultComputeClangResourceDir: Setting ClangResourceDir "
+   "to \"{0}\", verify = {1}",
+   clang_dir.str(), verify ? "true" : "false");
   file_spec.GetDirectory().SetString(clang_dir);
   FileSystem::Instance().Resolve(file_spec);
   return true;

diff  --git a/lldb/unittests/Expression/CppModuleConfigurationTest.cpp 
b/lldb/unittests/Expression/CppModuleConfigurationTest.cpp
index 69d9a15e9c03..c3cc134bd5a3 100644
--- a/lldb/unittests/Expression/CppModuleConfigurationTest.cpp
+++ b/lldb/unittests/Expression/CppModuleConfigurationTest.cpp
@@ -28,7 +28,7 @@ static std::string ResourceInc() {
   llvm::SmallString<256> resource_dir;
   llvm::sys::path::append(resource_dir, GetClangResourceDir().GetPath(),
   "include");
-  return resource_dir.str().str();
+  return std::string(resource_dir);
 }
 
 /// Utility function turningn a list of paths into a FileSpecList.



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

[Lldb-commits] [lldb] 12c185a - [lldb/Reproducers] Fix reproducer instrumentation formatting (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T21:38:30-08:00
New Revision: 12c185ac5e5c396018c60565eff50187bace7011

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

LOG: [lldb/Reproducers] Fix reproducer instrumentation formatting (NFC)

Added: 


Modified: 
lldb/include/lldb/Utility/ReproducerInstrumentation.h
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/ReproducerInstrumentation.h 
b/lldb/include/lldb/Utility/ReproducerInstrumentation.h
index 9dbe8dc547f5..71b85a266592 100644
--- a/lldb/include/lldb/Utility/ReproducerInstrumentation.h
+++ b/lldb/include/lldb/Utility/ReproducerInstrumentation.h
@@ -253,7 +253,8 @@ struct NotImplementedTag {};
 
 /// Return the deserialization tag for the given type T.
 template  struct serializer_tag {
-  typedef typename std::conditional::value, 
ValueTag, NotImplementedTag>::type type;
+  typedef typename std::conditional::value,
+ValueTag, NotImplementedTag>::type type;
 };
 template  struct serializer_tag {
   typedef

diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index f8a0d661c5a5..c8a493ccd22b 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -35,8 +35,8 @@ template <> const char *Deserializer::Deserialize() {
   const char *str = m_buffer.data();
   m_buffer = m_buffer.drop_front(pos + 1);
 #ifdef LLDB_REPRO_INSTR_TRACE
-llvm::errs() << "Deserializing with " << LLVM_PRETTY_FUNCTION << " -> \""
- << str << "\"\n";
+  llvm::errs() << "Deserializing with " << LLVM_PRETTY_FUNCTION << " -> \""
+   << str << "\"\n";
 #endif
   return str;
 }



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

[Lldb-commits] [lldb] 056f01f - [lldb/Reproducers] Assert when trying to get object for invalid index.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T21:34:54-08:00
New Revision: 056f01f895615686e63b9729096f7eb98b47223f

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

LOG: [lldb/Reproducers] Assert when trying to get object for invalid index.

Assert when trying to get an object for an index we haven't seen before.
This will crash anyway, the assertion is just a bit more informative.

Added: 


Modified: 
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index 4109f73bfef3..f8a0d661c5a5 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -15,6 +15,7 @@ using namespace lldb_private;
 using namespace lldb_private::repro;
 
 void *IndexToObject::GetObjectForIndexImpl(unsigned idx) {
+  assert(m_mapping.count(idx) && "No object for index");
   return m_mapping.lookup(idx);
 }
 



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

[Lldb-commits] [lldb] 0e67212 - Revert "[lldb/Reproducers] Assert when trying to get object for invalid index."

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-01-29T22:02:00-08:00
New Revision: 0e67212416f6f27b9a6270a73cf95e71cabef524

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

LOG: Revert "[lldb/Reproducers] Assert when trying to get object for invalid 
index."

Apparently this is not doing what I thought it was doing...

Added: 


Modified: 
lldb/source/Utility/ReproducerInstrumentation.cpp

Removed: 




diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp 
b/lldb/source/Utility/ReproducerInstrumentation.cpp
index c8a493ccd22b..1835e7098c25 100644
--- a/lldb/source/Utility/ReproducerInstrumentation.cpp
+++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
@@ -15,7 +15,6 @@ using namespace lldb_private;
 using namespace lldb_private::repro;
 
 void *IndexToObject::GetObjectForIndexImpl(unsigned idx) {
-  assert(m_mapping.count(idx) && "No object for index");
   return m_mapping.lookup(idx);
 }
 



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

[Lldb-commits] [PATCH] D73539: [AVR] Recognize the AVR architecture in lldb

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

That's fine. Thanks.


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

https://reviews.llvm.org/D73539



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