[Lldb-commits] [PATCH] D46810: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
labath added a comment. I **think** I understand what is going on here, but I can't say for certain. Could you elaborate on the implementation details of this somewhere. It would be good to keep a note of this for future maintainers. My understanding of this is: - if nothing has been parsed, m_die_array is empty and m_first_die is empty - if the cu die has been parsed, m_die_array us empty and m_first_die is full - if everything has been parsed m_first_die and m_die_array are full, **and** the first element of m_die_array contains a copy of m_first_die Is that an accurate description of what you are doing here? (If that is true, then I think this is workable, but there are still some details which need to be ironed out; e.g., `m_first_die.GetFirstChild()`) https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46726: build: use cmake to find the libedit content
labath added inline comments.
Comment at: cmake/modules/FindLibEdit.cmake:11-14
+# libedit_FOUND - true if libedit was found
+# libedit_INCLUDE_DIRS - include search path
+# libedit_LIBRARIES - libraries to link
+# libedit_VERSION - version number
compnerd wrote:
> labath wrote:
> > Should we make the capitalization of these match the file name
> > (LibEdit_FOUND) ?
> *shrug* I'm not tied to that. I found `LibEdit_FOUND` to be a bit jarring,
> which is why I went with `libedit_FOUND` which also mirror's the project's
> actual spelling (`libedit`).
Ok, whatever you prefer.
Comment at: cmake/modules/FindLibEdit.cmake:53-54
+set(libedit_VERSION_STRING
"${libedit_major_version}.${libedit_minor_version}")
+unset(libedit_major_version)
+unset(libedit_minor_version)
+ endif()
compnerd wrote:
> labath wrote:
> > Do you really need to unset these? I would hope that this file is evaluated
> > in a fresh context, and it won't pollute the callers namespace or anything..
> I'm pretty sure that they get evaluated in the global context :-(.
I just tried removing these and putting a `message` command after the
`find_package` call. The variables did not get exported.
However, I think we have a bigger issue here. If I apply your patch without any
modifications, I get an error first time I run it because of unrecognised
sequences in the regular expressions (`\s`, `\d`). I guess that's why you've
changed these to `[ \t]` and `[0-9]` in the first regex.
What particularly worries me is that the second time I ran cmake, without any
modifications, it succeeded, presumably because we took the true branch in the
`if` statement at the top. This looks wrong.
I think we should just remove the if check -- `find_path` and `find_library`
should already have caching internally, and if you want to cache the version
computation, then that should be guarded by a separate variable.
https://reviews.llvm.org/D46726
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty.
labath added a comment. Preserving the dot if it is the only path component is perfectly reasonable -- "" is not a valid path and we shouldn't convert a valid path into an invalid one. However, I think this should be done on the llvm side, in the `remote_dots` function and not as a workaround on top of it. https://reviews.llvm.org/D46783 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r332162 - [LLDB] Support GNU-style compressed debug sections (.zdebug)
I would suggest testing this the way we have started testing all other ObjectFile changes -- yaml2obj + lldb-test (see tests in lit/Modules). A full-scale integration test is interesting, but I would consider it optional, as this patch touches no files outside the object layer. On Sat, 12 May 2018 at 01:57, Davide Italiano via lldb-commits < lldb-commits@lists.llvm.org> wrote: > On Fri, May 11, 2018 at 5:29 PM, Davide Italiano via lldb-commits > wrote: > > Author: davide > > Date: Fri May 11 17:29:25 2018 > > New Revision: 332162 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=332162&view=rev > > Log: > > [LLDB] Support GNU-style compressed debug sections (.zdebug) > > > > Patch by Erik Welander! > > > Erik, I'm going to revert this because it broke the ubuntu 14.04 bot. > Please take a look and I'll recommit it for you once it's fixed. > Log: http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/23184 > Culprit: > ``` > Build Command Output: > objcopy: option '--compress-debug-sections' doesn't allow an argument > ``` > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil added a comment. In https://reviews.llvm.org/D46810#1097503, @labath wrote: > - if nothing has been parsed, m_die_array is empty and m_first_die is empty > - if the cu die has been parsed, m_die_array us empty and m_first_die is full > - if everything has been parsed m_first_die and m_die_array are full, **and** > the first element of m_die_array contains a copy of m_first_die Is that an > accurate description of what you are doing here? Yes. I will add a comment to the code. > (If that is true, then I think this is workable, but there are still some > details which need to be ironed out; e.g., `m_first_die.GetFirstChild()`) The current code calling `GetUnitDIEOnly()` (returning `DWARFDIE`) or `GetUnitDIEPtrOnly` (returning `DWARFDebugInfoEntry *`) is never dealing with child DIEs of what it gets - it also makes sense as there is no guarantee they have been read in. Such code would already cause ASAN error accessing memory behind `std::vector`. So I find such assertion check orthogonal to this patch but I will prepare another patch for that. Thanks for checking validity of this patch, I was not sure whether LLDB code is intended to be thread-safe in the first place. https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
labath added a comment. In https://reviews.llvm.org/D46810#1097740, @jankratochvil wrote: > In https://reviews.llvm.org/D46810#1097503, @labath wrote: > > > (If that is true, then I think this is workable, but there are still some > > details which need to be ironed out; e.g., `m_first_die.GetFirstChild()`) > > > The current code calling `GetUnitDIEOnly()` (returning `DWARFDIE`) or > `GetUnitDIEPtrOnly` (returning `DWARFDebugInfoEntry *`) is never dealing with > child DIEs of what it gets - it also makes sense as there is no guarantee > they have been read in. I am not sure it's that simple. I've found at least one case (`SymbolFileDWARF::ParseImportedModuleswhere we call GetFirstChild() on the value returned by `GetUnitDIEOnly` (there may be others which are not easily greppable). Previously that would work if one would call this only after he'd know that all DIEs have been parsed. Now this will never work because GetFirstChild will return whatever is in the memory after `m_first_die`. I am not sure if this would be caught by asan straight away, though it will most likely cause a crash very soon. I was thinking of making this safer by changing the `GetUnitDIEOnly` so that the caller has to explicitly request (either with an argument, or by splitting it into two functions) whether it wants a CU die, which can be used to access other DIEs, or just a bare attribute-only DIE. In second case, we would return &m_first_die, in the first case &m_die_array[0] (after making sure it's properly initialized). Then `m_first_die` can have `has_children` property set to false to enforce that noone accesses children that way. WDYT? > Thanks for checking validity of this patch, I was not sure whether LLDB code > is intended to be thread-safe in the first place. Yeah, thread safety is tricky. I think the DWARF parser was very single-threaded originally. Then, when we started building the index in a multi-threaded manner we needed to make a bunch of code thread-safe, which wasn't before. Now it looks like you are introducing even paralelization at an even deeper level (can't say I understand full what that means yet), so we'll need to make more code thread-safe. https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5
labath added a comment. Sounds like a plan. https://reviews.llvm.org/D32167 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil added a comment. In https://reviews.llvm.org/D46810#1097829, @labath wrote: > I am not sure it's that simple. I've found at least one case > (`SymbolFileDWARF::ParseImportedModuleswhere we call GetFirstChild() on the > value returned by `GetUnitDIEOnly` OK, that is clearly a bug there, thanks for finding it. I see some new assertion check would be good (I may try even a compile time check by some new class wrapper). `SymbolFileDWARF::ParseImportedModules` should just call `DIE()`. DWARFDIE GetUnitDIEOnly() -> DWARFDebugInfoEntry *GetUnitDIEPtrOnly() -> ExtractDIEsIfNeeded(true ) -> CU DIE only DWARFDIEDIE() -> DWARFDebugInfoEntry * DIEPtr() -> ExtractDIEsIfNeeded(false) -> all DIEs > I was thinking of making this safer by changing the `GetUnitDIEOnly` so that > the caller has to explicitly request (either with an argument, or by > splitting it into two functions) whether it wants a CU die, which can be used > to access other DIEs, or just a bare attribute-only DIE. In second case, we > would return &m_first_die, in the first case &m_die_array[0] (after making > sure it's properly initialized). But there is already such function, it is called `DIE()`, we need no new parameter. Maybe we should rename those functions, their current names are cryptic, what about changing the second line to: DWARFDIE GetUnitDIEOnly() -> DWARFDebugInfoEntry *GetUnitDIEPtrOnly() -> ExtractDIEsIfNeeded(true ) -> CU DIE only DWARFDIE GetUnitAllDIEs() -> DWARFDebugInfoEntry *GetUnitAllDIEsPtr() -> ExtractDIEsIfNeeded(false) -> all DIEs > Then `m_first_die` can have `has_children` property set to false to enforce > that noone accesses children that way. I would prefer an assert (or even a compilation error by some wrapper class) in such case but I agree this may be enough. https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D46810: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
On Mon, 14 May 2018 at 15:24, Jan Kratochvil via Phabricator < revi...@reviews.llvm.org> wrote: > jankratochvil added a comment. > In https://reviews.llvm.org/D46810#1097829, @labath wrote: > > I am not sure it's that simple. I've found at least one case (`SymbolFileDWARF::ParseImportedModuleswhere we call GetFirstChild() on the value returned by `GetUnitDIEOnly` > OK, that is clearly a bug there, thanks for finding it. I see some new assertion check would be good (I may try even a compile time check by some new class wrapper). `SymbolFileDWARF::ParseImportedModules` should just call `DIE()`. >DWARFDIE GetUnitDIEOnly() -> DWARFDebugInfoEntry *GetUnitDIEPtrOnly() -> ExtractDIEsIfNeeded(true ) -> CU DIE only >DWARFDIEDIE() -> DWARFDebugInfoEntry * DIEPtr() -> ExtractDIEsIfNeeded(false) -> all DIEs > > I was thinking of making this safer by changing the `GetUnitDIEOnly` so that the caller has to explicitly request (either with an argument, or by splitting it into two functions) whether it wants a CU die, which can be used to access other DIEs, or just a bare attribute-only DIE. In second case, we would return &m_first_die, in the first case &m_die_array[0] (after making sure it's properly initialized). > But there is already such function, it is called `DIE()`, we need no new parameter. Ah, I see. Thanks for pointing that out (I'm fairly new to this part of the code as well :)). > Maybe we should rename those functions, their current names are cryptic, what about changing the second line to: >DWARFDIE GetUnitDIEOnly() -> DWARFDebugInfoEntry *GetUnitDIEPtrOnly() -> ExtractDIEsIfNeeded(true ) -> CU DIE only >DWARFDIE GetUnitAllDIEs() -> DWARFDebugInfoEntry *GetUnitAllDIEsPtr() -> ExtractDIEsIfNeeded(false) -> all DIEs I'm not opposed to that, though I'm not sure if it makes things substantially better. I'm not sure if this is a thing that can be succinctly expressed in a name (DIEWhichCanBeUsedToAccessOtherDIEs). Maybe just a comment explaining their difference would be enough. > > Then `m_first_die` can have `has_children` property set to false to enforce that noone accesses children that way. > I would prefer an assert (or even a compilation error by some wrapper class) in such case but I agree this may be enough. Since it seems this is how things worked already, I don't think we need to go out of our way here. If you can get an assert there then great. Otherwise, I think the has_children should be enough. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46753: FileSpec: Remove PathSyntax enum and use llvm version instead
labath added a comment. We don't export the style right now. At some point we may need to teach SBFileSpec about path styles, but at that point I'd just do the translation at the SB level, and let everyone else use the llvm enum. https://reviews.llvm.org/D46753 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r332247 - FileSpec: Remove PathSyntax enum and use llvm version instead
Author: labath
Date: Mon May 14 07:52:47 2018
New Revision: 332247
URL: http://llvm.org/viewvc/llvm-project?rev=332247&view=rev
Log:
FileSpec: Remove PathSyntax enum and use llvm version instead
Summary:
The llvm version of the enum has the same enumerators, with stlightly
different names, so this is mostly just a search&replace exercise. One
concrete benefit of this is that we can remove the function for
converting between the two enums.
To avoid typing llvm::sys::path::Style::windows everywhere I import the
enum into the FileSpec class, so it can be referenced as
FileSpec::Style::windows.
Reviewers: zturner, clayborg
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D46753
Modified:
lldb/trunk/include/lldb/Utility/FileSpec.h
lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
lldb/trunk/source/Utility/FileSpec.cpp
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
lldb/trunk/unittests/Target/ModuleCacheTest.cpp
lldb/trunk/unittests/Utility/FileSpecTest.cpp
Modified: lldb/trunk/include/lldb/Utility/FileSpec.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/FileSpec.h?rev=332247&r1=332246&r2=332247&view=diff
==
--- lldb/trunk/include/lldb/Utility/FileSpec.h (original)
+++ lldb/trunk/include/lldb/Utility/FileSpec.h Mon May 14 07:52:47 2018
@@ -22,6 +22,7 @@
#include "llvm/ADT/StringRef.h" // for StringRef
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Path.h"
#include // for size_t
#include // for uint32_t, uint64_t
@@ -60,11 +61,7 @@ namespace lldb_private {
//--
class FileSpec {
public:
- enum PathSyntax : unsigned char {
-ePathSyntaxPosix,
-ePathSyntaxWindows,
-ePathSyntaxHostNative
- };
+ using Style = llvm::sys::path::Style;
FileSpec();
@@ -86,7 +83,7 @@ public:
/// @see FileSpec::SetFile (const char *path, bool resolve)
//--
explicit FileSpec(llvm::StringRef path, bool resolve_path,
-PathSyntax syntax = ePathSyntaxHostNative);
+Style style = Style::native);
explicit FileSpec(llvm::StringRef path, bool resolve_path,
const llvm::Triple &Triple);
@@ -261,7 +258,7 @@ public:
/// \b true if the file path is case sensitive (POSIX), false
/// if case insensitive (Windows).
//--
- bool IsCaseSensitive() const { return m_syntax != ePathSyntaxWindows; }
+ bool IsCaseSensitive() const { return m_style != Style::windows; }
//--
/// Dump this object to a Stream.
@@ -316,7 +313,7 @@ public:
uint64_t GetByteSize() const;
- PathSyntax GetPathSyntax() const;
+ Style GetPathStyle() const;
//--
/// Directory string get accessor.
@@ -494,7 +491,7 @@ public:
/// the static FileSpec::Resolve.
//--
void SetFile(llvm::StringRef path, bool resolve_path,
- PathSyntax syntax = ePathSyntaxHostNative);
+ Style style = Style::native);
void SetFile(llvm::StringRef path, bool resolve_path,
const llvm::Triple &Triple);
@@ -567,8 +564,7 @@ protected:
ConstString m_directory;///< The uniqued directory path
ConstString m_filename; ///< The uniqued filename path
mutable bool m_is_resolved = false; ///< True if this path has been resolved.
- PathSyntax
- m_syntax; ///< The syntax that this path uses (e.g. Windows / Posix)
+ Style m_style; ///< The syntax that this path uses (e.g. Windows / Posix)
};
//--
Modified: lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp?rev=332247&r1=332246&r2=332247&view=diff
==
--- lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp (original)
+++ lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp Mon May 14
07:52:47 2018
@@ -193,8 +193,7 @@ Status PlatformAndroid::GetFile(const Fi
if (IsHost() || !m_remote_platform_sp)
return PlatformLinux::GetFile(source, destination);
- FileSpec source_spec(source.GetPath(false), false,
- FileSpec::ePathSyntaxPosix);
+ FileSpec source_spec(source.GetPath(false), false, FileSpec::Style::posix);
if (source_spec.
[Lldb-commits] [PATCH] D46753: FileSpec: Remove PathSyntax enum and use llvm version instead
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332247: FileSpec: Remove PathSyntax enum and use llvm
version instead (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D46753
Files:
lldb/trunk/include/lldb/Utility/FileSpec.h
lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
lldb/trunk/source/Utility/FileSpec.cpp
lldb/trunk/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
lldb/trunk/unittests/Target/ModuleCacheTest.cpp
lldb/trunk/unittests/Utility/FileSpecTest.cpp
Index: lldb/trunk/include/lldb/Utility/FileSpec.h
===
--- lldb/trunk/include/lldb/Utility/FileSpec.h
+++ lldb/trunk/include/lldb/Utility/FileSpec.h
@@ -22,6 +22,7 @@
#include "llvm/ADT/StringRef.h" // for StringRef
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Path.h"
#include // for size_t
#include // for uint32_t, uint64_t
@@ -60,11 +61,7 @@
//--
class FileSpec {
public:
- enum PathSyntax : unsigned char {
-ePathSyntaxPosix,
-ePathSyntaxWindows,
-ePathSyntaxHostNative
- };
+ using Style = llvm::sys::path::Style;
FileSpec();
@@ -86,7 +83,7 @@
/// @see FileSpec::SetFile (const char *path, bool resolve)
//--
explicit FileSpec(llvm::StringRef path, bool resolve_path,
-PathSyntax syntax = ePathSyntaxHostNative);
+Style style = Style::native);
explicit FileSpec(llvm::StringRef path, bool resolve_path,
const llvm::Triple &Triple);
@@ -261,7 +258,7 @@
/// \b true if the file path is case sensitive (POSIX), false
/// if case insensitive (Windows).
//--
- bool IsCaseSensitive() const { return m_syntax != ePathSyntaxWindows; }
+ bool IsCaseSensitive() const { return m_style != Style::windows; }
//--
/// Dump this object to a Stream.
@@ -316,7 +313,7 @@
uint64_t GetByteSize() const;
- PathSyntax GetPathSyntax() const;
+ Style GetPathStyle() const;
//--
/// Directory string get accessor.
@@ -494,7 +491,7 @@
/// the static FileSpec::Resolve.
//--
void SetFile(llvm::StringRef path, bool resolve_path,
- PathSyntax syntax = ePathSyntaxHostNative);
+ Style style = Style::native);
void SetFile(llvm::StringRef path, bool resolve_path,
const llvm::Triple &Triple);
@@ -567,8 +564,7 @@
ConstString m_directory;///< The uniqued directory path
ConstString m_filename; ///< The uniqued filename path
mutable bool m_is_resolved = false; ///< True if this path has been resolved.
- PathSyntax
- m_syntax; ///< The syntax that this path uses (e.g. Windows / Posix)
+ Style m_style; ///< The syntax that this path uses (e.g. Windows / Posix)
};
//--
Index: lldb/trunk/unittests/Utility/FileSpecTest.cpp
===
--- lldb/trunk/unittests/Utility/FileSpecTest.cpp
+++ lldb/trunk/unittests/Utility/FileSpecTest.cpp
@@ -14,123 +14,122 @@
using namespace lldb_private;
TEST(FileSpecTest, FileAndDirectoryComponents) {
- FileSpec fs_posix("/foo/bar", false, FileSpec::ePathSyntaxPosix);
+ FileSpec fs_posix("/foo/bar", false, FileSpec::Style::posix);
EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
EXPECT_STREQ("/foo", fs_posix.GetDirectory().GetCString());
EXPECT_STREQ("bar", fs_posix.GetFilename().GetCString());
- FileSpec fs_windows("F:\\bar", false, FileSpec::ePathSyntaxWindows);
+ FileSpec fs_windows("F:\\bar", false, FileSpec::Style::windows);
EXPECT_STREQ("F:\\bar", fs_windows.GetCString());
// EXPECT_STREQ("F:\\", fs_windows.GetDirectory().GetCString()); // It returns
// "F:/"
EXPECT_STREQ("bar", fs_windows.GetFilename().GetCString());
- FileSpec fs_posix_root("/", false, FileSpec::ePathSyntaxPosix);
+ FileSpec fs_posix_root("/", false, FileSpec::Style::posix);
EXPECT_STREQ("/", fs_posix_root.GetCString());
EXPECT_EQ(nullptr, fs_posix_root.GetDirectory().GetCString());
EXPECT_STREQ("/", fs_posix_root.GetFilename().GetCString());
- FileSpec fs_windows_drive("F:", false, FileSpec::ePathSyntaxWindows);
+ FileSpec fs_windows_drive("F:", false, FileSpec::Style::windows);
EXPECT_STREQ("F:", fs_windows_drive.GetCString());
EXPECT_EQ(nullptr, fs
[Lldb-commits] [lldb] r332250 - Remove Process references from the Host module
Author: labath
Date: Mon May 14 08:13:13 2018
New Revision: 332250
URL: http://llvm.org/viewvc/llvm-project?rev=332250&view=rev
Log:
Remove Process references from the Host module
The Process class was only being referenced because of the last-ditch
effort in the process launchers to set a process death callback in case
one isn't set already.
Although launching a process for debugging is the most important kind of
"launch" we are doing, it is by far not the only one, so assuming this
particular callback is the one to be used is not a good idea (besides
breaking layering). Instead of assuming a particular exit callback, I
change the launcher code to require the callback to be set by the user (and fix
up the two call sites which did not set the callback already).
Reviewers: jingham, davide
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D46395
Modified:
lldb/trunk/include/lldb/Host/Host.h
lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h
lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
lldb/trunk/source/Host/macosx/Host.mm
lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/trunk/source/Target/ProcessLaunchInfo.cpp
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
Modified: lldb/trunk/include/lldb/Host/Host.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/Host.h?rev=332250&r1=332249&r2=332250&view=diff
==
--- lldb/trunk/include/lldb/Host/Host.h (original)
+++ lldb/trunk/include/lldb/Host/Host.h Mon May 14 08:13:13 2018
@@ -199,6 +199,9 @@ public:
static const lldb::UnixSignalsSP &GetUnixSignals();
+ /// Launch the process specified in launch_info. The monitoring callback in
+ /// launch_info must be set, and it will be called when the process
+ /// terminates.
static Status LaunchProcess(ProcessLaunchInfo &launch_info);
//--
Modified: lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h?rev=332250&r1=332249&r2=332250&view=diff
==
--- lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h (original)
+++ lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h Mon May 14
08:13:13 2018
@@ -24,6 +24,9 @@ public:
explicit MonitoringProcessLauncher(
std::unique_ptr delegate_launcher);
+ /// Launch the process specified in launch_info. The monitoring callback in
+ /// launch_info must be set, and it will be called when the process
+ /// terminates.
HostProcess LaunchProcess(const ProcessLaunchInfo &launch_info,
Status &error) override;
Modified: lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h?rev=332250&r1=332249&r2=332250&view=diff
==
--- lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h (original)
+++ lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h Mon May 14 08:13:13 2018
@@ -107,6 +107,12 @@ public:
return m_monitor_callback;
}
+ /// A Monitor callback which does not take any action on process events. Use
+ /// this if you don't need to take any particular action when the process
+ /// terminates, but you still need to reap it.
+ static bool NoOpMonitorCallback(lldb::pid_t pid, bool exited, int signal,
+ int status);
+
bool GetMonitorSignals() const { return m_monitor_signals; }
// If the LaunchInfo has a monitor callback, then arrange to monitor the
Modified: lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp?rev=332250&r1=332249&r2=332250&view=diff
==
--- lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp (original)
+++ lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp Mon May 14
08:13:13 2018
@@ -9,7 +9,6 @@
#include "lldb/Host/MonitoringProcessLauncher.h"
#include "lldb/Host/HostProcess.h"
-#include "lldb/Target/Process.h"
#include "lldb/Target/ProcessLaunchInfo.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Status.h"
@@ -58,18 +57,9 @@ MonitoringProcessLauncher::LaunchProcess
if (process.GetProcessId() != LLDB_INVALID_PROCESS_ID) {
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
-Host::MonitorChildProcessCallback callback =
-launch_info.GetMonitorProcessCallback();
-
-bool monitor_signals = f
[Lldb-commits] [PATCH] D46395: Remove Process references from the Host module
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332250: Remove Process references from the Host module
(authored by labath, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D46395
Files:
lldb/trunk/include/lldb/Host/Host.h
lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h
lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
lldb/trunk/source/Host/macosx/Host.mm
lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/trunk/source/Target/ProcessLaunchInfo.cpp
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
Index: lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -97,6 +97,11 @@
Info.SetArchitecture(arch_spec);
Info.SetArguments(args, true);
Info.GetEnvironment() = Host::GetEnvironment();
+ // TODO: Use this callback to detect botched launches. If lldb-server does not
+ // start, we can print a nice error message here instead of hanging in
+ // Accept().
+ Info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback,
+ false);
status = Host::LaunchProcess(Info);
if (status.Fail())
Index: lldb/trunk/source/Host/macosx/Host.mm
===
--- lldb/trunk/source/Host/macosx/Host.mm
+++ lldb/trunk/source/Host/macosx/Host.mm
@@ -57,7 +57,6 @@
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/HostInfo.h"
#include "lldb/Host/ThreadLauncher.h"
-#include "lldb/Target/Process.h"
#include "lldb/Utility/ArchSpec.h"
#include "lldb/Utility/CleanUp.h"
#include "lldb/Utility/DataBufferHeap.h"
@@ -68,6 +67,7 @@
#include "lldb/Utility/NameMatches.h"
#include "lldb/Utility/StreamString.h"
#include "lldb/Utility/StructuredData.h"
+#include "lldb/lldb-defines.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Errno.h"
@@ -1497,15 +1497,9 @@
launch_info.SetProcessID(pid);
// Make sure we reap any processes we spawn or we will have zombies.
-if (!launch_info.MonitorProcess()) {
- const bool monitor_signals = false;
- Host::MonitorChildProcessCallback callback = nullptr;
-
- if (!launch_info.GetFlags().Test(lldb::eLaunchFlagDontSetExitStatus))
-callback = Process::SetProcessExitStatus;
-
- StartMonitoringChildProcess(callback, pid, monitor_signals);
-}
+bool monitoring = launch_info.MonitorProcess());
+UNUSED_IF_ASSERT_DISABLED(monitoring);
+assert(monitoring);
} else {
// Invalid process ID, something didn't go well
if (error.Success())
Index: lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
===
--- lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
+++ lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
@@ -9,7 +9,6 @@
#include "lldb/Host/MonitoringProcessLauncher.h"
#include "lldb/Host/HostProcess.h"
-#include "lldb/Target/Process.h"
#include "lldb/Target/ProcessLaunchInfo.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Status.h"
@@ -58,18 +57,9 @@
if (process.GetProcessId() != LLDB_INVALID_PROCESS_ID) {
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
-Host::MonitorChildProcessCallback callback =
-launch_info.GetMonitorProcessCallback();
-
-bool monitor_signals = false;
-if (callback) {
- // If the ProcessLaunchInfo specified a callback, use that.
- monitor_signals = launch_info.GetMonitorSignals();
-} else {
- callback = Process::SetProcessExitStatus;
-}
-
-process.StartMonitoring(callback, monitor_signals);
+assert(launch_info.GetMonitorProcessCallback());
+process.StartMonitoring(launch_info.GetMonitorProcessCallback(),
+launch_info.GetMonitorSignals());
if (log)
log->PutCString("started monitoring child process.");
} else {
Index: lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
===
--- lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
+++ lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
@@ -13,7 +13,6 @@
#include "lldb/Host/common/NativeRegisterContext.h"
#include "lldb/Host/common/NativeThreadProtocol.h"
#include "lldb/Host/common/SoftwareBreakpoint.h"
-#include "lldb/Target/Process.h"
#include "lldb/Utility/LLDBAssert.h"
#include "lldb/Utility/Log.h"
#include "lldb/lldb-enumerations.h"
Index: lldb/trunk/source/Target/ProcessLaunchInfo.cpp
===
--
[Lldb-commits] [lldb] r332255 - Fix macosx build broken by r332250
Author: labath Date: Mon May 14 09:01:49 2018 New Revision: 332255 URL: http://llvm.org/viewvc/llvm-project?rev=332255&view=rev Log: Fix macosx build broken by r332250 Modified: lldb/trunk/source/Host/macosx/Host.mm Modified: lldb/trunk/source/Host/macosx/Host.mm URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/macosx/Host.mm?rev=332255&r1=332254&r2=332255&view=diff == --- lldb/trunk/source/Host/macosx/Host.mm (original) +++ lldb/trunk/source/Host/macosx/Host.mm Mon May 14 09:01:49 2018 @@ -57,6 +57,7 @@ #include "lldb/Host/ConnectionFileDescriptor.h" #include "lldb/Host/HostInfo.h" #include "lldb/Host/ThreadLauncher.h" +#include "lldb/Target/ProcessLaunchInfo.h" #include "lldb/Utility/ArchSpec.h" #include "lldb/Utility/CleanUp.h" #include "lldb/Utility/DataBufferHeap.h" ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46783: FileSpec objects that resolve to "." should have "." in m_filename and m_directory empty.
clayborg added a comment. I agree, but we should still guard against it and test in in LLDB to ensure this doesn't regress. I'll be happy to make a LLVM patch. Many people have branches that link against older LLVMs and the check is cheap. https://reviews.llvm.org/D46783 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. So this problem exists both in the LLDB and LLVM DWARF parsers. I am not sure this fix is safe. I would rather fix this by fixing DWARFDIE class to "do the right thing". We should be able to teach the DWARFDIE class to replace its "m_die" with the updated "m_die" if a method ever causes DWARFDIE to need to expand all DIEs in a DWARFUnit. That seems like a much safer fix. Having m_first_die is not safe because it if you call DWARFDIE::GetFirstChild() it will just add 1 to the "m_die" and we will crash. All parent, sibling and child code just do pointer arithmetic to find their counterparts. So since DWARFDIE has the "DWARFUnit *m_cu;" and "DWARFDebugInfoEntry *m_die;" we should use DWARFDIE to abstract this from users. Anyone playing directly with DWARFDebugInfoEntry must know the rules and do the right thing or just use DWARFDIE. https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r332261 - Revert "Remove Process references from the Host module"
Author: labath
Date: Mon May 14 09:54:53 2018
New Revision: 332261
URL: http://llvm.org/viewvc/llvm-project?rev=332261&view=rev
Log:
Revert "Remove Process references from the Host module"
The first fix wasn't enough, there is still a missing
ProcessInstanceInfo include in Host.mm. I won't be able to test a fix
before leaving work, so I am reverting both commits.
This reverts commit r332250 and the subsequent fix attempt.
Modified:
lldb/trunk/include/lldb/Host/Host.h
lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h
lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
lldb/trunk/source/Host/macosx/Host.mm
lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/trunk/source/Target/ProcessLaunchInfo.cpp
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
Modified: lldb/trunk/include/lldb/Host/Host.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/Host.h?rev=332261&r1=332260&r2=332261&view=diff
==
--- lldb/trunk/include/lldb/Host/Host.h (original)
+++ lldb/trunk/include/lldb/Host/Host.h Mon May 14 09:54:53 2018
@@ -199,9 +199,6 @@ public:
static const lldb::UnixSignalsSP &GetUnixSignals();
- /// Launch the process specified in launch_info. The monitoring callback in
- /// launch_info must be set, and it will be called when the process
- /// terminates.
static Status LaunchProcess(ProcessLaunchInfo &launch_info);
//--
Modified: lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h?rev=332261&r1=332260&r2=332261&view=diff
==
--- lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h (original)
+++ lldb/trunk/include/lldb/Host/MonitoringProcessLauncher.h Mon May 14
09:54:53 2018
@@ -24,9 +24,6 @@ public:
explicit MonitoringProcessLauncher(
std::unique_ptr delegate_launcher);
- /// Launch the process specified in launch_info. The monitoring callback in
- /// launch_info must be set, and it will be called when the process
- /// terminates.
HostProcess LaunchProcess(const ProcessLaunchInfo &launch_info,
Status &error) override;
Modified: lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h?rev=332261&r1=332260&r2=332261&view=diff
==
--- lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h (original)
+++ lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h Mon May 14 09:54:53 2018
@@ -107,12 +107,6 @@ public:
return m_monitor_callback;
}
- /// A Monitor callback which does not take any action on process events. Use
- /// this if you don't need to take any particular action when the process
- /// terminates, but you still need to reap it.
- static bool NoOpMonitorCallback(lldb::pid_t pid, bool exited, int signal,
- int status);
-
bool GetMonitorSignals() const { return m_monitor_signals; }
// If the LaunchInfo has a monitor callback, then arrange to monitor the
Modified: lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp?rev=332261&r1=332260&r2=332261&view=diff
==
--- lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp (original)
+++ lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp Mon May 14
09:54:53 2018
@@ -9,6 +9,7 @@
#include "lldb/Host/MonitoringProcessLauncher.h"
#include "lldb/Host/HostProcess.h"
+#include "lldb/Target/Process.h"
#include "lldb/Target/ProcessLaunchInfo.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Status.h"
@@ -57,9 +58,18 @@ MonitoringProcessLauncher::LaunchProcess
if (process.GetProcessId() != LLDB_INVALID_PROCESS_ID) {
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
-assert(launch_info.GetMonitorProcessCallback());
-process.StartMonitoring(launch_info.GetMonitorProcessCallback(),
-launch_info.GetMonitorSignals());
+Host::MonitorChildProcessCallback callback =
+launch_info.GetMonitorProcessCallback();
+
+bool monitor_signals = false;
+if (callback) {
+ // If the ProcessLaunchInfo specified a callback, use that.
+ monitor_signals = launch_info.GetMonitorSignals();
+} else {
+ callback = Process::SetProcessExitStatus;
+}
+
+process.StartMonitoring(callback, monitor
[Lldb-commits] [PATCH] D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target.
aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.
Thanks for converting the test!
Comment at: lit/Breakpoint/break-insert.test:2
+# RUN: %cc %p/Inputs/break-insert.c -g
+# RUN: %lldb_mi < %p/Inputs/break-insert.input | FileCheck %s
+
Please add a comment like:
`# Test that a breakpoint can be inserted before creating a target.`
Comment at: lit/Breakpoint/break-insert.test:5
+# cmd: -break-insert breakpoint
+# CHECK: ^done,bkpt={number="1"
+
Very nice!
What does the actual output format of lldb-mi look like?
If every reply is on a single line, we could make this test even stricter by
using `CHECK-NEXT` instead of `CHECK` to make sure that no other output appears
between two matches. Alternatively you could also insert a `CHECK-NOT: ^done`
line before every command to make the test even stricter.
Repository:
rL LLVM
https://reviews.llvm.org/D46588
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r332293 - [lit] Fix several tests that fail when using Python 3 or on Windows
Author: stella.stamenova
Date: Mon May 14 14:04:24 2018
New Revision: 332293
URL: http://llvm.org/viewvc/llvm-project?rev=332293&view=rev
Log:
[lit] Fix several tests that fail when using Python 3 or on Windows
Summary:
1) In logtest.cpp, the name of the file that is reported is not always
capitalized, so split the comparison to validate the file (case insensitive)
and function (case sensitive) separately
2) Update the gdb remote client tests to work with Python 3. In Python 3,
socket sends/receives data as bytes rather than byte strings. This also updates
the usage of .hex() - this is no longer available in Python 3, so use hexlify
instead
Reviewers: asmith, labath, zturner
Reviewed By: labath
Subscribers: llvm-commits
Differential Revision: https://reviews.llvm.org/D46773
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
lldb/trunk/unittests/Utility/LogTest.cpp
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py?rev=332293&r1=332292&r2=332293&view=diff
==
---
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
(original)
+++
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
Mon May 14 14:04:24 2018
@@ -1,4 +1,5 @@
import lldb
+import binascii
from lldbsuite.test.lldbtest import *
from lldbsuite.test.decorators import *
from gdbclientutils import *
@@ -25,7 +26,7 @@ class TestGDBRemoteClient(GDBRemoteTestB
# Then, when we are asked to attach, error out.
def vAttach(self, pid):
-return "E42;" + error_msg.encode("hex")
+return "E42;" + binascii.hexlify(error_msg.encode()).decode()
self.server.responder = MyResponder()
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py?rev=332293&r1=332292&r2=332293&view=diff
==
---
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
(original)
+++
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
Mon May 14 14:04:24 2018
@@ -76,7 +76,7 @@ def hex_decode_bytes(hex_bytes):
class MockGDBServerResponder:
"""
-A base class for handing client packets and issuing server responses for
+A base class for handling client packets and issuing server responses for
GDB tests.
This handles many typical situations, while still allowing subclasses to
@@ -278,10 +278,14 @@ class MockGDBServer:
data = self._client.recv(4096)
if data is None or len(data) == 0:
break
+# In Python 2, sockets return byte strings. In Python 3,
sockets return bytes.
+# If we got bytes (and not a byte string), decode them to a
string for later handling.
+if isinstance(data, bytes) and not isinstance(data, str):
+data = data.decode()
+self._receive(data)
except Exception as e:
self._client.close()
break
-self._receive(data)
def _receive(self, data):
"""
@@ -329,7 +333,7 @@ class MockGDBServer:
i += 1
else:
raise self.InvalidPacketException(
-"Unexexpected leading byte: %s" % data[0])
+"Unexpected leading byte: %s" % data[0])
# If we're looking beyond the start of the received data, then we're
# looking for the end of the packet content, denoted by a #.
@@ -370,9 +374,9 @@ class MockGDBServer:
return
response = ""
# We'll handle the ack stuff here since it's not something any of the
-# tests will be concerned about, and it'll get turned off quicly
anyway.
+# tests will be concerned about, and it'll get turned off quickly
anyway.
if self._shouldSendAck:
-self._client.sendall('+')
+self._client.sendall('+'.encode())
if packet == "QStartNoAckMode":
self._shouldSendAck = False
response = "OK"
@@ -382,6 +386,10 @@ class MockGDBServer:
# Handle packet framing since we don't want to bother tests with it.
if response is not None:
framed = frame_packet(response
[Lldb-commits] [PATCH] D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target.
polyakov.alex updated this revision to Diff 146693.
polyakov.alex added a comment.
Added more comments about the test.
Repository:
rL LLVM
https://reviews.llvm.org/D46588
Files:
lit/Breakpoint/Inputs/break-insert.c
lit/Breakpoint/Inputs/break-insert.input
lit/Breakpoint/break-insert.test
lit/lit.cfg
tools/lldb-mi/MICmdCmdBreak.cpp
tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
Index: tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
===
--- tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
+++ tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp
@@ -871,7 +871,10 @@
// Throws: None.
//--
lldb::SBTarget CMICmnLLDBDebugSessionInfo::GetTarget() const {
- return GetDebugger().GetSelectedTarget();
+ auto target = GetDebugger().GetSelectedTarget();
+ if (target.IsValid())
+return target;
+ return GetDebugger().GetDummyTarget();
}
//++
Index: tools/lldb-mi/MICmdCmdBreak.cpp
===
--- tools/lldb-mi/MICmdCmdBreak.cpp
+++ tools/lldb-mi/MICmdCmdBreak.cpp
@@ -148,6 +148,11 @@
CMICMDBASE_GETOPTION(pArgRestrictBrkPtToThreadId, OptionShort,
m_constStrArgNamedRestrictBrkPtToThreadId);
+ // Ask LLDB for the target to check if we have valid or dummy one.
+ CMICmnLLDBDebugSessionInfo &rSessionInfo(
+ CMICmnLLDBDebugSessionInfo::Instance());
+ lldb::SBTarget sbTarget = rSessionInfo.GetTarget();
+
m_bBrkPtEnabled = !pArgDisableBrkPt->GetFound();
m_bBrkPtIsTemp = pArgTempBrkPt->GetFound();
m_bHaveArgOptionThreadGrp = pArgThreadGroup->GetFound();
@@ -157,7 +162,12 @@
nThreadGrp);
m_strArgOptionThreadGrp = CMIUtilString::Format("i%d", nThreadGrp);
}
- m_bBrkPtIsPending = pArgPendingBrkPt->GetFound();
+
+ if (sbTarget == rSessionInfo.GetDebugger().GetDummyTarget())
+m_bBrkPtIsPending = true;
+ else
+m_bBrkPtIsPending = pArgPendingBrkPt->GetFound();
+
if (pArgLocation->GetFound())
m_brkName = pArgLocation->GetValue();
else if (m_bBrkPtIsPending) {
@@ -225,9 +235,6 @@
// Ask LLDB to create a breakpoint
bool bOk = MIstatus::success;
- CMICmnLLDBDebugSessionInfo &rSessionInfo(
- CMICmnLLDBDebugSessionInfo::Instance());
- lldb::SBTarget sbTarget = rSessionInfo.GetTarget();
switch (eBrkPtType) {
case eBreakPoint_ByAddress:
m_brkPt = sbTarget.BreakpointCreateByAddress(nAddress);
Index: lit/lit.cfg
===
--- lit/lit.cfg
+++ lit/lit.cfg
@@ -58,6 +58,8 @@
lldb = "%s -S %s/lit-lldb-init" % (lit.util.which('lldb', lldb_tools_dir),
config.test_source_root)
+lldb_mi = lit.util.which('lldb-mi', lldb_tools_dir)
+
if not os.path.exists(config.cc):
config.cc = lit.util.which(config.cc, config.environment['PATH'])
@@ -79,6 +81,7 @@
config.substitutions.append(('%cc', config.cc))
config.substitutions.append(('%cxx', config.cxx))
+config.substitutions.append(('%lldb_mi', lldb_mi))
config.substitutions.append(('%lldb', lldb))
if debugserver is not None:
Index: lit/Breakpoint/break-insert.test
===
--- /dev/null
+++ lit/Breakpoint/break-insert.test
@@ -0,0 +1,15 @@
+# RUN: %cc %p/Inputs/break-insert.c -g
+# RUN: %lldb_mi < %p/Inputs/break-insert.input | FileCheck %s
+
+# Test that a breakpoint can be inserted before creating a target.
+#
+# cmd: -break-insert breakpoint
+# CHECK: ^done,bkpt={number="1"
+#
+# cmd: -file-exec-and-symbols a.out
+# CHECK: ^done
+#
+# cmd: -exec-run
+# CHECK: ^running
+# CHECK-AFTER: *stopped,reason="breakpoint-hit"
+
Index: lit/Breakpoint/Inputs/break-insert.input
===
--- /dev/null
+++ lit/Breakpoint/Inputs/break-insert.input
@@ -0,0 +1,3 @@
+-break-insert breakpoint
+-file-exec-and-symbols a.out
+-exec-run
Index: lit/Breakpoint/Inputs/break-insert.c
===
--- /dev/null
+++ lit/Breakpoint/Inputs/break-insert.c
@@ -0,0 +1,7 @@
+int breakpoint() { // Breakpoint will be set here.
+ return 0;
+}
+
+int main() {
+ return breakpoint();
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target.
aprantl accepted this revision. aprantl added a comment. Thanks, LGTM. Repository: rL LLVM https://reviews.llvm.org/D46588 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target.
polyakov.alex added inline comments.
Comment at: lit/Breakpoint/break-insert.test:5
+# cmd: -break-insert breakpoint
+# CHECK: ^done,bkpt={number="1"
+
aprantl wrote:
> Very nice!
>
> What does the actual output format of lldb-mi look like?
> If every reply is on a single line, we could make this test even stricter by
> using `CHECK-NEXT` instead of `CHECK` to make sure that no other output
> appears between two matches. Alternatively you could also insert a
> `CHECK-NOT: ^done` line before every command to make the test even stricter.
Unfortunately, lldb-mi output may look like:
(gdb)
-exec-run
^running
=thread-group-started,id="i1",pid="4632"
=thread-created,id="1",group-id="i1"
=thread-selected,id="1"
(gdb)
=library-loaded,id="/lib/x86_64-linux-gnu/ld-2.23.so",target-name="/lib/x86_64-linux-gnu/ld-2.23.so",host-name="/lib/x86_64-linux-gnu/ld-2.23.so",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/ld-2.23.so",loaded_addr="-",size="0"
(gdb)
=library-loaded,id="[vdso]",target-name="[vdso]",host-name="[vdso]",symbols-loaded="1",symbols-path="",loaded_addr="0x77ffa000",size="0"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0041eed0",func="??",file="??",fullname="??/??",line="0",pending=["main"],times="0",original-location="main"}
(gdb)
(gdb)
(gdb)
=library-loaded,id="/bin/bash",target-name="/bin/bash",host-name="/bin/bash",symbols-loaded="0",loaded_addr="-",size="0"
(gdb)
*running,thread-id="all"
(gdb)
(gdb)
=library-loaded,id="/lib/x86_64-linux-gnu/libtinfo.so.5",target-name="/lib/x86_64-linux-gnu/libtinfo.so.5",host-name="/lib/x86_64-linux-gnu/libtinfo.so.5",symbols-loaded="0",loaded_addr="-",size="0"
(gdb)
=library-loaded,id="/lib/x86_64-linux-gnu/libdl.so.2",target-name="/lib/x86_64-linux-gnu/libdl.so.2",host-name="/lib/x86_64-linux-gnu/libdl.so.2",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libdl-2.23.so",loaded_addr="-",size="0"
(gdb)
=library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libc-2.23.so",loaded_addr="-",size="0"
(gdb)
=library-loaded,id="/lib/x86_64-linux-gnu/libtinfo.so.5",target-name="/lib/x86_64-linux-gnu/libtinfo.so.5",host-name="/lib/x86_64-linux-gnu/libtinfo.so.5",symbols-loaded="0",loaded_addr="-",size="0"
=library-loaded,id="/lib/x86_64-linux-gnu/libdl.so.2",target-name="/lib/x86_64-linux-gnu/libdl.so.2",host-name="/lib/x86_64-linux-gnu/libdl.so.2",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libdl-2.23.so",loaded_addr="-",size="0"
=library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libc-2.23.so",loaded_addr="-",size="0"
(gdb)
*stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={level="0",addr="0x0041eed0",func="main",args=[],file="??",fullname="??",line="-1"},thread-id="1",stopped-threads="all"
(gdb)
So, I think, it's not a best idea to write 'CHECK-NOT' for each line.
Repository:
rL LLVM
https://reviews.llvm.org/D46588
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46588: [LLDB][lldb-mi] Add possibility to set breakpoints without selecting a target.
polyakov.alex added inline comments.
Comment at: lit/Breakpoint/break-insert.test:5
+# cmd: -break-insert breakpoint
+# CHECK: ^done,bkpt={number="1"
+
polyakov.alex wrote:
> aprantl wrote:
> > Very nice!
> >
> > What does the actual output format of lldb-mi look like?
> > If every reply is on a single line, we could make this test even stricter
> > by using `CHECK-NEXT` instead of `CHECK` to make sure that no other output
> > appears between two matches. Alternatively you could also insert a
> > `CHECK-NOT: ^done` line before every command to make the test even stricter.
> Unfortunately, lldb-mi output may look like:
>
> (gdb)
> -exec-run
> ^running
> =thread-group-started,id="i1",pid="4632"
> =thread-created,id="1",group-id="i1"
> =thread-selected,id="1"
> (gdb)
> =library-loaded,id="/lib/x86_64-linux-gnu/ld-2.23.so",target-name="/lib/x86_64-linux-gnu/ld-2.23.so",host-name="/lib/x86_64-linux-gnu/ld-2.23.so",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/ld-2.23.so",loaded_addr="-",size="0"
> (gdb)
> =library-loaded,id="[vdso]",target-name="[vdso]",host-name="[vdso]",symbols-loaded="1",symbols-path="",loaded_addr="0x77ffa000",size="0"
> =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0041eed0",func="??",file="??",fullname="??/??",line="0",pending=["main"],times="0",original-location="main"}
> (gdb)
> (gdb)
> (gdb)
> =library-loaded,id="/bin/bash",target-name="/bin/bash",host-name="/bin/bash",symbols-loaded="0",loaded_addr="-",size="0"
> (gdb)
> *running,thread-id="all"
> (gdb)
> (gdb)
> =library-loaded,id="/lib/x86_64-linux-gnu/libtinfo.so.5",target-name="/lib/x86_64-linux-gnu/libtinfo.so.5",host-name="/lib/x86_64-linux-gnu/libtinfo.so.5",symbols-loaded="0",loaded_addr="-",size="0"
> (gdb)
> =library-loaded,id="/lib/x86_64-linux-gnu/libdl.so.2",target-name="/lib/x86_64-linux-gnu/libdl.so.2",host-name="/lib/x86_64-linux-gnu/libdl.so.2",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libdl-2.23.so",loaded_addr="-",size="0"
> (gdb)
> =library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libc-2.23.so",loaded_addr="-",size="0"
> (gdb)
> =library-loaded,id="/lib/x86_64-linux-gnu/libtinfo.so.5",target-name="/lib/x86_64-linux-gnu/libtinfo.so.5",host-name="/lib/x86_64-linux-gnu/libtinfo.so.5",symbols-loaded="0",loaded_addr="-",size="0"
> =library-loaded,id="/lib/x86_64-linux-gnu/libdl.so.2",target-name="/lib/x86_64-linux-gnu/libdl.so.2",host-name="/lib/x86_64-linux-gnu/libdl.so.2",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libdl-2.23.so",loaded_addr="-",size="0"
> =library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="1",symbols-path="/usr/lib/debug/lib/x86_64-linux-gnu/libc-2.23.so",loaded_addr="-",size="0"
> (gdb)
> *stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={level="0",addr="0x0041eed0",func="main",args=[],file="??",fullname="??",line="-1"},thread-id="1",stopped-threads="all"
> (gdb)
>
> So, I think, it's not a best idea to write 'CHECK-NOT' for each line.
Ah, my fault, there will not be such amount of lines if we use test's binary.
Repository:
rL LLVM
https://reviews.llvm.org/D46588
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
