[Lldb-commits] [lldb] dc0f098 - [lldb] Fix a crash in PlatformAppleSimulator::GetCoreSimulatorPath when Xcode developer directory can't be found

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-06-11T09:48:39+02:00
New Revision: dc0f09804886bdf26870162423c7432d6e6a4114

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

LOG: [lldb] Fix a crash in PlatformAppleSimulator::GetCoreSimulatorPath when 
Xcode developer directory can't be found

Summary:

`PlatformAppleSimulator::GetCoreSimulatorPath` currently checks if
`m_core_simulator_framework_path` wasn't set yet and then tries to calculate its
actual value. However, if `GetXcodeDeveloperDirectory` returns an invalid
FileSpec, `m_core_simulator_framework_path` is never assigned a value which
causes that the `return m_core_simulator_framework_path.getValue();` at the end
of the function will trigger an assert.

This patch just assigns an invalid FileSpec to `m_core_simulator_framework_path`
which seems what the calling code in `PlatformAppleSimulator::LoadCoreSimulator`
expects as an error value.

I assume this can be reproduces on machines that don't have an Xcode
installation, but this patch is mostly based on this backtrace I received from
someone else that tried to run the test suite:

```
Assertion failed: (hasVal), function getValue, file 
llvm/include/llvm/ADT/Optional.h, line 73.
[...]
3   libsystem_c.dylib   0x7fff682a1ac6 __assert_rtn + 314
4   liblldb.11.0.0git.dylib 0x00010b835931 
PlatformAppleSimulator::GetCoreSimulatorPath() (.cold.1) + 33
5   liblldb.11.0.0git.dylib 0x000107e92f11 
PlatformAppleSimulator::GetCoreSimulatorPath() + 369
6   liblldb.11.0.0git.dylib 0x000107e9383e void 
std::__1::__call_once_proxy
 >(void*) + 30
7   libc++.1.dylib  0x7fff654d5bea 
std::__1::__call_once(unsigned long volatile&, void*, void (*)(void*)) + 139
8   liblldb.11.0.0git.dylib 0x000107e92019 
PlatformAppleSimulator::LaunchProcess(lldb_private::ProcessLaunchInfo&) + 89
9   liblldb.11.0.0git.dylib 0x000107e92be5 
PlatformAppleSimulator::DebugProcess(lldb_private::ProcessLaunchInfo&, 
lldb_private::Debugger&, lldb_private::Target*, lldb_private::Status&) + 101
10  liblldb.11.0.0git.dylib 0x000107cb044d 
lldb_private::Target::Launch(lldb_private::ProcessLaunchInfo&, 
lldb_private::Stream*) + 669
11  liblldb.11.0.0git.dylib 0x00010792c9c5 
lldb::SBTarget::Launch(lldb::SBLaunchInfo&, lldb::SBError&) + 1109
12  liblldb.11.0.0git.dylib 0x000107a92acd 
_wrap_SBTarget_Launch(_object*, _object*) + 477
13  org.python.python   0x00010681076f PyCFunction_Call + 
321
14  org.python.python   0x00010689ee12 
_PyEval_EvalFrameDefault + 7738
```

Reviewers: JDevlieghere, jasonmolenda

Reviewed By: JDevlieghere

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

Added: 


Modified: 
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp 
b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
index 3f83a23de8df..7fc042601256 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -223,7 +223,8 @@ FileSpec PlatformAppleSimulator::GetCoreSimulatorPath() {
   developer_dir.c_str());
   m_core_simulator_framework_path = FileSpec(cs_path.GetData());
   FileSystem::Instance().Resolve(*m_core_simulator_framework_path);
-}
+} else
+  m_core_simulator_framework_path = FileSpec();
   }
 
   return m_core_simulator_framework_path.getValue();



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

[Lldb-commits] [PATCH] D81612: [lldb/Test] Assert that no targets or global modules remain after a test completes.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm pretty indifferent about this functionality -- I don't think it hurts, but 
it also doesn't seem like a pressing problem that needs addressing.

Regarding the implementation, be aware that assertion failures during test 
teardown are reproted pretty weirdly -- IIRC at this point the test has already 
been declared "successful", and these failures manifest as "CLEANUP ERROR"s 
somewhere. And I have a feeling they don't actually fail the check-lldb 
command. I don't know if this is due to something we've done, or if it's just 
how the python unittest framework works...


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

https://reviews.llvm.org/D81612



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

[Lldb-commits] [PATCH] D80997: [lldb] Fix a crash in PlatformAppleSimulator::GetCoreSimulatorPath when Xcode developer directory can't be found

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc0f09804886: [lldb] Fix a crash in 
PlatformAppleSimulator::GetCoreSimulatorPath when Xcode… (authored by 
teemperor).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80997

Files:
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp


Index: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -223,7 +223,8 @@
   developer_dir.c_str());
   m_core_simulator_framework_path = FileSpec(cs_path.GetData());
   FileSystem::Instance().Resolve(*m_core_simulator_framework_path);
-}
+} else
+  m_core_simulator_framework_path = FileSpec();
   }
 
   return m_core_simulator_framework_path.getValue();


Index: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -223,7 +223,8 @@
   developer_dir.c_str());
   m_core_simulator_framework_path = FileSpec(cs_path.GetData());
   FileSystem::Instance().Resolve(*m_core_simulator_framework_path);
-}
+} else
+  m_core_simulator_framework_path = FileSpec();
   }
 
   return m_core_simulator_framework_path.getValue();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D81561: [lldb] Add basic -flimit-debug-info support to expression evaluator

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 270097.
labath added a comment.

add more comments and logging


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81561

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h
  lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/functionalities/limit-debug-info/Makefile
  lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
  lldb/test/API/functionalities/limit-debug-info/foo.cpp
  lldb/test/API/functionalities/limit-debug-info/main.cpp
  lldb/test/API/functionalities/limit-debug-info/one.cpp
  lldb/test/API/functionalities/limit-debug-info/onetwo.h
  lldb/test/API/functionalities/limit-debug-info/two.cpp

Index: lldb/test/API/functionalities/limit-debug-info/two.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/limit-debug-info/two.cpp
@@ -0,0 +1,3 @@
+#include "onetwo.h"
+
+Two::~Two() = default;
Index: lldb/test/API/functionalities/limit-debug-info/onetwo.h
===
--- /dev/null
+++ lldb/test/API/functionalities/limit-debug-info/onetwo.h
@@ -0,0 +1,11 @@
+struct One {
+  int one = 142;
+  constexpr One() = default;
+  virtual ~One();
+};
+
+struct Two : One {
+  int two = 242;
+  constexpr Two() = default;
+  ~Two() override;
+};
Index: lldb/test/API/functionalities/limit-debug-info/one.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/limit-debug-info/one.cpp
@@ -0,0 +1,3 @@
+#include "onetwo.h"
+
+One::~One() = default;
Index: lldb/test/API/functionalities/limit-debug-info/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/limit-debug-info/main.cpp
@@ -0,0 +1,13 @@
+#include "onetwo.h"
+
+struct InheritsFromOne : One {
+  constexpr InheritsFromOne() = default;
+  int member = 47;
+} inherits_from_one;
+
+struct InheritsFromTwo : Two {
+  constexpr InheritsFromTwo() = default;
+  int member = 47;
+} inherits_from_two;
+
+int main() { return 0; }
Index: lldb/test/API/functionalities/limit-debug-info/foo.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/limit-debug-info/foo.cpp
@@ -0,0 +1,6 @@
+struct A {
+  int a = 47;
+  virtual ~A();
+};
+
+A::~A() = default;
Index: lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
===
--- /dev/null
+++ lldb/test/API/functionalities/limit-debug-info/TestLimitDebugInfo.py
@@ -0,0 +1,85 @@
+"""
+Test completing types using information from other shared libraries.
+"""
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class LimitDebugInfoTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def _check_type(self, target, name):
+exe = target.FindModule(lldb.SBFileSpec("a.out"))
+the_type = exe.FindFirstType(name)
+self.assertTrue(the_type)
+self.trace("the_type: %s"%the_type)
+field_names = map(lambda f: f.GetName(), the_type.get_fields_array())
+self.assertEquals(list(field_names), ["member"])
+
+def _check_debug_info_is_limited(self, target):
+# Without other shared libraries we should only see the member declared
+# in the derived class. This serves as a sanity check that we are truly
+# building with limited debug info.
+self._check_type(target, "InheritsFromOne")
+self._check_type(target, "InheritsFromTwo")
+
+def test_one_and_two_debug(self):
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+self._check_debug_info_is_limited(target)
+
+self.registerSharedLibrariesWithTarget(target, ["one", "two"])
+
+# But when other shared libraries are loaded, we should be able to see
+# all members.
+self.expect_expr("inherits_from_one.member", result_value="47")
+self.expect_expr("inherits_from_one.one", result_value="142")
+
+self.expect_expr("inherits_from_two.member", result_value="47")
+self.expect_expr("inherits_from_two.one", result_value="142")
+self.expect_expr("inherits_from_two.two", result_value="242")
+
+def test_two_debug(self):
+self.build(dictionary=dict(STRIP_ONE="1"))
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+
+self._check_debug_info_is_li

[Lldb-commits] [PATCH] D81561: [lldb] Add basic -flimit-debug-info support to expression evaluator

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D81561#2086435 , @clayborg wrote:

> If we run into a class in the AST importer that was forcefully completed and 
> we can't find a better definition, what do we do? Error out? Do what we did. 
> I would like there to be a nice log line in the "log enable lldb expr" to 
> tell us when this happens so we can know why an expression is failing if we 
> don't emit an error and stop the expression.


We do what we did previously -- import the empty class. The included test case 
exercises this scenario as well.

I'll print a log message when that happens.

> Also, for "frame variable" and the any ValueObject objects, we can easily see 
> what a BaseClass is marked as forcefully completed and just do a simple type 
> search for the type given a full decl context as context and substitute the 
> type. That won't use any of this code, it will just need to know to grab the 
> the full type from the target by doing a search. We could make a function 
> similar to CompilerType::GetCompleteType() that takes a target so that it can 
> search all of the modules in a target for a type.

Yes, that's the rough idea, but I haven't tried implementing it yet.

In D81561#2086580 , @aprantl wrote:

> It's great to see this being addressed! I have a high-level question: When 
> completing types across lldb::Modules — in which ASTContext is the complete 
> type created? Since a per-module TypeSystem can be shared by many debuggers, 
> I want to make sure that types from another module don't pollute another 
> module's ASTContext, and that they are created in the/a scratch context 
> instead.


Apart from the adding the "forcefully completed" flag, this solution does not 
change the contents of module ASTs in any way. If we take the example in the 
test case, the individual modules ASTs would look something like:
libone:

  struct One {
int one;
  };

libtwo:

  struct [[forcefully_completed]] One { };
  struct Two : One {
int two;
  };

a.out:

  struct [[forcefully_completed]] One { };
  struct [[forcefully_completed]] Two { };
  struct InheritsFromOne: One {
int member;
  };
  ...

When importing into the expression AST, we pick the best representation for 
each of the types and create a merged class. I'm not entirely sure what happens 
when importing this into the AST for persistent expression results (what's the 
name for that?). Ideally the merged type would be imported there too, but I am 
not sure if that actually happens right now -- `expr inherits_from_one.one` 
works, but a plain `expr inherits_from_one` just displays the `member` member. 
I'm not yet sure if this is simply due to lack of support in the "frame 
variable" machinery which is supposed to display the result, or if there is 
some more importing that needs to happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81561



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

[Lldb-commits] [lldb] e966a5d - [lldb] Remove Scalar operator= overloads

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-06-11T13:55:02+02:00
New Revision: e966a5deaa50f1ddca3810e945acd14b899824c7

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

LOG: [lldb] Remove Scalar operator= overloads

The are not needed as Scalar is implicitly constructible from all of
these types (so the compiler will use a combination of a constructor +
move assignment instead), and they make it very easy for implementations
of assignment and construction operations to diverge.

Added: 


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

Removed: 




diff  --git a/lldb/include/lldb/Utility/Scalar.h 
b/lldb/include/lldb/Utility/Scalar.h
index 0c8f7452cfc3..1865a34775ee 100644
--- a/lldb/include/lldb/Utility/Scalar.h
+++ b/lldb/include/lldb/Utility/Scalar.h
@@ -153,16 +153,6 @@ class Scalar {
   // automagically by the compiler, so no temporary objects will need to be
   // created. As a result, we currently don't need a variety of overloaded set
   // value accessors.
-  Scalar &operator=(const int i);
-  Scalar &operator=(unsigned int v);
-  Scalar &operator=(long v);
-  Scalar &operator=(unsigned long v);
-  Scalar &operator=(long long v);
-  Scalar &operator=(unsigned long long v);
-  Scalar &operator=(float v);
-  Scalar &operator=(double v);
-  Scalar &operator=(long double v);
-  Scalar &operator=(llvm::APInt v);
   Scalar &operator+=(const Scalar &rhs);
   Scalar &operator<<=(const Scalar &rhs); // Shift left
   Scalar &operator>>=(const Scalar &rhs); // Shift right (arithmetic)

diff  --git a/lldb/source/Utility/Scalar.cpp b/lldb/source/Utility/Scalar.cpp
index 827a3f68c62a..759e2a7b56fc 100644
--- a/lldb/source/Utility/Scalar.cpp
+++ b/lldb/source/Utility/Scalar.cpp
@@ -305,101 +305,6 @@ const char *Scalar::GetTypeAsCString() const {
   return "";
 }
 
-Scalar &Scalar::operator=(const int v) {
-  m_type = e_sint;
-  m_integer = llvm::APInt(sizeof(int) * 8, v, true);
-  return *this;
-}
-
-Scalar &Scalar::operator=(unsigned int v) {
-  m_type = e_uint;
-  m_integer = llvm::APInt(sizeof(int) * 8, v);
-  return *this;
-}
-
-Scalar &Scalar::operator=(long v) {
-  m_type = e_slong;
-  m_integer = llvm::APInt(sizeof(long) * 8, v, true);
-  return *this;
-}
-
-Scalar &Scalar::operator=(unsigned long v) {
-  m_type = e_ulong;
-  m_integer = llvm::APInt(sizeof(long) * 8, v);
-  return *this;
-}
-
-Scalar &Scalar::operator=(long long v) {
-  m_type = e_slonglong;
-  m_integer = llvm::APInt(sizeof(long long) * 8, v, true);
-  return *this;
-}
-
-Scalar &Scalar::operator=(unsigned long long v) {
-  m_type = e_ulonglong;
-  m_integer = llvm::APInt(sizeof(long long) * 8, v);
-  return *this;
-}
-
-Scalar &Scalar::operator=(float v) {
-  m_type = e_float;
-  m_float = llvm::APFloat(v);
-  return *this;
-}
-
-Scalar &Scalar::operator=(double v) {
-  m_type = e_double;
-  m_float = llvm::APFloat(v);
-  return *this;
-}
-
-Scalar &Scalar::operator=(long double v) {
-  m_type = e_long_double;
-  m_float = llvm::APFloat(llvm::APFloat::x87DoubleExtended(),
-  llvm::APInt(BITWIDTH_INT128, NUM_OF_WORDS_INT128,
-  (reinterpret_cast(&v))->x));
-  return *this;
-}
-
-Scalar &Scalar::operator=(llvm::APInt rhs) {
-  m_integer = llvm::APInt(rhs);
-  switch (m_integer.getBitWidth()) {
-  case 8:
-  case 16:
-  case 32:
-if (m_integer.isSignedIntN(sizeof(sint_t) * 8))
-  m_type = e_sint;
-else
-  m_type = e_uint;
-break;
-  case 64:
-if (m_integer.isSignedIntN(sizeof(slonglong_t) * 8))
-  m_type = e_slonglong;
-else
-  m_type = e_ulonglong;
-break;
-  case 128:
-if (m_integer.isSignedIntN(BITWIDTH_INT128))
-  m_type = e_sint128;
-else
-  m_type = e_uint128;
-break;
-  case 256:
-if (m_integer.isSignedIntN(BITWIDTH_INT256))
-  m_type = e_sint256;
-else
-  m_type = e_uint256;
-break;
-  case 512:
-if (m_integer.isSignedIntN(BITWIDTH_INT512))
-  m_type = e_sint512;
-else
-  m_type = e_uint512;
-break;
-  }
-  return *this;
-}
-
 Scalar::~Scalar() = default;
 
 Scalar::Type Scalar::GetBestTypeForBitSize(size_t bit_size, bool sign) {



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

[Lldb-commits] [PATCH] D81516: [lldb/Test] Ensure inline tests have a unique build directory

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: lldb/packages/Python/lldbsuite/test/lldbinline.py:209
 test_class = type(name, (InlineTest,), dict(test=test_func,
-name=name, _build_dict=build_dict))
+name=name, __inline_name__=name, _build_dict=build_dict))
 

JDevlieghere wrote:
> labath wrote:
> > `__inline_name__` is not a good name according to 
> > :
> > ```
> > __double_leading_and_trailing_underscore__: "magic" objects or attributes 
> > that live in user-controlled namespaces. E.g. __init__, __import__ or 
> > __file__. Never invent such names; only use them as documented.
> > ```
> > 
> I did it for consistency with `__no_debug_info_test__`. Based on that PEP we 
> should rename it to `__no_debug_info_test`?
I think a single underscore should be enough. I am not sure the mangling magic 
invoked by a double underscore will  work for this case:
```
Note 2: Name mangling can make certain uses, such as debugging and 
__getattr__(), less convenient.
```
The PEP is very cautious about (not) encouraging double underscores, and a 
single leading underscore already says this is private.


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

https://reviews.llvm.org/D81516



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

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint

[Pavel Labath via Phabricator via lldb-commits]

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

In D74136#2085076 , @kwk wrote:

> IMPORTANT: The behavior of `target.inline-breakpoint-strategy` when set to 
> `headers` is still subject to change!
>
> I think the setting is not respected correctly...


Yes. I'm guessing you also need a call to 
`FileSpec::IsSourceImplementationFile` somewhere. The logic should be different 
only if `target.inline-breakpoint-strategy` is`headers` AND 
`FileSpec::IsSourceImplementationFile` is true.

To Jim: I'm still unable to decide whether we really should be respecting the 
inline-breakpoint-strategy setting or not in this case. I mean, this is an 
optimization for setting file+line breakpoints, but in this case it does not 
buy us anything, since we search for the functions by name. OTOH, it is nice to 
have a consistent treatment of the `--file` argument to `breakpoint set`...




Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:77-80
+settings set target.inline-breakpoint-strategy headers
+breakpoint set -n function_in_header -f search-support-files.h
+# CHECK: (lldb) breakpoint set -n function_in_header -f search-support-files.h
+# CHECK-NEXT: Breakpoint 10: no locations (pending).

The function in a .h file should be found regardless of the value of the 
setting.



Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:88-91
+settings set target.inline-breakpoint-strategy headers
+breakpoint set -n func -f search-support-files-func.cpp
+# CHECK: (lldb) breakpoint set -n func -f search-support-files-func.cpp
+# CHECK-NEXT: Breakpoint 12: no locations (pending).

This is right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136



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

[Lldb-commits] [PATCH] D81561: [lldb] Add basic -flimit-debug-info support to expression evaluator

[Raphael Isemann via Phabricator via lldb-commits]

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

I think the approach here makes sense. The only issue I see is how we get the 
complete replacement type (see the inline comment).

There is the issue if having an artificial empty class in an expression is an 
error, but we anyway already do this so I don't think this discussion should 
block this patch.

Also the FindCompleteType refactoring should IMHO land as it's own NFC commit 
just because it's such a huge part of the patch (I don't think that needs a 
separate review, it LGTM).




Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp:891
+
+if (auto *proxy = llvm::dyn_cast(
+getToContext().getExternalSource())) {

This check can really easily break. We should always be able to replace the 
ExternalASTSource with some kind of multiplexer (like 
MultiplexExternalSemaSource) without breaking functionality, so doing an RTTI 
check for a specific class here will break that concept. For example activating 
the import-std-module setting will make Clang add its own ASTReader source to 
the AST, so the ClangASTSourceProxy is hidden in some Multiplex* class (that 
contains ClangASTSourceProxy and the ASTReader) and this check will fail.

I think can simplified to this logic which doesn't require any RTTI support or 
ClangASTSource refactoring (even though the FindCompleteType refactor is IMHO 
something we should do even if we don't need it for this patch). I also removed 
that we forcibly complete the class which I don't think is necessary:

```
lang=c++
  if (td && md && md->IsForcefullyCompleted()) {
Expected dc_or_err = ImportContext(td->getDeclContext());
if (!dc_or_err)
  return dc_or_err.takeError();
Expected dn_or_err = Import(td->getDeclName());
if (!dn_or_err)
  return dn_or_err.takeError();
DeclContext *dc = *dc_or_err;
DeclarationName dn = *dn_or_err;
DeclContext::lookup_result lr = dc->lookup(dn);
if (lr.size()) {
  clang::Decl *lookup_found = lr.front();
  RegisterImportedDecl(From, lookup_found);
  m_decls_to_ignore.insert(lookup_found);
  return lookup_found;
}
  }
```

The only required change for getting this to work is to get rid of the 
`ImportInProgress` flag that completely disables all lookups when we copy a 
type. I don't think there is a good reason for even having this flag as it's 
anyway usually not correctly set when we do some kind of import, so I think 
deleting it is fine.



Comment at: lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp:11
+#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
+#include "Plugins/ExpressionParser/Clang/ClangASTMetadata.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"

I guess that's WIP code



Comment at: lldb/test/API/functionalities/limit-debug-info/Makefile:5
+
+ONE_CXXFLAGS := -flimit-debug-info
+ifdef STRIP_ONE

The in-tree limit-debug-info test already uses the LIMIT_DEBUG_INFO_FLAGS 
variable instead of hardcoding the flag, so I think we might as well do it here 
too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81561



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

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum updated this revision to Diff 270148.
fallkrum added a comment.

Code review fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Process.cpp
  lldb/test/API/functionalities/thread/ignore_suspended/Makefile
  
lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py
  lldb/test/API/functionalities/thread/ignore_suspended/main.cpp
  lldb/unittests/CMakeLists.txt
  lldb/unittests/Process/CMakeLists.txt
  lldb/unittests/Process/ProcessEventDataTest.cpp
  lldb/unittests/Thread/CMakeLists.txt
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- /dev/null
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -0,0 +1,168 @@
+//===-- ThreadTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/Thread.h"
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/StopInfo.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Reproducer.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class ThreadTest : public ::testing::Test {
+public:
+  void SetUp() override {
+llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None));
+FileSystem::Initialize();
+HostInfo::Initialize();
+platform_linux::PlatformLinux::Initialize();
+  }
+  void TearDown() override {
+platform_linux::PlatformLinux::Terminate();
+HostInfo::Terminate();
+FileSystem::Terminate();
+Reproducer::Terminate();
+  }
+};
+
+class DummyProcess : public Process {
+public:
+  using Process::Process;
+
+  virtual bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) {
+return true;
+  }
+  virtual Status DoDestroy() { return {}; }
+  virtual void RefreshStateAfterStop() {}
+  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+  Status &error) {
+return 0;
+  }
+  virtual bool UpdateThreadList(ThreadList &old_thread_list,
+ThreadList &new_thread_list) {
+return false;
+  }
+  virtual ConstString GetPluginName() { return ConstString("Dummy"); }
+  virtual uint32_t GetPluginVersion() { return 0; }
+
+  ProcessModID &GetModIDNonConstRef() { return m_mod_id; }
+};
+
+class DummyThread : public Thread {
+public:
+  using Thread::Thread;
+
+  ~DummyThread() override { DestroyThread(); }
+
+  void RefreshStateAfterStop() override {}
+
+  lldb::RegisterContextSP GetRegisterContext() override { return nullptr; }
+
+  lldb::RegisterContextSP
+  CreateRegisterContextForFrame(StackFrame *frame) override {
+return nullptr;
+  }
+
+  bool CalculateStopInfo() override { return false; }
+
+  bool IsStillAtLastBreakpointHit() override { return true; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  Status error;
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  error = debugger_sp->GetTargetList().CreateTarget(
+  *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+
+  if (target_sp) {
+debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+  }
+
+  return target_sp;
+}
+
+TEST_F(ThreadTest, SetStopInfo) {
+  ArchSpec arch("powerpc64-pc-linux");
+
+  Platform::SetHostPlatform(
+  platform_linux::PlatformLinux::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast(process_sp.get());
+
+  ThreadSP thread_sp = std::make_shared(*process_sp.get(), 0);
+  ASSERT_TRUE(thread_sp);
+
+  StopInfoSP stopinfo_sp =
+  StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp.get(), 0);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+
+  /*
+   Should make stopinfo valid.
+   */
+  process->GetModIDNonConstRef().BumpStopID();
+  ASSERT_TRUE(stopinfo_sp->IsValid() == false);
+
+  thread_sp->SetStopInfo(stopinfo_sp);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+}
+
+TEST_F(ThreadTest, GetPrivateStopInfo) {
+  ArchSpec arch("powerpc64-pc-linux");
+
+  Platform:

[Lldb-commits] [lldb] 5a33ba5 - [lldb/Test] Ensure inline tests have a unique build directory

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-06-11T09:36:54-07:00
New Revision: 5a33ba52b66ceb336b2dd941c9e3f5313a388e63

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

LOG: [lldb/Test] Ensure inline tests have a unique build directory

Inline tests have one method named 'test' which means that multiple
inline tests in the same file end up sharing the same build directory
per variant.

This patch overrides the getBuildDirBasename method for the InlineTest
class to include the test name.

Differential revision: https://reviews.llvm.org/D81516

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/lldbinline.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/lldbinline.py 
b/lldb/packages/Python/lldbsuite/test/lldbinline.py
index 71143ce3f16a..29a708440c2a 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbinline.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbinline.py
@@ -82,6 +82,11 @@ def handle_breakpoint(self, test, breakpoint_id):
 
 
 class InlineTest(TestBase):
+# Overrides
+
+def getBuildDirBasename(self):
+return self.__class__.__name__ + "." + self.testMethodName
+
 # Internal implementation
 
 def BuildMakefile(self):



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

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I need to read this in more detail at this point but I'm caught up in something 
else.

One of the not so great design decisions I made was to try to have "break set" 
be a single command governed by flags.  The way the command turned out, there 
are some flags that tell you what kind of breakpoint you are setting, and then 
other flags that qualify that kind of search.  But that means --file can mean 
"I'm specifying the file to use to set a file & line breakpoint" or it can be a 
narrowing specifier for a source regex or function name based breakpoint.  
Doing it this way makes it hard to have consistent meanings for the flags in 
all contexts.

If I had it to do over, I would make:

(lldb) break set source
(lldb) break set symbol
(lldb) break set source-pattern
(lldb) break set address

etc...  Then the flags could have the meanings appropriate to the breakpoint 
type, and furthermore be documented appropriately.

I don't think we should strain the meaning of a particular flag just to keep it 
consistent with other orthogonal uses.  We should probably start making the 
flag documentation specify "when used for a -n breakpoint, -f means...".  I 
think that would be sufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136



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

[Lldb-commits] [PATCH] D81516: [lldb/Test] Ensure inline tests have a unique build directory

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 3 inline comments as done.
JDevlieghere added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbinline.py:209
 test_class = type(name, (InlineTest,), dict(test=test_func,
-name=name, _build_dict=build_dict))
+name=name, __inline_name__=name, _build_dict=build_dict))
 

labath wrote:
> JDevlieghere wrote:
> > labath wrote:
> > > `__inline_name__` is not a good name according to 
> > > :
> > > ```
> > > __double_leading_and_trailing_underscore__: "magic" objects or attributes 
> > > that live in user-controlled namespaces. E.g. __init__, __import__ or 
> > > __file__. Never invent such names; only use them as documented.
> > > ```
> > > 
> > I did it for consistency with `__no_debug_info_test__`. Based on that PEP 
> > we should rename it to `__no_debug_info_test`?
> I think a single underscore should be enough. I am not sure the mangling 
> magic invoked by a double underscore will  work for this case:
> ```
> Note 2: Name mangling can make certain uses, such as debugging and 
> __getattr__(), less convenient.
> ```
> The PEP is very cautious about (not) encouraging double underscores, and a 
> single leading underscore already says this is private.
Alright, I'll fix that in a follow-up commit. I just wasn't sure if we relied 
on it, but if we do we'll find out ;-)


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

https://reviews.llvm.org/D81516



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

[Lldb-commits] [PATCH] D81612: [lldb/Test] Assert that no targets or global modules remain after a test completes.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D81612#2086939 , @labath wrote:

> I'm pretty indifferent about this functionality -- I don't think it hurts, 
> but it also doesn't seem like a pressing problem that needs addressing.


Yeah. I agree it's not a problem we face right now, but the reproducer scenario 
showed that we can get into a situation where we're not testing what we thing 
we are. I took me and Jim a little time to understand why that was happening 
and I think this is a small price to pay to avoid that in the future and 
guarantee some sanity.

> Regarding the implementation, be aware that assertion failures during test 
> teardown are reproted pretty weirdly -- IIRC at this point the test has 
> already been declared "successful", and these failures manifest as "CLEANUP 
> ERROR"s somewhere. And I have a feeling they don't actually fail the 
> check-lldb command. I don't know if this is due to something we've done, or 
> if it's just how the python unittest framework works...

Okay, I was on the fence between using the unittest2 assert method or the built 
in Python assertion. The latter might be a better fit.


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

https://reviews.llvm.org/D81612



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

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D74136#2087863 , @jingham wrote:

> I need to read this in more detail at this point but I'm caught up in 
> something else.
>
> One of the not so great design decisions I made was to try to have "break 
> set" be a single command governed by flags.  The way the command turned out, 
> there are some flags that tell you what kind of breakpoint you are setting, 
> and then other flags that qualify that kind of search.  But that means --file 
> can mean "I'm specifying the file to use to set a file & line breakpoint" or 
> it can be a narrowing specifier for a source regex or function name based 
> breakpoint.  Doing it this way makes it hard to have consistent meanings for 
> the flags in all contexts.
>
> If I had it to do over, I would make:
>
> (lldb) break set source
>  (lldb) break set symbol
>  (lldb) break set source-pattern
>  (lldb) break set address


Of course, if I were going to do this, I'd choose names that had different 
initial letters, so it really wouldn't have been more onerous to type than the 
current setup, and it would be easy to adjust the "__regex_break" command to 
accommodate...

> etc...  Then the flags could have the meanings appropriate to the breakpoint 
> type, and furthermore be documented appropriately.
> 
> I don't think we should strain the meaning of a particular flag just to keep 
> it consistent with other orthogonal uses.  We should probably start making 
> the flag documentation specify "when used for a -n breakpoint, -f means...".  
> I think that would be sufficient.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136



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

[Lldb-commits] [PATCH] D81001: [lldb] Display autosuggestion part in gray if there is one possible suggestion

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a reviewer: LLDB.
teemperor added a subscriber: lldb-commits.
teemperor added a comment.

I think this is ready to get a review from the rest. I'll add the other LLDB 
folks to the review.




Comment at: lldb/include/lldb/Host/Editline.h:377
   void *m_completion_callback_baton = nullptr;
+  std::string m_add_completion = "";
 

`m_current_autosuggestion` (completions are the thing we do when the user 
presses tab).



Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:353
 
+  void HandleSuggestion(llvm::StringRef line, std::string &result);
+

Some documentation maybe:
```
lang=c++
/// Returns the auto-suggestion string that should be added to the given 
command line.
```



Comment at: lldb/source/Core/CoreProperties.td:136
+Global,
+DefaultTrue,
+Desc<"Whether to show autosuggestions or not.">;

This should be off by default (until the feature is mature somewhen in the 
future enough to be on by default)/



Comment at: lldb/source/Core/CoreProperties.td:137
+DefaultTrue,
+Desc<"Whether to show autosuggestions or not.">;
 }

Usually these settings start like `If true, LLDB will show suggestions on 
possible commands the user might want to type.` or something like that.



Comment at: lldb/source/Core/Debugger.cpp:349
 
+bool Debugger::GetUseAutosuggestion() const {
+  const uint32_t idx = ePropertyShowAutosuggestion;

You declared the function, but you don't use it anywhere. You should move all 
the code you added behind `if (GetShowAutosuggestion())` so that it is only 
active if the user activated the setting (by doing `settings set 
show-autosuggestion true`).



Comment at: lldb/source/Host/common/Editline.cpp:1244
+llvm::StringRef indent_chars =
+"abcdefghijklmnopqrstuvwxzyABCDEFGHIJKLMNOPQRSTUVWXZY1234567890 ";
+for (char c : indent_chars) {

`.-/()[]{};\"'\\!@#$%^&*_` are missing here. Maybe we should just iterate over 
all ASCII characters and add all printables except newline or something like 
that to the alphabet.



Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1869
 
+void CommandInterpreter::HandleSuggestion(llvm::StringRef line,
+  std::string &result) {

gedatsu217 wrote:
> teemperor wrote:
> > This could just return a `std::string`. Even better, it could return a 
> > `llvm::Optional` to better mark when no suggestion could be 
> > found (and you can return `return llvm::None` to return an empty 
> > llvm::Optional value to indicate no return value)
> Does this mean that this function should return llvm::Optional 
> instead of void? I do not know what the intent is.
> I personally think either way makes sense.
It just makes the API hard to use incorrectly.

E.g., without knowing how HandleSuggestion is implemented, which of the 
following calls is correct or incorrect?

```
std::string line = GetLine();
std::string result;
interpreter->HandleSuggestion(line, result); // is this correct?
interpreter->HandleSuggestion(result, line); // or is this correct?
```

Versus:

```
std::string line = GetLine();
std::string result = interpreter->HandleSuggestion(line); // can't call this 
function in the wrong way
```

The `llvm::Optional` would make it more obvious what the function returns in 
case there is no suggestion.

I think a name like "GetAutoSuggestionForCommand" or something like that would 
make it more obvious what this is doing.



Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1875
+if (entry.startswith(line)) {
+  auto res = entry.substr(line.size());
+  result = res.str();

`auto` -> `llvm::StringRef` (it's short enough of a name).


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

https://reviews.llvm.org/D81001



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

[Lldb-commits] [PATCH] D81516: [lldb/Test] Ensure inline tests have a unique build directory

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Closed by commit rG5a33ba52b66c: [lldb/Test] Ensure inline tests have a unique 
build directory (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81516

Files:
  lldb/packages/Python/lldbsuite/test/lldbinline.py


Index: lldb/packages/Python/lldbsuite/test/lldbinline.py
===
--- lldb/packages/Python/lldbsuite/test/lldbinline.py
+++ lldb/packages/Python/lldbsuite/test/lldbinline.py
@@ -82,6 +82,11 @@
 
 
 class InlineTest(TestBase):
+# Overrides
+
+def getBuildDirBasename(self):
+return self.__class__.__name__ + "." + self.testMethodName
+
 # Internal implementation
 
 def BuildMakefile(self):


Index: lldb/packages/Python/lldbsuite/test/lldbinline.py
===
--- lldb/packages/Python/lldbsuite/test/lldbinline.py
+++ lldb/packages/Python/lldbsuite/test/lldbinline.py
@@ -82,6 +82,11 @@
 
 
 class InlineTest(TestBase):
+# Overrides
+
+def getBuildDirBasename(self):
+return self.__class__.__name__ + "." + self.testMethodName
+
 # Internal implementation
 
 def BuildMakefile(self):
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum marked 9 inline comments as done.
fallkrum added inline comments.



Comment at: lldb/source/Target/Process.cpp:3977
+} else {
+  /*
+   For the sake of logical consistency. For example we have only

jingham wrote:
> I'm not entirely sure about this part.  Setting the "have_valid_stopinfo_ptr 
> would only matter if we stopped and no non-suspended thread had a valid stop 
> reason.  That's really only going to happen because there was a bug in the 
> stub, but when this happens we really can't figure out what to do.  The 
> suspended thread's StopInfo isn't going to help us because it is stale by now.
> 
> I think the right thing to do in this case is say nobody had an opinion, and 
> let the upper layers deal with whether they want to ignore a seemingly 
> spurious stop, or stop and let the user decide what to do.
Removed, will return false. 



Comment at: 
lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py:46
+#The breakpoint list should show 1 locations.
+self.expect(
+"breakpoint list -f",

jingham wrote:
> What you are testing in this `self.expect` is already all tested by 
> run_break_set_by_file_and_line.  I don't think you need to repeat it.  If you 
> want to assert that the breakpoint was set exactly on the line number 
> requested, just pass `loc_exact = True` as well as num_expected_locations.
I'v copied it from another test. Have no need in this, removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112



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

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum updated this revision to Diff 270176.
fallkrum added a comment.

Removed _ptr from found_valid_stopinfo_ptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Process.cpp
  lldb/test/API/functionalities/thread/ignore_suspended/Makefile
  
lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py
  lldb/test/API/functionalities/thread/ignore_suspended/main.cpp
  lldb/unittests/CMakeLists.txt
  lldb/unittests/Process/CMakeLists.txt
  lldb/unittests/Process/ProcessEventDataTest.cpp
  lldb/unittests/Thread/CMakeLists.txt
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- /dev/null
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -0,0 +1,168 @@
+//===-- ThreadTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/Thread.h"
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/StopInfo.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Reproducer.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class ThreadTest : public ::testing::Test {
+public:
+  void SetUp() override {
+llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None));
+FileSystem::Initialize();
+HostInfo::Initialize();
+platform_linux::PlatformLinux::Initialize();
+  }
+  void TearDown() override {
+platform_linux::PlatformLinux::Terminate();
+HostInfo::Terminate();
+FileSystem::Terminate();
+Reproducer::Terminate();
+  }
+};
+
+class DummyProcess : public Process {
+public:
+  using Process::Process;
+
+  virtual bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) {
+return true;
+  }
+  virtual Status DoDestroy() { return {}; }
+  virtual void RefreshStateAfterStop() {}
+  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+  Status &error) {
+return 0;
+  }
+  virtual bool UpdateThreadList(ThreadList &old_thread_list,
+ThreadList &new_thread_list) {
+return false;
+  }
+  virtual ConstString GetPluginName() { return ConstString("Dummy"); }
+  virtual uint32_t GetPluginVersion() { return 0; }
+
+  ProcessModID &GetModIDNonConstRef() { return m_mod_id; }
+};
+
+class DummyThread : public Thread {
+public:
+  using Thread::Thread;
+
+  ~DummyThread() override { DestroyThread(); }
+
+  void RefreshStateAfterStop() override {}
+
+  lldb::RegisterContextSP GetRegisterContext() override { return nullptr; }
+
+  lldb::RegisterContextSP
+  CreateRegisterContextForFrame(StackFrame *frame) override {
+return nullptr;
+  }
+
+  bool CalculateStopInfo() override { return false; }
+
+  bool IsStillAtLastBreakpointHit() override { return true; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  Status error;
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  error = debugger_sp->GetTargetList().CreateTarget(
+  *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+
+  if (target_sp) {
+debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+  }
+
+  return target_sp;
+}
+
+TEST_F(ThreadTest, SetStopInfo) {
+  ArchSpec arch("powerpc64-pc-linux");
+
+  Platform::SetHostPlatform(
+  platform_linux::PlatformLinux::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast(process_sp.get());
+
+  ThreadSP thread_sp = std::make_shared(*process_sp.get(), 0);
+  ASSERT_TRUE(thread_sp);
+
+  StopInfoSP stopinfo_sp =
+  StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp.get(), 0);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+
+  /*
+   Should make stopinfo valid.
+   */
+  process->GetModIDNonConstRef().BumpStopID();
+  ASSERT_TRUE(stopinfo_sp->IsValid() == false);
+
+  thread_sp->SetStopInfo(stopinfo_sp);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+}
+
+TEST_F(ThreadTest, GetPrivateStopInfo) {
+  ArchSpec arch("powerpc64-p

[Lldb-commits] [PATCH] D81589: [lldb/SymbolFile] Don't parse the whole line table for the support files

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 270184.
JDevlieghere retitled this revision from "[lldb/SymbolFile] Don't parse the 
whole line table for the support files (WIP)" to "[lldb/SymbolFile] Don't parse 
the whole line table for the support files".
JDevlieghere edited the summary of this revision.
JDevlieghere edited reviewers, added: labath, jankratochvil; removed: jdoerfert.
JDevlieghere added a comment.
Herald added a subscriber: aprantl.
Herald added a reviewer: jdoerfert.

Use `GetLineTableOffset()`


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

https://reviews.llvm.org/D81589

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -174,6 +174,33 @@
   return *line_table;
 }
 
+static bool ParseLLVMLineTablePrologue(lldb_private::DWARFContext &context,
+   llvm::DWARFDebugLine::Prologue 
&prologue,
+   dw_offset_t line_offset,
+   dw_offset_t unit_offset) {
+  Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_INFO);
+  bool success = true;
+  llvm::DWARFDataExtractor data = context.getOrLoadLineData().GetAsLLVM();
+  llvm::DWARFContext &ctx = context.GetAsLLVM();
+  uint64_t offset = line_offset;
+  llvm::Error error = prologue.parse(
+  data, &offset,
+  [&](llvm::Error e) {
+success = false;
+LLDB_LOG_ERROR(log, std::move(e),
+   "SymbolFileDWARF::ParseSupportFiles failed to parse "
+   "line table prologue");
+  },
+  ctx, nullptr);
+  if (error) {
+LLDB_LOG_ERROR(log, std::move(error),
+   "SymbolFileDWARF::ParseSupportFiles failed to parse line "
+   "table prologue");
+return false;
+  }
+  return success;
+}
+
 static llvm::Optional
 GetFileByIndex(const llvm::DWARFDebugLine::Prologue &prologue, size_t idx,
llvm::StringRef compile_dir, FileSpec::Style style) {
@@ -854,8 +881,24 @@
 
 bool SymbolFileDWARF::ParseSupportFiles(CompileUnit &comp_unit,
 FileSpecList &support_files) {
-  if (!comp_unit.GetLineTable())
-ParseLineTable(comp_unit);
+  std::lock_guard guard(GetModuleMutex());
+  DWARFUnit *dwarf_cu = GetDWARFCompileUnit(&comp_unit);
+  if (!dwarf_cu)
+return false;
+
+  dw_offset_t offset = dwarf_cu->GetLineTableOffset();
+  if (offset == DW_INVALID_OFFSET)
+return false;
+
+  llvm::DWARFDebugLine::Prologue prologue;
+  if (!ParseLLVMLineTablePrologue(m_context, prologue, offset,
+  dwarf_cu->GetOffset()))
+return false;
+
+  comp_unit.SetSupportFiles(ParseSupportFilesFromPrologue(
+  comp_unit.GetModule(), prologue, dwarf_cu->GetPathStyle(),
+  dwarf_cu->GetCompilationDirectory().GetCString()));
+
   return true;
 }
 
@@ -978,18 +1021,13 @@
   if (!dwarf_cu)
 return false;
 
-  const DWARFBaseDIE dwarf_cu_die = dwarf_cu->GetUnitDIEOnly();
-  if (!dwarf_cu_die)
-return false;
-
-  const dw_offset_t cu_line_offset = dwarf_cu_die.GetAttributeValueAsUnsigned(
-  DW_AT_stmt_list, DW_INVALID_OFFSET);
-  if (cu_line_offset == DW_INVALID_OFFSET)
+  dw_offset_t offset = dwarf_cu->GetLineTableOffset();
+  if (offset == DW_INVALID_OFFSET)
 return false;
 
   llvm::DWARFDebugLine line;
-  const llvm::DWARFDebugLine::LineTable *line_table = ParseLLVMLineTable(
-  m_context, line, cu_line_offset, dwarf_cu->GetOffset());
+  const llvm::DWARFDebugLine::LineTable *line_table =
+  ParseLLVMLineTable(m_context, line, offset, dwarf_cu->GetOffset());
 
   if (!line_table)
 return false;
@@ -1024,10 +1062,6 @@
 comp_unit.SetLineTable(line_table_up.release());
   }
 
-  comp_unit.SetSupportFiles(ParseSupportFilesFromPrologue(
-  comp_unit.GetModule(), line_table->Prologue, dwarf_cu->GetPathStyle(),
-  dwarf_cu->GetCompilationDirectory().GetCString()));
-
   return true;
 }
 


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -174,6 +174,33 @@
   return *line_table;
 }
 
+static bool ParseLLVMLineTablePrologue(lldb_private::DWARFContext &context,
+   llvm::DWARFDebugLine::Prologue &prologue,
+   dw_offset_t line_offset,
+   dw_offset_t unit_offset) {
+  Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_INFO);
+  bool success = true;
+  llvm::DWARFDataExtractor data = context.getOrLoadLineData().GetAsLLVM();
+  llvm::DWARFContext &ctx 

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Jim Ingham via Phabricator via lldb-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112



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

[Lldb-commits] [PATCH] D81589: [lldb/SymbolFile] Don't parse the whole line table for the support files

[Greg Clayton via Phabricator via lldb-commits]

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

Nice! Our DWARFUnit::GetLineTableOffset() isn't adding some extra offset to the 
line table offset that the LLVM parser is adding, but we can do this in a 
separate patch.


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

https://reviews.llvm.org/D81589



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

[Lldb-commits] [PATCH] D81200: [vscode] set default values for terminateDebuggee for the disconnect request

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:796
+if (!error.Success())
+  response.try_emplace("error", error.GetCString());
 g_vsc.debugger.SetAsync(true);

I am not sure what returning an error to disconnect will do other than hosing 
the IDE in some way. It really doesn't matter if we succeeded, we should 
probably just not mess with the return value. Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81200



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

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Follow DW_AT_decl_file when setting breakpoint

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

I would separate/remove all [nfc] changes from this patch as they complicate it 
a bit. Such cleaned up patch I have prepared for myself: 
https://people.redhat.com/jkratoch/D74136-cleanup.patch

Then also did you notice there is a regression for (I do not know why):

  FAIL: LLDB 
(/home/jkratoch/redhat/llvm-monorepo-clangassert/bin/clang-11-x86_64) :: 
test_scripted_resolver (TestScriptedResolver.TestScriptedResolver)
File 
"/home/jkratoch/redhat/llvm-monorepo/lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py",
 line 23, in test_scripted_resolver
  self.do_test()
File 
"/home/jkratoch/redhat/llvm-monorepo/lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py",
 line 152, in do_test
  self.assertEqual(wrong[i].GetNumLocations(), 0, "Breakpoint %d has 
locations."%(i))
  AssertionError: 1 != 0 : Breakpoint 1 has locations.




Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:292
+filter_by_function), // include symbols only if we
+ // aren't filtering by CU or function
   include_inlines, func_list);

[nfc] It could use `include_symbols` here.




Comment at: lldb/source/Core/SearchFilter.cpp:713
+  if (!type)
+return SearchFilterByModuleList::FunctionPasses(function);
+

If we cannot determine which file the function is from then rather ignore it 
(the current call returns `true`):
```
  return false;
```



Comment at: lldb/source/Core/SearchFilter.cpp:726
+  if (m_target_sp &&
+  m_target_sp->GetInlineStrategy() == eInlineBreakpointsHeaders) {
+if (!sym_ctx.comp_unit) {

This value should be cached.
But besides that there should be `GetInlineStrategy() == 
eInlineBreakpointsNever || (GetInlineStrategy() == eInlineBreakpointsHeaders && 
cu_spec.IsSourceImplementationFile()modulosomeReverseRemapPath?`.
As for `eInlineBreakpointsAlways` the check is being delegated to 
`FunctionPasses()`.




Comment at: lldb/source/Core/SearchFilter.cpp:732
+FileSpec cu_spec;
+if (sym_ctx.comp_unit) {
+  cu_spec = sym_ctx.comp_unit->GetPrimaryFile();

This condition is always `true` as there is already above:
```
if (!sym_ctx.comp_unit)
```




Comment at: lldb/source/Core/SearchFilter.cpp:835
+  if (m_target_sp &&
+  m_target_sp->GetInlineStrategy() == eInlineBreakpointsHeaders)
+return flags | eSymbolContextCompUnit;

Here should be `eInlineBreakpointsNever || eInlineBreakpointsHeaders` (and 
probably cache the `GetInlineStrategy()` value).
For `eInlineBreakpointsHeaders` we do not know for sure as we cannot call 
`IsSourceImplementationFile()` for `cu_spec` which is not known yet here.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74136



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

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum added a comment.

It says "This revision is now accepted and ready to land." If I understand 
correctly I should be able to merge it somehow to the repository. Do not see 
how to do it though... Could you help to figure it out?
Also very unclear why Harbormaster always fails with errors like:

> [2020-06-11T18:15:11.019Z] Error while processing 
> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/include/lldb/Target/Process.h.
> [2020-06-11T18:15:11.019Z] 
> [2020-06-11T18:15:11.988Z] 480 warnings and 1 error generated.
> [2020-06-11T18:15:11.988Z] Error while processing 
> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/source/Target/Process.cpp.
> [2020-06-11T18:15:11.988Z] 
> [2020-06-11T18:15:12.555Z] 442 warnings and 1 error generated.
> [2020-06-11T18:15:12.555Z] Error while processing 
> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/unittests/Process/ProcessEventDataTest.cpp.
> [2020-06-11T18:15:12.555Z] 
> [2020-06-11T18:15:13.122Z] 442 warnings and 1 error generated.
> [2020-06-11T18:15:13.122Z] Error while processing 
> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/unittests/Thread/ThreadTest.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112



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

[Lldb-commits] [lldb] 1c03389 - Re-land "Migrate the rest of COFFObjectFile to Error"

[Reid Kleckner via lldb-commits]

Author: Reid Kleckner
Date: 2020-06-11T14:46:16-07:00
New Revision: 1c03389c29f32cce81a642365c484c71aba1a1cb

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

LOG: Re-land "Migrate the rest of COFFObjectFile to Error"

This reverts commit 101fbc01382edd89ea7b671104c68b30b2446cc0.

Remove leftover debugging attribute.

Update LLDB as well, which was missed before.

Added: 


Modified: 
lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
llvm/include/llvm/Object/COFF.h
llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp
llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp
llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.h
llvm/lib/DebugInfo/Symbolize/Symbolize.cpp
llvm/lib/Object/COFFObjectFile.cpp
llvm/tools/llvm-objcopy/COFF/Reader.cpp
llvm/tools/llvm-objdump/COFFDump.cpp
llvm/tools/llvm-objdump/llvm-objdump.cpp
llvm/tools/llvm-readobj/COFFDumper.cpp
llvm/tools/obj2yaml/coff2yaml.cpp
llvm/tools/sancov/sancov.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp 
b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
index fe567187e2b4..0a5a1d318fbe 100644
--- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -969,13 +969,12 @@ uint32_t ObjectFilePECOFF::ParseDependentModules() {
 
   for (const auto &entry : COFFObj->import_directories()) {
 llvm::StringRef dll_name;
-auto ec = entry.getName(dll_name);
 // Report a bogus entry.
-if (ec != std::error_code()) {
+if (llvm::Error e = entry.getName(dll_name)) {
   LLDB_LOGF(log,
 "ObjectFilePECOFF::ParseDependentModules() - failed to get "
 "import directory entry name: %s",
-ec.message().c_str());
+llvm::toString(std::move(e)).c_str());
   continue;
 }
 

diff  --git a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp 
b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
index 2f8b1dcc9135..cce06473d92f 100644
--- a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -129,14 +129,15 @@ loadMatchingPDBFile(std::string exe_path, 
llvm::BumpPtrAllocator &allocator) {
 
   // If it doesn't have a debug directory, fail.
   llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
+  if (llvm::Error e = obj->getDebugPDBInfo(pdb_info, pdb_file)) {
+consumeError(std::move(e));
 return nullptr;
+  }
 
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
   llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(std::string(pdb_file), allocator);

diff  --git a/llvm/include/llvm/Object/COFF.h b/llvm/include/llvm/Object/COFF.h
index a8455e02563f..8aef00a8809d 100644
--- a/llvm/include/llvm/Object/COFF.h
+++ b/llvm/include/llvm/Object/COFF.h
@@ -799,13 +799,13 @@ class COFFObjectFile : public ObjectFile {
   // Finish initializing the object and return success or an error.
   Error initialize();
 
-  std::error_code initSymbolTablePtr();
-  std::error_code initImportTablePtr();
-  std::error_code initDelayImportTablePtr();
-  std::error_code initExportTablePtr();
-  std::error_code initBaseRelocPtr();
-  std::error_code initDebugDirectoryPtr();
-  std::error_code initLoadConfigPtr();
+  Error initSymbolTablePtr();
+  Error initImportTablePtr();
+  Error initDelayImportTablePtr();
+  Error initExportTablePtr();
+  Error initBaseRelocPtr();
+  Error initDebugDirectoryPtr();
+  Error initLoadConfigPtr();
 
 public:
   static Expected>
@@ -989,8 +989,7 @@ class COFFObjectFile : public ObjectFile {
   const pe32_header *getPE32Header() const { return PE32Header; }
   const pe32plus_header *getPE32PlusHeader() const { return PE32PlusHeader; }
 
-  std::error_code getDataDirectory(uint32_t index,
-   const data_directory *&Res) const;
+  const data_directory *getDataDirectory(uint32_t index) const;
   Expected getSection(int32_t index) const;
 
   Expected getSymbol(uint32_t index) const {
@@ -1004,12 +1003,12 @@ class COFFObjectFile : public ObjectFile {
   }
 
   template 
-  std::error_code getAuxSymbol(uint32_t index, const T *&Res) const {
+  Error getAuxSymbol(uint32_t index, const T *&Res) const {
 Expected S = getSymbol(index);
 if (Error E = S.takeError())
-  return errorToErrorCode(std::move(E));
+ 

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D80112#2088504 , @fallkrum wrote:

> It says "This revision is now accepted and ready to land." If I understand 
> correctly I should be able to merge it somehow to the repository. Do not see 
> how to do it though... Could you help to figure it out?


Do you have commit access 
? If not 
you can ask someone to commit if for you 
,
 Jim or I would be happy to do so.

> Also very unclear why Harbormaster always fails with errors like:
> 
>> [2020-06-11T18:15:11.019Z] Error while processing 
>> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/include/lldb/Target/Process.h.
>> [2020-06-11T18:15:11.019Z] 
>> [2020-06-11T18:15:11.988Z] 480 warnings and 1 error generated.
>> [2020-06-11T18:15:11.988Z] Error while processing 
>> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/source/Target/Process.cpp.
>> [2020-06-11T18:15:11.988Z] 
>> [2020-06-11T18:15:12.555Z] 442 warnings and 1 error generated.
>> [2020-06-11T18:15:12.555Z] Error while processing 
>> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/unittests/Process/ProcessEventDataTest.cpp.
>> [2020-06-11T18:15:12.555Z] 
>> [2020-06-11T18:15:13.122Z] 442 warnings and 1 error generated.
>> [2020-06-11T18:15:13.122Z] Error while processing 
>> /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/lldb/unittests/Thread/ThreadTest.cpp.

I wouldn't worry about it. Pre-commit testing is still in beta and I think you 
need to use the arc tool for it to work. I'm also not sure if LLDB has been 
completely integrated yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112



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

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum added a comment.

It says to provide the name and e-mail:
Ilya Bukonkin fallk...@yahoo.com


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112



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

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Ilya Bukonkin via Phabricator via lldb-commits]

fallkrum added a comment.

@JDevlieghere thanks for help. No, I have no commit access, please do it for 
me. 
Should I request the access now or you will grant it in the future?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112



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

[Lldb-commits] [lldb] 3b43f00 - [lldb] Check if thread was suspended during previous stop added.

[Jonas Devlieghere via lldb-commits]

Author: Ilya Bukonkin
Date: 2020-06-11T15:02:46-07:00
New Revision: 3b43f006294971b8049d4807110032169780e5b8

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

LOG: [lldb] Check if thread was suspended during previous stop added.

Encountered the following situation: Let we started thread T1 and it hit
breakpoint on B1 location. We suspended T1 and continued the process.
Then we started thread T2 which hit for example the same location B1.
This time in a breakpoint callback we decided not to stop returning
false.

Expected result: process continues (as if T2 did not hit breakpoint) its
workflow with T1 still suspended. Actual result: process do stops (as if
T2 callback returned true).

Solution: We need invalidate StopInfo for threads that was previously
suspended just because something that is already inactive can not be the
reason of stop. Thread::GetPrivateStopInfo() may be appropriate place to
do it, because it gets called (through Thread::GetStopInfo()) every time
before process reports stop and user gets chance to change
m_resume_state again i.e if we see m_resume_state == eStateSuspended
it definitely means it was set during previous stop and it also means
this thread can not be stopped again (cos' it was frozen during
previous stop).

Differential revision: https://reviews.llvm.org/D80112

Added: 
lldb/test/API/functionalities/thread/ignore_suspended/Makefile

lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py
lldb/test/API/functionalities/thread/ignore_suspended/main.cpp
lldb/unittests/Process/ProcessEventDataTest.cpp
lldb/unittests/Thread/CMakeLists.txt
lldb/unittests/Thread/ThreadTest.cpp

Modified: 
lldb/include/lldb/Target/Process.h
lldb/source/Target/Process.cpp
lldb/unittests/CMakeLists.txt
lldb/unittests/Process/CMakeLists.txt

Removed: 




diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index 700f74f26958..01e2a0d77111 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -452,6 +452,8 @@ class Process : public 
std::enable_shared_from_this,
 
 void Dump(Stream *s) const override;
 
+virtual bool ShouldStop(Event *event_ptr, bool &found_valid_stopinfo);
+
 void DoOnRemoval(Event *event_ptr) override;
 
 static const Process::ProcessEventData *

diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 25a940b66430..9cb38556a6ec 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -3998,6 +3998,114 @@ ConstString Process::ProcessEventData::GetFlavor() 
const {
   return ProcessEventData::GetFlavorString();
 }
 
+bool Process::ProcessEventData::ShouldStop(Event *event_ptr,
+   bool &found_valid_stopinfo) {
+  found_valid_stopinfo = false;
+
+  ProcessSP process_sp(m_process_wp.lock());
+  if (!process_sp)
+return false;
+
+  ThreadList &curr_thread_list = process_sp->GetThreadList();
+  uint32_t num_threads = curr_thread_list.GetSize();
+  uint32_t idx;
+
+  // The actions might change one of the thread's stop_info's opinions about
+  // whether we should stop the process, so we need to query that as we go.
+
+  // One other complication here, is that we try to catch any case where the
+  // target has run (except for expressions) and immediately exit, but if we
+  // get that wrong (which is possible) then the thread list might have
+  // changed, and that would cause our iteration here to crash.  We could
+  // make a copy of the thread list, but we'd really like to also know if it
+  // has changed at all, so we make up a vector of the thread ID's and check
+  // what we get back against this list & bag out if anything 
diff ers.
+  ThreadList not_suspended_thread_list(process_sp.get());
+  std::vector thread_index_array(num_threads);
+  uint32_t not_suspended_idx = 0;
+  for (idx = 0; idx < num_threads; ++idx) {
+lldb::ThreadSP thread_sp = curr_thread_list.GetThreadAtIndex(idx);
+
+/*
+ Filter out all suspended threads, they could not be the reason
+ of stop and no need to perform any actions on them.
+ */
+if (thread_sp->GetResumeState() != eStateSuspended) {
+  not_suspended_thread_list.AddThread(thread_sp);
+  thread_index_array[not_suspended_idx] = thread_sp->GetIndexID();
+  not_suspended_idx++;
+}
+  }
+
+  // Use this to track whether we should continue from here.  We will only
+  // continue the target running if no thread says we should stop.  Of course
+  // if some thread's PerformAction actually sets the target running, then it
+  // doesn't matter what the other threads say...
+
+  bool still_should_stop = false;
+
+  // Sometimes - for instance if we ha

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D80112#2088585 , @fallkrum wrote:

> It says to provide the name and e-mail:
>  Ilya Bukonkin fallk...@yahoo.com


Thanks, committed! I took the liberty of running clang-format over the changes 
and fixing some trailing whitespace issues.

In D80112#2088583 , @fallkrum wrote:

> @JDevlieghere thanks for help. No, I have no commit access, please do it for 
> me.


The review will be automatically updated once Phabricator imports the commit.

> Should I request the access now or you will grant it in the future?

You'll have to e-mail Chris, I don't have that power :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112



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

[Lldb-commits] [PATCH] D81200: [vscode] set default values for terminateDebuggee for the disconnect request

[walter erquinigo via Phabricator via lldb-commits]

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



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:796
+if (!error.Success())
+  response.try_emplace("error", error.GetCString());
 g_vsc.debugger.SetAsync(true);

clayborg wrote:
> I am not sure what returning an error to disconnect will do other than hosing 
> the IDE in some way. It really doesn't matter if we succeeded, we should 
> probably just not mess with the return value. Thoughts?
This is actually quite useful when analyzing logs, as we could know if any 
disconnection ended up in an internal failure. 
This is safe, as I've tried returning a hardcoded error in this case and the 
IDE terminated the debug session gracefully anyway. The error field is accepted 
by the protocol, btw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81200



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

[Lldb-commits] [PATCH] D80112: Check if thread was suspended during previous stop added.

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3b43f0062949: [lldb] Check if thread was suspended during 
previous stop added. (authored by fallkrum, committed by JDevlieghere).

Changed prior to commit:
  https://reviews.llvm.org/D80112?vs=270176&id=270247#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80112

Files:
  lldb/include/lldb/Target/Process.h
  lldb/source/Target/Process.cpp
  lldb/test/API/functionalities/thread/ignore_suspended/Makefile
  
lldb/test/API/functionalities/thread/ignore_suspended/TestIgnoreSuspendedThread.py
  lldb/test/API/functionalities/thread/ignore_suspended/main.cpp
  lldb/unittests/CMakeLists.txt
  lldb/unittests/Process/CMakeLists.txt
  lldb/unittests/Process/ProcessEventDataTest.cpp
  lldb/unittests/Thread/CMakeLists.txt
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- /dev/null
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -0,0 +1,168 @@
+//===-- ThreadTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/Thread.h"
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/StopInfo.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Reproducer.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class ThreadTest : public ::testing::Test {
+public:
+  void SetUp() override {
+llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None));
+FileSystem::Initialize();
+HostInfo::Initialize();
+platform_linux::PlatformLinux::Initialize();
+  }
+  void TearDown() override {
+platform_linux::PlatformLinux::Terminate();
+HostInfo::Terminate();
+FileSystem::Terminate();
+Reproducer::Terminate();
+  }
+};
+
+class DummyProcess : public Process {
+public:
+  using Process::Process;
+
+  virtual bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) {
+return true;
+  }
+  virtual Status DoDestroy() { return {}; }
+  virtual void RefreshStateAfterStop() {}
+  virtual size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+  Status &error) {
+return 0;
+  }
+  virtual bool UpdateThreadList(ThreadList &old_thread_list,
+ThreadList &new_thread_list) {
+return false;
+  }
+  virtual ConstString GetPluginName() { return ConstString("Dummy"); }
+  virtual uint32_t GetPluginVersion() { return 0; }
+
+  ProcessModID &GetModIDNonConstRef() { return m_mod_id; }
+};
+
+class DummyThread : public Thread {
+public:
+  using Thread::Thread;
+
+  ~DummyThread() override { DestroyThread(); }
+
+  void RefreshStateAfterStop() override {}
+
+  lldb::RegisterContextSP GetRegisterContext() override { return nullptr; }
+
+  lldb::RegisterContextSP
+  CreateRegisterContextForFrame(StackFrame *frame) override {
+return nullptr;
+  }
+
+  bool CalculateStopInfo() override { return false; }
+
+  bool IsStillAtLastBreakpointHit() override { return true; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  Status error;
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  error = debugger_sp->GetTargetList().CreateTarget(
+  *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+
+  if (target_sp) {
+debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+  }
+
+  return target_sp;
+}
+
+TEST_F(ThreadTest, SetStopInfo) {
+  ArchSpec arch("powerpc64-pc-linux");
+
+  Platform::SetHostPlatform(
+  platform_linux::PlatformLinux::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast(process_sp.get());
+
+  ThreadSP thread_sp = std::make_shared(*process_sp.get(), 0);
+  ASSERT_TRUE(thread_sp);
+
+  StopInfoSP stopinfo_sp =
+  StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp.get(), 0);
+  ASSERT_TRUE(stopinfo_sp->IsValid() == true);
+
+  /*
+   Should make stopinfo valid.
+   */
+  process->GetModIDNonConstRef().BumpStopID();
+  ASSERT_TRUE(stopinfo_sp->Is

[Lldb-commits] [lldb] 8d8ec55 - [lldb/Test] Unify DYLD_INSERT_LIBRARIES solution for ASan and TSan

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-06-11T16:04:46-07:00
New Revision: 8d8ec55035bda2cff5ef446cfacec93c67665c52

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

LOG: [lldb/Test] Unify DYLD_INSERT_LIBRARIES solution for ASan and TSan

Add the same fix for loading the sanitizer runtime for TSan as we
currently have for ASan and unify the code with a helper function.

Added: 


Modified: 
lldb/test/API/lit.cfg.py

Removed: 




diff  --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index f0cfe69511ac..288d667c18dd 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -20,17 +20,13 @@
 config.test_source_root = os.path.dirname(__file__)
 config.test_exec_root = config.test_source_root
 
-if 'Address' in config.llvm_use_sanitizer:
-  config.environment['ASAN_OPTIONS'] = 'detect_stack_use_after_return=1'
-  # macOS flags needed for LLDB built with address sanitizer.
-  if 'Darwin' in config.host_os and 'x86' in config.host_triple:
-import subprocess
-resource_dir = subprocess.check_output(
-[config.cmake_cxx_compiler,
- '-print-resource-dir']).decode('utf-8').strip()
-runtime = os.path.join(resource_dir, 'lib', 'darwin',
-   'libclang_rt.asan_osx_dynamic.dylib')
-config.environment['DYLD_INSERT_LIBRARIES'] = runtime
+
+def find_sanitizer_runtime(name):
+  import subprocess
+  resource_dir = subprocess.check_output(
+  [config.cmake_cxx_compiler,
+   '-print-resource-dir']).decode('utf-8').strip()
+  return os.path.join(resource_dir, 'lib', 'darwin', name)
 
 
 def find_shlibpath_var():
@@ -42,6 +38,17 @@ def find_shlibpath_var():
 yield 'PATH'
 
 
+if 'Address' in config.llvm_use_sanitizer:
+  config.environment['ASAN_OPTIONS'] = 'detect_stack_use_after_return=1'
+  if 'Darwin' in config.host_os and 'x86' in config.host_triple:
+config.environment['DYLD_INSERT_LIBRARIES'] = find_sanitizer_runtime(
+'libclang_rt.asan_osx_dynamic.dylib')
+
+if 'Thread' in config.llvm_use_sanitizer:
+  if 'Darwin' in config.host_os and 'x86' in config.host_triple:
+config.environment['DYLD_INSERT_LIBRARIES'] = find_sanitizer_runtime(
+'libclang_rt.tsan_osx_dynamic.dylib')
+
 # Shared library build of LLVM may require LD_LIBRARY_PATH or equivalent.
 if config.shared_libs:
   for shlibpath_var in find_shlibpath_var():



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

[Lldb-commits] [PATCH] D81697: Add support for batch-testing to the LLDB testsuite.

[Benson Li via Phabricator via lldb-commits]

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

[LLDB] Hi, this is a patch for the proposed GSoC project "Add support for 
batch-testing to the LLDB testsuite". The project aimed to add continuation 
functionality when multiple assertions are called one after another, similar to 
how GoogleTest's EXPECT_* macros work. Test results from running "ninja 
check-lldb" were the same before and after the patch, and I have included a 
test case for the new functionality.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81697

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/test/API/test_batch/TestBatch.py
  lldb/third_party/Python/module/unittest2/unittest2/case.py
  lldb/third_party/Python/module/unittest2/unittest2/result.py

Index: lldb/third_party/Python/module/unittest2/unittest2/result.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/result.py
+++ lldb/third_party/Python/module/unittest2/unittest2/result.py
@@ -58,6 +58,7 @@
 self._original_stdout = sys.stdout
 self._original_stderr = sys.stderr
 self._mirrorOutput = False
+self.batchSuccess = True
 
 def startTest(self, test):
 "Called when the given test is about to be run"
Index: lldb/third_party/Python/module/unittest2/unittest2/case.py
===
--- lldb/third_party/Python/module/unittest2/unittest2/case.py
+++ lldb/third_party/Python/module/unittest2/unittest2/case.py
@@ -234,6 +234,10 @@
 """
 self._testMethodName = methodName
 self._resultForDoCleanups = None
+
+self._batch = False
+self._result = None
+
 try:
 testMethod = getattr(self, methodName)
 except AttributeError:
@@ -410,6 +414,8 @@
 SB objects and prevent them from being deleted in time.
 """
 try:
+self._result = result
+self._result.batchSuccess = True # Just in case
 testMethod()
 except self.failureException:
 result.addFailure(self, sys.exc_info())
@@ -437,7 +443,8 @@
 except Exception:
 result.addError(self, sys.exc_info())
 else:
-return True
+return self._result.batchSuccess
+self._result = None
 return False
 
 def doCleanups(self):
Index: lldb/test/API/test_batch/TestBatch.py
===
--- /dev/null
+++ lldb/test/API/test_batch/TestBatch.py
@@ -0,0 +1,31 @@
+from lldbsuite.test.lldbtest import *
+
+class TestBatch(TestBase):
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+x = 0
+
+def test_batch(self):
+self.enter_batch()
+
+self.assertTrue(False)
+self.assertTrue(False)
+
+self.assertTrue(True)
+
+self.exit_batch()
+
+def test_context_manager(self):
+with self.batchTest():
+self.x += 1
+self.assertFalse(True)
+self.assertFalse(True)
+self.assertEqual(self.x, 1)
+
+def test_early_exit(self):
+with self.batchTest():
+self.assertFalse(True)
+self.assertFalse(False)
+raise IndexError
+self.assertTrue(False)
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -50,6 +50,7 @@
 import time
 import traceback
 import distutils.spawn
+import contextlib
 
 # Third-party modules
 import unittest2
@@ -1850,6 +1851,285 @@
 # Can be overridden by the LLDB_TIME_WAIT_NEXT_LAUNCH environment variable.
 timeWaitNextLaunch = 1.0
 
+def enter_batch(self):
+self._batch = True
+
+def exit_batch(self):
+self._batch = False
+
+@contextlib.contextmanager
+def batchTest(self):
+try:
+self.enter_batch()
+yield
+finally:
+self.exit_batch()
+
+# Assertion Overides
+def assertTrue(self, expr, msg=None):
+if self._result is not None and self._batch:
+try:
+return super(TestBase,self).assertTrue(expr, msg)
+except self.failureException:
+self._result.addFailure(self, sys.exc_info())
+self._batchSuccess = False
+else:
+return super(TestBase,self).assertTrue(expr, msg)
+
+def assertFalse(self, expr, msg=None):
+if self._result is not None and self._batch:
+try:
+return super(TestBase,self).assertFalse( expr, msg)
+except self.failureException:
+self._result.addFailure(self, sys.exc_info())
+self._batchSuccess = False
+el

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: vsk, aprantl.
JDevlieghere updated this revision to Diff 270256.
JDevlieghere added a comment.
JDevlieghere updated this revision to Diff 270258.

Avoid a race that would result in another test finding the copied python before 
it has been tested.


JDevlieghere added a comment.

Fix typo


The Python 3 interpreter in Xcode has a relative RPATH and dyld fails to load 
it when we copy it into the build directory. Provide a workaround for the 
workaround...

  dyld: Library not loaded: @executable_path/../../../../Python3
Referenced from: 
/Users/jonas/llvm/build-tsan/lldb-test-build.noindex/copied-python
Reason: image not found


https://reviews.llvm.org/D81696

Files:
  lldb/test/API/lldbtest.py


Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -29,6 +29,50 @@
 if not os.path.isdir(path):
 raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def get_darwin_python_interpreter(builddir, executable):
+# Avoid doing any work if we already copied the binary. This serves as
+# synchronization between multiple API tests.
+copied_python = os.path.join(builddir, 'copied-python')
+if os.path.isfile(copied_python):
+return copied_python
+
+# Find the "real" python binary.
+import shutil, subprocess
+real_python = subprocess.check_output([
+executable,
+os.path.join(os.path.dirname(os.path.realpath(__file__)),
+ 'get_darwin_real_python.py')
+]).decode('utf-8').strip()
+
+# Copy over the real python to a temporary location. We can't put it in the
+# right place yet, because that might race with other tests running
+# concurrently.
+temp_python = os.path.join(builddir, 'temp-python')
+shutil.copy(real_python, temp_python)
+
+# Now make sure the copied Python works. The Python in Xcode has a relative
+# RPATH and cannot be copied.
+try:
+# We don't care about the output, just make sure it runs.
+subprocess.check_output([temp_python, '-V'],
+stderr=subprocess.STDOUT)
+except subprocess.CalledProcessError:
+# The copied Python didn't work. Assume we're dealing with the Python
+# interpreter in Xcode. Given that this is not a system binary SIP
+# won't prevent us form injecting the interceptors so we get away
+# without using the copy.
+return real_python
+
+# The copied Python works. Time to put it in place and start using it from
+# now on.
+shutil.copy(temp_python, copied_python)
+return copied_python
+
+
 class LLDBTest(TestFormat):
 def __init__(self, dotest_cmd):
 self.dotest_cmd = dotest_cmd
@@ -75,25 +119,9 @@
 
 builddir = getBuildDir(cmd)
 mkdir_p(builddir)
-
-# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim
-# python binary as the ASan interceptors get loaded too late. Also,
-# when SIP is enabled, we can't inject libraries into system binaries
-# at all, so we need a copy of the "real" python to work with.
-#
-# Find the "real" python binary, copy it, and invoke it.
 if 'DYLD_INSERT_LIBRARIES' in test.config.environment and \
 platform.system() == 'Darwin':
-copied_python = os.path.join(builddir, 'copied-system-python')
-if not os.path.isfile(copied_python):
-import shutil, subprocess
-python = subprocess.check_output([
-executable,
-os.path.join(os.path.dirname(os.path.realpath(__file__)),
-'get_darwin_real_python.py')
-]).decode('utf-8').strip()
-shutil.copy(python, copied_python)
-cmd[0] = copied_python
+cmd[0] = get_darwin_python_interpreter(builddir, executable)
 
 if 'lldb-repro-capture' in test.config.available_features or \
'lldb-repro-replay' in test.config.available_features:


Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -29,6 +29,50 @@
 if not os.path.isdir(path):
 raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" pyth

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Fix typo


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

https://reviews.llvm.org/D81696

Files:
  lldb/test/API/lldbtest.py


Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -29,6 +29,50 @@
 if not os.path.isdir(path):
 raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def get_darwin_python_interpreter(builddir, executable):
+# Avoid doing any work if we already copied the binary. This serves as
+# synchronization between multiple API tests.
+copied_python = os.path.join(builddir, 'copied-python')
+if os.path.isfile(copied_python):
+return copied_python
+
+# Find the "real" python binary.
+import shutil, subprocess
+real_python = subprocess.check_output([
+executable,
+os.path.join(os.path.dirname(os.path.realpath(__file__)),
+ 'get_darwin_real_python.py')
+]).decode('utf-8').strip()
+
+# Copy over the real python to a temporary location. We can't put it in the
+# right place yet, because that might race with other tests running
+# concurrently.
+temp_python = os.path.join(builddir, 'temp-python')
+shutil.copy(real_python, temp_python)
+
+# Now make sure the copied Python works. The Python in Xcode has a relative
+# RPATH and cannot be copied.
+try:
+# We don't care about the output, just make sure it runs.
+subprocess.check_output([temp_python, '-V'],
+stderr=subprocess.STDOUT)
+except subprocess.CalledProcessError:
+# The copied Python didn't work. Assume we're dealing with the Python
+# interpreter in Xcode. Given that this is not a system binary SIP
+# won't prevent us form injecting the interceptors so we get away
+# without using the copy.
+return real_python
+
+# The copied Python works. Time to put it in place and start using it from
+# now on.
+shutil.copy(temp_python, copied_python)
+return copied_python
+
+
 class LLDBTest(TestFormat):
 def __init__(self, dotest_cmd):
 self.dotest_cmd = dotest_cmd
@@ -75,25 +119,9 @@
 
 builddir = getBuildDir(cmd)
 mkdir_p(builddir)
-
-# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim
-# python binary as the ASan interceptors get loaded too late. Also,
-# when SIP is enabled, we can't inject libraries into system binaries
-# at all, so we need a copy of the "real" python to work with.
-#
-# Find the "real" python binary, copy it, and invoke it.
 if 'DYLD_INSERT_LIBRARIES' in test.config.environment and \
 platform.system() == 'Darwin':
-copied_python = os.path.join(builddir, 'copied-system-python')
-if not os.path.isfile(copied_python):
-import shutil, subprocess
-python = subprocess.check_output([
-executable,
-os.path.join(os.path.dirname(os.path.realpath(__file__)),
-'get_darwin_real_python.py')
-]).decode('utf-8').strip()
-shutil.copy(python, copied_python)
-cmd[0] = copied_python
+cmd[0] = get_darwin_python_interpreter(builddir, executable)
 
 if 'lldb-repro-capture' in test.config.available_features or \
'lldb-repro-replay' in test.config.available_features:


Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -29,6 +29,50 @@
 if not os.path.isdir(path):
 raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def get_darwin_python_interpreter(builddir, executable):
+# Avoid doing any work if we already copied the binary. This serves as
+# synchronization between multiple API tests.
+copied_python = os.path.join(builddir, 'copied-python')
+if os.path.isfile(copied_python):
+return copied_python
+
+# Find the "real" python binary.
+import shutil, subprocess
+real_python = subprocess.check_output([
+executable,
+os.path.join(os.path.dirname(os.path.realpath(__file__)),
+   

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Avoid a race that would result in another test finding the copied python before 
it has been tested.


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

https://reviews.llvm.org/D81696

Files:
  lldb/test/API/lldbtest.py


Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -29,6 +29,50 @@
 if not os.path.isdir(path):
 raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def get_darwin_python_interpreter(builddir, executable):
+# Avoid doing any work if we already copied the binary. This serves as
+# synchronization between multiple API tests.
+copied_python = os.path.join(builddir, 'copied-python')
+if os.path.isfile(copied_python):
+return copied_python
+
+# Find the "real" python binary.
+import shutil, subprocess
+real_python = subprocess.check_output([
+executable,
+os.path.join(os.path.dirname(os.path.realpath(__file__)),
+ 'get_darwin_real_python.py')
+]).decode('utf-8').strip()
+
+# Copy over the real python to a temporary python. We can't put it in the
+# right place yet, because that might race with other tests running
+# concurrently.
+temp_python = os.path.join(builddir, 'copied-python')
+shutil.copy(real_python, temp_python)
+
+# Now make sure the copied Python works. The Python in Xcode has a relative
+# RPATH and cannot be copied.
+try:
+# We don't care about the output, just make sure it runs.
+subprocess.check_output([temp_python, '-V'],
+stderr=subprocess.STDOUT)
+except subprocess.CalledProcessError:
+# The copied Python didn't work. Assume we're dealing with the Python
+# interpreter in Xcode. Given that this is not a system binary SIP
+# won't prevent us form injecting the interceptors so we get away
+# without using the copy.
+return real_python
+
+# The copied Python works. Time to put it in place and start using it from
+# now on.
+shutil.copy(temp_python, copied_python)
+return copied_python
+
+
 class LLDBTest(TestFormat):
 def __init__(self, dotest_cmd):
 self.dotest_cmd = dotest_cmd
@@ -75,25 +119,9 @@
 
 builddir = getBuildDir(cmd)
 mkdir_p(builddir)
-
-# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim
-# python binary as the ASan interceptors get loaded too late. Also,
-# when SIP is enabled, we can't inject libraries into system binaries
-# at all, so we need a copy of the "real" python to work with.
-#
-# Find the "real" python binary, copy it, and invoke it.
 if 'DYLD_INSERT_LIBRARIES' in test.config.environment and \
 platform.system() == 'Darwin':
-copied_python = os.path.join(builddir, 'copied-system-python')
-if not os.path.isfile(copied_python):
-import shutil, subprocess
-python = subprocess.check_output([
-executable,
-os.path.join(os.path.dirname(os.path.realpath(__file__)),
-'get_darwin_real_python.py')
-]).decode('utf-8').strip()
-shutil.copy(python, copied_python)
-cmd[0] = copied_python
+cmd[0] = get_darwin_python_interpreter(builddir, executable)
 
 if 'lldb-repro-capture' in test.config.available_features or \
'lldb-repro-replay' in test.config.available_features:


Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -29,6 +29,50 @@
 if not os.path.isdir(path):
 raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def get_darwin_python_interpreter(builddir, executable):
+# Avoid doing any work if we already copied the binary. This serves as
+# synchronization between multiple API tests.
+copied_python = os.path.join(builddir, 'copied-python')
+if os.path.isfile(copied_python):
+return copied_python
+
+# Find the "real" python binary.
+import shutil, subprocess
+real_python = subprocess.check_output([
+exe

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Move work out of `LLDBTest` so we only have to compute it once.


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

https://reviews.llvm.org/D81696

Files:
  lldb/test/API/lit.cfg.py
  lldb/test/API/lit.site.cfg.py.in
  lldb/test/API/lldbtest.py

Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -10,24 +10,6 @@
 import lit.util
 from lit.formats.base import TestFormat
 
-def getBuildDir(cmd):
-found = False
-for arg in cmd:
-if found:
-return arg
-if arg == '--build-dir':
-found = True
-return None
-
-def mkdir_p(path):
-import errno
-try:
-os.makedirs(path)
-except OSError as e:
-if e.errno != errno.EEXIST:
-raise
-if not os.path.isdir(path):
-raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
 class LLDBTest(TestFormat):
 def __init__(self, dotest_cmd):
@@ -73,33 +55,10 @@
 # python exe as the first parameter of the command.
 cmd = [executable] + self.dotest_cmd + [testPath, '-p', testFile]
 
-builddir = getBuildDir(cmd)
-mkdir_p(builddir)
-
-# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim
-# python binary as the ASan interceptors get loaded too late. Also,
-# when SIP is enabled, we can't inject libraries into system binaries
-# at all, so we need a copy of the "real" python to work with.
-#
-# Find the "real" python binary, copy it, and invoke it.
-if 'DYLD_INSERT_LIBRARIES' in test.config.environment and \
-platform.system() == 'Darwin':
-copied_python = os.path.join(builddir, 'copied-system-python')
-if not os.path.isfile(copied_python):
-import shutil, subprocess
-python = subprocess.check_output([
-executable,
-os.path.join(os.path.dirname(os.path.realpath(__file__)),
-'get_darwin_real_python.py')
-]).decode('utf-8').strip()
-shutil.copy(python, copied_python)
-cmd[0] = copied_python
-
 if 'lldb-repro-capture' in test.config.available_features or \
'lldb-repro-replay' in test.config.available_features:
-reproducer_root = os.path.join(builddir, 'reproducers')
-mkdir_p(reproducer_root)
-reproducer_path = os.path.join(reproducer_root, testFile)
+reproducer_path = os.path.join(
+test.config.lldb_reproducer_directory, testFile)
 if 'lldb-repro-capture' in test.config.available_features:
 cmd.extend(['--capture-path', reproducer_path])
 else:
Index: lldb/test/API/lit.site.cfg.py.in
===
--- lldb/test/API/lit.site.cfg.py.in
+++ lldb/test/API/lit.site.cfg.py.in
@@ -18,6 +18,7 @@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.lldb_build_directory = "@LLDB_TEST_BUILD_DIRECTORY@"
+config.lldb_reproducer_directory = os.path.join("@LLDB_TEST_BUILD_DIRECTORY@", "reproducers")
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.dotest_path = "@LLDB_SOURCE_DIR@/test/API/dotest.py"
 config.dotest_args_str = "@LLDB_DOTEST_ARGS@"
@@ -29,6 +30,7 @@
 config.test_compiler = '@LLDB_TEST_COMPILER@'
 config.dsymutil = '@LLDB_TEST_DSYMUTIL@'
 config.filecheck = '@LLDB_TEST_FILECHECK@'
+
 # The API tests use their own module caches.
 config.lldb_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_LLDB@", "lldb-api")
 config.clang_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_CLANG@", "lldb-api")
@@ -61,5 +63,6 @@
 key, = e.args
 lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))
 
+
 # Let the main config do the real work.
 lit_config.load_config(config, "@LLDB_SOURCE_DIR@/test/API/lit.cfg.py")
Index: lldb/test/API/lit.cfg.py
===
--- lldb/test/API/lit.cfg.py
+++ lldb/test/API/lit.cfg.py
@@ -21,6 +21,17 @@
 config.test_exec_root = config.test_source_root
 
 
+def mkdir_p(path):
+import errno
+try:
+os.makedirs(path)
+except OSError as e:
+if e.errno != errno.EEXIST:
+raise
+if not os.path.isdir(path):
+raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
+
+
 def find_sanitizer_runtime(name):
   import subprocess
   resource_dir = subprocess.check_output(
@@ -38,6 +49,44 @@
 yield 'PATH'
 
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, 

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.



Comment at: lldb/test/API/lit.cfg.py:56
+# copy of the "real" python to work with.
+def find_python_interpreter():
+  # Avoid doing any work if we already copied the binary. This serves as

This isn't relevant for the unit or shell tests because the API tests are the 
only ones that are executed within a python process, right?



Comment at: lldb/test/API/lit.cfg.py:58
+  # Avoid doing any work if we already copied the binary. This serves as
+  # synchronization between multiple API tests.
+  copied_python = os.path.join(config.lldb_build_directory, 'copied-python')

Is the part about synchronization still applicable? Maybe it is, if you run 
llvm-lit test/API from two different directories?



Comment at: lldb/test/API/lit.cfg.py:102
+if 'DYLD_INSERT_LIBRARIES' in config.environment and platform.system() == 
'Darwin':
+  config.python_executable = find_python_interpreter()
+

What does setting python_executable do? I looked this up in llvm and found:

```
test/lit.cfg.py:('%llvm-locstats', "'%s' %s" % (config.python_executable, 
llvm_locstats_tool)))
test/lit.site.cfg.py.in:config.python_executable = "@PYTHON_EXECUTABLE@"
test/tools/UpdateTestChecks/lit.local.cfg:config.python_executable, 
script_path, extra_args)))
```

Am I missing something, or does something here translate into a lit directive 
to run python files under python_interpreter?


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

https://reviews.llvm.org/D81696



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

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 270275.
JDevlieghere marked 3 inline comments as done.

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

https://reviews.llvm.org/D81696

Files:
  lldb/test/API/lit.cfg.py
  lldb/test/API/lit.site.cfg.py.in
  lldb/test/API/lldbtest.py

Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -10,24 +10,6 @@
 import lit.util
 from lit.formats.base import TestFormat
 
-def getBuildDir(cmd):
-found = False
-for arg in cmd:
-if found:
-return arg
-if arg == '--build-dir':
-found = True
-return None
-
-def mkdir_p(path):
-import errno
-try:
-os.makedirs(path)
-except OSError as e:
-if e.errno != errno.EEXIST:
-raise
-if not os.path.isdir(path):
-raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
 class LLDBTest(TestFormat):
 def __init__(self, dotest_cmd):
@@ -73,33 +55,10 @@
 # python exe as the first parameter of the command.
 cmd = [executable] + self.dotest_cmd + [testPath, '-p', testFile]
 
-builddir = getBuildDir(cmd)
-mkdir_p(builddir)
-
-# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim
-# python binary as the ASan interceptors get loaded too late. Also,
-# when SIP is enabled, we can't inject libraries into system binaries
-# at all, so we need a copy of the "real" python to work with.
-#
-# Find the "real" python binary, copy it, and invoke it.
-if 'DYLD_INSERT_LIBRARIES' in test.config.environment and \
-platform.system() == 'Darwin':
-copied_python = os.path.join(builddir, 'copied-system-python')
-if not os.path.isfile(copied_python):
-import shutil, subprocess
-python = subprocess.check_output([
-executable,
-os.path.join(os.path.dirname(os.path.realpath(__file__)),
-'get_darwin_real_python.py')
-]).decode('utf-8').strip()
-shutil.copy(python, copied_python)
-cmd[0] = copied_python
-
 if 'lldb-repro-capture' in test.config.available_features or \
'lldb-repro-replay' in test.config.available_features:
-reproducer_root = os.path.join(builddir, 'reproducers')
-mkdir_p(reproducer_root)
-reproducer_path = os.path.join(reproducer_root, testFile)
+reproducer_path = os.path.join(
+test.config.lldb_reproducer_directory, testFile)
 if 'lldb-repro-capture' in test.config.available_features:
 cmd.extend(['--capture-path', reproducer_path])
 else:
Index: lldb/test/API/lit.site.cfg.py.in
===
--- lldb/test/API/lit.site.cfg.py.in
+++ lldb/test/API/lit.site.cfg.py.in
@@ -18,6 +18,7 @@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.lldb_build_directory = "@LLDB_TEST_BUILD_DIRECTORY@"
+config.lldb_reproducer_directory = os.path.join("@LLDB_TEST_BUILD_DIRECTORY@", "reproducers")
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.dotest_path = "@LLDB_SOURCE_DIR@/test/API/dotest.py"
 config.dotest_args_str = "@LLDB_DOTEST_ARGS@"
Index: lldb/test/API/lit.cfg.py
===
--- lldb/test/API/lit.cfg.py
+++ lldb/test/API/lit.cfg.py
@@ -21,6 +21,17 @@
 config.test_exec_root = config.test_source_root
 
 
+def mkdir_p(path):
+import errno
+try:
+os.makedirs(path)
+except OSError as e:
+if e.errno != errno.EEXIST:
+raise
+if not os.path.isdir(path):
+raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
+
+
 def find_sanitizer_runtime(name):
   import subprocess
   resource_dir = subprocess.check_output(
@@ -38,6 +49,43 @@
 yield 'PATH'
 
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def find_python_interpreter():
+  # Avoid doing any work if we already copied the binary.
+  copied_python = os.path.join(config.lldb_build_directory, 'copied-python')
+  if os.path.isfile(copied_python):
+return copied_python
+
+  # Find the "real" python binary.
+  import shutil, subprocess
+  real_python = subprocess.check_output([
+  config.python_executable,
+  os.path.join(os.path.dirname(os.path.realpath(__file__)),
+   'get_darwin_real_python.py')
+  ]).decode('utf-8').strip()
+
+  shutil.copy(real_python, copied_python)
+
+  # Now make sure the copied Python works. 

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 6 inline comments as done.
JDevlieghere added inline comments.



Comment at: lldb/test/API/lit.cfg.py:56
+# copy of the "real" python to work with.
+def find_python_interpreter():
+  # Avoid doing any work if we already copied the binary. This serves as

vsk wrote:
> This isn't relevant for the unit or shell tests because the API tests are the 
> only ones that are executed within a python process, right?
Correct.



Comment at: lldb/test/API/lit.cfg.py:58
+  # Avoid doing any work if we already copied the binary. This serves as
+  # synchronization between multiple API tests.
+  copied_python = os.path.join(config.lldb_build_directory, 'copied-python')

vsk wrote:
> Is the part about synchronization still applicable? Maybe it is, if you run 
> llvm-lit test/API from two different directories?
I guess it is, but not something I'd optimize for. I'll remove the comment. 



Comment at: lldb/test/API/lit.cfg.py:102
+if 'DYLD_INSERT_LIBRARIES' in config.environment and platform.system() == 
'Darwin':
+  config.python_executable = find_python_interpreter()
+

vsk wrote:
> What does setting python_executable do? I looked this up in llvm and found:
> 
> ```
> test/lit.cfg.py:('%llvm-locstats', "'%s' %s" % (config.python_executable, 
> llvm_locstats_tool)))
> test/lit.site.cfg.py.in:config.python_executable = "@PYTHON_EXECUTABLE@"
> test/tools/UpdateTestChecks/lit.local.cfg:
> config.python_executable, script_path, extra_args)))
> ```
> 
> Am I missing something, or does something here translate into a lit directive 
> to run python files under python_interpreter?
`test/lit.site.cfg.py.in` will be configured with the path to the real python 
executable in the build dir.


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

https://reviews.llvm.org/D81696



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

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Vedant Kumar via Phabricator via lldb-commits]

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

Lgtm, thanks.




Comment at: lldb/test/API/lit.cfg.py:102
+if 'DYLD_INSERT_LIBRARIES' in config.environment and platform.system() == 
'Darwin':
+  config.python_executable = find_python_interpreter()
+

JDevlieghere wrote:
> vsk wrote:
> > What does setting python_executable do? I looked this up in llvm and found:
> > 
> > ```
> > test/lit.cfg.py:('%llvm-locstats', "'%s' %s" % 
> > (config.python_executable, llvm_locstats_tool)))
> > test/lit.site.cfg.py.in:config.python_executable = "@PYTHON_EXECUTABLE@"
> > test/tools/UpdateTestChecks/lit.local.cfg:
> > config.python_executable, script_path, extra_args)))
> > ```
> > 
> > Am I missing something, or does something here translate into a lit 
> > directive to run python files under python_interpreter?
> `test/lit.site.cfg.py.in` will be configured with the path to the real python 
> executable in the build dir.
This is what I was missing: `lldb/test/API/lldbtest.py:executable = 
test.config.python_executable`.



Comment at: lldb/test/API/lldbtest.py:51
 # build with.
 executable = test.config.python_executable
 

.. so by the time this is reached, executable should point to a known-good 
python binary.


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

https://reviews.llvm.org/D81696



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

[Lldb-commits] [lldb] 526e0c8 - [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-06-11T19:36:42-07:00
New Revision: 526e0c8d15216e6c49b1769c63f5433df6841c64

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

LOG: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

The Python 3 interpreter in Xcode has a relative RPATH and dyld fails to
load it when we copy it into the build directory.

This patch adds an additional check that the copied binary can be
executed. If it doesn't, we assume we're dealing with the Xcode python
interpreter and return the path to the real executable. That is
sufficient for the sanitizers because only system binaries need to be
copied to work around SIP.

This patch also moves all that logic out of LLDBTest and into the lit
configuration so that it's executed only once per test run, instead of
once for every test. Although I didn't benchmark the difference this
should result in a mild speedup.

Differential revision: https://reviews.llvm.org/D81696

Added: 


Modified: 
lldb/test/API/lit.cfg.py
lldb/test/API/lit.site.cfg.py.in
lldb/test/API/lldbtest.py

Removed: 




diff  --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index 288d667c18dd..632d883e0da9 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -21,6 +21,17 @@
 config.test_exec_root = config.test_source_root
 
 
+def mkdir_p(path):
+import errno
+try:
+os.makedirs(path)
+except OSError as e:
+if e.errno != errno.EEXIST:
+raise
+if not os.path.isdir(path):
+raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
+
+
 def find_sanitizer_runtime(name):
   import subprocess
   resource_dir = subprocess.check_output(
@@ -38,6 +49,43 @@ def find_shlibpath_var():
 yield 'PATH'
 
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def find_python_interpreter():
+  # Avoid doing any work if we already copied the binary.
+  copied_python = os.path.join(config.lldb_build_directory, 'copied-python')
+  if os.path.isfile(copied_python):
+return copied_python
+
+  # Find the "real" python binary.
+  import shutil, subprocess
+  real_python = subprocess.check_output([
+  config.python_executable,
+  os.path.join(os.path.dirname(os.path.realpath(__file__)),
+   'get_darwin_real_python.py')
+  ]).decode('utf-8').strip()
+
+  shutil.copy(real_python, copied_python)
+
+  # Now make sure the copied Python works. The Python in Xcode has a relative
+  # RPATH and cannot be copied.
+  try:
+# We don't care about the output, just make sure it runs.
+subprocess.check_output([copied_python, '-V'], stderr=subprocess.STDOUT)
+  except subprocess.CalledProcessError:
+# The copied Python didn't work. Assume we're dealing with the Python
+# interpreter in Xcode. Given that this is not a system binary SIP
+# won't prevent us form injecting the interceptors so we get away with
+# not copying the executable.
+os.remove(copied_python)
+return real_python
+
+  # The copied Python works.
+  return copied_python
+
+
 if 'Address' in config.llvm_use_sanitizer:
   config.environment['ASAN_OPTIONS'] = 'detect_stack_use_after_return=1'
   if 'Darwin' in config.host_os and 'x86' in config.host_triple:
@@ -49,6 +97,9 @@ def find_shlibpath_var():
 config.environment['DYLD_INSERT_LIBRARIES'] = find_sanitizer_runtime(
 'libclang_rt.tsan_osx_dynamic.dylib')
 
+if 'DYLD_INSERT_LIBRARIES' in config.environment and platform.system() == 
'Darwin':
+  config.python_executable = find_python_interpreter()
+
 # Shared library build of LLVM may require LD_LIBRARY_PATH or equivalent.
 if config.shared_libs:
   for shlibpath_var in find_shlibpath_var():

diff  --git a/lldb/test/API/lit.site.cfg.py.in 
b/lldb/test/API/lit.site.cfg.py.in
index e5d5c00c43be..e97f867b265b 100644
--- a/lldb/test/API/lit.site.cfg.py.in
+++ b/lldb/test/API/lit.site.cfg.py.in
@@ -18,6 +18,7 @@ config.shared_libs = @LLVM_ENABLE_SHARED_LIBS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.lldb_build_directory = "@LLDB_TEST_BUILD_DIRECTORY@"
+config.lldb_reproducer_directory = os.path.join("@LLDB_TEST_BUILD_DIRECTORY@", 
"reproducers")
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.dotest_path = "@LLDB_SOURCE_DIR@/test/API/dotest.py"
 config.dotest_args_str = "@LLDB_DOTEST_ARGS@"

diff  --git a/lldb/test/API/lldbtest.py b/lldb/test/API/lldbtest.py
index fcc13a07eded..94eb8ebd054a 100644
--- a/lldb/test/API/lldbtest.py
+++ b/lldb/test/API/lldbtest.py
@@ -10,24 +10,6 @@

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

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



Comment at: lldb/test/API/lldbtest.py:51
 # build with.
 executable = test.config.python_executable
 

vsk wrote:
> .. so by the time this is reached, executable should point to a known-good 
> python binary.
Yep!


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

https://reviews.llvm.org/D81696



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

[Lldb-commits] [PATCH] D81696: [lldb/Test] Fix ASan/TSan workaround for Xcode Python 3

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Closed by commit rG526e0c8d1521: [lldb/Test] Fix ASan/TSan workaround for Xcode 
Python 3 (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81696

Files:
  lldb/test/API/lit.cfg.py
  lldb/test/API/lit.site.cfg.py.in
  lldb/test/API/lldbtest.py

Index: lldb/test/API/lldbtest.py
===
--- lldb/test/API/lldbtest.py
+++ lldb/test/API/lldbtest.py
@@ -10,24 +10,6 @@
 import lit.util
 from lit.formats.base import TestFormat
 
-def getBuildDir(cmd):
-found = False
-for arg in cmd:
-if found:
-return arg
-if arg == '--build-dir':
-found = True
-return None
-
-def mkdir_p(path):
-import errno
-try:
-os.makedirs(path)
-except OSError as e:
-if e.errno != errno.EEXIST:
-raise
-if not os.path.isdir(path):
-raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
 
 class LLDBTest(TestFormat):
 def __init__(self, dotest_cmd):
@@ -73,33 +55,10 @@
 # python exe as the first parameter of the command.
 cmd = [executable] + self.dotest_cmd + [testPath, '-p', testFile]
 
-builddir = getBuildDir(cmd)
-mkdir_p(builddir)
-
-# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim
-# python binary as the ASan interceptors get loaded too late. Also,
-# when SIP is enabled, we can't inject libraries into system binaries
-# at all, so we need a copy of the "real" python to work with.
-#
-# Find the "real" python binary, copy it, and invoke it.
-if 'DYLD_INSERT_LIBRARIES' in test.config.environment and \
-platform.system() == 'Darwin':
-copied_python = os.path.join(builddir, 'copied-system-python')
-if not os.path.isfile(copied_python):
-import shutil, subprocess
-python = subprocess.check_output([
-executable,
-os.path.join(os.path.dirname(os.path.realpath(__file__)),
-'get_darwin_real_python.py')
-]).decode('utf-8').strip()
-shutil.copy(python, copied_python)
-cmd[0] = copied_python
-
 if 'lldb-repro-capture' in test.config.available_features or \
'lldb-repro-replay' in test.config.available_features:
-reproducer_root = os.path.join(builddir, 'reproducers')
-mkdir_p(reproducer_root)
-reproducer_path = os.path.join(reproducer_root, testFile)
+reproducer_path = os.path.join(
+test.config.lldb_reproducer_directory, testFile)
 if 'lldb-repro-capture' in test.config.available_features:
 cmd.extend(['--capture-path', reproducer_path])
 else:
Index: lldb/test/API/lit.site.cfg.py.in
===
--- lldb/test/API/lit.site.cfg.py.in
+++ lldb/test/API/lit.site.cfg.py.in
@@ -18,6 +18,7 @@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.lldb_build_directory = "@LLDB_TEST_BUILD_DIRECTORY@"
+config.lldb_reproducer_directory = os.path.join("@LLDB_TEST_BUILD_DIRECTORY@", "reproducers")
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.dotest_path = "@LLDB_SOURCE_DIR@/test/API/dotest.py"
 config.dotest_args_str = "@LLDB_DOTEST_ARGS@"
Index: lldb/test/API/lit.cfg.py
===
--- lldb/test/API/lit.cfg.py
+++ lldb/test/API/lit.cfg.py
@@ -21,6 +21,17 @@
 config.test_exec_root = config.test_source_root
 
 
+def mkdir_p(path):
+import errno
+try:
+os.makedirs(path)
+except OSError as e:
+if e.errno != errno.EEXIST:
+raise
+if not os.path.isdir(path):
+raise OSError(errno.ENOTDIR, "%s is not a directory"%path)
+
+
 def find_sanitizer_runtime(name):
   import subprocess
   resource_dir = subprocess.check_output(
@@ -38,6 +49,43 @@
 yield 'PATH'
 
 
+# On macOS, we can't do the DYLD_INSERT_LIBRARIES trick with a shim python
+# binary as the ASan interceptors get loaded too late. Also, when SIP is
+# enabled, we can't inject libraries into system binaries at all, so we need a
+# copy of the "real" python to work with.
+def find_python_interpreter():
+  # Avoid doing any work if we already copied the binary.
+  copied_python = os.path.join(config.lldb_build_directory, 'copied-python')
+  if os.path.isfile(copied_python):
+return copied_python
+
+  # Find the "real" python binary.
+  import shutil, subprocess
+  real_python = subprocess.check_output([
+  config.python_executable,
+  os.path.join(os.pa

[Lldb-commits] [PATCH] D81200: [vscode] set default values for terminateDebuggee for the disconnect request

[Greg Clayton via Phabricator via lldb-commits]

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

I'm ok with this then if the error being returned for disconnect doesn't affect 
the IDE. Anyone else should chime in quickly if you have any objections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81200



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