[Lldb-commits] [PATCH] D90769: [lldb][ObjectFile] Relocate sections for in-memory objects (e.g. received via JITLoaderGDB)

2020-11-06 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

In D90769#2376557 , @sgraenitz wrote:

> Thanks for having a look!
>
> The JIT implementations in LLVM operate on relocatable object files, so 
> someone needs to resolve them. MCJIT has a flag `ProcessAllSections` to 
> control which sections will be loaded and relocated. As it is off by default, 
> it will usually ignore sections that it doesn't need for execution. LLDB does 
> it the other way around in ObjectFileELF::RelocateSection(), it only 
> processes sections with the ".debug" prefix. This seems to be a reasonable 
> distribution of tasks. In general, relocations are resolved in place, so it 
> doesn't rise memory consumption.

But you still have to allocate memory to store the relocations. And that memory 
is not freed after the relocations are resolved, right?

>> I do wonder though if the jit could be changed to avoid relocation.
>
> Well, I don't think we are anywhere close to optimizations like this, but it 
> would be nice indeed. If we compile from bitcode on the JIT side, we could 
> lookup external symbols at compile-time and don't produce relocations for 
> them in the first place. I guess it would heavily reduce the number of 
> relocations and potentially save time. On the other hand, thinking about 
> concurrent compile jobs and cross-dependencies.. I can imagine it gets hairy 
> quickly. Plus: the way it is now, we can cache the object files and reuse 
> them thanks to position-independent code.

In a "normal" compilation, there are two kinds of relocations. The ones that 
are resolved by the normal (static) linker, and the ones that are resolved by 
the dynamic linker (loader). to make a module relocatable and have it interface 
with external code, you only need the second kind. The debug info relocations 
are all of the first kind, so they could be resolved (or, not emitted in the 
first place -- like what happens with MachO object files) without impacting 
this ability.

But that's for another discussion...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90769

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

[Lldb-commits] [PATCH] D90875: [lldb] [test] Avoid double negatives in llgs/debugserver logic

2020-11-06 Thread Michał Górny via Phabricator via lldb-commits 
mgorny marked 2 inline comments as done.
mgorny added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:950
+configuration.llgs_platform = (
+target_platform in ["linux", "freebsd", "netbsd", "windows"])
+

labath wrote:
> This is not entirely equivalent to the previous version -- it won't match 
> "platforms" like "freebsd4.7". Fortunately, we already have some 
> canonicalization code in `lldbplatformutil.getPlatform()`, so if you switch 
> to that, it should be fine.
Good catch! However, this doesn't seem to work:

```
Command Output (stderr):
--
Traceback (most recent call last):
  File "/home/mgorny/llvm-project/llvm/tools/lldb/test/API/dotest.py", line 7, 
in 
lldbsuite.test.run_suite()
  File 
"/usr/home/mgorny/llvm-project/lldb/packages/Python/lldbsuite/test/dotest.py", 
line 855, in run_suite
from lldbsuite.test import lldbplatformutil
  File 
"/usr/home/mgorny/llvm-project/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py",
 line 19, in 
import lldb
ModuleNotFoundError: No module named 'lldb'

--
```

There is apparently some obscure problem behind the scenes and it seems above 
my pay grade. I'll just revert to the old code.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:953
+# Perform debugserver tests on Darwin platforms.  Currently, this
+# means all platform that do not use LLGS but additional platforms
+# may be excluded in the future.

labath wrote:
> mgorny wrote:
> > @labath, can we assume that all future platforms will use LLGS?
> I certainly hope so. I doubt anyone would port debugserver to another os, and 
> I definitely would not want to carry another debugserver around.
Ok, I will remove the comment about adding more platforms then.


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

https://reviews.llvm.org/D90875

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

[Lldb-commits] [PATCH] D90875: [lldb] [test] Avoid double negations in llgs/debugserver logic

2020-11-06 Thread Michał Górny via Phabricator via lldb-commits 
mgorny updated this revision to Diff 303349.
mgorny marked 2 inline comments as done.
mgorny retitled this revision from "[lldb] [test] Avoid double negatives in 
llgs/debugserver logic" to "[lldb] [test] Avoid double negations in 
llgs/debugserver logic".
mgorny added a comment.

Removed the part about adding new platforms without LLGS, and restored old 
conditions for matching platforms.


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

https://reviews.llvm.org/D90875

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -945,19 +945,15 @@
 checkWatchpointSupport()
 checkDebugInfoSupport()
 
-# Don't do debugserver tests on anything except OS X.
-configuration.dont_do_debugserver_test = (
-"linux" in target_platform or
-"freebsd" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
-
-# Don't do lldb-server (llgs) tests on platforms not supporting it.
-configuration.dont_do_llgs_test = not (
-"freebsd" in target_platform or
-"linux" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
+# Perform LLGS tests only on platforms using it.
+configuration.llgs_platform = (
+"freebsd" in target_platform or
+"linux" in target_platform or
+"netbsd" in target_platform or
+"windows" in target_platform)
+
+# Perform debugserver tests elsewhere (i.e. on Darwin platforms).
+configuration.debugserver_platform = not configuration.llgs_platform
 
 for testdir in configuration.testdirs:
 for (dirpath, dirnames, filenames) in os.walk(testdir):
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -375,14 +375,18 @@
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
 def should_skip_debugserver_test():
-return "debugserver tests" if configuration.dont_do_debugserver_test 
else None
+return ("debugserver tests"
+if not configuration.debugserver_platform
+else None)
 return skipTestIfFn(should_skip_debugserver_test)(func)
 
 
 def llgs_test(func):
 """Decorate the item as a lldb-server test."""
 def should_skip_llgs_tests():
-return "llgs tests" if configuration.dont_do_llgs_test else None
+return ("llgs tests"
+if not configuration.llgs_platform
+else None)
 return skipTestIfFn(should_skip_llgs_tests)(func)
 
 


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -945,19 +945,15 @@
 checkWatchpointSupport()
 checkDebugInfoSupport()
 
-# Don't do debugserver tests on anything except OS X.
-configuration.dont_do_debugserver_test = (
-"linux" in target_platform or
-"freebsd" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
-
-# Don't do lldb-server (llgs) tests on platforms not supporting it.
-configuration.dont_do_llgs_test = not (
-"freebsd" in target_platform or
-"linux" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
+# Perform LLGS tests only on platforms using it.
+configuration.llgs_platform = (
+"freebsd" in target_platform or
+"linux" in target_platform or
+"netbsd" in target_platform or
+"windows" in target_platform)
+
+# Perform debugserver tests elsewhere (i.e. on Darwin platforms).
+configuration.debugserver_platform = not configuration.llgs_platform
 
 for testdir in configuration.testdirs:
 for (dirpath, dirnames, filenames) in os.walk(testdir):
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -375,14 +375,18 @@
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
 def should_skip_debugserver_test():
-return "debugserver tests" if configuration.dont_do_debugserver_test else None
+return ("debugserver tests"
+if not configuration.debugserver_platform
+else None)
 return skipTest

[Lldb-commits] [PATCH] D90875: [lldb] [test] Avoid double negations in llgs/debugserver logic

2020-11-06 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:950
+configuration.llgs_platform = (
+target_platform in ["linux", "freebsd", "netbsd", "windows"])
+

mgorny wrote:
> labath wrote:
> > This is not entirely equivalent to the previous version -- it won't match 
> > "platforms" like "freebsd4.7". Fortunately, we already have some 
> > canonicalization code in `lldbplatformutil.getPlatform()`, so if you switch 
> > to that, it should be fine.
> Good catch! However, this doesn't seem to work:
> 
> ```
> Command Output (stderr):
> --
> Traceback (most recent call last):
>   File "/home/mgorny/llvm-project/llvm/tools/lldb/test/API/dotest.py", line 
> 7, in 
> lldbsuite.test.run_suite()
>   File 
> "/usr/home/mgorny/llvm-project/lldb/packages/Python/lldbsuite/test/dotest.py",
>  line 855, in run_suite
> from lldbsuite.test import lldbplatformutil
>   File 
> "/usr/home/mgorny/llvm-project/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py",
>  line 19, in 
> import lldb
> ModuleNotFoundError: No module named 'lldb'
> 
> --
> ```
> 
> There is apparently some obscure problem behind the scenes and it seems above 
> my pay grade. I'll just revert to the old code.
This is fiddly, because all of this happens very early, before everything is 
set up.
You've tried to import this package much too early. The paths necessary for 
"import lldb" to work are only set up on line 869 (setupSysPath()). And 
`lldb.selected_platform` is only set on line ~931. You need to put the import 
statement after those -- say line 941, to replace the manual triple chopping...


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

https://reviews.llvm.org/D90875

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

[Lldb-commits] [PATCH] D90875: [lldb] [test] Avoid double negations in llgs/debugserver logic

2020-11-06 Thread Michał Górny via Phabricator via lldb-commits 
mgorny updated this revision to Diff 303374.
mgorny added a comment.

Used `lldbplatformutil.getPlatform()` as requested.


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

https://reviews.llvm.org/D90875

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -938,26 +938,20 @@
 # Note that it's not dotest's job to clean this directory.
 lldbutil.mkdir_p(configuration.test_build_dir)
 
-target_platform = lldb.selected_platform.GetTriple().split('-')[2]
+from . import lldbplatformutil
+target_platform = lldbplatformutil.getPlatform()
 
 checkLibcxxSupport()
 checkLibstdcxxSupport()
 checkWatchpointSupport()
 checkDebugInfoSupport()
 
-# Don't do debugserver tests on anything except OS X.
-configuration.dont_do_debugserver_test = (
-"linux" in target_platform or
-"freebsd" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
-
-# Don't do lldb-server (llgs) tests on platforms not supporting it.
-configuration.dont_do_llgs_test = not (
-"freebsd" in target_platform or
-"linux" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
+# Perform LLGS tests only on platforms using it.
+configuration.llgs_platform = (
+target_platform in ["freebsd", "linux", "netbsd", "windows"])
+
+# Perform debugserver tests elsewhere (i.e. on Darwin platforms).
+configuration.debugserver_platform = not configuration.llgs_platform
 
 for testdir in configuration.testdirs:
 for (dirpath, dirnames, filenames) in os.walk(testdir):
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -375,14 +375,18 @@
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
 def should_skip_debugserver_test():
-return "debugserver tests" if configuration.dont_do_debugserver_test 
else None
+return ("debugserver tests"
+if not configuration.debugserver_platform
+else None)
 return skipTestIfFn(should_skip_debugserver_test)(func)
 
 
 def llgs_test(func):
 """Decorate the item as a lldb-server test."""
 def should_skip_llgs_tests():
-return "llgs tests" if configuration.dont_do_llgs_test else None
+return ("llgs tests"
+if not configuration.llgs_platform
+else None)
 return skipTestIfFn(should_skip_llgs_tests)(func)
 
 


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -938,26 +938,20 @@
 # Note that it's not dotest's job to clean this directory.
 lldbutil.mkdir_p(configuration.test_build_dir)
 
-target_platform = lldb.selected_platform.GetTriple().split('-')[2]
+from . import lldbplatformutil
+target_platform = lldbplatformutil.getPlatform()
 
 checkLibcxxSupport()
 checkLibstdcxxSupport()
 checkWatchpointSupport()
 checkDebugInfoSupport()
 
-# Don't do debugserver tests on anything except OS X.
-configuration.dont_do_debugserver_test = (
-"linux" in target_platform or
-"freebsd" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
-
-# Don't do lldb-server (llgs) tests on platforms not supporting it.
-configuration.dont_do_llgs_test = not (
-"freebsd" in target_platform or
-"linux" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
+# Perform LLGS tests only on platforms using it.
+configuration.llgs_platform = (
+target_platform in ["freebsd", "linux", "netbsd", "windows"])
+
+# Perform debugserver tests elsewhere (i.e. on Darwin platforms).
+configuration.debugserver_platform = not configuration.llgs_platform
 
 for testdir in configuration.testdirs:
 for (dirpath, dirnames, filenames) in os.walk(testdir):
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -375,14 +375,18 @@
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
 def should_skip_debugserver_test():
-

[Lldb-commits] [PATCH] D90757: [lldb] Enable FreeBSDRemote plugin by default and update test status

2020-11-06 Thread David Chisnall via Phabricator via lldb-commits 
theraven added a comment.

Does the new plugin work with processes that are created with `pdfork`?  The 
last time I tried this, it caused the old plugin to lock up the debugger 
entirely.  Please can you ensure that there are tests that cover `pdfork` and 
`cap_enter` in the child?  These are currently quite badly broken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90757

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

[Lldb-commits] [PATCH] D88387: Create "skinny corefiles" for Mach-O with process save-core / reading

2020-11-06 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:43-45
+if (m_dirty_pages.hasValue()) {
+  m_dirty_pages.getValue().clear();
+}

This bit is unnecessary. In fact, I'd implement the entire function as `*this = 
MemoryRegionInfo();`



Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:118
+  /// detail this.
+  llvm::Optional> GetDirtyPageList() {
+return m_dirty_pages;

`const Optional<...> &` to avoid a copy.



Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:125-127
+if (m_dirty_pages.hasValue())
+  m_dirty_pages.getValue().clear();
+m_dirty_pages = pagelist;

`m_dirty_pages = std::move(pagelist);`



Comment at: lldb/include/lldb/lldb-private-interfaces.h:58-59
+   const FileSpec &outfile,
+   lldb::SaveCoreStyle requested_core_style,
+   lldb::SaveCoreStyle &created_core_style,
+   Status &error);

Maybe just have one argument which will be set on exit to reflect the actual 
style that was used?



Comment at: lldb/source/API/SBMemoryRegionInfo.cpp:142
+  addr_t dirty_page_addr = LLDB_INVALID_ADDRESS;
+  llvm::Optional> dirty_page_list =
+  m_opaque_up->GetDirtyPageList();

`const T &`



Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1742
   name, section_name ? " " : "", section_name);
+  llvm::Optional> dirty_page_list =
+  range_info.GetDirtyPageList();

const T &



Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1750-1760
+  bool print_comma = false;
+  result.AppendMessageWithFormat("Dirty pages: ");
+  for (size_t i = 0; i < page_count; i++) {
+if (print_comma)
+  result.AppendMessageWithFormat(", ");
+else
+  print_comma = true;

I think something like this ought to do:
`AppendMessageWithFormatv("Dirty pages: {0:@[x]}.\n", 
llvm::make_range(dirty_page_list->begin(), dirty_page_list->end()));`



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:225
 lldb_private::Status &error) {
+  created_core_style = SaveCoreStyle::eSaveCoreMiniDump;
   return SaveMiniDump(process_sp, outfile, error);

I don't think this is a good use of that constant. Minidumps can also come in 
"full" styles, which includes all memory, or some smaller ones, where only the 
heap (or it's modified portion, or just stack, etc.) is included.

In essence, "minidump" is just a format, just like "elf" and "macho". It's 
peculiarity is that it is different from the native object file format, but 
that's not relevant here.

I think that the best way to handle this is to make the choice of format should 
be orthogonal to the choice of what goes into the dump. Currently we don't 
allow the user to pick a format, and that's fine -- we'll just pick whatever is 
the native format for the given platform. But I don't think we should start 
mixing the two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88387

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

[Lldb-commits] [PATCH] D90757: [lldb] Enable FreeBSDRemote plugin by default and update test status

2020-11-06 Thread Kamil Rytarowski via Phabricator via lldb-commits 
krytarowski added a comment.

In D90757#2378404 , @theraven wrote:

> Does the new plugin work with processes that are created with `pdfork`?  The 
> last time I tried this, it caused the old plugin to lock up the debugger 
> entirely.  Please can you ensure that there are tests that cover `pdfork` and 
> `cap_enter` in the child?  These are currently quite badly broken.

Can you show an example of a program (ideally minimal) that is supposed to work 
but hangs? Does GDB support pdfork-ed children?

There is also TRAP_CAP and I'm not sure what should we do upon it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90757

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

[Lldb-commits] [PATCH] D90875: [lldb] [test] Avoid double negations in llgs/debugserver logic

2020-11-06 Thread Michał Górny via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1a8d52820f8e: [lldb] [test] Avoid double negation in 
llgs/debugserver logic (authored by mgorny).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90875

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -938,26 +938,20 @@
 # Note that it's not dotest's job to clean this directory.
 lldbutil.mkdir_p(configuration.test_build_dir)
 
-target_platform = lldb.selected_platform.GetTriple().split('-')[2]
+from . import lldbplatformutil
+target_platform = lldbplatformutil.getPlatform()
 
 checkLibcxxSupport()
 checkLibstdcxxSupport()
 checkWatchpointSupport()
 checkDebugInfoSupport()
 
-# Don't do debugserver tests on anything except OS X.
-configuration.dont_do_debugserver_test = (
-"linux" in target_platform or
-"freebsd" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
-
-# Don't do lldb-server (llgs) tests on platforms not supporting it.
-configuration.dont_do_llgs_test = not (
-"freebsd" in target_platform or
-"linux" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
+# Perform LLGS tests only on platforms using it.
+configuration.llgs_platform = (
+target_platform in ["freebsd", "linux", "netbsd", "windows"])
+
+# Perform debugserver tests elsewhere (i.e. on Darwin platforms).
+configuration.debugserver_platform = not configuration.llgs_platform
 
 for testdir in configuration.testdirs:
 for (dirpath, dirnames, filenames) in os.walk(testdir):
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -375,14 +375,18 @@
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
 def should_skip_debugserver_test():
-return "debugserver tests" if configuration.dont_do_debugserver_test 
else None
+return ("debugserver tests"
+if not configuration.debugserver_platform
+else None)
 return skipTestIfFn(should_skip_debugserver_test)(func)
 
 
 def llgs_test(func):
 """Decorate the item as a lldb-server test."""
 def should_skip_llgs_tests():
-return "llgs tests" if configuration.dont_do_llgs_test else None
+return ("llgs tests"
+if not configuration.llgs_platform
+else None)
 return skipTestIfFn(should_skip_llgs_tests)(func)
 
 


Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -938,26 +938,20 @@
 # Note that it's not dotest's job to clean this directory.
 lldbutil.mkdir_p(configuration.test_build_dir)
 
-target_platform = lldb.selected_platform.GetTriple().split('-')[2]
+from . import lldbplatformutil
+target_platform = lldbplatformutil.getPlatform()
 
 checkLibcxxSupport()
 checkLibstdcxxSupport()
 checkWatchpointSupport()
 checkDebugInfoSupport()
 
-# Don't do debugserver tests on anything except OS X.
-configuration.dont_do_debugserver_test = (
-"linux" in target_platform or
-"freebsd" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
-
-# Don't do lldb-server (llgs) tests on platforms not supporting it.
-configuration.dont_do_llgs_test = not (
-"freebsd" in target_platform or
-"linux" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
+# Perform LLGS tests only on platforms using it.
+configuration.llgs_platform = (
+target_platform in ["freebsd", "linux", "netbsd", "windows"])
+
+# Perform debugserver tests elsewhere (i.e. on Darwin platforms).
+configuration.debugserver_platform = not configuration.llgs_platform
 
 for testdir in configuration.testdirs:
 for (dirpath, dirnames, filenames) in os.walk(testdir):
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -375,14

[Lldb-commits] [lldb] 1a8d528 - [lldb] [test] Avoid double negation in llgs/debugserver logic

2020-11-06 Thread Michał Górny via lldb-commits 

Author: Michał Górny
Date: 2020-11-06T14:34:12+01:00
New Revision: 1a8d52820f8e55f66567011e0ed852794b7f687d

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

LOG: [lldb] [test] Avoid double negation in llgs/debugserver logic

Use positive logic (i.e. llgs_platform/debugserver_platform) for
indicating which platforms use the particular server variant.
Deduplicate the lists — it is rather expected that none of the platforms
using LLGS would use debugserver.

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

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/decorators.py
lldb/packages/Python/lldbsuite/test/dotest.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/decorators.py 
b/lldb/packages/Python/lldbsuite/test/decorators.py
index 358005dba70f..4dfc7b108ff3 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -375,14 +375,18 @@ def should_skip_simulator_test():
 def debugserver_test(func):
 """Decorate the item as a debugserver test."""
 def should_skip_debugserver_test():
-return "debugserver tests" if configuration.dont_do_debugserver_test 
else None
+return ("debugserver tests"
+if not configuration.debugserver_platform
+else None)
 return skipTestIfFn(should_skip_debugserver_test)(func)
 
 
 def llgs_test(func):
 """Decorate the item as a lldb-server test."""
 def should_skip_llgs_tests():
-return "llgs tests" if configuration.dont_do_llgs_test else None
+return ("llgs tests"
+if not configuration.llgs_platform
+else None)
 return skipTestIfFn(should_skip_llgs_tests)(func)
 
 

diff  --git a/lldb/packages/Python/lldbsuite/test/dotest.py 
b/lldb/packages/Python/lldbsuite/test/dotest.py
index 420c52a07427..5a81011b3583 100644
--- a/lldb/packages/Python/lldbsuite/test/dotest.py
+++ b/lldb/packages/Python/lldbsuite/test/dotest.py
@@ -938,26 +938,20 @@ def run_suite():
 # Note that it's not dotest's job to clean this directory.
 lldbutil.mkdir_p(configuration.test_build_dir)
 
-target_platform = lldb.selected_platform.GetTriple().split('-')[2]
+from . import lldbplatformutil
+target_platform = lldbplatformutil.getPlatform()
 
 checkLibcxxSupport()
 checkLibstdcxxSupport()
 checkWatchpointSupport()
 checkDebugInfoSupport()
 
-# Don't do debugserver tests on anything except OS X.
-configuration.dont_do_debugserver_test = (
-"linux" in target_platform or
-"freebsd" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
-
-# Don't do lldb-server (llgs) tests on platforms not supporting it.
-configuration.dont_do_llgs_test = not (
-"freebsd" in target_platform or
-"linux" in target_platform or
-"netbsd" in target_platform or
-"windows" in target_platform)
+# Perform LLGS tests only on platforms using it.
+configuration.llgs_platform = (
+target_platform in ["freebsd", "linux", "netbsd", "windows"])
+
+# Perform debugserver tests elsewhere (i.e. on Darwin platforms).
+configuration.debugserver_platform = not configuration.llgs_platform
 
 for testdir in configuration.testdirs:
 for (dirpath, dirnames, filenames) in os.walk(testdir):



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

[Lldb-commits] [PATCH] D90938: [lldb] [Process/FreeBSDRemote] Handle exec() from inferior

2020-11-06 Thread Michał Górny via Phabricator via lldb-commits 
mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski.
mgorny requested review of this revision.

https://reviews.llvm.org/D90938

Files:
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/test/API/functionalities/exec/TestExec.py


Index: lldb/test/API/functionalities/exec/TestExec.py
===
--- lldb/test/API/functionalities/exec/TestExec.py
+++ lldb/test/API/functionalities/exec/TestExec.py
@@ -20,7 +20,6 @@
 @expectedFailureAll(oslist=["ios", "tvos", "watchos", "bridgeos"], 
bugnumber="rdar://problem/34559552") # this exec test has problems on ios 
systems
 @expectedFailureNetBSD
 @skipIfAsan # rdar://problem/43756823
-@skipIfFreeBSD  # hangs
 @skipIfWindows
 def test_hitting_exec (self):
 self.do_test(False)
@@ -29,7 +28,6 @@
 @expectedFailureAll(oslist=["ios", "tvos", "watchos", "bridgeos"], 
bugnumber="rdar://problem/34559552") # this exec test has problems on ios 
systems
 @expectedFailureNetBSD
 @skipIfAsan # rdar://problem/43756823
-@skipIfFreeBSD  # hangs
 @skipIfWindows
 def test_skipping_exec (self):
 self.do_test(True)
Index: lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
===
--- lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
+++ lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
@@ -211,6 +211,22 @@
 return;
   }
 
+  if (info.pl_flags & PL_FLAG_EXEC) {
+Status error = ReinitializeThreads();
+if (error.Fail()) {
+  SetState(StateType::eStateInvalid);
+  return;
+}
+
+// Let our delegate know we have just exec'd.
+NotifyDidExec();
+
+for (const auto &thread : m_threads)
+  static_cast(*thread).SetStoppedByExec();
+SetState(StateType::eStateStopped, true);
+return;
+  }
+
   if (info.pl_lwpid > 0) {
 for (const auto &t : m_threads) {
   if (t->GetID() == static_cast(info.pl_lwpid))


Index: lldb/test/API/functionalities/exec/TestExec.py
===
--- lldb/test/API/functionalities/exec/TestExec.py
+++ lldb/test/API/functionalities/exec/TestExec.py
@@ -20,7 +20,6 @@
 @expectedFailureAll(oslist=["ios", "tvos", "watchos", "bridgeos"], bugnumber="rdar://problem/34559552") # this exec test has problems on ios systems
 @expectedFailureNetBSD
 @skipIfAsan # rdar://problem/43756823
-@skipIfFreeBSD  # hangs
 @skipIfWindows
 def test_hitting_exec (self):
 self.do_test(False)
@@ -29,7 +28,6 @@
 @expectedFailureAll(oslist=["ios", "tvos", "watchos", "bridgeos"], bugnumber="rdar://problem/34559552") # this exec test has problems on ios systems
 @expectedFailureNetBSD
 @skipIfAsan # rdar://problem/43756823
-@skipIfFreeBSD  # hangs
 @skipIfWindows
 def test_skipping_exec (self):
 self.do_test(True)
Index: lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
===
--- lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
+++ lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
@@ -211,6 +211,22 @@
 return;
   }
 
+  if (info.pl_flags & PL_FLAG_EXEC) {
+Status error = ReinitializeThreads();
+if (error.Fail()) {
+  SetState(StateType::eStateInvalid);
+  return;
+}
+
+// Let our delegate know we have just exec'd.
+NotifyDidExec();
+
+for (const auto &thread : m_threads)
+  static_cast(*thread).SetStoppedByExec();
+SetState(StateType::eStateStopped, true);
+return;
+  }
+
   if (info.pl_lwpid > 0) {
 for (const auto &t : m_threads) {
   if (t->GetID() == static_cast(info.pl_lwpid))
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D90938: [lldb] [Process/FreeBSDRemote] Handle exec() from inferior

2020-11-06 Thread Kamil Rytarowski via Phabricator via lldb-commits 
krytarowski accepted this revision.
krytarowski added a comment.
This revision is now accepted and ready to land.

I wonder why NetBSD fails having the same event handling.


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

https://reviews.llvm.org/D90938

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

[Lldb-commits] [PATCH] D90769: [lldb][ObjectFile] Relocate sections for in-memory objects (e.g. received via JITLoaderGDB)

2020-11-06 Thread Stefan Gränitz via Phabricator via lldb-commits 
sgraenitz added a comment.

That's right, but I don't see how even the static ones could be resolved. 
Wouldn't it hard-wire the objects together? In the next session we want to load 
some object files from cache while others have changed.. Maybe one day I should 
check how ccache and the likes handle it.

IIRC OSO entries in MachO files point to the object file on disk, which allows 
LLDB to load them lazily? The DWARF parsing for ELF should follow the same 
laziness: each section is only relocated once we need it. It's just not loading 
the object lazily right? However, we couldn't do that even with OSO-style 
entries, because there is no "pull mechanism" in JITLoaderGDB :) Well, for now 
it just works.

I will leave the review here for another day or so, in case there's more 
feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90769

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

[Lldb-commits] [PATCH] D90857: [lldb] add a missing dependency on intrinsics_gen

2020-11-06 Thread Nathan Lanza via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG137ff7331705: [lldb] add a missing dependency on 
intrinsics_gen (authored by rmaz, committed by lanza).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D90857?vs=303468&id=303496#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90857

Files:
  lldb/source/Symbol/CMakeLists.txt


Index: lldb/source/Symbol/CMakeLists.txt
===
--- lldb/source/Symbol/CMakeLists.txt
+++ lldb/source/Symbol/CMakeLists.txt
@@ -39,6 +39,9 @@
 
   ${PLATFORM_SOURCES}
 
+  DEPENDS
+intrinsics_gen
+
   LINK_LIBS
 lldbCore
 lldbExpression


Index: lldb/source/Symbol/CMakeLists.txt
===
--- lldb/source/Symbol/CMakeLists.txt
+++ lldb/source/Symbol/CMakeLists.txt
@@ -39,6 +39,9 @@
 
   ${PLATFORM_SOURCES}
 
+  DEPENDS
+intrinsics_gen
+
   LINK_LIBS
 lldbCore
 lldbExpression
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 137ff73 - [lldb] add a missing dependency on intrinsics_gen

2020-11-06 Thread Nathan Lanza via lldb-commits 

Author: Richard Howell
Date: 2020-11-06T13:15:08-05:00
New Revision: 137ff7331705179c83533a074d800c481b7df1ac

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

LOG: [lldb] add a missing dependency on intrinsics_gen

Sometimes builds will fail with errors like:

```
In file included from 
/build/external/llvm-project/lldb/source/Symbol/SwiftASTContext.cpp:52:
In file included from /build/external/swift/include/swift/IRGen/Linking.h:22:
In file included from /build/external/swift/include/swift/SIL/SILFunction.h:24:
In file included from 
/build/external/swift/include/swift/SIL/SILBasicBlock.h:23:
In file included from 
/build/external/swift/include/swift/SIL/SILInstruction.h:21:
In file included from /build/external/swift/include/swift/AST/Builtins.h:24:
**/build/external/llvm-project/llvm/include/llvm/IR/Attributes.h:74:14: fatal 
error: 'llvm/IR/Attributes.inc' file not found**
**^~~~**
```
This change ensures the `Attributes.inc` file is generated before building 
`SwiftASTContext.cpp`.

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

Added: 


Modified: 
lldb/source/Symbol/CMakeLists.txt

Removed: 




diff  --git a/lldb/source/Symbol/CMakeLists.txt 
b/lldb/source/Symbol/CMakeLists.txt
index 95bf156ff538..2ce0b6887e5f 100644
--- a/lldb/source/Symbol/CMakeLists.txt
+++ b/lldb/source/Symbol/CMakeLists.txt
@@ -39,6 +39,9 @@ add_lldb_library(lldbSymbol
 
   ${PLATFORM_SOURCES}
 
+  DEPENDS
+intrinsics_gen
+
   LINK_LIBS
 lldbCore
 lldbExpression



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

[Lldb-commits] [PATCH] D90857: [lldb] add a missing dependency on intrinsics_gen

2020-11-06 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor added a comment.

Just to clarify: This only a dependency in the downstream Swift branch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90857

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

[Lldb-commits] [PATCH] D90987: [lldb] Avoid confusing crashes during reproducer replay when initialization failed

2020-11-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere created this revision.
JDevlieghere added a reviewer: vsk.
JDevlieghere requested review of this revision.

During active replay, the `::Initialize` call is replayed like any other SB API
call and the return value is ignored. Since we can't intercept this, we
terminate here before the uninitialized debugger inevitably crashes.


https://reviews.llvm.org/D90987

Files:
  lldb/source/API/SystemInitializerFull.cpp


Index: lldb/source/API/SystemInitializerFull.cpp
===
--- lldb/source/API/SystemInitializerFull.cpp
+++ lldb/source/API/SystemInitializerFull.cpp
@@ -17,6 +17,7 @@
 #include "lldb/Target/ProcessTrace.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/Support/TargetSelect.h"
+#include "lldb/Utility/Reproducer.h"
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wglobal-constructors"
@@ -24,6 +25,7 @@
 #pragma clang diagnostic pop
 
 #include 
+#include 
 
 #define LLDB_PLUGIN(p) LLDB_PLUGIN_DECLARE(p)
 #include "Plugins/Plugins.def"
@@ -34,8 +36,16 @@
 SystemInitializerFull::~SystemInitializerFull() = default;
 
 llvm::Error SystemInitializerFull::Initialize() {
-  if (auto e = SystemInitializerCommon::Initialize())
-return e;
+  llvm::Error error = SystemInitializerCommon::Initialize();
+  if (error) {
+// During active replay, the ::Initialize call is replayed like any other
+// SB API call and the return value is ignored. Since we can't intercept
+// this, we terminate here before the uninitialized debugger inevitably
+// crashes.
+if (repro::Reproducer::Instance().IsReplaying())
+  std::_Exit(EXIT_FAILURE);
+return error;
+  }
 
   // Initialize LLVM and Clang
   llvm::InitializeAllTargets();


Index: lldb/source/API/SystemInitializerFull.cpp
===
--- lldb/source/API/SystemInitializerFull.cpp
+++ lldb/source/API/SystemInitializerFull.cpp
@@ -17,6 +17,7 @@
 #include "lldb/Target/ProcessTrace.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/Support/TargetSelect.h"
+#include "lldb/Utility/Reproducer.h"
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wglobal-constructors"
@@ -24,6 +25,7 @@
 #pragma clang diagnostic pop
 
 #include 
+#include 
 
 #define LLDB_PLUGIN(p) LLDB_PLUGIN_DECLARE(p)
 #include "Plugins/Plugins.def"
@@ -34,8 +36,16 @@
 SystemInitializerFull::~SystemInitializerFull() = default;
 
 llvm::Error SystemInitializerFull::Initialize() {
-  if (auto e = SystemInitializerCommon::Initialize())
-return e;
+  llvm::Error error = SystemInitializerCommon::Initialize();
+  if (error) {
+// During active replay, the ::Initialize call is replayed like any other
+// SB API call and the return value is ignored. Since we can't intercept
+// this, we terminate here before the uninitialized debugger inevitably
+// crashes.
+if (repro::Reproducer::Instance().IsReplaying())
+  std::_Exit(EXIT_FAILURE);
+return error;
+  }
 
   // Initialize LLVM and Clang
   llvm::InitializeAllTargets();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D90987: [lldb] Avoid confusing crashes during reproducer replay when initialization failed

2020-11-06 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 303581.
JDevlieghere added a comment.

clang-format


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

https://reviews.llvm.org/D90987

Files:
  lldb/source/API/SystemInitializerFull.cpp


Index: lldb/source/API/SystemInitializerFull.cpp
===
--- lldb/source/API/SystemInitializerFull.cpp
+++ lldb/source/API/SystemInitializerFull.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Initialization/SystemInitializerCommon.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Target/ProcessTrace.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/Support/TargetSelect.h"
 
@@ -23,6 +24,7 @@
 #include "llvm/ExecutionEngine/MCJIT.h"
 #pragma clang diagnostic pop
 
+#include 
 #include 
 
 #define LLDB_PLUGIN(p) LLDB_PLUGIN_DECLARE(p)
@@ -34,8 +36,16 @@
 SystemInitializerFull::~SystemInitializerFull() = default;
 
 llvm::Error SystemInitializerFull::Initialize() {
-  if (auto e = SystemInitializerCommon::Initialize())
-return e;
+  llvm::Error error = SystemInitializerCommon::Initialize();
+  if (error) {
+// During active replay, the ::Initialize call is replayed like any other
+// SB API call and the return value is ignored. Since we can't intercept
+// this, we terminate here before the uninitialized debugger inevitably
+// crashes.
+if (repro::Reproducer::Instance().IsReplaying())
+  std::_Exit(EXIT_FAILURE);
+return error;
+  }
 
   // Initialize LLVM and Clang
   llvm::InitializeAllTargets();


Index: lldb/source/API/SystemInitializerFull.cpp
===
--- lldb/source/API/SystemInitializerFull.cpp
+++ lldb/source/API/SystemInitializerFull.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Initialization/SystemInitializerCommon.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Target/ProcessTrace.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/Support/TargetSelect.h"
 
@@ -23,6 +24,7 @@
 #include "llvm/ExecutionEngine/MCJIT.h"
 #pragma clang diagnostic pop
 
+#include 
 #include 
 
 #define LLDB_PLUGIN(p) LLDB_PLUGIN_DECLARE(p)
@@ -34,8 +36,16 @@
 SystemInitializerFull::~SystemInitializerFull() = default;
 
 llvm::Error SystemInitializerFull::Initialize() {
-  if (auto e = SystemInitializerCommon::Initialize())
-return e;
+  llvm::Error error = SystemInitializerCommon::Initialize();
+  if (error) {
+// During active replay, the ::Initialize call is replayed like any other
+// SB API call and the return value is ignored. Since we can't intercept
+// this, we terminate here before the uninitialized debugger inevitably
+// crashes.
+if (repro::Reproducer::Instance().IsReplaying())
+  std::_Exit(EXIT_FAILURE);
+return error;
+  }
 
   // Initialize LLVM and Clang
   llvm::InitializeAllTargets();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results

2020-11-06 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie added inline comments.



Comment at: lldb/source/Expression/DWARFExpression.cpp:944
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+  auto to_generic = [addr_size = opcodes.GetAddressByteSize()](auto v) {
+bool is_signed = std::is_signed::value;

Local/non-escaping lambdas may be simpler written with default ref capture - is 
GetAddressByteSize() expensive or would it be fine to call it every time this 
lambda is called? (& capturing opcodes by ref)



Comment at: lldb/source/Expression/DWARFExpression.cpp:945
+  auto to_generic = [addr_size = opcodes.GetAddressByteSize()](auto v) {
+bool is_signed = std::is_signed::value;
+return Scalar(

labath wrote:
> This is probably not conforming either -- dwarf says the generic type is of 
> "unspecified signedness", but I think it means a single global value -- not 
> choosing signedness on a per-operand basis.
> 
> I've kept that for now, since that was the original behavior, though I didn't 
> notice any issues with this bit removed.
Fair, would be good to mark it as at least suspect/uncertain with a comment or 
such. (or if there's some inverse that would assert if this ever did become 
relevant, that assertion would be great - but I guess there probably isn't 
anything we can assert to check that this doesn't matter in practice)

Re: non-conforming: I think the presence of constu and consts encodings that 
build "generic type"d values probably means the generic type itself may have no 
particular notion of signedness, but the values that populate it do and I guess 
intend the value to be interpreted contextually or to carry the sign 
information? I think it's probably good to carry the sign information from the 
operands, at least. Maybe with some truncation/sign extension. It's probably 
pretty close to right... 



Comment at: lldb/source/Expression/DWARFExpression.cpp:1270
 case DW_OP_const1u:
-  stack.push_back(Scalar((uint8_t)opcodes.GetU8(&offset)));
+  stack.emplace_back(to_generic(opcodes.GetU8(&offset)));
   break;

Why the change to emplace_back? Generally I'd advocate for push_back where 
possible, since it can't invoke explicit conversions which can require more 
scrutiny (eg: a vector of unique_ptrs can have a unique_ptr rvalue push_back'd, 
but can't have a raw pointer push_back'd (a raw pointer would have to be 
emplace_back'd) - so when I see push_back I can assume it's a "safe" operation)



Comment at: lldb/source/Expression/DWARFExpression.cpp:1293-1300
+// These should also use to_generic, but we can't do that due to a
+// producer-side bug in llvm. See llvm.org/pr48087.
 case DW_OP_constu:
-  stack.push_back(Scalar(opcodes.GetULEB128(&offset)));
+  stack.emplace_back(opcodes.GetULEB128(&offset));
   break;
 case DW_OP_consts:
+  stack.emplace_back(opcodes.GetSLEB128(&offset));

Seems fair


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90840

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

[Lldb-commits] [PATCH] D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands

2020-11-06 Thread walter erquinigo via Phabricator via lldb-commits 
wallace updated this revision to Diff 303614.
wallace marked 14 inline comments as done.
wallace added a comment.

address all comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90729

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Interpreter/CommandObjectDelegate.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CMakeLists.txt
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/CommandObjectThreadUtil.cpp
  lldb/source/Commands/CommandObjectThreadUtil.h
  lldb/source/Core/PluginManager.cpp
  lldb/source/Interpreter/CMakeLists.txt
  lldb/source/Interpreter/CommandObject.cpp
  lldb/source/Interpreter/CommandObjectDelegate.cpp
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py

Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -0,0 +1,67 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceLoad(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+TestBase.setUp(self)
+if 'intel-pt' not in configuration.enabled_plugins:
+self.skipTest("The intel-pt test plugin is not enabled")
+
+def expectGenericHelpMessageForStartCommand(self):
+self.expect("help thread trace start",
+substrs=["Syntax: thread trace start []"])
+
+def testStartStopSessionFileThreads(self):
+# it should fail for processes from json session files
+self.expect("trace load -v " + os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"))
+self.expect("thread trace start", error=True,
+substrs=["error: tracing is not supported for the current process. Not implemented"])
+
+# the help command should be the generic one, as it's not a live process
+self.expectGenericHelpMessageForStartCommand()
+
+# this should fail because 'trace stop' is not yet implemented
+self.expect("thread trace stop", error=True,
+substrs=["error: failed stopping thread 3842849"])
+
+@skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
+def testStartStopLiveThreads(self):
+# The help command should be the generic one if there's no process running
+self.expectGenericHelpMessageForStartCommand()
+
+self.expect("file " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+self.expect("b main")
+
+# The help command should be the generic one if there's still no process running
+self.expectGenericHelpMessageForStartCommand()
+
+self.expect("r")
+
+# the help command should be the intel-pt one now
+self.expect("help thread trace start",
+substrs=["Start tracing one or more threads with intel-pt.",
+ "Syntax: thread trace start [  ...] []"])
+
+self.expect("thread trace start",
+patterns=["would trace tid .* with size_in_kb 4 and custom_config 0"])
+
+self.expect("thread trace start --size 20 --custom-config 1",
+patterns=["would trace tid .* with size_in_kb 20 and custom_config 1"])
+
+# This fails because "trace stop" is not yet implemented.
+self.expect("thread trace stop", error=True,
+substrs=["error: Process is not being traced"])
+
+self.expect("c")
+# Now the process has finished, so the commands should fail
+self.expect("thread trace start", error=True,
+substrs=["error: Process must be launched."])
+
+self.expect("thread trace stop", error=True,
+substrs=["error: Process must be launched."])
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- lldb/test/API/commands/trace/TestTraceDumpInstructions.py
+++ lldb/test/API/commands/trace/TestTraceDumpInstructions.py
@@ -32,7 +32,7 @@
 self.expect("run")
 
 self.expect("thread trace dump instructions",
-substrs=["error: this thread is not being traced"],
+substrs=["error: Process is not being traced"],
 error=True)
 
 def testRawDumpInstructions(self):
Index: lldb/source/Plugins/Trace/intel-pt/Trace