[Lldb-commits] [PATCH] D59913: Get Version SWIG wrapper should fill the list it makes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The fix looks correct, but I am wondering if it wouldn't be possible to test 
this in a more platform-independent manner. Please see inline comment below.




Comment at: 
packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py:1-49
+"""
+Read in a library with a version number of 0.0.0, make sure we produce a good 
version.
+"""
+
+from __future__ import print_function
+
+

Would it be possible to generate this binary via yaml2obj (e.g. compile it 
manually, like you did above, and then yamlize it)? Then, if you avoid running 
the target and just load the module by creating the SBModule object directly 
from a ModuleSpec, you should get a test that is able to run on all platforms, 
and not just darwin.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59913



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

Re: [Lldb-commits] [lldb] r357141 - Copy the breakpoint site owner's collection so we can drop

[Pavel Labath via lldb-commits]

On 28/03/2019 02:54, Davide Italiano via lldb-commits wrote:

On Wed, Mar 27, 2019 at 6:49 PM Jim Ingham via lldb-commits
 wrote:


Author: jingham
Date: Wed Mar 27 18:51:33 2019
New Revision: 357141

URL: http://llvm.org/viewvc/llvm-project?rev=357141&view=rev
Log:
Copy the breakpoint site owner's collection so we can drop
the collection lock before we iterate over the owners calling ShouldStop.

BreakpointSite::ShouldStop can do a lot of work, and might by chance hit the 
same breakpoint
site again on another thread.  So instead of holding the site's owners lock
while iterating over them calling ShouldStop, I make a local copy of the list, 
drop the lock
and then iterate over the copy calling BreakpointLocation::ShouldStop.

It's actually quite difficult to make this cause problems because usually all 
the
action happens on the private state thread, and the lock is recursive.

I have a report where some code hit the ASAN error breakpoint, went to
compile the ASAN error gathering expression, in the course of compiling
that we went to fetch the ObjC runtime data, but the state of the program
was such that the ObjC runtime grubbing function triggered an ASAN error and
we were executing that function on another thread.

I couldn't figure out a way to reproduce that situation in a test.  But this is 
an
NFC change anyway, it just makes the locking strategy more narrowly focused.



Modified:
 lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h
 lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp
 lldb/trunk/source/Breakpoint/BreakpointSite.cpp

Modified: lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h?rev=357141&r1=357140&r2=357141&view=diff
==
--- lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h (original)
+++ lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h Wed Mar 
27 18:51:33 2019
@@ -22,6 +22,8 @@ public:
BreakpointLocationCollection();

~BreakpointLocationCollection();
+
+  BreakpointLocationCollection &operator=(const BreakpointLocationCollection 
&rhs);

//--
/// Add the breakpoint \a bp_loc_sp to the list.

Modified: lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp?rev=357141&r1=357140&r2=357141&view=diff
==
--- lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp (original)
+++ lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp Wed Mar 27 
18:51:33 2019
@@ -178,3 +178,20 @@ void BreakpointLocationCollection::GetDe
  (*pos)->GetDescription(s, level);
}
  }
+
+BreakpointLocationCollection &BreakpointLocationCollection::operator=(
+const BreakpointLocationCollection &rhs) {
+  // Same trick as in ModuleList to avoid lock inversion.
+  if (this != &rhs) {
+if (uintptr_t(this) > uintptr_t(&rhs)) {
+  std::lock_guard lhs_guard(m_collection_mutex);
+  std::lock_guard rhs_guard(rhs.m_collection_mutex);
+  m_break_loc_collection = rhs.m_break_loc_collection;
+} else {
+  std::lock_guard lhs_guard(m_collection_mutex);
+  std::lock_guard rhs_guard(rhs.m_collection_mutex);
+  m_break_loc_collection = rhs.m_break_loc_collection;
+}
+  }


The two branches are identical, is this intentional?



It probably isn't. Also, a better way to avoid lock inversion would be 
with std::lock .

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

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 192588.
teemperor marked an inline comment as done.
teemperor added a comment.

- Addressed (most of) Aleksei's comments.


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

https://reviews.llvm.org/D59485

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -304,6 +304,14 @@
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 
+public:
+  /// Allocates an ASTImporter (or one of its subclasses).
+  typedef std::function
+  ImporterConstructor;
+
+private:
   // Buffer for the To context, must live in the test scope.
   std::string ToCode;
 
@@ -316,22 +324,33 @@
 std::unique_ptr Unit;
 TranslationUnitDecl *TUDecl = nullptr;
 std::unique_ptr Importer;
-TU(StringRef Code, StringRef FileName, ArgVector Args)
+// The lambda that constructs the ASTImporter we use in this test.
+ImporterConstructor Creator;
+TU(StringRef Code, StringRef FileName, ArgVector Args,
+   ImporterConstructor C = ImporterConstructor())
 : Code(Code), FileName(FileName),
   Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
  this->FileName)),
-  TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
+  TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Creator(C) {
   Unit->enableSourceFileDiagnostics();
+
+  // If the test doesn't need a specific ASTImporter, we just create a
+  // normal ASTImporter with it.
+  if (!Creator)
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport, LookupTable);
+};
 }
 
 void lazyInitImporter(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST) {
   assert(ToAST);
-  if (!Importer) {
-Importer.reset(
-new ASTImporter(ToAST->getASTContext(), ToAST->getFileManager(),
-Unit->getASTContext(), Unit->getFileManager(),
-false, &LookupTable));
-  }
+  if (!Importer)
+Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
+   Unit->getASTContext(), Unit->getFileManager(),
+   false, &LookupTable));
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
@@ -401,11 +420,12 @@
   // Must not be called more than once within the same test.
   std::tuple
   getImportedDecl(StringRef FromSrcCode, Language FromLang, StringRef ToSrcCode,
-  Language ToLang, StringRef Identifier = DeclToImportID) {
+  Language ToLang, StringRef Identifier = DeclToImportID,
+  ImporterConstructor Creator = ImporterConstructor()) {
 ArgVector FromArgs = getArgVectorForLanguage(FromLang),
   ToArgs = getArgVectorForLanguage(ToLang);
 
-FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
+FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs, Creator);
 TU &FromTU = FromTUs.back();
 
 assert(!ToAST);
@@ -544,6 +564,73 @@
   EXPECT_THAT(RedeclsD1, ::testing::ContainerEq(RedeclsD2));
 }
 
+struct CustomImporter : ASTImporterOptionSpecificTestBase {};
+
+namespace {
+struct RedirectingImporter : public ASTImporter {
+  using ASTImporter::ASTImporter;
+  // ImporterConstructor that constructs this class.
+  static ASTImporterOptionSpecificTestBase::ImporterConstructor Constructor;
+
+protected:
+  llvm::Expected ImportImpl(Decl *FromD) override {
+auto *ND = dyn_cast(FromD);
+if (!ND || ND->getName() != "shouldNotBeImported")
+  return ASTImporter::ImportImpl(FromD);
+for (Decl *D : getToContext().getTranslationUnitDecl()->decls()) {
+  if (auto *ND = dyn_cast(D))
+if (ND->getName() == "realDecl")
+  return ND;
+}
+return ASTImporter::ImportImpl(FromD);
+  }
+};
+
+ASTImporterOptionSpecificTestBase::ImporterConstructor
+RedirectingImporter::Constructor =
+[](ASTContext &ToContext, FileManager &ToFileManager,
+   ASTContext &FromContext, FileManager &FromFileManager,
+   bool MinimalImport, ASTImporterLookupTable *LookupTabl) {
+  return new RedirectingImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+  

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:590
+  new RedirectingImporter(ToContext, ToFileManager, FromContext,
+  FromFileManager, MinimalImport, LookupTabl));
+};

a_sidorin wrote:
> LookupTable?
Not sure if I can follow?


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

https://reviews.llvm.org/D59485



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

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:590
+  new RedirectingImporter(ToContext, ToFileManager, FromContext,
+  FromFileManager, MinimalImport, LookupTabl));
+};

teemperor wrote:
> a_sidorin wrote:
> > LookupTable?
> Not sure if I can follow?
I think Alexey's comment relates to the parameter in 
`ASTImporterOptionSpecificTestBase::ImporterConstructor`. There is a typo in 
the name of the parameter an 'e' is missing: `ASTImporterLookupTable 
*LookupTabl`


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

https://reviews.llvm.org/D59485



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

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 192594.
teemperor added a comment.

Ah, I get it now. Fixed!


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

https://reviews.llvm.org/D59485

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -304,6 +304,14 @@
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 
+public:
+  /// Allocates an ASTImporter (or one of its subclasses).
+  typedef std::function
+  ImporterConstructor;
+
+private:
   // Buffer for the To context, must live in the test scope.
   std::string ToCode;
 
@@ -316,22 +324,33 @@
 std::unique_ptr Unit;
 TranslationUnitDecl *TUDecl = nullptr;
 std::unique_ptr Importer;
-TU(StringRef Code, StringRef FileName, ArgVector Args)
+// The lambda that constructs the ASTImporter we use in this test.
+ImporterConstructor Creator;
+TU(StringRef Code, StringRef FileName, ArgVector Args,
+   ImporterConstructor C = ImporterConstructor())
 : Code(Code), FileName(FileName),
   Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
  this->FileName)),
-  TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
+  TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Creator(C) {
   Unit->enableSourceFileDiagnostics();
+
+  // If the test doesn't need a specific ASTImporter, we just create a
+  // normal ASTImporter with it.
+  if (!Creator)
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport, LookupTable);
+};
 }
 
 void lazyInitImporter(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST) {
   assert(ToAST);
-  if (!Importer) {
-Importer.reset(
-new ASTImporter(ToAST->getASTContext(), ToAST->getFileManager(),
-Unit->getASTContext(), Unit->getFileManager(),
-false, &LookupTable));
-  }
+  if (!Importer)
+Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
+   Unit->getASTContext(), Unit->getFileManager(),
+   false, &LookupTable));
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
@@ -401,11 +420,12 @@
   // Must not be called more than once within the same test.
   std::tuple
   getImportedDecl(StringRef FromSrcCode, Language FromLang, StringRef ToSrcCode,
-  Language ToLang, StringRef Identifier = DeclToImportID) {
+  Language ToLang, StringRef Identifier = DeclToImportID,
+  ImporterConstructor Creator = ImporterConstructor()) {
 ArgVector FromArgs = getArgVectorForLanguage(FromLang),
   ToArgs = getArgVectorForLanguage(ToLang);
 
-FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
+FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs, Creator);
 TU &FromTU = FromTUs.back();
 
 assert(!ToAST);
@@ -544,6 +564,73 @@
   EXPECT_THAT(RedeclsD1, ::testing::ContainerEq(RedeclsD2));
 }
 
+struct CustomImporter : ASTImporterOptionSpecificTestBase {};
+
+namespace {
+struct RedirectingImporter : public ASTImporter {
+  using ASTImporter::ASTImporter;
+  // ImporterConstructor that constructs this class.
+  static ASTImporterOptionSpecificTestBase::ImporterConstructor Constructor;
+
+protected:
+  llvm::Expected ImportImpl(Decl *FromD) override {
+auto *ND = dyn_cast(FromD);
+if (!ND || ND->getName() != "shouldNotBeImported")
+  return ASTImporter::ImportImpl(FromD);
+for (Decl *D : getToContext().getTranslationUnitDecl()->decls()) {
+  if (auto *ND = dyn_cast(D))
+if (ND->getName() == "realDecl")
+  return ND;
+}
+return ASTImporter::ImportImpl(FromD);
+  }
+};
+
+ASTImporterOptionSpecificTestBase::ImporterConstructor
+RedirectingImporter::Constructor =
+[](ASTContext &ToContext, FileManager &ToFileManager,
+   ASTContext &FromContext, FileManager &FromFileManager,
+   bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+  return new RedirectingImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ LookupTable);
+};
+} // names

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for writing this up. I think there is a lot of confusion around this 
topic, and a document like this is a good step towards clearing it up.

Not really related to this patch, but since we're talking about lldb_assert... 
The thing that has always bugged me about this function is that it is really 
hard to use the way it's supposed to be used. Usually, you have to repeat the 
condition you are asserting twice. For instance, you often have to write 
something like this

  lldb_assert(thing_that_should_not_happen);
  // Now I cannot just assume assertion is true, because that may crash. So I 
still need to handle the situation somehow.
  if(!thing_that_should_not_happen)
return default_value_or_error_or_something;

At that point I just give up, and don't write the lldb_assert line at all.
Maybe if lldb_assert returned a value (possibly even marked with 
warn_unused_result), then one could "inline" the assert into the if statement, 
and things would look less awkward (?)

In D59911#1445392 , @JDevlieghere 
wrote:

> (https://lldb.llvm.org/new-www/)


Hmm.. cool. I didn't know about this. Is there a timeline for moving this to 
the main website (and getting rid of the html docs)?




Comment at: lldb/docs/resources/source.rst:73-78
+* Invalid input. To deal with invalid input, such as malformed DWARF,
+  missing object files, or otherwise inconsistent debug info, LLVM's
+  error handling types such as `llvm::Expected
+  `_ should be
+  used. Functions that may fail should return an `llvm::Optional
+  `_ result.

This looks like it tries to specify when `Expected` and when `Optional` 
should be used, but its not really clear to me what the distinction is.

The way I see it, both "Expected" and "Optional" can be used for "invalid 
input" and "functions that may fail", and the difference is whether there is a 
need to attach a error message or error code to explain why the failure 
happened.



Comment at: lldb/docs/resources/source.rst:81-82
+* Soft assertions. LLDB provides lldb_assert() as a soft alternative
+  to cover the middle ground of situations that really indicate a bug
+  in LLDB, but could conceivably happen. In a Debug configuration
+  lldb_assert() behaves like assert(). In a Release configuration it

I am not sure this really explains the difference between lldb_assert and 
assert. A "failed internal consistency check" is definitely a "bug in lldb", 
and it can certainly "happen". Perhaps an example would help make things 
clearer?
I tried making one up, but I realized I couldn't (maybe that's the intention 
since you say they should be used sparingly?). The one place where I'd consider 
using lldb_assert is for things that are definitely a "bug", but they are not 
an "lldb bug", such as a compiler emitting DWARF that is obviously broken. 
However, this document makes it pretty clear this falls into the "invalid 
input" case.



Comment at: lldb/docs/resources/source.rst:88
+  feasible.
+
+Overall, please keep in mind that the debugger is often used as a last

I am wondering whether we should also mention logging here. What I often do 
when there is no reasonable way to bubble an error up the stack or display it 
to the user is that I write it to the log. In that sense it is a form of error 
handling. WDYT?


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.
Herald added a project: LLDB.

Anyone?
We still have this patch applied on our recently-rebased fork with no 
problems...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Or better yet, create a structure that everyone must use and have the locking 
exist in the class itself:

  struct ProcessLocker {
std::lock_guard guard;
Process::StopLocker stop_locker;
  
void ProcessLocker(Process &process) {
   ...
}
  };




Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

@clayborg, I'm not sure how that would work. There's many places that lock the 
process run lock without locking the target API mutex, and vice versa.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D59847: Regression test to ensure that we handling importing of std::vector of enums correctly

[Shafik Yaghmour via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357188: Regression test to ensure that we handling importing 
of std::vector of enums… (authored by shafik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59847?vs=192359&id=192675#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59847

Files:
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
  
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp


Index: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
@@ -0,0 +1,28 @@
+"""
+Test Expression Parser regression test to ensure that we handle enums
+correctly, in this case specifically std::vector of enums.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestVectorOfEnums(TestBase):
+
+  mydir = TestBase.compute_mydir(__file__)
+
+  def test_vector_of_enums(self):
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))
+
+self.expect("expr v", substrs=[
+ 'size=3',
+ '[0] = a',
+ '[1] = b',
+ '[2] = c',
+ '}'
+])
Index: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum E {
+a,
+b,
+c,
+d
+} ;
+
+int main() {
+  std::vector v = {E::a, E::b, E::c};
+
+  return v.size(); // break here
+}
Index: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
@@ -0,0 +1,5 @@
+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules


Index: lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
+++ lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
@@ -0,0 +1,28 @@
+"""
+Test Expression Parser regression test to ensure that we handle enums
+correctly, in this case specifically std::vector of enums.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestVectorOfEnums(TestBase):
+
+  mydir = TestBase.compute_mydir(__file__)
+
+  def test_vector_of_enums(self):
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))
+
+self.expect("expr v", substrs=[
+ 'size=3',
+ '[0] = a',
+ '[1] = b',
+ '[2] = c',
+ '}'
+])
Index: lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
===
--- lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
+++ lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
@@ -0,0 +1,14 @@
+#include 
+
+enum E {
+a,
+b,
+c,
+d
+} ;
+
+int main() {
+  std::vector v = {E::a, E::b, E::c};
+
+  return v.size(); // break here
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
===
--- lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
+++ lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
@@ -0,0 +1,5 @@
+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r357188 - Regression test to ensure that we handling importing of std::vector of enums correctly

[Shafik Yaghmour via lldb-commits]

Author: shafik
Date: Thu Mar 28 10:22:13 2019
New Revision: 357188

URL: http://llvm.org/viewvc/llvm-project?rev=357188&view=rev
Log:
Regression test to ensure that we handling importing of std::vector of enums 
correctly

Summary:
https://reviews.llvm.org/D59845 added a fix for the IsStructuralMatch(...) 
specialization for EnumDecl this test should pass once this fix is committed.

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

Added:

lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/

lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile

lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py

lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp

Added: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile?rev=357188&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/Makefile
 Thu Mar 28 10:22:13 2019
@@ -0,0 +1,5 @@
+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

Added: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py?rev=357188&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
 Thu Mar 28 10:22:13 2019
@@ -0,0 +1,28 @@
+"""
+Test Expression Parser regression test to ensure that we handle enums
+correctly, in this case specifically std::vector of enums.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestVectorOfEnums(TestBase):
+
+  mydir = TestBase.compute_mydir(__file__)
+
+  def test_vector_of_enums(self):
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("main.cpp", False))
+
+self.expect("expr v", substrs=[
+ 'size=3',
+ '[0] = a',
+ '[1] = b',
+ '[2] = c',
+ '}'
+])

Added: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp?rev=357188&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/main.cpp
 Thu Mar 28 10:22:13 2019
@@ -0,0 +1,14 @@
+#include 
+
+enum E {
+a,
+b,
+c,
+d
+} ;
+
+int main() {
+  std::vector v = {E::a, E::b, E::c};
+
+  return v.size(); // break here
+}


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

[Lldb-commits] [PATCH] D59913: Get Version SWIG wrapper should fill the list it makes

[Jim Ingham via Phabricator via lldb-commits]

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

Use yaml2obj to create the dylib with version 0.0.0


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59913

Files:
  
packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
  packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
  scripts/Python/python-typemaps.swig

Index: scripts/Python/python-typemaps.swig
===
--- scripts/Python/python-typemaps.swig
+++ scripts/Python/python-typemaps.swig
@@ -333,18 +333,13 @@
 PyObject* list = PyList_New(count);
 for (uint32_t j = 0; j < count; j++)
 {
-if ($1[j] < UINT32_MAX)
+PyObject* item = PyInt_FromLong($1[j]);
+int ok = PyList_SetItem(list,j,item);
+if (ok != 0)
 {
-PyObject* item = PyInt_FromLong($1[j]);
-int ok = PyList_SetItem(list,j,item);
-if (ok != 0)
-{
-$result = Py_None;
-break;
-}
-}
-else
+$result = Py_None;
 break;
+}
 }
 $result = list;
 }
Index: packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
===
--- packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
+++ packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
@@ -0,0 +1,220 @@
+--- !mach-o
+FileHeader:  
+  magic:   0xFEEDFACF
+  cputype: 0x0107
+  cpusubtype:  0x0003
+  filetype:0x0006
+  ncmds:   12
+  sizeofcmds:  672
+  flags:   0x00100085
+  reserved:0x
+LoadCommands:
+  - cmd: LC_SEGMENT_64
+cmdsize: 232
+segname: __TEXT
+vmaddr:  0
+vmsize:  4096
+fileoff: 0
+filesize:4096
+maxprot: 5
+initprot:5
+nsects:  2
+flags:   0
+Sections:
+  - sectname:__text
+segname: __TEXT
+addr:0x0FA0
+size:11
+offset:  0x0FA0
+align:   4
+reloff:  0x
+nreloc:  0
+flags:   0x8400
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+  - sectname:__unwind_info
+segname: __TEXT
+addr:0x0FAC
+size:72
+offset:  0x0FAC
+align:   2
+reloff:  0x
+nreloc:  0
+flags:   0x
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+  - cmd: LC_SEGMENT_64
+cmdsize: 72
+segname: __LINKEDIT
+vmaddr:  4096
+vmsize:  4096
+fileoff: 4096
+filesize:528
+maxprot: 1
+initprot:1
+nsects:  0
+flags:   0
+  - cmd: LC_ID_DYLIB
+cmdsize: 56
+dylib:   
+  name:24
+  timestamp:   1
+  current_version: 0
+  compatibility_version: 0
+PayloadString:   '@executable_path/libDylib.dylib'
+ZeroPadBytes:1
+  - cmd: LC_DYLD_INFO_ONLY
+cmdsize: 48
+rebase_off:  0
+rebase_size: 0
+bind_off:0
+bind_size:   0
+weak_bind_off:   0
+weak_bind_size:  0
+lazy_bind_off:   0
+lazy_bind_size:  0
+export_off:  4096
+export_size: 16
+  - cmd: LC_SYMTAB
+cmdsize: 24
+symoff:  4120
+nsyms:   10
+stroff:  4280
+strsize: 344
+  - cmd: LC_DYSYMTAB
+cmdsize: 80
+ilocalsym:   0
+nlocalsym:   8
+iextdefsym:  8
+nextdefsym:  1
+iundefsym:   9
+nundefsym:   1
+tocoff:  0
+ntoc:0
+modtaboff:   0
+nmodtab: 0
+extrefsymoff:0
+nextrefsyms: 0
+indirectsymoff:  0
+nindirectsyms:   0
+extreloff:   0
+nextrel: 0
+locreloff:   0
+nlocrel: 0
+  - cmd: LC_UUID
+cmdsize: 24
+uuid:5F76D8E3-7EA5-3092-8A9D-0D0E36429550
+  - cmd: LC_BUILD_VERSION
+cmdsize: 32
+platform:1
+minos:   659200
+sdk: 659200
+ntools:  1
+Tools:   
+  - tool:3
+version: 33227776
+  - cmd: LC_SOURCE_VERSION
+cmdsize: 16
+version: 

[Lldb-commits] [lldb] r357198 - [NFC] find_first_of/find_last_of -> find/rfind for single char.

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Thu Mar 28 11:10:14 2019
New Revision: 357198

URL: http://llvm.org/viewvc/llvm-project?rev=357198&view=rev
Log:
[NFC] find_first_of/find_last_of -> find/rfind for single char.

For a single char argument, find_first_of is equal to find and
find_last_of is equal to rfind. While playing around with the plugin
stuff this caused an export failure because it always got inlined except
once, which resulted in an undefined symbol.

Modified:
lldb/trunk/source/Interpreter/CommandInterpreter.cpp
lldb/trunk/source/Interpreter/OptionValueProperties.cpp
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
lldb/trunk/source/Target/CPPLanguageRuntime.cpp

Modified: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Interpreter/CommandInterpreter.cpp?rev=357198&r1=357197&r2=357198&view=diff
==
--- lldb/trunk/source/Interpreter/CommandInterpreter.cpp (original)
+++ lldb/trunk/source/Interpreter/CommandInterpreter.cpp Thu Mar 28 11:10:14 
2019
@@ -2589,7 +2589,7 @@ void CommandInterpreter::OutputHelpText(
   uint32_t chars_left = max_columns;
 
   auto nextWordLength = [](llvm::StringRef S) {
-size_t pos = S.find_first_of(' ');
+size_t pos = S.find(' ');
 return pos == llvm::StringRef::npos ? S.size() : pos;
   };
 

Modified: lldb/trunk/source/Interpreter/OptionValueProperties.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Interpreter/OptionValueProperties.cpp?rev=357198&r1=357197&r2=357198&view=diff
==
--- lldb/trunk/source/Interpreter/OptionValueProperties.cpp (original)
+++ lldb/trunk/source/Interpreter/OptionValueProperties.cpp Thu Mar 28 11:10:14 
2019
@@ -159,7 +159,7 @@ OptionValueProperties::GetSubValue(const
 // args if executable basename is "test" and arch is "x86_64"
 if (sub_name[1]) {
   llvm::StringRef predicate_start = sub_name.drop_front();
-  size_t pos = predicate_start.find_first_of('}');
+  size_t pos = predicate_start.find('}');
   if (pos != llvm::StringRef::npos) {
 auto predicate = predicate_start.take_front(pos);
 auto rest = predicate_start.drop_front(pos);
@@ -204,7 +204,6 @@ Status OptionValueProperties::SetSubValu
 if (Properties::IsSettingExperimental(part))
   name_contains_experimental = true;
 
-  
   lldb::OptionValueSP value_sp(GetSubValue(exe_ctx, name, will_modify, error));
   if (value_sp)
 error = value_sp->SetValueFromString(value, op);

Modified: 
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp?rev=357198&r1=357197&r2=357198&view=diff
==
--- lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp 
(original)
+++ lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp 
Thu Mar 28 11:10:14 2019
@@ -109,7 +109,7 @@ PythonString PythonObject::Str() const {
 PythonObject
 PythonObject::ResolveNameWithDictionary(llvm::StringRef name,
 const PythonDictionary &dict) {
-  size_t dot_pos = name.find_first_of('.');
+  size_t dot_pos = name.find('.');
   llvm::StringRef piece = name.substr(0, dot_pos);
   PythonObject result = dict.GetItemForKey(PythonString(piece));
   if (dot_pos == llvm::StringRef::npos) {
@@ -133,7 +133,7 @@ PythonObject PythonObject::ResolveName(l
   // refers to the `sys` module, and `name` == "path.append", then it will find
   // the function `sys.path.append`.
 
-  size_t dot_pos = name.find_first_of('.');
+  size_t dot_pos = name.find('.');
   if (dot_pos == llvm::StringRef::npos) {
 // No dots in the name, we should be able to find the value immediately as
 // an attribute of `m_py_obj`.

Modified: lldb/trunk/source/Target/CPPLanguageRuntime.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/CPPLanguageRuntime.cpp?rev=357198&r1=357197&r2=357198&view=diff
==
--- lldb/trunk/source/Target/CPPLanguageRuntime.cpp (original)
+++ lldb/trunk/source/Target/CPPLanguageRuntime.cpp Thu Mar 28 11:10:14 2019
@@ -170,7 +170,7 @@ CPPLanguageRuntime::FindLibCppStdFunctio
   //
   // This covers the case of the lambda known at compile time.
   size_t first_open_angle_bracket = vtable_name.find('<') + 1;
-  size_t first_comma = vtable_name.find_first_of(',');
+  size_t first_comma = vtable_name.find(',');
 
   llvm::StringRef first_template_parameter =
   vtable_name.slice(first_open_angle_bracket, first_comma);


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

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D53412#1446252 , @cameron314 wrote:

> @clayborg, I'm not sure how that would work. There's many places that lock 
> the process run lock without locking the target API mutex, and vice versa.


Add an argument to the ProcessLocker constructor maybe?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

[Lldb-commits] [PATCH] D59847: Regression test to ensure that we handling importing of std::vector of enums correctly

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

This is causing failures on the windows bot. Please fix it or revert it.

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/3066


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59847



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

[Lldb-commits] [lldb] r357207 - Fix the swig typemap for "uint32_t *versions, uint32_t num_versions".

[Jim Ingham via lldb-commits]

Author: jingham
Date: Thu Mar 28 12:25:54 2019
New Revision: 357207

URL: http://llvm.org/viewvc/llvm-project?rev=357207&view=rev
Log:
Fix the swig typemap for "uint32_t *versions, uint32_t num_versions".

It was making a list of a certain size but not always filling in that
many elements, which would lead to a crash iterating over the list.

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

Added:
lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/

lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py

lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
Modified:
lldb/trunk/scripts/Python/python-typemaps.swig

Added: 
lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py?rev=357207&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
 Thu Mar 28 12:25:54 2019
@@ -0,0 +1,47 @@
+"""
+Read in a library with a version number of 0.0.0, make sure we produce a good 
version.
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+from lldbsuite.test import decorators
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestGetVersionForZero(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# If your test case doesn't stress debug info, the
+# set this to true.  That way it won't be run once for
+# each debug info format.
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_get_version_zero(self):
+"""Read in a library with a version of 0.0.0.  Test 
SBModule::GetVersion"""
+self.yaml2obj("libDylib.dylib.yaml", 
self.getBuildArtifact("libDylib.dylib"))
+self.do_test()
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def do_test(self):
+lib_name = "libDylib.dylib"
+target = lldbutil.run_to_breakpoint_make_target(self, 
exe_name=lib_name)
+module = target.FindModule(lldb.SBFileSpec(lib_name))
+self.assertTrue(module.IsValid(), "Didn't find the libDylib.dylib 
module")
+# For now the actual version numbers are wrong for a library of 0.0.0
+# but the previous code would crash iterating over the resultant
+# list.  So we are testing that that doesn't happen.
+did_iterate = False
+for elem in module.GetVersion():
+did_iterate = True
+self.assertTrue(did_iterate, "Didn't get into the GetVersion loop")
+

Added: 
lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml?rev=357207&view=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
 Thu Mar 28 12:25:54 2019
@@ -0,0 +1,220 @@
+--- !mach-o
+FileHeader:  
+  magic:   0xFEEDFACF
+  cputype: 0x0107
+  cpusubtype:  0x0003
+  filetype:0x0006
+  ncmds:   12
+  sizeofcmds:  672
+  flags:   0x00100085
+  reserved:0x
+LoadCommands:
+  - cmd: LC_SEGMENT_64
+cmdsize: 232
+segname: __TEXT
+vmaddr:  0
+vmsize:  4096
+fileoff: 0
+filesize:4096
+maxprot: 5
+initprot:5
+nsects:  2
+flags:   0
+Sections:
+  - sectname:__text
+segname: __TEXT
+addr:0x0FA0
+size:11
+offset:  0x0FA0
+align:   4
+reloff:  0x
+nreloc:  0
+flags:   0x8400
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+  - sectname:__unwind_info
+segname: __TEXT
+addr:0x0FAC
+size:72
+offset:  0x0FAC
+align:   2
+reloff:  0x
+nreloc:  0
+flags:   0x
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+  - cmd: LC_SEGMENT_64
+cmdsize: 72
+segname: __LINKEDIT
+vmaddr:  

[Lldb-commits] [PATCH] D59913: Get Version SWIG wrapper should fill the list it makes

[Jim Ingham via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB357207: Fix the swig typemap for "uint32_t 
*versions, uint32_t num_versions". (authored by jingham, committed by ).

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59913

Files:
  
packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
  packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
  scripts/Python/python-typemaps.swig

Index: packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
===
--- packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
+++ packages/Python/lldbsuite/test/macosx/version_zero/TestGetVersionZeroVersion.py
@@ -0,0 +1,47 @@
+"""
+Read in a library with a version number of 0.0.0, make sure we produce a good version.
+"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import re
+import lldb
+from lldbsuite.test import decorators
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestGetVersionForZero(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# If your test case doesn't stress debug info, the
+# set this to true.  That way it won't be run once for
+# each debug info format.
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_get_version_zero(self):
+"""Read in a library with a version of 0.0.0.  Test SBModule::GetVersion"""
+self.yaml2obj("libDylib.dylib.yaml", self.getBuildArtifact("libDylib.dylib"))
+self.do_test()
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def do_test(self):
+lib_name = "libDylib.dylib"
+target = lldbutil.run_to_breakpoint_make_target(self, exe_name=lib_name)
+module = target.FindModule(lldb.SBFileSpec(lib_name))
+self.assertTrue(module.IsValid(), "Didn't find the libDylib.dylib module")
+# For now the actual version numbers are wrong for a library of 0.0.0
+# but the previous code would crash iterating over the resultant
+# list.  So we are testing that that doesn't happen.
+did_iterate = False
+for elem in module.GetVersion():
+did_iterate = True
+self.assertTrue(did_iterate, "Didn't get into the GetVersion loop")
+
Index: packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
===
--- packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
+++ packages/Python/lldbsuite/test/macosx/version_zero/libDylib.dylib.yaml
@@ -0,0 +1,220 @@
+--- !mach-o
+FileHeader:  
+  magic:   0xFEEDFACF
+  cputype: 0x0107
+  cpusubtype:  0x0003
+  filetype:0x0006
+  ncmds:   12
+  sizeofcmds:  672
+  flags:   0x00100085
+  reserved:0x
+LoadCommands:
+  - cmd: LC_SEGMENT_64
+cmdsize: 232
+segname: __TEXT
+vmaddr:  0
+vmsize:  4096
+fileoff: 0
+filesize:4096
+maxprot: 5
+initprot:5
+nsects:  2
+flags:   0
+Sections:
+  - sectname:__text
+segname: __TEXT
+addr:0x0FA0
+size:11
+offset:  0x0FA0
+align:   4
+reloff:  0x
+nreloc:  0
+flags:   0x8400
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+  - sectname:__unwind_info
+segname: __TEXT
+addr:0x0FAC
+size:72
+offset:  0x0FAC
+align:   2
+reloff:  0x
+nreloc:  0
+flags:   0x
+reserved1:   0x
+reserved2:   0x
+reserved3:   0x
+  - cmd: LC_SEGMENT_64
+cmdsize: 72
+segname: __LINKEDIT
+vmaddr:  4096
+vmsize:  4096
+fileoff: 4096
+filesize:528
+maxprot: 1
+initprot:1
+nsects:  0
+flags:   0
+  - cmd: LC_ID_DYLIB
+cmdsize: 56
+dylib:   
+  name:24
+  timestamp:   1
+  current_version: 0
+  compatibility_version: 0
+PayloadString:   '@executable_path/libDylib.dylib'
+ZeroPadBytes:1
+  - cmd: LC_DYLD_INFO_ONLY
+cmdsize: 48
+rebase_off:  0
+rebase_size: 

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

There's dozens of places that take the API mutex without taking the process 
mutex. Take `Kill` for example: It needs to take the API mutex, but cannot take 
the run lock since it will be taken by the private state thread. Another 
example is `HandleCommand`, which takes the API mutex but has no process that 
it could ask to lock the API mutex for it.

On the flip side, all the `SBFrame` methods lock the process run lock but not 
the API mutex. And so on.

I just really don't think this can be refactored in a useful way without 
rewriting the way all SB locks are taken, which is almost impossible to do at 
this point.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



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

Re: [Lldb-commits] [lldb] r357141 - Copy the breakpoint site owner's collection so we can drop

[Jim Ingham via lldb-commits]

Does 

https://reviews.llvm.org/D59957 

look right?  I hadn't used this before but it seems to do the right thing.

Jim


> On Mar 28, 2019, at 2:01 AM, Pavel Labath  wrote:
> 
> On 28/03/2019 02:54, Davide Italiano via lldb-commits wrote:
>> On Wed, Mar 27, 2019 at 6:49 PM Jim Ingham via lldb-commits
>>  wrote:
>>> 
>>> Author: jingham
>>> Date: Wed Mar 27 18:51:33 2019
>>> New Revision: 357141
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=357141&view=rev
>>> Log:
>>> Copy the breakpoint site owner's collection so we can drop
>>> the collection lock before we iterate over the owners calling ShouldStop.
>>> 
>>> BreakpointSite::ShouldStop can do a lot of work, and might by chance hit 
>>> the same breakpoint
>>> site again on another thread.  So instead of holding the site's owners lock
>>> while iterating over them calling ShouldStop, I make a local copy of the 
>>> list, drop the lock
>>> and then iterate over the copy calling BreakpointLocation::ShouldStop.
>>> 
>>> It's actually quite difficult to make this cause problems because usually 
>>> all the
>>> action happens on the private state thread, and the lock is recursive.
>>> 
>>> I have a report where some code hit the ASAN error breakpoint, went to
>>> compile the ASAN error gathering expression, in the course of compiling
>>> that we went to fetch the ObjC runtime data, but the state of the program
>>> was such that the ObjC runtime grubbing function triggered an ASAN error and
>>> we were executing that function on another thread.
>>> 
>>> I couldn't figure out a way to reproduce that situation in a test.  But 
>>> this is an
>>> NFC change anyway, it just makes the locking strategy more narrowly focused.
>>> 
>>> 
>>> 
>>> Modified:
>>> lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h
>>> lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp
>>> lldb/trunk/source/Breakpoint/BreakpointSite.cpp
>>> 
>>> Modified: lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h
>>> URL: 
>>> http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h?rev=357141&r1=357140&r2=357141&view=diff
>>> ==
>>> --- lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h 
>>> (original)
>>> +++ lldb/trunk/include/lldb/Breakpoint/BreakpointLocationCollection.h Wed 
>>> Mar 27 18:51:33 2019
>>> @@ -22,6 +22,8 @@ public:
>>>BreakpointLocationCollection();
>>> 
>>>~BreakpointLocationCollection();
>>> +
>>> +  BreakpointLocationCollection &operator=(const 
>>> BreakpointLocationCollection &rhs);
>>> 
>>>//--
>>>/// Add the breakpoint \a bp_loc_sp to the list.
>>> 
>>> Modified: lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp
>>> URL: 
>>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp?rev=357141&r1=357140&r2=357141&view=diff
>>> ==
>>> --- lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp (original)
>>> +++ lldb/trunk/source/Breakpoint/BreakpointLocationCollection.cpp Wed Mar 
>>> 27 18:51:33 2019
>>> @@ -178,3 +178,20 @@ void BreakpointLocationCollection::GetDe
>>>  (*pos)->GetDescription(s, level);
>>>}
>>>  }
>>> +
>>> +BreakpointLocationCollection &BreakpointLocationCollection::operator=(
>>> +const BreakpointLocationCollection &rhs) {
>>> +  // Same trick as in ModuleList to avoid lock inversion.
>>> +  if (this != &rhs) {
>>> +if (uintptr_t(this) > uintptr_t(&rhs)) {
>>> +  std::lock_guard lhs_guard(m_collection_mutex);
>>> +  std::lock_guard rhs_guard(rhs.m_collection_mutex);
>>> +  m_break_loc_collection = rhs.m_break_loc_collection;
>>> +} else {
>>> +  std::lock_guard lhs_guard(m_collection_mutex);
>>> +  std::lock_guard rhs_guard(rhs.m_collection_mutex);
>>> +  m_break_loc_collection = rhs.m_break_loc_collection;
>>> +}
>>> +  }
>> The two branches are identical, is this intentional?
> 
> It probably isn't. Also, a better way to avoid lock inversion would be with 
> std::lock .

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

[Lldb-commits] [PATCH] D59957: Convert = operators that take object mutexes to the multi-lock version of std::lock

[Jim Ingham via Phabricator via lldb-commits]

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

I made a copy-paste error copying the ModuleList trick for avoiding priority 
inversion in taking the lhs & rhs mutexes.  Pavel pointed out you can use the 
std::lock(lock1, lock2) form to do this more cleanly.  I fixed the one I got 
wrong and I think I found all the other places doing this (or just locking both 
mutexes) and replaced them with this form.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D59957

Files:
  include/lldb/Core/ModuleSpec.h
  include/lldb/Utility/StreamTee.h
  source/Breakpoint/BreakpointLocationCollection.cpp
  source/Core/ModuleList.cpp
  source/Target/SectionLoadList.cpp
  source/Target/ThreadList.cpp

Index: source/Target/ThreadList.cpp
===
--- source/Target/ThreadList.cpp
+++ source/Target/ThreadList.cpp
@@ -37,8 +37,10 @@
   if (this != &rhs) {
 // Lock both mutexes to make sure neither side changes anyone on us while
 // the assignment occurs
-std::lock_guard guard(GetMutex());
-std::lock_guard rhs_guard(rhs.GetMutex());
+std::lock(GetMutex(), rhs.GetMutex());
+std::lock_guard guard(GetMutex(), std::adopt_lock);
+std::lock_guard rhs_guard(rhs.GetMutex(), 
+std::adopt_lock);
 
 m_process = rhs.m_process;
 m_stop_id = rhs.m_stop_id;
Index: source/Target/SectionLoadList.cpp
===
--- source/Target/SectionLoadList.cpp
+++ source/Target/SectionLoadList.cpp
@@ -27,8 +27,9 @@
 }
 
 void SectionLoadList::operator=(const SectionLoadList &rhs) {
-  std::lock_guard lhs_guard(m_mutex);
-  std::lock_guard rhs_guard(rhs.m_mutex);
+  std::lock(m_mutex, rhs.m_mutex);
+  std::lock_guard lhs_guard(m_mutex, std::adopt_lock);
+  std::lock_guard rhs_guard(rhs.m_mutex, std::adopt_lock);
   m_addr_to_sect = rhs.m_addr_to_sect;
   m_sect_to_addr = rhs.m_sect_to_addr;
 }
Index: source/Core/ModuleList.cpp
===
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -130,25 +130,12 @@
 
 const ModuleList &ModuleList::operator=(const ModuleList &rhs) {
   if (this != &rhs) {
-// That's probably me nit-picking, but in theoretical situation:
-//
-// * that two threads A B and
-// * two ModuleList's x y do opposite assignments ie.:
-//
-//  in thread A: | in thread B:
-//x = y; |   y = x;
-//
-// This establishes correct(same) lock taking order and thus avoids
-// priority inversion.
-if (uintptr_t(this) > uintptr_t(&rhs)) {
-  std::lock_guard lhs_guard(m_modules_mutex);
-  std::lock_guard rhs_guard(rhs.m_modules_mutex);
-  m_modules = rhs.m_modules;
-} else {
-  std::lock_guard lhs_guard(m_modules_mutex);
-  std::lock_guard rhs_guard(rhs.m_modules_mutex);
-  m_modules = rhs.m_modules;
-}
+std::lock(m_modules_mutex, rhs.m_modules_mutex);
+std::lock_guard lhs_guard(m_modules_mutex, 
+std::adopt_lock);
+std::lock_guard rhs_guard(rhs.m_modules_mutex, 
+std::adopt_lock);
+m_modules = rhs.m_modules;
   }
   return *this;
 }
Index: source/Breakpoint/BreakpointLocationCollection.cpp
===
--- source/Breakpoint/BreakpointLocationCollection.cpp
+++ source/Breakpoint/BreakpointLocationCollection.cpp
@@ -181,17 +181,11 @@
 
 BreakpointLocationCollection &BreakpointLocationCollection::operator=(
 const BreakpointLocationCollection &rhs) {
-  // Same trick as in ModuleList to avoid lock inversion.
   if (this != &rhs) {
-if (uintptr_t(this) > uintptr_t(&rhs)) {
-  std::lock_guard lhs_guard(m_collection_mutex);
-  std::lock_guard rhs_guard(rhs.m_collection_mutex);
+  std::lock(m_collection_mutex, rhs.m_collection_mutex);
+  std::lock_guard lhs_guard(m_collection_mutex, std::adopt_lock);
+  std::lock_guard rhs_guard(rhs.m_collection_mutex, std::adopt_lock);
   m_break_loc_collection = rhs.m_break_loc_collection;
-} else {
-  std::lock_guard lhs_guard(m_collection_mutex);
-  std::lock_guard rhs_guard(rhs.m_collection_mutex);
-  m_break_loc_collection = rhs.m_break_loc_collection;
-}
   }
   return *this;
 }
Index: include/lldb/Utility/StreamTee.h
===
--- include/lldb/Utility/StreamTee.h
+++ include/lldb/Utility/StreamTee.h
@@ -49,8 +49,11 @@
   StreamTee &operator=(const StreamTee &rhs) {
 if (this != &rhs) {
   Stream::operator=(rhs);
-  std::lock_guard lhs_locker(m_streams_mutex);
-  std::lock_guard rhs_locker(rhs.m_streams_mutex);
+  std::lock(m_streams_mutex, rhs.m_streams_mutex);
+  std::lock_guard lhs_locker(m_streams_mutex,
+  

[Lldb-commits] [PATCH] D59957: Convert = operators that take object mutexes to the multi-lock version of std::lock

[Davide Italiano via Phabricator via lldb-commits]

davide added subscribers: labath, davide.
davide added a comment.

This looks correct to me, but I'm not extremely familiar either, so I'd wait 
for @labath to sign off.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59957



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

[Lldb-commits] [PATCH] D59847: Regression test to ensure that we handling importing of std::vector of enums correctly

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

In D59847#1446571 , @stella.stamenova 
wrote:

> This is causing failures on the windows bot. Please fix it or revert it.
>
> http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/3066


I think I know why this is breaking, it relies on the formatter for 
`std::vector` I believe add the `"libc++"` test category should fix this, let 
me work on getting that fix in.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59847



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

[Lldb-commits] [lldb] r357210 - Fix for regression test, since we rely on the formatter for std::vector in the test we need a libc++ category.

[Shafik Yaghmour via lldb-commits]

Author: shafik
Date: Thu Mar 28 13:25:57 2019
New Revision: 357210

URL: http://llvm.org/viewvc/llvm-project?rev=357210&view=rev
Log:
Fix for regression test, since we rely on the formatter for std::vector in the 
test we need a libc++ category.

See differential https://reviews.llvm.org/D59847 for initial change that this 
fixes

Modified:

lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py?rev=357210&r1=357209&r2=357210&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/expression_command/vector_of_enums/TestVectorOfEnums.py
 Thu Mar 28 13:25:57 2019
@@ -13,6 +13,7 @@ class TestVectorOfEnums(TestBase):
 
   mydir = TestBase.compute_mydir(__file__)
 
+  @add_test_categories(["libc++"])
   def test_vector_of_enums(self):
 self.build()
 


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

[Lldb-commits] [PATCH] D59847: Regression test to ensure that we handling importing of std::vector of enums correctly

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

@stella.stamenova I committed a fix, please let me know if this does not 
address the regression:

http://llvm.org/viewvc/llvm-project?view=revision&revision=357210


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59847



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

[Lldb-commits] [PATCH] D59847: Regression test to ensure that we handling importing of std::vector of enums correctly

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

In D59847#1446671 , @shafik wrote:

> @stella.stamenova I committed a fix, please let me know if this does not 
> address the regression:
>
> http://llvm.org/viewvc/llvm-project?view=revision&revision=357210


@shafik Thanks! You can keep an eye to make sure the buildbot returns to green 
here: http://lab.llvm.org:8011/buildslaves/win-py3-buildbot, but I will as well.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59847



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl updated this revision to Diff 192708.
aprantl added a comment.

Address review feedback!


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

https://reviews.llvm.org/D59911

Files:
  lldb/docs/index.rst
  lldb/docs/resources/source.rst
  lldb/source/Utility/LLDBAssert.cpp
  lldb/www/source.html

Index: lldb/www/source.html
===
--- lldb/www/source.html
+++ lldb/www/source.html
@@ -62,7 +62,58 @@
 
 For anything not explicitly listed here, assume that LLDB follows the LLVM policy.
   
-
+
+
+			
+Error handling and use of assertions in LLDB
+			
+
+Contrary to clang, which is typically a short-lived process, LLDB debuggers
+stay up and running for a long time, often serving multiple debug sessions
+initiated by an IDE. For this reason LLDB code needs to be extra thoughtful
+about how to handle errors. Below are a couple rules of thumb:
+
+  Invalid input.
+To deal with invalid input, such as malformed DWARF, missing object files, or otherwise
+inconsistent debug info, LLVM's error handling types such as
+http://llvm.org/doxygen/classllvm_1_1Expected.html";>llvm::Expected or
+http://llvm.org/doxygen/classllvm_1_1Optional.html";>llvm::Optional
+should be used. Functions that may fail should return their result using these wrapper
+types instead of using a bool to indicate success. Returning a default value when an
+error occured is also discouraged.
+  
+  Assertions.
+Assertions (from assert.h) should be used liberally to assert internal consistency.
+Assertions shall never be used to detect invalid user input, such as malformed DWARF.
+An assertion should be placed to assert invariants that the developer is convinced will
+always hold, regardless what an end-user does with LLDB. Because assertions are not
+present in release builds, the checks in an assertion may be more expensive than
+otherwise permissible. In combination with the LLDB test suite,
+assertions are what allows us to refactor and evolve the LLDB code base.
+  
+  Logging.
+LLDB provides a very rich logging API. When recoverable errors cannot reasonably
+by surfaced to the end user, the error may be written to a topical log channel.
+  Soft assertions.
+LLDB provides lldb_assert() as a soft alternative to cover the middle ground
+of situations that indicate a recoverable bug in LLDB.
+In a Debug configuration lldb_assert() behaves like
+assert(). In a Release configuration it will print a warning and encourage the
+user to file a bug report, similar to LLVM's crash handler, and then return
+execution. Use these sparingly and only if error handling is not otherwise feasible.
+  
+  Fatal errors.
+Aborting LLDB's process using llvm::report_fatal_error() or abort
+should be avoided at all costs.  It's acceptable to use
+http://llvm.org/doxygen/Support_2ErrorHandling_8h.html";>llvm_unreachable()
+for actually unreachable code such as the default in an otherwise exhaustive switch statement.
+  
+
+Overall, please keep in mind that the debugger is often used as a last resort,
+and a crash in the debugger is rarely appreciated by the end-user.
+
+
+
 
 			
 		
Index: lldb/source/Utility/LLDBAssert.cpp
===
--- lldb/source/Utility/LLDBAssert.cpp
+++ lldb/source/Utility/LLDBAssert.cpp
@@ -27,5 +27,4 @@
   llvm::sys::PrintStackTrace(errs());
   errs() << "please file a bug report against lldb reporting this failure "
 "log, and as many details as possible\n";
-  abort();
 }
Index: lldb/docs/resources/source.rst
===

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

While writing this I came to the conclusion that `lldb_assert()` is really a 
lazy way of handling errors. If we can we should replace it with error handling 
and perhaps logging.


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59960: Fix for ambiguous lookup in expressions between local variable and namespace

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik created this revision.
shafik added reviewers: jingham, teemperor.
Herald added a subscriber: jdoerfert.

In an Objective-C context a local variable and namespace can cause an ambiguous 
name lookup when used in an expression. The solution involves mimicking the 
existing C++ solution which is to add local using declarations for local 
variables. This causes a different type of lookup to be used which eliminates 
the namespace during acceptable results filtering.


https://reviews.llvm.org/D59960

Files:
  
packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/Makefile
  
packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/TestNamespaceLocalVarSameName.py
  
packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/main.mm
  
packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/util.mm
  source/Expression/ExpressionSourceCode.cpp

Index: source/Expression/ExpressionSourceCode.cpp
===
--- source/Expression/ExpressionSourceCode.cpp
+++ source/Expression/ExpressionSourceCode.cpp
@@ -168,6 +168,7 @@
 
 ConstString var_name = var_sp->GetName();
 if (!var_name || var_name == ConstString("this") ||
+var_name == ConstString("self") || var_name == ConstString("_cmd") ||
 var_name == ConstString(".block_descriptor"))
   continue;
 
@@ -252,7 +253,7 @@
   }
 }
 
-if (add_locals) {
+if (add_locals)
   if (Language::LanguageIsCPlusPlus(frame->GetLanguage())) {
 if (target->GetInjectLocalVariables(&exe_ctx)) {
   lldb::VariableListSP var_list_sp =
@@ -260,7 +261,6 @@
   AddLocalVariableDecls(var_list_sp, lldb_local_var_decls);
 }
   }
-}
   }
 
   if (m_wrap) {
@@ -326,10 +326,12 @@
 "@implementation $__lldb_objc_class ($__lldb_category)   \n"
 "+(void)%s:(void *)$__lldb_arg   \n"
 "{   \n"
+"%s;\n"
 "%s"
 "}   \n"
 "@end\n",
-m_name.c_str(), m_name.c_str(), tagged_body.c_str());
+m_name.c_str(), m_name.c_str(), lldb_local_var_decls.GetData(),
+tagged_body.c_str());
   } else {
 wrap_stream.Printf(
 "@interface $__lldb_objc_class ($__lldb_category)   \n"
Index: packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/util.mm
===
--- /dev/null
+++ packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/util.mm
@@ -0,0 +1,16 @@
+#import 
+
+namespace error {
+int blah;
+}
+
+@interface Util : NSObject
++ (void)debugPrintError;
+@end
+
+@implementation Util
++ (void)debugPrintError {
+  NSError* error = [NSError errorWithDomain:NSURLErrorDomain code:-1 userInfo:nil];
+  NSLog(@"xxx, error = %@", error); // break here
+}
+@end
Index: packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/main.mm
===
--- /dev/null
+++ packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/main.mm
@@ -0,0 +1,11 @@
+#import 
+@interface Util : NSObject
++ (void)debugPrintError;
+@end
+
+int main(int argc, const char * argv[]) {
+  [Util debugPrintError];
+
+  return 0;
+}
+
Index: packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/TestNamespaceLocalVarSameName.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/TestNamespaceLocalVarSameName.py
@@ -0,0 +1,19 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestNamespaceLocalVarSameName(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipUnlessDarwin
+@add_test_categories(["gmodules"])
+def test_namespace_local_var_same_name(self):
+self.build()
+
+lldbutil.run_to_source_breakpoint(self, '// break here',
+lldb.SBFileSpec("util.mm", False))
+
+self.expect("expr error",
+substrs=['(NSError *) $0 ='])
Index: packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/Makefile
===
--- /dev/null
+++ packages/Python/lldbsuite/test/expression_command/namespace_local_var_same_name/Makefile
@@ -0,0 +1,5 @@
+LEVEL = ../../make
+OBJCXX_SOURCES := main.mm util.mm
+include $(LEVEL)/Makefile.rules
+
+LDFLAGS += -framework Foundation

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

My (maybe unpopolar) opinion on the subject is that "soft assertions" are a way 
to cleanse your conscience of guilt, but they don't work really well in 
practice.
When I started working on lldb, I was a fairly strong proponent of assertions 
everywhere. My view changed somewhat radically over the course of the past 18 
months, and I would like to summarize some points here.

1. Vendors (some) ship a release or two of debuggers every 6 months. Even if 
you just look at open source, llvm gets released every 6 months and 
distributions downstream package just the release version. Not everybody runs 
gentoo or exherbo or compiles his toolchain from svn. For these people the 
debugger has just to work. Assertions are always compiled out in release mode 
so this is not an issue. I strongly agree that for internal interfaces they 
have to be used as liberally as possible, mostly because they catch bugs in 
development. llvm libraries tend to use assertions galore, and developers put 
them in "just in case", and I think this is really not something we should do 
in lldb.

2. Errors have to be handled, properly. I don't think returning a default 
constructed object in case something goes bad is better than throwing an error. 
We now have rich/proper error handling in llvm to make sure the process blows 
up if the error is not checked. This is a good thing.

3. The above points are IMHO the only two needed ways of handling 
invariants/invalid input. Anything else in the middle is just going to cause 
confusion to new and existing developer. Adrian (or anybody else), do you have 
any real example where something wasn't either a user error or a strong 
invariant that couldn't be violated? I personally didn't, hence I don't 
understand the need for "soft" assertions here.


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59960: Fix for ambiguous lookup in expressions between local variable and namespace

[Frederic Riss via Phabricator via lldb-commits]

friss added inline comments.



Comment at: source/Expression/ExpressionSourceCode.cpp:171
 if (!var_name || var_name == ConstString("this") ||
+var_name == ConstString("self") || var_name == ConstString("_cmd") ||
 var_name == ConstString(".block_descriptor"))

This seems dangerous. Now people having a variable called 'self' or '_cmd' in 
their C++ code will not be able to evaluate them anymore. This filtering needs 
to be per-language.



Comment at: source/Expression/ExpressionSourceCode.cpp:257
+if (add_locals)
   if (Language::LanguageIsCPlusPlus(frame->GetLanguage())) {
 if (target->GetInjectLocalVariables(&exe_ctx)) {

How does this work? The locals seem to be added only in C++, I'm not sure how 
this patch makes any difference?



Comment at: source/Expression/ExpressionSourceCode.cpp:334
+m_name.c_str(), m_name.c_str(), lldb_local_var_decls.GetData(),
+tagged_body.c_str());
   } else {

Why only in the static_method case?


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

https://reviews.llvm.org/D59960



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Thanks for summarizing your thoughts, Davide.

I think that what you wrote is a much better explanation of what I was trying 
to say with

  Use these sparingly and only if error handling is not otherwise feasible.

I think that unless we can eliminate all uses of lldb_assert() from the code we 
should document it, but discourage its use. Do you have any suggestions for a 
better wording?


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In D59911#1446787 , @aprantl wrote:

> Thanks for summarizing your thoughts, Davide.
>
> I think that what you wrote is a much better explanation of what I was trying 
> to say with
>
>   Use these sparingly and only if error handling is not otherwise feasible.
>
>
> I think that unless we can eliminate all uses of lldb_assert() from the code 
> we should document it, but discourage its use. Do you have any suggestions 
> for a better wording?


I do personally believe that your wording is reasonable. If it was me, I would 
just be a little stronger and say that new code should never use lldbassert, 
and if you're touching existing code you might consider replacing it with 
proper error handling.
I would also like to thank you personally for clarifying this whole situation. 
I am also responsible for using carelessly lldbassert in some places of the 
codebase, but your doc was good to make me think about it (and why to not use), 
and I guess it would be very useful for others as well [including new 
contributors].


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

Thank you for taking the time to write this up! I wish I could have read this 
when I started working on LLDB. :)


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl updated this revision to Diff 192730.

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

https://reviews.llvm.org/D59911

Files:
  lldb/docs/index.rst
  lldb/docs/resources/source.rst
  lldb/source/Utility/LLDBAssert.cpp
  lldb/www/source.html

Index: lldb/www/source.html
===
--- lldb/www/source.html
+++ lldb/www/source.html
@@ -62,7 +62,61 @@
 
 For anything not explicitly listed here, assume that LLDB follows the LLVM policy.
   
-
+
+
+			
+Error handling and use of assertions in LLDB
+			
+
+Contrary to clang, which is typically a short-lived process, LLDB debuggers
+stay up and running for a long time, often serving multiple debug sessions
+initiated by an IDE. For this reason LLDB code needs to be extra thoughtful
+about how to handle errors. Below are a couple rules of thumb:
+
+  Invalid input.
+To deal with invalid input, such as malformed DWARF, missing object files, or otherwise
+inconsistent debug info, LLVM's error handling types such as
+http://llvm.org/doxygen/classllvm_1_1Expected.html";>llvm::Expected or
+http://llvm.org/doxygen/classllvm_1_1Optional.html";>llvm::Optional
+should be used. Functions that may fail should return their result using these wrapper
+types instead of using a bool to indicate success. Returning a default value when an
+error occured is also discouraged.
+  
+  Assertions.
+Assertions (from assert.h) should be used liberally to assert internal consistency.
+Assertions shall never be used to detect invalid user input, such as malformed DWARF.
+An assertion should be placed to assert invariants that the developer is convinced will
+always hold, regardless what an end-user does with LLDB. Because assertions are not
+present in release builds, the checks in an assertion may be more expensive than
+otherwise permissible. In combination with the LLDB test suite,
+assertions are what allows us to refactor and evolve the LLDB code base.
+  
+  Logging.
+LLDB provides a very rich logging API. When recoverable errors cannot reasonably
+by surfaced to the end user, the error may be written to a topical log channel.
+  Soft assertions.
+LLDB provides lldb_assert() as a soft alternative to cover the middle ground
+of situations that indicate a recoverable bug in LLDB.
+In a Debug configuration lldb_assert() behaves like
+assert(). In a Release configuration it will print a warning and encourage the
+user to file a bug report, similar to LLVM's crash handler, and then return
+execution. Use these sparingly and only if error handling is not otherwise feasible.
+
+Specifically, new code should not be using lldb_assert() and existing uses
+should be replaced by other means of error handling.
+  
+  Fatal errors.
+Aborting LLDB's process using llvm::report_fatal_error() or abort
+should be avoided at all costs.  It's acceptable to use
+http://llvm.org/doxygen/Support_2ErrorHandling_8h.html";>llvm_unreachable()
+for actually unreachable code such as the default in an otherwise exhaustive switch statement.
+  
+
+Overall, please keep in mind that the debugger is often used as a last resort,
+and a crash in the debugger is rarely appreciated by the end-user.
+
+
+
 
 			
 		
Index: lldb/source/Utility/LLDBAssert.cpp
===
--- lldb/source/Utility/LLDBAssert.cpp
+++ lldb/source/Utility/LLDBAssert.cpp
@@ -27,5 +27,4 @@
   llvm::sys::PrintStackTrace(errs());
   errs() << "please file a bug report against lldb reporting this f

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Davide Italiano via Phabricator via lldb-commits]

davide accepted this revision.
davide added a comment.

LGTM


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

In D59911#1446835 , @davide wrote:

>




> I do personally believe that your wording is reasonable. If it was me, I 
> would just be a little stronger and say that new code should never use 
> lldbassert, and if you're touching existing code you might consider replacing 
> it with proper error handling.

Done!


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Works for me.

In D59911#1445917 , @labath wrote:

> In D59911#1445392 , @JDevlieghere 
> wrote:
>
> > (https://lldb.llvm.org/new-www/)
>
>
> Hmm.. cool. I didn't know about this. Is there a timeline for moving this to 
> the main website (and getting rid of the html docs)?


Tanya has to manually modify the scripts that generate this. I've sent her an 
e-mail asking if anything is blocking the transition.


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D59911#1446745 , @davide wrote:

> My (maybe unpopolar) opinion on the subject is that "soft assertions" are a 
> way to cleanse your conscience of guilt, but they don't work really well in 
> practice.
>  When I started working on lldb, I was a fairly strong proponent of 
> assertions everywhere. My view changed somewhat radically over the course of 
> the past 18 months, and I would like to summarize some points here.
>
> 1. Vendors (some) ship a release or two of debuggers every 6 months. Even if 
> you just look at open source, llvm gets released every 6 months and 
> distributions downstream package just the release version. Not everybody runs 
> gentoo or exherbo or compiles his toolchain from svn. For these people the 
> debugger has just to work. Assertions are always compiled out in release mode 
> so this is not an issue. I strongly agree that for internal interfaces they 
> have to be used as liberally as possible, mostly because they catch bugs in 
> development. llvm libraries tend to use assertions galore, and developers put 
> them in "just in case", and I think this is really not something we should do 
> in lldb.
> 2. Errors have to be handled, properly. I don't think returning a default 
> constructed object in case something goes bad is better than throwing an 
> error. We now have rich/proper error handling in llvm to make sure the 
> process blows up if the error is not checked. This is a good thing.
> 3. The above points are IMHO the only two needed ways of handling 
> invariants/invalid input. Anything else in the middle is just going to cause 
> confusion to new and existing developer. Adrian (or anybody else), do you 
> have any real example where something wasn't either a user error or a strong 
> invariant that couldn't be violated? I personally didn't, hence I don't 
> understand the need for "soft" assertions here.


Here is a concrete example of the sort of thing I thought lldbassert was for.   
In DWARFExpression::AddressRangeForLocationListEntry if we come across a 
LocationList format we don't understand, we do:

  default:
// Not supported entry type
lldbassert(false && "Not supported location list type");
return false;

We should be running the testsuite against the in development compiler, so if 
the compiler adds a new location list format but somebody forgets to tell us, 
if we are lucky it will get emitted in some test's code, and that test will die 
with this assertion.  Then we will know we have to go add that format.  Since 
we can run the testsuite against other compilers we can widen the lookout by 
doing that for any compilers we care about.

If somebody runs lldb against the output of a new compiler in the field and 
that has a location list format we don't support, then we can recover, so 
unreachable is not appropriate.  And we do return false when we don't 
understand the format, so everybody upstream will recover.  In fact, in my 
understanding lldbassert explicitly says "you can assert in testing but you 
have to recover from this condition".  So that seems alright.

However, in this case it might be good to print a message saying "you have some 
debug info I don't understand" at the point where we didn't understand it, as 
being easier to debug than a downstream message.  Logging is another way to do 
this, but that involves a couple of round trips with whoever encounters the 
problem, since the user would see some downstream error (I couldn't print the 
value of this variable) then we have to figure out that it is a DWARF problem 
and not some other variable reconstruction problem and get them to turn on that 
log...

In this case it might be better to print an error at the problem site.  You 
could certainly do this with an assert and a printf.  lldbassert just provides 
a convenient way to do that.

So I would say: use lldbassert for recoverable errors that you would want to 
catch at the error site in testing, and would want to warn about (as opposed to 
logging) at the error site in the field.

However, I do agree that lldbasserts should be used sparingly.  I'm thinking of 
the gdb "complaints" which would print a message for everything that wasn't 
copasetic in debug info.  In practice you would either get no complaints, or 
you would get a new compiler that did one or two bad things MANY times, and so 
you would get a spew of complaints and you really wouldn't be able to use the 
debugger w/o turning them off altogether.

So you have to make sure that they are not used in situations where they are 
going to be noisy.


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In D59911#1446942 , @jingham wrote:

> In D59911#1446745 , @davide wrote:
>
> > My (maybe unpopolar) opinion on the subject is that "soft assertions" are a 
> > way to cleanse your conscience of guilt, but they don't work really well in 
> > practice.
> >  When I started working on lldb, I was a fairly strong proponent of 
> > assertions everywhere. My view changed somewhat radically over the course 
> > of the past 18 months, and I would like to summarize some points here.
> >
> > 1. Vendors (some) ship a release or two of debuggers every 6 months. Even 
> > if you just look at open source, llvm gets released every 6 months and 
> > distributions downstream package just the release version. Not everybody 
> > runs gentoo or exherbo or compiles his toolchain from svn. For these people 
> > the debugger has just to work. Assertions are always compiled out in 
> > release mode so this is not an issue. I strongly agree that for internal 
> > interfaces they have to be used as liberally as possible, mostly because 
> > they catch bugs in development. llvm libraries tend to use assertions 
> > galore, and developers put them in "just in case", and I think this is 
> > really not something we should do in lldb.
> > 2. Errors have to be handled, properly. I don't think returning a default 
> > constructed object in case something goes bad is better than throwing an 
> > error. We now have rich/proper error handling in llvm to make sure the 
> > process blows up if the error is not checked. This is a good thing.
> > 3. The above points are IMHO the only two needed ways of handling 
> > invariants/invalid input. Anything else in the middle is just going to 
> > cause confusion to new and existing developer. Adrian (or anybody else), do 
> > you have any real example where something wasn't either a user error or a 
> > strong invariant that couldn't be violated? I personally didn't, hence I 
> > don't understand the need for "soft" assertions here.
>
>
> Here is a concrete example of the sort of thing I thought lldbassert was for. 
>   In DWARFExpression::AddressRangeForLocationListEntry if we come across a 
> LocationList format we don't understand, we do:
>
>   default:
> // Not supported entry type
> lldbassert(false && "Not supported location list type");
> return false;


IMO, this example should be changed to

  return llvm::make_error("Unsupported location list type");

and let someone higher up handle this.


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Aleksei Sidorin via Phabricator via lldb-commits]

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

Hello Raphael,
I think we should accept this change. I don't see an easy way to merge the LLDB 
stuff into ASTImporter; also, this patch provides a good extension point for 
ASTImporter since it is designed to be a parent class. @martong @shafik Gabor, 
Shafik, what do you think?


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

https://reviews.llvm.org/D59485



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

[Lldb-commits] [PATCH] D59968: [Cmake] Unify python variables

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: zturner, stella.stamenova, labath, sgraenitz.
Herald added subscribers: lldb-commits, abidh, mgorny.
Herald added a project: LLDB.

Both FindPythonInterp and FindPythonLibs do two things, they'll set some 
variables (PYTHON_LIBRARIES, PYTHON_INCLUDE_DIRS) and update the cached 
variables (PYTHON_LIBRARY, PYTHON_INCLUDE_DIR) that are also used to specify a 
custom python installation.

I believe the canonical way to do this is to use the PYTHON_LIBRARIES and 
PYTHON_INCLUDE_DIRS variables instead of the cached ones. However, since the 
cached variables are accessible from the cache and GUI, this is a lot less 
confusing when you're trying to debug why a variable did or didn't get the 
value you expected. Furthermore, as far as I can tell,  the implementation uses 
the cached variables to set their LIBRARIES/DIRS counterparts. This is also the 
reason this works today even though we mix-and-match.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D59968

Files:
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/scripts/CMakeLists.txt
  lldb/scripts/Python/modules/readline/CMakeLists.txt
  lldb/source/Utility/CMakeLists.txt

Index: lldb/source/Utility/CMakeLists.txt
===
--- lldb/source/Utility/CMakeLists.txt
+++ lldb/source/Utility/CMakeLists.txt
@@ -27,7 +27,7 @@
 endif()
 
 if (NOT LLDB_DISABLE_PYTHON AND NOT LLVM_BUILD_STATIC)
-  list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARIES})
+  list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARY})
 endif()
 
 list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
Index: lldb/scripts/Python/modules/readline/CMakeLists.txt
===
--- lldb/scripts/Python/modules/readline/CMakeLists.txt
+++ lldb/scripts/Python/modules/readline/CMakeLists.txt
@@ -1,6 +1,5 @@
 # FIXME: if a non-standard version of python is requested, the cmake macro
 # below will need Python_ADDITIONAL_VERSIONS set in order to find it.
-include(FindPythonInterp)
 SET(PYTHON_DIRECTORY python${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}/site-packages)
 
 # Build the readline python module
Index: lldb/scripts/CMakeLists.txt
===
--- lldb/scripts/CMakeLists.txt
+++ lldb/scripts/CMakeLists.txt
@@ -9,8 +9,6 @@
   ${LLDB_SOURCE_DIR}/include/lldb/lldb-versioning.h
 )
 
-include(FindPythonInterp)
-
 if(LLDB_BUILD_FRAMEWORK)
   set(framework_arg --framework --target-platform Darwin)
 endif()
Index: lldb/cmake/modules/LLDBConfig.cmake
===
--- lldb/cmake/modules/LLDBConfig.cmake
+++ lldb/cmake/modules/LLDBConfig.cmake
@@ -135,10 +135,10 @@
 return()
   endif()
 
-  file(TO_CMAKE_PATH "${PYTHON_HOME}/Include" PYTHON_INCLUDE_DIRS)
+  file(TO_CMAKE_PATH "${PYTHON_HOME}/Include" PYTHON_INCLUDE_DIR)
 
-  if(EXISTS "${PYTHON_INCLUDE_DIRS}/patchlevel.h")
-file(STRINGS "${PYTHON_INCLUDE_DIRS}/patchlevel.h" python_version_str
+  if(EXISTS "${PYTHON_INCLUDE_DIR}/patchlevel.h")
+file(STRINGS "${PYTHON_INCLUDE_DIR}/patchlevel.h" python_version_str
  REGEX "^#define[ \t]+PY_VERSION[ \t]+\"[^\"]+\"")
 string(REGEX REPLACE "^#define[ \t]+PY_VERSION[ \t]+\"([^\"+]+)[+]?\".*" "\\1"
  PYTHONLIBS_VERSION_STRING "${python_version_str}")
@@ -146,7 +146,7 @@
 string(REGEX REPLACE "([0-9]+)[.]([0-9]+)[.][0-9]+" "python\\1\\2" PYTHONLIBS_BASE_NAME "${PYTHONLIBS_VERSION_STRING}")
 unset(python_version_str)
   else()
-message("Unable to find ${PYTHON_INCLUDE_DIRS}/patchlevel.h, Python installation is corrupt.")
+message("Unable to find ${PYTHON_INCLUDE_DIR}/patchlevel.h, Python installation is corrupt.")
 message("Python support will be disabled for this build.")
 set(LLDB_DISABLE_PYTHON 1 PARENT_SCOPE)
 return()
@@ -225,12 +225,12 @@
   set (PYTHON_EXECUTABLE ${PYTHON_EXECUTABLE} PARENT_SCOPE)
   set (PYTHON_LIBRARY ${PYTHON_LIBRARY} PARENT_SCOPE)
   set (PYTHON_DLL ${PYTHON_DLL} PARENT_SCOPE)
-  set (PYTHON_INCLUDE_DIRS ${PYTHON_INCLUDE_DIRS} PARENT_SCOPE)
+  set (PYTHON_INCLUDE_DIR ${PYTHON_INCLUDE_DIR} PARENT_SCOPE)
 
   message("-- LLDB Found PythonExecutable: ${PYTHON_RELEASE_EXE} and ${PYTHON_DEBUG_EXE}")
   message("-- LLDB Found PythonLibs: ${PYTHON_RELEASE_LIB} and ${PYTHON_DEBUG_LIB}")
   message("-- LLDB Found PythonDLL: ${PYTHON_RELEASE_DLL} and ${PYTHON_DEBUG_DLL}")
-  message("-- LLDB Found PythonIncludeDirs: ${PYTHON_INCLUDE_DIRS}")
+  message("-- LLDB Found PythonIncludeDirs: ${PYTHON_INCLUDE_DIR}")
 endfunction(find_python_libs_windows)
 
 if (NOT LLDB_DISABLE_PYTHON)
@@ -243,17 +243,19 @@
   add_definitions( -DLLDB_PYTHON_HOME="${LLDB_PYTHON_HOME}" )
 endif()
   else()
-find_package(PythonLibs REQUIRED)
+include(FindPythonInterp)
+include(FindPythonLibs)
   endif()
-  
-  if (PYTHON_INCLUDE_DIRS)
-include_directories(${PYTHON_INCLUDE_DIRS})
+
+  if

[Lldb-commits] [PATCH] D59968: [Cmake] Unify python variables

[Stella Stamenova via Phabricator via lldb-commits]

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

LGTM


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59968



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

[Lldb-commits] [PATCH] D59970: [CMake] Untangle linking of dependencies

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 192745.
JDevlieghere added a comment.

And move python to the ScriptInterpreter


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

https://reviews.llvm.org/D59970

Files:
  lldb/source/Core/CMakeLists.txt
  lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
  lldb/source/Utility/CMakeLists.txt

Index: lldb/source/Utility/CMakeLists.txt
===
--- lldb/source/Utility/CMakeLists.txt
+++ lldb/source/Utility/CMakeLists.txt
@@ -1,46 +1,15 @@
 set(LLDB_SYSTEM_LIBS)
 
-# Windows-only libraries
-if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
-  list(APPEND LLDB_SYSTEM_LIBS
-ws2_32
-rpcrt4
-)
-endif ()
+list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
 
-if (NOT LLDB_DISABLE_LIBEDIT)
-  list(APPEND LLDB_SYSTEM_LIBS ${libedit_LIBRARIES})
-endif()
-if (NOT LLDB_DISABLE_CURSES)
-  list(APPEND LLDB_SYSTEM_LIBS ${CURSES_LIBRARIES})
-  if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
-list(APPEND LLDB_SYSTEM_LIBS ${TERMINFO_LIBS})
-  endif()
-endif()
+if (CMAKE_SYSTEM_NAME MATCHES "Windows")
+  list(APPEND LLDB_SYSTEM_LIBS ws2_32 rpcrt4)
+endif ()
 
 if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
 list(APPEND LLDB_SYSTEM_LIBS atomic)
 endif()
 
-if(Backtrace_FOUND)
-  list(APPEND LLDB_SYSTEM_LIBS ${Backtrace_LIBRARY})
-endif()
-
-if (NOT LLDB_DISABLE_PYTHON AND NOT LLVM_BUILD_STATIC)
-  list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARIES})
-endif()
-
-list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
-
-if (LLVM_BUILD_STATIC)
-  if (NOT LLDB_DISABLE_PYTHON)
-list(APPEND LLDB_SYSTEM_LIBS python2.7 util)
-  endif()
-  if (NOT LLDB_DISABLE_CURSES)
-list(APPEND LLDB_SYSTEM_LIBS gpm)
-  endif()
-endif()
-
 add_lldb_library(lldbUtility
   ArchSpec.cpp
   Args.cpp
Index: lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
===
--- lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
+++ lldb/source/Plugins/ScriptInterpreter/Python/CMakeLists.txt
@@ -23,6 +23,8 @@
 lldbHost
 lldbInterpreter
 lldbTarget
+${PYTHON_LIBRARY}
+
   LINK_COMPONENTS
 Support
   )
Index: lldb/source/Core/CMakeLists.txt
===
--- lldb/source/Core/CMakeLists.txt
+++ lldb/source/Core/CMakeLists.txt
@@ -1,10 +1,21 @@
 set(LLDB_CURSES_LIBS)
+set(LLDB_LIBEDIT_LIBS)
 
 if (NOT LLDB_DISABLE_CURSES)
   list(APPEND LLDB_CURSES_LIBS ${CURSES_LIBRARIES})
   if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
 list(APPEND LLDB_CURSES_LIBS ${TERMINFO_LIBS})
   endif()
+  if (LLVM_BUILD_STATIC)
+list(APPEND LLDB_CURSES_LIBS gpm)
+  endif()
+endif()
+
+if (NOT LLDB_DISABLE_LIBEDIT)
+  list(APPEND LLDB_LIBEDIT_LIBS ${libedit_LIBRARIES})
+  if (LLVM_BUILD_STATIC)
+list(APPEND LLDB_SYSTEM_LIBS gpm)
+  endif()
 endif()
 
 add_lldb_library(lldbCore
@@ -67,6 +78,7 @@
 lldbPluginCPlusPlusLanguage
 lldbPluginObjCLanguage
 ${LLDB_CURSES_LIBS}
+${LLDB_LIBEDIT_LIBS}
 
   LINK_COMPONENTS
 BinaryFormat
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D59911#1446946 , @zturner wrote:

> In D59911#1446942 , @jingham wrote:
>
> > In D59911#1446745 , @davide wrote:
> >
> > > My (maybe unpopolar) opinion on the subject is that "soft assertions" are 
> > > a way to cleanse your conscience of guilt, but they don't work really 
> > > well in practice.
> > >  When I started working on lldb, I was a fairly strong proponent of 
> > > assertions everywhere. My view changed somewhat radically over the course 
> > > of the past 18 months, and I would like to summarize some points here.
> > >
> > > 1. Vendors (some) ship a release or two of debuggers every 6 months. Even 
> > > if you just look at open source, llvm gets released every 6 months and 
> > > distributions downstream package just the release version. Not everybody 
> > > runs gentoo or exherbo or compiles his toolchain from svn. For these 
> > > people the debugger has just to work. Assertions are always compiled out 
> > > in release mode so this is not an issue. I strongly agree that for 
> > > internal interfaces they have to be used as liberally as possible, mostly 
> > > because they catch bugs in development. llvm libraries tend to use 
> > > assertions galore, and developers put them in "just in case", and I think 
> > > this is really not something we should do in lldb.
> > > 2. Errors have to be handled, properly. I don't think returning a default 
> > > constructed object in case something goes bad is better than throwing an 
> > > error. We now have rich/proper error handling in llvm to make sure the 
> > > process blows up if the error is not checked. This is a good thing.
> > > 3. The above points are IMHO the only two needed ways of handling 
> > > invariants/invalid input. Anything else in the middle is just going to 
> > > cause confusion to new and existing developer. Adrian (or anybody else), 
> > > do you have any real example where something wasn't either a user error 
> > > or a strong invariant that couldn't be violated? I personally didn't, 
> > > hence I don't understand the need for "soft" assertions here.
> >
> >
> > Here is a concrete example of the sort of thing I thought lldbassert was 
> > for.   In DWARFExpression::AddressRangeForLocationListEntry if we come 
> > across a LocationList format we don't understand, we do:
> >
> >   default:
> > // Not supported entry type
> > lldbassert(false && "Not supported location list type");
> > return false;
>
>
> IMO, this example should be changed to
>
>   return llvm::make_error("Unsupported location list type");
>
>
> and let someone higher up handle this.


We may be talking at cross-purposes...

I think it would be fine to convert this FUNCTION to return an llvm::Error that 
enforces the error gets handled properly.  But that's orthogonal to whether we 
do an lldbassert before returning the make_error (or "return false").

The whole point of lldbasserts as I understood it was is to catch problems that 
we can and should always recover from IRL as efficiently as possible in the 
testsuite.

If this function returned an error, and all the code above was doing the right 
thing, then some one in the call stack will deal with the return and finally 
end up producing some user-visible failure (i.e. "can't view variable X").   
However, you will only catch the failure in the testsuite if the place where we 
ran across this bad format happened to be a variable that a test asserted.  
Then we would see a test case failure and would go debug it and figure out what 
we needed to do, and all would be good.

But if we put an lldbassert before the return, then we wouldn't have to rely on 
what exactly the test checked.  As long as we passed through this code with the 
unexpected format we would assert, the test would fail at that point, and we  
would know something went wrong and that we should go fix it.  So the chance 
that the testsuite would catch this problem for us is increased by the 
lldbassert.


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

https://reviews.llvm.org/D59911



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

[Lldb-commits] [PATCH] D59972: [Python] Remove readline module

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: labath.
Herald added subscribers: jdoerfert, mgorny.
Herald added a project: LLDB.

Todd added this empty readline module to workaround an issue with an old 
version of Python on Ubuntu in 2014 (18841). In the meantime, Python seems to 
have fixed the underlying issue, and indeed, I wasn't able to reproduce this. I 
propose to remove this.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D59972

Files:
  lldb/scripts/CMakeLists.txt
  lldb/scripts/Python/modules/CMakeLists.txt
  lldb/scripts/Python/modules/readline/CMakeLists.txt
  lldb/scripts/Python/modules/readline/readline.cpp

Index: lldb/scripts/Python/modules/readline/readline.cpp
===
--- lldb/scripts/Python/modules/readline/readline.cpp
+++ /dev/null
@@ -1,87 +0,0 @@
-// NOTE: Since Python may define some pre-processor definitions which affect the
-// standard headers on some systems, you must include Python.h before any
-// standard headers are included.
-#include "Python.h"
-
-#include 
-
-#ifndef LLDB_DISABLE_LIBEDIT
-#include 
-#endif
-
-// Simple implementation of the Python readline module using libedit.
-// In the event that libedit is excluded from the build, this turns
-// back into a null implementation that blocks the module from pulling
-// in the GNU readline shared lib, which causes linkage confusion when
-// both readline and libedit's readline compatibility symbols collide.
-//
-// Currently it only installs a PyOS_ReadlineFunctionPointer, without
-// implementing any of the readline module methods. This is meant to
-// work around LLVM pr18841 to avoid seg faults in the stock Python
-// readline.so linked against GNU readline.
-
-#ifndef LLDB_DISABLE_LIBEDIT
-PyDoc_STRVAR(moduleDocumentation,
- "Simple readline module implementation based on libedit.");
-#else
-PyDoc_STRVAR(moduleDocumentation,
- "Stub module meant to avoid linking GNU readline.");
-#endif
-
-#if PY_MAJOR_VERSION >= 3
-static struct PyModuleDef readline_module = {
-PyModuleDef_HEAD_INIT, // m_base
-"readline",// m_name
-moduleDocumentation,   // m_doc
--1,// m_size
-nullptr,   // m_methods
-nullptr,   // m_reload
-nullptr,   // m_traverse
-nullptr,   // m_clear
-nullptr,   // m_free
-};
-#else
-static struct PyMethodDef moduleMethods[] = {{nullptr, nullptr, 0, nullptr}};
-#endif
-
-#ifndef LLDB_DISABLE_LIBEDIT
-static char *
-#if PY_MAJOR_VERSION >= 3
-simple_readline(FILE *stdin, FILE *stdout, const char *prompt)
-#else
-simple_readline(FILE *stdin, FILE *stdout, char *prompt)
-#endif
-{
-  rl_instream = stdin;
-  rl_outstream = stdout;
-  char *line = readline(prompt);
-  if (!line) {
-char *ret = (char *)PyMem_Malloc(1);
-if (ret != NULL)
-  *ret = '\0';
-return ret;
-  }
-  if (*line)
-add_history(line);
-  int n = strlen(line);
-  char *ret = (char *)PyMem_Malloc(n + 2);
-  strncpy(ret, line, n);
-  free(line);
-  ret[n] = '\n';
-  ret[n + 1] = '\0';
-  return ret;
-}
-#endif
-
-PyMODINIT_FUNC initreadline(void) {
-#ifndef LLDB_DISABLE_LIBEDIT
-  PyOS_ReadlineFunctionPointer = simple_readline;
-#endif
-
-#if PY_MAJOR_VERSION >= 3
-  return PyModule_Create(&readline_module);
-#else
-  Py_InitModule4("readline", moduleMethods, moduleDocumentation,
- static_cast(NULL), PYTHON_API_VERSION);
-#endif
-}
Index: lldb/scripts/Python/modules/readline/CMakeLists.txt
===
--- lldb/scripts/Python/modules/readline/CMakeLists.txt
+++ /dev/null
@@ -1,27 +0,0 @@
-# FIXME: if a non-standard version of python is requested, the cmake macro
-# below will need Python_ADDITIONAL_VERSIONS set in order to find it.
-include(FindPythonInterp)
-SET(PYTHON_DIRECTORY python${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}/site-packages)
-
-# Build the readline python module
-include_directories(${PYTHON_INCLUDE_DIR})
-add_library(readline SHARED readline.cpp)
-target_link_libraries(readline ${PYTHON_LIBRARY})
-
-if (NOT LLDB_DISABLE_LIBEDIT)
-  target_include_directories(readline
- PRIVATE
-   ${libedit_INCLUDE_DIRS})
-  target_link_libraries(readline ${libedit_LIBRARIES})
-endif()
-
-# FIXME: the LIBRARY_OUTPUT_PATH seems to be ignored - this is not a
-# functional issue for the build dir, though, since the shared lib dir
-# for the build is in the python shared library load path, and thus
-# python finds it when loading the python readline module.
-set_target_properties(readline PROPERTIES
-   PREFIX ""
-   LIBRARY_OUTPUT_PATH ${CMAKE_CURRENT_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/${PYTHON_DIRECTORY})
-
-# Install the readline module.
-install(TARGETS readline LIBRARY DESTINATION ${CMAKE_INSTALL_PREFIX}/

[Lldb-commits] [PATCH] D59970: [CMake] Untangle linking of dependencies

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: zturner, stella.stamenova, labath, sgraenitz.
Herald added a subscriber: mgorny.
Herald added a project: LLDB.

The utility library shouldn't depend on curses, libedit or python since it uses 
none of them. Move the first two to libCore where they are actually used.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D59970

Files:
  lldb/source/Core/CMakeLists.txt
  lldb/source/Utility/CMakeLists.txt


Index: lldb/source/Utility/CMakeLists.txt
===
--- lldb/source/Utility/CMakeLists.txt
+++ lldb/source/Utility/CMakeLists.txt
@@ -1,46 +1,15 @@
 set(LLDB_SYSTEM_LIBS)
 
-# Windows-only libraries
-if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
-  list(APPEND LLDB_SYSTEM_LIBS
-ws2_32
-rpcrt4
-)
-endif ()
+list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
 
-if (NOT LLDB_DISABLE_LIBEDIT)
-  list(APPEND LLDB_SYSTEM_LIBS ${libedit_LIBRARIES})
-endif()
-if (NOT LLDB_DISABLE_CURSES)
-  list(APPEND LLDB_SYSTEM_LIBS ${CURSES_LIBRARIES})
-  if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
-list(APPEND LLDB_SYSTEM_LIBS ${TERMINFO_LIBS})
-  endif()
-endif()
+if (CMAKE_SYSTEM_NAME MATCHES "Windows")
+  list(APPEND LLDB_SYSTEM_LIBS ws2_32 rpcrt4)
+endif ()
 
 if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
 list(APPEND LLDB_SYSTEM_LIBS atomic)
 endif()
 
-if(Backtrace_FOUND)
-  list(APPEND LLDB_SYSTEM_LIBS ${Backtrace_LIBRARY})
-endif()
-
-if (NOT LLDB_DISABLE_PYTHON AND NOT LLVM_BUILD_STATIC)
-  list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARIES})
-endif()
-
-list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
-
-if (LLVM_BUILD_STATIC)
-  if (NOT LLDB_DISABLE_PYTHON)
-list(APPEND LLDB_SYSTEM_LIBS python2.7 util)
-  endif()
-  if (NOT LLDB_DISABLE_CURSES)
-list(APPEND LLDB_SYSTEM_LIBS gpm)
-  endif()
-endif()
-
 add_lldb_library(lldbUtility
   ArchSpec.cpp
   Args.cpp
Index: lldb/source/Core/CMakeLists.txt
===
--- lldb/source/Core/CMakeLists.txt
+++ lldb/source/Core/CMakeLists.txt
@@ -1,10 +1,21 @@
 set(LLDB_CURSES_LIBS)
+set(LLDB_LIBEDIT_LIBS)
 
 if (NOT LLDB_DISABLE_CURSES)
   list(APPEND LLDB_CURSES_LIBS ${CURSES_LIBRARIES})
   if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
 list(APPEND LLDB_CURSES_LIBS ${TERMINFO_LIBS})
   endif()
+  if (LLVM_BUILD_STATIC)
+list(APPEND LLDB_CURSES_LIBS gpm)
+  endif()
+endif()
+
+if (NOT LLDB_DISABLE_LIBEDIT)
+  list(APPEND LLDB_LIBEDIT_LIBS ${libedit_LIBRARIES})
+  if (LLVM_BUILD_STATIC)
+list(APPEND LLDB_SYSTEM_LIBS gpm)
+  endif()
 endif()
 
 add_lldb_library(lldbCore
@@ -67,6 +78,7 @@
 lldbPluginCPlusPlusLanguage
 lldbPluginObjCLanguage
 ${LLDB_CURSES_LIBS}
+${LLDB_LIBEDIT_LIBS}
 
   LINK_COMPONENTS
 BinaryFormat


Index: lldb/source/Utility/CMakeLists.txt
===
--- lldb/source/Utility/CMakeLists.txt
+++ lldb/source/Utility/CMakeLists.txt
@@ -1,46 +1,15 @@
 set(LLDB_SYSTEM_LIBS)
 
-# Windows-only libraries
-if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
-  list(APPEND LLDB_SYSTEM_LIBS
-ws2_32
-rpcrt4
-)
-endif ()
+list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
 
-if (NOT LLDB_DISABLE_LIBEDIT)
-  list(APPEND LLDB_SYSTEM_LIBS ${libedit_LIBRARIES})
-endif()
-if (NOT LLDB_DISABLE_CURSES)
-  list(APPEND LLDB_SYSTEM_LIBS ${CURSES_LIBRARIES})
-  if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
-list(APPEND LLDB_SYSTEM_LIBS ${TERMINFO_LIBS})
-  endif()
-endif()
+if (CMAKE_SYSTEM_NAME MATCHES "Windows")
+  list(APPEND LLDB_SYSTEM_LIBS ws2_32 rpcrt4)
+endif ()
 
 if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
 list(APPEND LLDB_SYSTEM_LIBS atomic)
 endif()
 
-if(Backtrace_FOUND)
-  list(APPEND LLDB_SYSTEM_LIBS ${Backtrace_LIBRARY})
-endif()
-
-if (NOT LLDB_DISABLE_PYTHON AND NOT LLVM_BUILD_STATIC)
-  list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARIES})
-endif()
-
-list(APPEND LLDB_SYSTEM_LIBS ${system_libs})
-
-if (LLVM_BUILD_STATIC)
-  if (NOT LLDB_DISABLE_PYTHON)
-list(APPEND LLDB_SYSTEM_LIBS python2.7 util)
-  endif()
-  if (NOT LLDB_DISABLE_CURSES)
-list(APPEND LLDB_SYSTEM_LIBS gpm)
-  endif()
-endif()
-
 add_lldb_library(lldbUtility
   ArchSpec.cpp
   Args.cpp
Index: lldb/source/Core/CMakeLists.txt
===
--- lldb/source/Core/CMakeLists.txt
+++ lldb/source/Core/CMakeLists.txt
@@ -1,10 +1,21 @@
 set(LLDB_CURSES_LIBS)
+set(LLDB_LIBEDIT_LIBS)
 
 if (NOT LLDB_DISABLE_CURSES)
   list(APPEND LLDB_CURSES_LIBS ${CURSES_LIBRARIES})
   if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
 list(APPEND LLDB_CURSES_LIBS ${TERMINFO_LIBS})
   endif()
+  if (LLVM_BUILD_STATIC)
+list(APPEND LLDB_CURSES_LIBS gpm)
+  endif()
+endif()
+
+if (NOT LLDB_DISABLE_LIBEDIT)
+  list(APPEND LLDB_LIBEDIT_LIBS ${libedit_LIBRARIES})
+  if (LLVM_BUILD_STATIC)
+list(APPEND LLDB_SYSTEM_LIBS gpm)
+  endif()
 endif()
 
 add_lldb_library(lldbCore
@@ -67,6 +78,7 @@
 lldbPluginC

[Lldb-commits] [PATCH] D59976: [Python] Remove Python include from ScriptInterpreterPython.h

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: zturner, davide.
Herald added a subscriber: abidh.
Herald added a project: LLDB.

This patch limits the scope of the python header to the python script 
interpreter plugin by removing the include from ScriptInterpreterPython.h. To 
make that possible I had to forward declare the PythonDataObjects and move the 
PythonScriptInterpreterLocker into ScriptInterpreterPython.cpp.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D59976

Files:
  lldb/source/API/SBHostOS.cpp
  lldb/source/API/SystemInitializerFull.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
  lldb/source/Plugins/ScriptInterpreter/Python/lldb-python.h

Index: lldb/source/Plugins/ScriptInterpreter/Python/lldb-python.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/lldb-python.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/lldb-python.h
@@ -32,6 +32,12 @@
 #undef _XOPEN_SOURCE
 #endif
 
+// Include locale before Python so _PY_PORT_CTYPE_UTF8_ISSUE doesn't cause
+// macro redefinitions.
+#if defined(__APPLE__)
+#include 
+#endif
+
 // Include python for non windows machines
 #include 
 #endif // LLDB_DISABLE_PYTHON
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -19,16 +19,23 @@
 #include 
 #include 
 
-#include "PythonDataObjects.h"
 #include "lldb/Breakpoint/BreakpointOptions.h"
 #include "lldb/Core/IOHandler.h"
 #include "lldb/Host/Terminal.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/lldb-private.h"
 
+// Python forward declarations
+struct _ts;
+typedef _ts PyThreadState;
+
 class IOHandlerPythonInterpreter;
 
 namespace lldb_private {
+class PythonScriptInterpreterLocker;
+class PythonFile;
+class PythonObject;
+class PythonDictionary;
 
 class ScriptInterpreterPython : public ScriptInterpreter,
 public IOHandlerDelegateMultiline {
@@ -41,6 +48,7 @@
   };
 
   friend class ::IOHandlerPythonInterpreter;
+  friend class lldb_private::PythonScriptInterpreterLocker;
 
   ScriptInterpreterPython(CommandInterpreter &interpreter);
 
@@ -324,46 +332,6 @@
 
   uint32_t GetPluginVersion() override;
 
-  class Locker : public ScriptInterpreterLocker {
-  public:
-enum OnEntry {
-  AcquireLock = 0x0001,
-  InitSession = 0x0002,
-  InitGlobals = 0x0004,
-  NoSTDIN = 0x0008
-};
-
-enum OnLeave {
-  FreeLock = 0x0001,
-  FreeAcquiredLock = 0x0002, // do not free the lock if we already held it
- // when calling constructor
-  TearDownSession = 0x0004
-};
-
-Locker(ScriptInterpreterPython *py_interpreter = nullptr,
-   uint16_t on_entry = AcquireLock | InitSession,
-   uint16_t on_leave = FreeLock | TearDownSession, FILE *in = nullptr,
-   FILE *out = nullptr, FILE *err = nullptr);
-
-~Locker() override;
-
-  private:
-bool DoAcquireLock();
-
-bool DoInitSession(uint16_t on_entry_flags, FILE *in, FILE *out, FILE *err);
-
-bool DoFreeLock();
-
-bool DoTearDownSession();
-
-static void ReleasePythonLock();
-
-bool m_teardown_session;
-ScriptInterpreterPython *m_python_interpreter;
-//	FILE*m_tmp_fh;
-PyGILState_STATE m_GILState;
-  };
-
 protected:
   static void InitializePrivate();
 
@@ -422,15 +390,16 @@
   bool SetStdHandle(File &file, const char *py_name, PythonFile &save_file,
 const char *mode);
 
-  PythonFile m_saved_stdin;
-  PythonFile m_saved_stdout;
-  PythonFile m_saved_stderr;
-  PythonObject m_main_module;
-  PythonObject m_lldb_module;
-  PythonDictionary m_session_dict;
-  PythonDictionary m_sys_module_dict;
-  PythonObject m_run_one_line_function;
-  PythonObject m_run_one_line_str_global;
+  // Unique pointers so the corresponding class can be forward declared.
+  std::unique_ptr m_saved_stdin;
+  std::unique_ptr m_saved_stdout;
+  std::unique_ptr m_saved_stderr;
+  std::unique_ptr m_main_module;
+  std::unique_ptr m_lldb_module;
+  std::unique_ptr m_session_dict;
+  std::unique_ptr m_sys_module_dict;
+  std::unique_ptr m_run_one_line_function;
+  std::unique_ptr m_run_one_line_str_global;
   std::string m_dictionary_name;
   TerminalState m_terminal_state;
   ActiveIOHandler m_active_io_handler;
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -193,6 +193,111 @@

[Lldb-commits] [PATCH] D59957: Convert = operators that take object mutexes to the multi-lock version of std::lock

[Pavel Labath via Phabricator via lldb-commits]

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

This looks great. Thank you.

As of c++17 there will be an RAII way to do this 
https://en.cppreference.com/w/cpp/thread/scoped_lock. Theoretically we could 
jump ahead, and implement something like that ourselves, but there aren't too 
many uses of this pattern now, so it's not really necessary at this point.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59957



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

[Lldb-commits] [PATCH] D59968: [Cmake] Unify python variables

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/cmake/modules/LLDBConfig.cmake:246-247
   else()
-find_package(PythonLibs REQUIRED)
+include(FindPythonInterp)
+include(FindPythonLibs)
   endif()

Is there any difference between `include(FindPythonInterp)` and 
`find_package(PythonInterp)`? It sounds like the latter should be used, because 
it's more specific, but I have no idea its possible to do that..


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59968



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