[Lldb-commits] [PATCH] D30402: Modernize Enable/DisableLogChannel interface a bit
labath added a comment.
Comment at: include/lldb/Interpreter/Args.h:196-197
+ llvm::ArrayRef GetArgumentArrayRef() const {
+return {static_cast(m_argv.data()),
+m_argv.size() - 1};
+ }
zturner wrote:
> can this be written `return makeArrayRef(m_argv).drop_back();`?
Yes, it looks like it can.
Comment at: source/Core/Log.cpp:74-80
+auto cat = llvm::find_if(
+entry.second.channel.categories,
+[&](const Log::Category &c) { return c.name.equals_lower(category); });
if (cat != entry.second.channel.categories.end()) {
flags |= cat->flag;
continue;
}
zturner wrote:
> How about
>
> ```
> bool exists = llvm::any(entry.second.channel.categories,
> [&](const Log::Category &c) { return c.name.equals_lower(category);
> }));
> if (exists) {
> ...
> }
> ```
That won't work, because I actually need the relevant iterator to get the
associated flag (`flags |= cat->flag` below).
https://reviews.llvm.org/D30402
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30402: Modernize Enable/DisableLogChannel interface a bit
labath updated this revision to Diff 89996.
labath marked 3 inline comments as done.
labath added a comment.
Address review comments
https://reviews.llvm.org/D30402
Files:
include/lldb/Core/Debugger.h
include/lldb/Core/Log.h
include/lldb/Interpreter/Args.h
source/API/SBDebugger.cpp
source/Commands/CommandObjectLog.cpp
source/Core/Debugger.cpp
source/Core/Log.cpp
tools/lldb-server/LLDBServerUtilities.cpp
unittests/Core/LogTest.cpp
unittests/Interpreter/TestArgs.cpp
Index: unittests/Interpreter/TestArgs.cpp
===
--- unittests/Interpreter/TestArgs.cpp
+++ unittests/Interpreter/TestArgs.cpp
@@ -169,6 +169,14 @@
EXPECT_STREQ("4", args.GetArgumentAtIndex(3));
}
+TEST(ArgsTest, GetArgumentArrayRef) {
+ Args args("foo bar");
+ auto ref = args.GetArgumentArrayRef();
+ ASSERT_EQ(2u, ref.size());
+ EXPECT_STREQ("foo", ref[0]);
+ EXPECT_STREQ("bar", ref[1]);
+}
+
TEST(ArgsTest, StringToBoolean) {
bool success = false;
EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, nullptr));
Index: unittests/Core/LogTest.cpp
===
--- unittests/Core/LogTest.cpp
+++ unittests/Core/LogTest.cpp
@@ -93,7 +93,7 @@
llvm::llvm_shutdown_obj obj;
Log::Register("chan", test_channel);
EXPECT_EQ(nullptr, test_channel.GetLogIfAny(FOO));
- const char *cat1[] = {"foo", nullptr};
+ const char *cat1[] = {"foo"};
std::string message;
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
@@ -110,20 +110,20 @@
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
StreamString err;
- EXPECT_FALSE(Log::EnableLogChannel(stream_sp, 0, "chanchan", nullptr, err));
+ EXPECT_FALSE(Log::EnableLogChannel(stream_sp, 0, "chanchan", {}, err));
EXPECT_EQ("Invalid log channel 'chanchan'.\n", err.GetString());
err.Clear();
- EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", nullptr, err));
- EXPECT_EQ("", err.GetString());
+ EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
+ EXPECT_EQ("", err.GetString()) << "err: " << err.GetString().str();
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
- const char *cat2[] = {"bar", nullptr};
+ const char *cat2[] = {"bar"};
EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat2, err));
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO | BAR));
- const char *cat3[] = {"baz", nullptr};
+ const char *cat3[] = {"baz"};
EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat3, err));
EXPECT_TRUE(err.GetString().contains("unrecognized log category 'baz'"))
<< "err: " << err.GetString().str();
@@ -146,28 +146,28 @@
TEST_F(LogChannelTest, Disable) {
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(FOO));
- const char *cat12[] = {"foo", "bar", nullptr};
+ const char *cat12[] = {"foo", "bar"};
std::string message;
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
StreamString err;
EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat12, err));
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO | BAR));
- const char *cat2[] = {"bar", nullptr};
+ const char *cat2[] = {"bar"};
EXPECT_TRUE(Log::DisableLogChannel("chan", cat2, err));
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
- const char *cat3[] = {"baz", nullptr};
+ const char *cat3[] = {"baz"};
EXPECT_TRUE(Log::DisableLogChannel("chan", cat3, err));
EXPECT_TRUE(err.GetString().contains("unrecognized log category 'baz'"))
<< "err: " << err.GetString().str();
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
err.Clear();
- EXPECT_TRUE(Log::DisableLogChannel("chan", nullptr, err));
+ EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
EXPECT_EQ(nullptr, test_channel.GetLogIfAny(FOO | BAR));
}
@@ -195,13 +195,13 @@
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
StreamString err;
- EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", nullptr, err));
+ EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
Log *log = test_channel.GetLogIfAll(FOO);
// Start logging on one thread. Concurrently, try disabling the log channel.
std::thread log_thread([log] { LLDB_LOG(log, "Hello World"); });
- EXPECT_TRUE(Log::DisableLogChannel("chan", nullptr, err));
+ EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
log_thread.join();
// The log thread either managed to write to the log in time, or it didn't. In
Index: tools/lldb-server/LLDBServerUtilities.cpp
===
--- tools/lldb-server/LLDBServerUtilities.cpp
+++ tools/lldb-server/LLDBServerUtilities.cpp
@@ -52,8 +52,8 @@
channel_then_categories.Sh
Re: [Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap
It looks like that in this case (and possibly many others) a wrapper class is not necessary. If we just want to log the inputs and outputs of mmap(), we just need to wrap the factory function (as we have done here) and let the users access the llvm class directly. On 27 February 2017 at 20:05, Jim Ingham wrote: > Having library functions that don't return good errors seems like such an > obvious failing that it shouldn't be hard to motivate fixing that. Then our > logging can go in the wrapper classes, using those errors. That seems like a > pattern that solves the "don't duplicate code" problem and the "lldb needs > more logging" problems nicely. > > Jim > >> On Feb 27, 2017, at 12:00 PM, Zachary Turner wrote: >> >> Oh that wasn't in response to the comment about wrapper classes, that was >> just a general comment about the fact that we lose logging by moving to >> LLVM's implementation at all. If we have our own implementation, we could >> in theory log at various stages of the mmaping process, but by moving to a >> library implementation we can only log before or after. >> >> On Mon, Feb 27, 2017 at 11:55 AM Jim Ingham wrote: >> >> > On Feb 27, 2017, at 11:49 AM, Zachary Turner wrote: >> > >> > There may be some cases where we're no longer mmaping where we used to, >> > but looking at LLVM's implementation, that would only happen if the file >> > is fairly small, and there's a comment in LLVM explaining why they don't >> > mmap small files. So I think that's actually an improvement for LLDB >> > (although I doubt we are frequently mapping very small files, so it might >> > be a non issue). >> > >> > You are correct that we lose some logging this way, but in exchange we get >> > better tested code. The code in question that we lose though is basically >> > just the call to mmap and the setup to prepare that call. As long as we >> > have the return code from mmap, we should be able to log the error just as >> > gracefully. Granted, the LLVM code doesn't return an error code right >> > now, but it seems like a worthy improvement. >> > >> > I'm not sure where the right balance is between facilitating post-mortem >> > diagnostics and reducing the amount of problems that happen in the first >> > place, but I feel kind of dirty duplicating code just so that we can add >> > logging. Maybe there's a case to be made for adding logging hooks into >> > LLVM. >> >> Why do wrapper classes mean you get significantly less well tested code? If >> they are just to support logging, you really only need to know that the >> values are passed correctly, which is easy to test, and then you could test >> that the logging is correct, though we don't currently do logging tests for >> the most part. >> >> Jim >> >> >> > >> > >> > On Mon, Feb 27, 2017 at 11:00 AM Jim Ingham wrote: >> > I worry about stripping out the wrappers, because there are some >> > differences in how lldb operates from llvm that I don't think we want to >> > push down into llvm - in this case I'm thinking particularly about >> > logging. DataBufferMemoryMap did a bunch of logging, which presumably >> > would get lost if you just went directly to the llvm classes. Most of the >> > tools that build out of the llvm toolset are one-pass tools, and >> > reproducing problems with their operation is generally not terribly hard. >> > So having a rich logging feature is not all that necessary, and the burden >> > of adding it to the code not worth the benefit. But lldb is much more >> > state dependent, and is exposed to a wider variety of the vagaries of >> > system behavior, and being both an interactive tool and the API's for GUI >> > debuggers, has a wider and more unpredictable array of of use cases. >> > Having good logging has saved my day many and many's the time over the >> > years, and I don't want to lose that. >> > >> > So while I don't have any objection to changing the backing >> > implementation, I still see value in the wrapper classes. >> > >> > You say LLVM memory buffers can be heap or mmap, but presumably when used >> > by what used to be DataBufferMemoryMap they are using the memory map >> > version, right? Still seems useful to have the class name say what the >> > thing is. >> > >> > Jim >> > >> > > On Feb 27, 2017, at 10:40 AM, Zachary Turner wrote: >> > > >> > > I didn't refer to mmaping in the name because LLVM's MemoryBuffer is not >> > > necessarily mmap'ed. It might be mmap'ed and it might not be. Depends >> > > on various factors such as whether you specify the IsVolatile flag, how >> > > big the file is, and maybe a few other things. >> > > >> > > After this change we have DataBufferLLVM and DataBufferHeap. But it >> > > turns out an LLVM MemoryBuffer can also be backed by the heap, which now >> > > makes DataBufferHeap redundant as well. So I think longer term we might >> > > be able to get rid of all of LLDB's DataBuffer stuff entirely, and >> > > everything w
[Lldb-commits] [PATCH] D30402: Modernize Enable/DisableLogChannel interface a bit
labath updated this revision to Diff 9.
labath added a comment.
rebase on top of tree
https://reviews.llvm.org/D30402
Files:
include/lldb/Core/Debugger.h
include/lldb/Core/Log.h
include/lldb/Interpreter/Args.h
source/API/SBDebugger.cpp
source/Commands/CommandObjectLog.cpp
source/Core/Debugger.cpp
source/Core/Log.cpp
tools/lldb-server/LLDBServerUtilities.cpp
unittests/Core/LogTest.cpp
unittests/Interpreter/TestArgs.cpp
Index: unittests/Interpreter/TestArgs.cpp
===
--- unittests/Interpreter/TestArgs.cpp
+++ unittests/Interpreter/TestArgs.cpp
@@ -169,6 +169,14 @@
EXPECT_STREQ("4", args.GetArgumentAtIndex(3));
}
+TEST(ArgsTest, GetArgumentArrayRef) {
+ Args args("foo bar");
+ auto ref = args.GetArgumentArrayRef();
+ ASSERT_EQ(2u, ref.size());
+ EXPECT_STREQ("foo", ref[0]);
+ EXPECT_STREQ("bar", ref[1]);
+}
+
TEST(ArgsTest, StringToBoolean) {
bool success = false;
EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, nullptr));
Index: unittests/Core/LogTest.cpp
===
--- unittests/Core/LogTest.cpp
+++ unittests/Core/LogTest.cpp
@@ -93,7 +93,7 @@
llvm::llvm_shutdown_obj obj;
Log::Register("chan", test_channel);
EXPECT_EQ(nullptr, test_channel.GetLogIfAny(FOO));
- const char *cat1[] = {"foo", nullptr};
+ const char *cat1[] = {"foo"};
std::string message;
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
@@ -110,20 +110,20 @@
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
StreamString err;
- EXPECT_FALSE(Log::EnableLogChannel(stream_sp, 0, "chanchan", nullptr, err));
+ EXPECT_FALSE(Log::EnableLogChannel(stream_sp, 0, "chanchan", {}, err));
EXPECT_EQ("Invalid log channel 'chanchan'.\n", err.GetString());
err.Clear();
- EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", nullptr, err));
- EXPECT_EQ("", err.GetString());
+ EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
+ EXPECT_EQ("", err.GetString()) << "err: " << err.GetString().str();
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
- const char *cat2[] = {"bar", nullptr};
+ const char *cat2[] = {"bar"};
EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat2, err));
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO | BAR));
- const char *cat3[] = {"baz", nullptr};
+ const char *cat3[] = {"baz"};
EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat3, err));
EXPECT_TRUE(err.GetString().contains("unrecognized log category 'baz'"))
<< "err: " << err.GetString().str();
@@ -137,37 +137,37 @@
new llvm::raw_string_ostream(message));
StreamString err;
EXPECT_TRUE(Log::EnableLogChannel(stream_sp, LLDB_LOG_OPTION_VERBOSE, "chan",
-nullptr, err));
+{}, err));
Log *log = test_channel.GetLogIfAll(FOO);
ASSERT_NE(nullptr, log);
EXPECT_TRUE(log->GetVerbose());
}
TEST_F(LogChannelTest, Disable) {
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(FOO));
- const char *cat12[] = {"foo", "bar", nullptr};
+ const char *cat12[] = {"foo", "bar"};
std::string message;
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
StreamString err;
EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat12, err));
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO | BAR));
- const char *cat2[] = {"bar", nullptr};
+ const char *cat2[] = {"bar"};
EXPECT_TRUE(Log::DisableLogChannel("chan", cat2, err));
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
- const char *cat3[] = {"baz", nullptr};
+ const char *cat3[] = {"baz"};
EXPECT_TRUE(Log::DisableLogChannel("chan", cat3, err));
EXPECT_TRUE(err.GetString().contains("unrecognized log category 'baz'"))
<< "err: " << err.GetString().str();
EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
err.Clear();
- EXPECT_TRUE(Log::DisableLogChannel("chan", nullptr, err));
+ EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
EXPECT_EQ(nullptr, test_channel.GetLogIfAny(FOO | BAR));
}
@@ -195,13 +195,13 @@
std::shared_ptr stream_sp(
new llvm::raw_string_ostream(message));
StreamString err;
- EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", nullptr, err));
+ EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
Log *log = test_channel.GetLogIfAll(FOO);
// Start logging on one thread. Concurrently, try disabling the log channel.
std::thread log_thread([log] { LLDB_LOG(log, "Hello World"); });
- EXPECT_TRUE(Log::DisableLogChannel("chan", nullptr, err));
+ EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
log_thread.join();
// The log thread
[Lldb-commits] [PATCH] D30453: test: pass correct objcopy and ar paths to the test runner
labath created this revision.
Herald added a subscriber: mgorny.
The test runner has code to autodetect this, but it's not very smart --
in particular, it fails in the case where we build the test executables
with the just-built clang. Since cmake already has the knowledge about
the right toolchain, we can just have it pass the appropriate flags to
the test runner.
This also removes the "temporary" cache-scrubbing hack added a couple
months ago.
https://reviews.llvm.org/D30453
Files:
test/CMakeLists.txt
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -43,11 +43,6 @@
${LLDB_DEFAULT_TEST_ARCH}
CACHE STRING "Specify the architecture to run LLDB tests as (x86|x64).
Determines whether tests are compiled with -m32 or -m64")
-# Scrub LLDB_TEST_COMPILER out of the CMake caches
-# TODO: remove the replace lines and the FORCE parameter in a few days once the
-# change has made its way through bots to clean up their CMake caches.
-string(REPLACE "-C;${LLDB_TEST_COMPILER}" "" LLDB_TEST_USER_ARGS_New
"${LLDB_TEST_USER_ARGS}")
-
if(LLDB_TEST_CLANG)
set(LLDB_TEST_COMPILER $)
endif()
@@ -97,6 +92,11 @@
list(APPEND LLDB_TEST_COMMON_ARGS --framework $)
endif()
+if (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Windows|Darwin")
+ list(APPEND LLDB_TEST_COMMON_ARGS
+--env ARCHIVER=${CMAKE_AR} --env OBJCOPY=${CMAKE_OBJCOPY})
+endif()
+
add_python_test_target(check-lldb-single
${LLDB_SOURCE_DIR}/test/dotest.py
"--no-multiprocess;${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}"
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -43,11 +43,6 @@
${LLDB_DEFAULT_TEST_ARCH}
CACHE STRING "Specify the architecture to run LLDB tests as (x86|x64). Determines whether tests are compiled with -m32 or -m64")
-# Scrub LLDB_TEST_COMPILER out of the CMake caches
-# TODO: remove the replace lines and the FORCE parameter in a few days once the
-# change has made its way through bots to clean up their CMake caches.
-string(REPLACE "-C;${LLDB_TEST_COMPILER}" "" LLDB_TEST_USER_ARGS_New "${LLDB_TEST_USER_ARGS}")
-
if(LLDB_TEST_CLANG)
set(LLDB_TEST_COMPILER $)
endif()
@@ -97,6 +92,11 @@
list(APPEND LLDB_TEST_COMMON_ARGS --framework $)
endif()
+if (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Windows|Darwin")
+ list(APPEND LLDB_TEST_COMMON_ARGS
+--env ARCHIVER=${CMAKE_AR} --env OBJCOPY=${CMAKE_OBJCOPY})
+endif()
+
add_python_test_target(check-lldb-single
${LLDB_SOURCE_DIR}/test/dotest.py
"--no-multiprocess;${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}"
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30454: [LLDB][MIPS] Fix typo in MatchesModuleSpec()
nitesh.jain created this revision.
The MatchesModuleSpec() should return false if module_ref is not found.
https://reviews.llvm.org/D30454
Files:
source/Core/Module.cpp
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -1615,10 +1615,10 @@
const ConstString &object_name = module_ref.GetObjectName();
if (object_name) {
-if (object_name != GetObjectName())
- return false;
+if (object_name == GetObjectName())
+ return true;
}
- return true;
+ return false;
}
bool Module::FindSourceFile(const FileSpec &orig_spec,
Index: source/Core/Module.cpp
===
--- source/Core/Module.cpp
+++ source/Core/Module.cpp
@@ -1615,10 +1615,10 @@
const ConstString &object_name = module_ref.GetObjectName();
if (object_name) {
-if (object_name != GetObjectName())
- return false;
+if (object_name == GetObjectName())
+ return true;
}
- return true;
+ return false;
}
bool Module::FindSourceFile(const FileSpec &orig_spec,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r296470 - Switch SBWatchpoint to use a weak_ptr to the underlying object
Author: labath
Date: Tue Feb 28 06:32:45 2017
New Revision: 296470
URL: http://llvm.org/viewvc/llvm-project?rev=296470&view=rev
Log:
Switch SBWatchpoint to use a weak_ptr to the underlying object
Modified:
lldb/trunk/include/lldb/API/SBWatchpoint.h
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py
lldb/trunk/source/API/SBWatchpoint.cpp
Modified: lldb/trunk/include/lldb/API/SBWatchpoint.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/API/SBWatchpoint.h?rev=296470&r1=296469&r2=296470&view=diff
==
--- lldb/trunk/include/lldb/API/SBWatchpoint.h (original)
+++ lldb/trunk/include/lldb/API/SBWatchpoint.h Tue Feb 28 06:32:45 2017
@@ -72,7 +72,7 @@ private:
friend class SBTarget;
friend class SBValue;
- lldb::WatchpointSP m_opaque_sp;
+ std::weak_ptr m_opaque_wp;
};
} // namespace lldb
Modified:
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py?rev=296470&r1=296469&r2=296470&view=diff
==
---
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py
(original)
+++
lldb/trunk/packages/Python/lldbsuite/test/python_api/watchpoint/TestSetWatchpoint.py
Tue Feb 28 06:32:45 2017
@@ -108,3 +108,6 @@ class SetWatchpointAPITestCase(TestBase)
self.assertTrue(
process.GetState() == lldb.eStateExited,
PROCESS_EXITED)
+
+self.dbg.DeleteTarget(target)
+self.assertFalse(watchpoint.IsValid())
Modified: lldb/trunk/source/API/SBWatchpoint.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/API/SBWatchpoint.cpp?rev=296470&r1=296469&r2=296470&view=diff
==
--- lldb/trunk/source/API/SBWatchpoint.cpp (original)
+++ lldb/trunk/source/API/SBWatchpoint.cpp Tue Feb 28 06:32:45 2017
@@ -27,28 +27,24 @@
using namespace lldb;
using namespace lldb_private;
-SBWatchpoint::SBWatchpoint() : m_opaque_sp() {}
+SBWatchpoint::SBWatchpoint() {}
SBWatchpoint::SBWatchpoint(const lldb::WatchpointSP &wp_sp)
-: m_opaque_sp(wp_sp) {
+: m_opaque_wp(wp_sp) {
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
if (log) {
SBStream sstr;
GetDescription(sstr, lldb::eDescriptionLevelBrief);
-log->Printf("SBWatchpoint::SBWatchpoint (const lldb::WatchpointSP &wp_sp"
-"=%p) => this.sp = %p (%s)",
-static_cast(wp_sp.get()),
-static_cast(m_opaque_sp.get()), sstr.GetData());
+LLDB_LOG(log, "watchpoint = {0} ({1})", wp_sp.get(), sstr.GetData());
}
}
SBWatchpoint::SBWatchpoint(const SBWatchpoint &rhs)
-: m_opaque_sp(rhs.m_opaque_sp) {}
+: m_opaque_wp(rhs.m_opaque_wp) {}
const SBWatchpoint &SBWatchpoint::operator=(const SBWatchpoint &rhs) {
- if (this != &rhs)
-m_opaque_sp = rhs.m_opaque_sp;
+ m_opaque_wp = rhs.m_opaque_wp;
return *this;
}
@@ -74,7 +70,7 @@ watch_id_t SBWatchpoint::GetID() {
return watch_id;
}
-bool SBWatchpoint::IsValid() const { return (bool)m_opaque_sp; }
+bool SBWatchpoint::IsValid() const { return bool(m_opaque_wp.lock()); }
SBError SBWatchpoint::GetError() {
SBError sb_error;
@@ -223,11 +219,11 @@ bool SBWatchpoint::GetDescription(SBStre
return true;
}
-void SBWatchpoint::Clear() { m_opaque_sp.reset(); }
+void SBWatchpoint::Clear() { m_opaque_wp.reset(); }
-lldb::WatchpointSP SBWatchpoint::GetSP() const { return m_opaque_sp; }
+lldb::WatchpointSP SBWatchpoint::GetSP() const { return m_opaque_wp.lock(); }
-void SBWatchpoint::SetSP(const lldb::WatchpointSP &sp) { m_opaque_sp = sp; }
+void SBWatchpoint::SetSP(const lldb::WatchpointSP &sp) { m_opaque_wp = sp; }
bool SBWatchpoint::EventIsWatchpointEvent(const lldb::SBEvent &event) {
return Watchpoint::WatchpointEventData::GetEventDataFromEvent(event.get()) !=
@@ -245,7 +241,7 @@ SBWatchpoint::GetWatchpointEventTypeFrom
SBWatchpoint SBWatchpoint::GetWatchpointFromEvent(const lldb::SBEvent &event) {
SBWatchpoint sb_watchpoint;
if (event.IsValid())
-sb_watchpoint.m_opaque_sp =
+sb_watchpoint =
Watchpoint::WatchpointEventData::GetWatchpointFromEvent(event.GetSP());
return sb_watchpoint;
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30457: [LLDB][MIPS] Core Dump Support
nitesh.jain created this revision.
This patch add support for core file. This patch includes
- Separation of register context which was earlier share between Linux and
FreeBSD.
- Add support to analyse Linux core file for all three ABI (N32, N64 and O32)
https://reviews.llvm.org/D30457
Files:
source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_mips64.h
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h
source/Plugins/Process/Utility/RegisterContextFreeBSD_mips64.cpp
source/Plugins/Process/Utility/RegisterContextFreeBSD_mips64.h
source/Plugins/Process/Utility/RegisterContextLinux_mips.cpp
source/Plugins/Process/Utility/RegisterContextLinux_mips.h
source/Plugins/Process/Utility/RegisterContextLinux_mips64.cpp
source/Plugins/Process/Utility/RegisterContextLinux_mips64.h
source/Plugins/Process/Utility/RegisterContextPOSIX_mips64.cpp
source/Plugins/Process/Utility/RegisterContextPOSIX_mips64.h
source/Plugins/Process/Utility/RegisterInfoInterface.h
source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp
source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h
source/Plugins/Process/elf-core/ThreadElfCore.cpp
source/Plugins/Process/elf-core/ThreadElfCore.h
Index: source/Plugins/Process/elf-core/ThreadElfCore.h
===
--- source/Plugins/Process/elf-core/ThreadElfCore.h
+++ source/Plugins/Process/elf-core/ThreadElfCore.h
@@ -66,6 +66,17 @@
// members are smaller -
// so the layout is not the same
static size_t GetSize(lldb_private::ArchSpec &arch) {
+if (arch.IsMIPS()) {
+ std::string abi = arch.GetTargetABI();
+ assert(!abi.empty() && "ABI is not set");
+ if (!abi.compare("n64"))
+return sizeof(ELFLinuxPrStatus);
+ else if (!abi.compare("o32"))
+return 96;
+ // N32 ABI
+ return 72;
+}
+
switch (arch.GetCore()) {
case lldb_private::ArchSpec::eCore_s390x_generic:
case lldb_private::ArchSpec::eCore_x86_64_x86_64:
@@ -98,6 +109,8 @@
// members are smaller -
// so the layout is not the same
static size_t GetSize(const lldb_private::ArchSpec &arch) {
+if (arch.IsMIPS())
+return sizeof(ELFLinuxSigInfo);
switch (arch.GetCore()) {
case lldb_private::ArchSpec::eCore_x86_64_x86_64:
return sizeof(ELFLinuxSigInfo);
@@ -144,6 +157,13 @@
// members are smaller -
// so the layout is not the same
static size_t GetSize(lldb_private::ArchSpec &arch) {
+if (arch.IsMIPS()) {
+ uint8_t address_byte_size = arch.GetAddressByteSize();
+ if (address_byte_size == 8)
+return sizeof(ELFLinuxPrPsInfo);
+ return 128;
+}
+
switch (arch.GetCore()) {
case lldb_private::ArchSpec::eCore_s390x_generic:
case lldb_private::ArchSpec::eCore_x86_64_x86_64:
Index: source/Plugins/Process/elf-core/ThreadElfCore.cpp
===
--- source/Plugins/Process/elf-core/ThreadElfCore.cpp
+++ source/Plugins/Process/elf-core/ThreadElfCore.cpp
@@ -18,6 +18,8 @@
#include "Plugins/Process/Utility/RegisterContextFreeBSD_mips64.h"
#include "Plugins/Process/Utility/RegisterContextFreeBSD_powerpc.h"
#include "Plugins/Process/Utility/RegisterContextFreeBSD_x86_64.h"
+#include "Plugins/Process/Utility/RegisterContextLinux_mips64.h"
+#include "Plugins/Process/Utility/RegisterContextLinux_mips.h"
#include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
#include "Plugins/Process/Utility/RegisterContextLinux_s390x.h"
#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
@@ -118,6 +120,12 @@
case llvm::Triple::aarch64:
reg_interface = new RegisterInfoPOSIX_arm64(arch);
break;
+ case llvm::Triple::mipsel:
+reg_interface = new RegisterContextLinux_mips(arch);
+break;
+ case llvm::Triple::mips64el:
+reg_interface = new RegisterContextLinux_mips64(arch);
+break;
case llvm::Triple::systemz:
reg_interface = new RegisterContextLinux_s390x(arch);
break;
@@ -153,7 +161,13 @@
m_thread_reg_ctx_sp.reset(new RegisterContextCorePOSIX_arm(
*this, reg_interface, m_gpregset_data, m_fpregset_data));
break;
+case llvm::Triple::mipsel:
+case llvm::Triple::mips:
+ m_thread_reg_ctx_sp.reset(new RegisterContextCorePOSIX_mips64(
+ *this, reg_interface, m_gpregset_data, m_fpregset_data));
+ break;
case llvm::Triple::mips64:
+case llvm::Triple::mips64el:
m_thread_reg_ctx_sp.reset(new RegisterContextCorePOSIX_mips64(
*this, reg_interface, m_gpregset_data, m_fpregset_data));
break;
Index: source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.h
===
--- source/Plugins/Process/elf-core/RegisterContextPOS
[Lldb-commits] [PATCH] D30460: Fix "process load" on new android targets
labath created this revision.
Herald added subscribers: srhines, danalbert, emaste.
On older android targets, we needed a dlopen rename workaround to get
"process load" working. Since API 25 this is not required as the targets
have a proper libdl so with the function names one would expect.
To make this work I've had to remove the const qualifier from the
GetLibdlFunctionDeclarations function (as now the declarations can
depend on the connected target). Since I was already modifying the
prototype (and the lower levels were already converted to StringRef) I
took the oportunity to convert this function as well.
https://reviews.llvm.org/D30460
Files:
source/Plugins/Platform/Android/PlatformAndroid.cpp
source/Plugins/Platform/Android/PlatformAndroid.h
source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
source/Plugins/Platform/POSIX/PlatformPOSIX.h
Index: source/Plugins/Platform/POSIX/PlatformPOSIX.h
===
--- source/Plugins/Platform/POSIX/PlatformPOSIX.h
+++ source/Plugins/Platform/POSIX/PlatformPOSIX.h
@@ -195,10 +195,10 @@
lldb_private::Error
EvaluateLibdlExpression(lldb_private::Process *process, const char
*expr_cstr,
- const char *expr_prefix,
+ llvm::StringRef expr_prefix,
lldb::ValueObjectSP &result_valobj_sp);
- virtual const char *GetLibdlFunctionDeclarations() const;
+ virtual llvm::StringRef GetLibdlFunctionDeclarations();
private:
DISALLOW_COPY_AND_ASSIGN(PlatformPOSIX);
Index: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -880,7 +880,7 @@
Error PlatformPOSIX::EvaluateLibdlExpression(
lldb_private::Process *process, const char *expr_cstr,
-const char *expr_prefix, lldb::ValueObjectSP &result_valobj_sp) {
+llvm::StringRef expr_prefix, lldb::ValueObjectSP &result_valobj_sp) {
DynamicLoader *loader = process->GetDynamicLoader();
if (loader) {
Error error = loader->CanLoadImage();
@@ -940,7 +940,7 @@
the_result;
)",
path);
- const char *prefix = GetLibdlFunctionDeclarations();
+ llvm::StringRef prefix = GetLibdlFunctionDeclarations();
lldb::ValueObjectSP result_valobj_sp;
error = EvaluateLibdlExpression(process, expr.GetData(), prefix,
result_valobj_sp);
@@ -988,7 +988,7 @@
StreamString expr;
expr.Printf("dlclose((void *)0x%" PRIx64 ")", image_addr);
- const char *prefix = GetLibdlFunctionDeclarations();
+ llvm::StringRef prefix = GetLibdlFunctionDeclarations();
lldb::ValueObjectSP result_valobj_sp;
Error error = EvaluateLibdlExpression(process, expr.GetData(), prefix,
result_valobj_sp);
@@ -1020,7 +1020,7 @@
error);
}
-const char *PlatformPOSIX::GetLibdlFunctionDeclarations() const {
+llvm::StringRef PlatformPOSIX::GetLibdlFunctionDeclarations() {
return R"(
extern "C" void* dlopen(const char*, int);
extern "C" void* dlsym(void*, const char*);
Index: source/Plugins/Platform/Android/PlatformAndroid.h
===
--- source/Plugins/Platform/Android/PlatformAndroid.h
+++ source/Plugins/Platform/Android/PlatformAndroid.h
@@ -76,7 +76,7 @@
Error DownloadSymbolFile(const lldb::ModuleSP &module_sp,
const FileSpec &dst_file_spec) override;
- const char *GetLibdlFunctionDeclarations() const override;
+ llvm::StringRef GetLibdlFunctionDeclarations() override;
private:
AdbClient::SyncService *GetSyncService(Error &error);
Index: source/Plugins/Platform/Android/PlatformAndroid.cpp
===
--- source/Plugins/Platform/Android/PlatformAndroid.cpp
+++ source/Plugins/Platform/Android/PlatformAndroid.cpp
@@ -366,13 +366,17 @@
return m_major_os_version != 0;
}
-const char *PlatformAndroid::GetLibdlFunctionDeclarations() const {
- return R"(
+llvm::StringRef PlatformAndroid::GetLibdlFunctionDeclarations() {
+ // Older platform versions have the dl function symbols mangled
+ if (GetSdkVersion() < 25)
+return R"(
extern "C" void* dlopen(const char*, int) asm("__dl_dlopen");
extern "C" void* dlsym(void*, const char*) asm("__dl_dlsym");
extern "C" int dlclose(void*) asm("__dl_dlclose");
extern "C" char* dlerror(void) asm("__dl_dlerror");
)";
+
+ return PlatformPOSIX::GetLibdlFunctionDeclarations();
}
AdbClient::SyncService *PlatformAndroid::GetSyncService(Error &error) {
Index: source/Plugins/Platform/POSIX/PlatformPOSIX.h
===
[Lldb-commits] [PATCH] D30410: testsuite/android: build test executables with the android ndk directly
tberghammer accepted this revision. tberghammer added a comment. This revision is now accepted and ready to land. LGTM, assuming that it works on all 3 OSes Comment at: packages/Python/lldbsuite/test/make/Android.rules:1 +NDK_ROOT := $(shell dirname $(CC))/../../../../.. +NDK_ROOT := $(realpath $(NDK_ROOT)) Will this file work on Windows (e.g. dirname, realpath, forward slash)? Comment at: packages/Python/lldbsuite/test/make/Android.rules:4-13 +ifeq "$(findstring $(ARCH), aarch64 x86_64)" "$(ARCH)" + # lowest 64-bit API level + API_LEVEL := 21 +else ifeq "$(ARCH)" "i386" + # clone(2) declaration is present only since this api level + API_LEVEL := 17 +else Does it make sense to use a different API level for every architecture? I think making it somewhat consistent would make it easier to use it https://reviews.llvm.org/D30410 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30460: Fix "process load" on new android targets
labath abandoned this revision. labath added a comment. The sdk version I used here is incorrect. Shelve this until we can get the right version here. https://reviews.llvm.org/D30460 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30457: [LLDB][MIPS] Core Dump Support
labath added a comment.
Have you tried running the
`test/testcases/functionalities/postmortem/elf-core/make-core.sh` script? Does
it generate a reasonably-sized core file (potentially you may need to increase
the stack limit slightly). It would be great to have a mips core file test for
this.
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h:10
-#if defined(__mips__)
These need to stay, otherwise you'll get multiply-defined symbols on non-mips
platforms.
These register contexts were meant to be used in lldb-server only and I am
pretty sure you don't need them for mips core file support.
Comment at: source/Plugins/Process/Utility/RegisterInfoInterface.h:32
+ virtual const lldb_private::RegisterSet *
+ GetRegisterSet(size_t set) const {return nullptr;}
While I don't see anything obviously wrong about adding this interface, I am
wondering why the other subclasses have not needed this.
I'd defer to @clayborg judgement on the appropriateness of the interface. What
I don't like however, is that the default implementation will blatantly lie
about the number of register sets for the non-mips case.
What I can suggest is to avoid putting these functions in the generic class --
you seem to be calling them from mips code only, so I don't see any immediate
need to have them here. (e.g. have GetRegisterInfoInterface() cast to the
appropriate type).
https://reviews.llvm.org/D30457
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30454: [LLDB][MIPS] Fix typo in MatchesModuleSpec()
labath added a comment. I am not sure this is correct... looking at the other checks in this function, they seem to be following the same pattern... could you explain where you ran into this? https://reviews.llvm.org/D30454 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30410: testsuite/android: build test executables with the android ndk directly
labath added a comment. Comment at: packages/Python/lldbsuite/test/make/Android.rules:1 +NDK_ROOT := $(shell dirname $(CC))/../../../../.. +NDK_ROOT := $(realpath $(NDK_ROOT)) tberghammer wrote: > Will this file work on Windows (e.g. dirname, realpath, forward slash)? I copied this from the main makefile, so I am pretty sure it will. Why will it work is a different question though. :) Comment at: packages/Python/lldbsuite/test/make/Android.rules:4-13 +ifeq "$(findstring $(ARCH), aarch64 x86_64)" "$(ARCH)" + # lowest 64-bit API level + API_LEVEL := 21 +else ifeq "$(ARCH)" "i386" + # clone(2) declaration is present only since this api level + API_LEVEL := 17 +else tberghammer wrote: > Does it make sense to use a different API level for every architecture? I > think making it somewhat consistent would make it easier to use it Well.. there's no guarantee that an API 21 file will run on an older device. They seem to run fine, but I'd like to avoid relying on that if it is possible. We can always increase the number if it becomes a maintenance burden. https://reviews.llvm.org/D30410 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30454: [LLDB][MIPS] Fix typo in MatchesModuleSpec()
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. This logic is wrong. If there is no object name, true should be returned. The old logic is correct. https://reviews.llvm.org/D30454 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30454: [LLDB][MIPS] Fix typo in MatchesModuleSpec()
clayborg added a comment. Object name is used for a module spec where the file is "/tmp/foo.a" and the object name is "bar.o". So this is for named objects within a container file like a BSD archive. If there is no object name, it doesn't need to be matched. If there is an object name, then the object name must match. All of the other tests are returning false when a there is no match. The original code is correct. https://reviews.llvm.org/D30454 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D30402: Modernize Enable/DisableLogChannel interface a bit
Lgtm On Tue, Feb 28, 2017 at 3:08 AM Pavel Labath via Phabricator < revi...@reviews.llvm.org> wrote: > labath updated this revision to Diff 9. > labath added a comment. > > rebase on top of tree > > > https://reviews.llvm.org/D30402 > > Files: > include/lldb/Core/Debugger.h > include/lldb/Core/Log.h > include/lldb/Interpreter/Args.h > source/API/SBDebugger.cpp > source/Commands/CommandObjectLog.cpp > source/Core/Debugger.cpp > source/Core/Log.cpp > tools/lldb-server/LLDBServerUtilities.cpp > unittests/Core/LogTest.cpp > unittests/Interpreter/TestArgs.cpp > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30457: [LLDB][MIPS] Core Dump Support
clayborg resigned from this revision. clayborg added a comment. I will let Pavel do this review as it is in his area of expertise. https://reviews.llvm.org/D30457 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
Author: zturner
Date: Tue Feb 28 11:59:59 2017
New Revision: 296495
URL: http://llvm.org/viewvc/llvm-project?rev=296495&view=rev
Log:
Fix incorrect logic in StackFrame::Disassemble.
This had broken as the result of some previous cleanup.
Modified:
lldb/trunk/source/Target/StackFrame.cpp
Modified: lldb/trunk/source/Target/StackFrame.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StackFrame.cpp?rev=296495&r1=296494&r2=296495&view=diff
==
--- lldb/trunk/source/Target/StackFrame.cpp (original)
+++ lldb/trunk/source/Target/StackFrame.cpp Tue Feb 28 11:59:59 2017
@@ -221,18 +221,20 @@ bool StackFrame::ChangePC(addr_t pc) {
const char *StackFrame::Disassemble() {
std::lock_guard guard(m_mutex);
- if (m_disassembly.Empty())
-return nullptr;
-
- ExecutionContext exe_ctx(shared_from_this());
- Target *target = exe_ctx.GetTargetPtr();
- if (target) {
-const char *plugin_name = nullptr;
-const char *flavor = nullptr;
-Disassembler::Disassemble(target->GetDebugger(), target->GetArchitecture(),
- plugin_name, flavor, exe_ctx, 0, false, 0, 0,
- m_disassembly);
+ if (m_disassembly.Empty()) {
+ExecutionContext exe_ctx(shared_from_this());
+Target *target = exe_ctx.GetTargetPtr();
+if (target) {
+ const char *plugin_name = nullptr;
+ const char *flavor = nullptr;
+ Disassembler::Disassemble(target->GetDebugger(),
+target->GetArchitecture(), plugin_name, flavor,
+exe_ctx, 0, false, 0, 0, m_disassembly);
+}
+if (m_disassembly.Empty())
+ return nullptr;
}
+
return m_disassembly.GetData();
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
No test case?
Jim
> On Feb 28, 2017, at 9:59 AM, Zachary Turner via lldb-commits
> wrote:
>
> Author: zturner
> Date: Tue Feb 28 11:59:59 2017
> New Revision: 296495
>
> URL: http://llvm.org/viewvc/llvm-project?rev=296495&view=rev
> Log:
> Fix incorrect logic in StackFrame::Disassemble.
>
> This had broken as the result of some previous cleanup.
>
> Modified:
>lldb/trunk/source/Target/StackFrame.cpp
>
> Modified: lldb/trunk/source/Target/StackFrame.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StackFrame.cpp?rev=296495&r1=296494&r2=296495&view=diff
> ==
> --- lldb/trunk/source/Target/StackFrame.cpp (original)
> +++ lldb/trunk/source/Target/StackFrame.cpp Tue Feb 28 11:59:59 2017
> @@ -221,18 +221,20 @@ bool StackFrame::ChangePC(addr_t pc) {
>
> const char *StackFrame::Disassemble() {
> std::lock_guard guard(m_mutex);
> - if (m_disassembly.Empty())
> -return nullptr;
> -
> - ExecutionContext exe_ctx(shared_from_this());
> - Target *target = exe_ctx.GetTargetPtr();
> - if (target) {
> -const char *plugin_name = nullptr;
> -const char *flavor = nullptr;
> -Disassembler::Disassemble(target->GetDebugger(),
> target->GetArchitecture(),
> - plugin_name, flavor, exe_ctx, 0, false, 0, 0,
> - m_disassembly);
> + if (m_disassembly.Empty()) {
> +ExecutionContext exe_ctx(shared_from_this());
> +Target *target = exe_ctx.GetTargetPtr();
> +if (target) {
> + const char *plugin_name = nullptr;
> + const char *flavor = nullptr;
> + Disassembler::Disassemble(target->GetDebugger(),
> +target->GetArchitecture(), plugin_name,
> flavor,
> +exe_ctx, 0, false, 0, 0, m_disassembly);
> +}
> +if (m_disassembly.Empty())
> + return nullptr;
> }
> +
> return m_disassembly.GetData();
> }
>
>
>
> ___
> 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
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
I'm not even sure how to exercise this code path. Granted, the reason it
broke at all is because of no test case, but I feel like someone who
understands this code should probably prepare a test case for it. (I know
in the past Jason has said that it was notoriously hard to write test cases
for disassembler)
On Tue, Feb 28, 2017 at 10:13 AM Jim Ingham wrote:
> No test case?
>
> Jim
>
> > On Feb 28, 2017, at 9:59 AM, Zachary Turner via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > Author: zturner
> > Date: Tue Feb 28 11:59:59 2017
> > New Revision: 296495
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=296495&view=rev
> > Log:
> > Fix incorrect logic in StackFrame::Disassemble.
> >
> > This had broken as the result of some previous cleanup.
> >
> > Modified:
> >lldb/trunk/source/Target/StackFrame.cpp
> >
> > Modified: lldb/trunk/source/Target/StackFrame.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StackFrame.cpp?rev=296495&r1=296494&r2=296495&view=diff
> >
> ==
> > --- lldb/trunk/source/Target/StackFrame.cpp (original)
> > +++ lldb/trunk/source/Target/StackFrame.cpp Tue Feb 28 11:59:59 2017
> > @@ -221,18 +221,20 @@ bool StackFrame::ChangePC(addr_t pc) {
> >
> > const char *StackFrame::Disassemble() {
> > std::lock_guard guard(m_mutex);
> > - if (m_disassembly.Empty())
> > -return nullptr;
> > -
> > - ExecutionContext exe_ctx(shared_from_this());
> > - Target *target = exe_ctx.GetTargetPtr();
> > - if (target) {
> > -const char *plugin_name = nullptr;
> > -const char *flavor = nullptr;
> > -Disassembler::Disassemble(target->GetDebugger(),
> target->GetArchitecture(),
> > - plugin_name, flavor, exe_ctx, 0, false,
> 0, 0,
> > - m_disassembly);
> > + if (m_disassembly.Empty()) {
> > +ExecutionContext exe_ctx(shared_from_this());
> > +Target *target = exe_ctx.GetTargetPtr();
> > +if (target) {
> > + const char *plugin_name = nullptr;
> > + const char *flavor = nullptr;
> > + Disassembler::Disassemble(target->GetDebugger(),
> > +target->GetArchitecture(), plugin_name,
> flavor,
> > +exe_ctx, 0, false, 0, 0, m_disassembly);
> > +}
> > +if (m_disassembly.Empty())
> > + return nullptr;
> > }
> > +
> > return m_disassembly.GetData();
> > }
> >
> >
> >
> > ___
> > 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
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
SBStackFrame::Disassemble calls StackFrame::Disassemble to do its job. From
the looks of it, your goof would cause lldb never to return any instructions
when asked to disassemble a stack frame, since StackFrame does the work lazily
and the error was to NOT do the work when the disassembly is empty, rather than
to ONLY do the work... It should be straightforward to write a test that
disassembles a stack frame with the SB API's and see if it gets ANY
instructions. You can't do more than that without getting architecture
specific, but testing that in the future somebody didn't break stack frame
disassembly altogether should be pretty simple.
Jim
> On Feb 28, 2017, at 10:17 AM, Zachary Turner wrote:
>
> I'm not even sure how to exercise this code path. Granted, the reason it
> broke at all is because of no test case, but I feel like someone who
> understands this code should probably prepare a test case for it. (I know in
> the past Jason has said that it was notoriously hard to write test cases for
> disassembler)
>
> On Tue, Feb 28, 2017 at 10:13 AM Jim Ingham wrote:
> No test case?
>
> Jim
>
> > On Feb 28, 2017, at 9:59 AM, Zachary Turner via lldb-commits
> > wrote:
> >
> > Author: zturner
> > Date: Tue Feb 28 11:59:59 2017
> > New Revision: 296495
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=296495&view=rev
> > Log:
> > Fix incorrect logic in StackFrame::Disassemble.
> >
> > This had broken as the result of some previous cleanup.
> >
> > Modified:
> >lldb/trunk/source/Target/StackFrame.cpp
> >
> > Modified: lldb/trunk/source/Target/StackFrame.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StackFrame.cpp?rev=296495&r1=296494&r2=296495&view=diff
> > ==
> > --- lldb/trunk/source/Target/StackFrame.cpp (original)
> > +++ lldb/trunk/source/Target/StackFrame.cpp Tue Feb 28 11:59:59 2017
> > @@ -221,18 +221,20 @@ bool StackFrame::ChangePC(addr_t pc) {
> >
> > const char *StackFrame::Disassemble() {
> > std::lock_guard guard(m_mutex);
> > - if (m_disassembly.Empty())
> > -return nullptr;
> > -
> > - ExecutionContext exe_ctx(shared_from_this());
> > - Target *target = exe_ctx.GetTargetPtr();
> > - if (target) {
> > -const char *plugin_name = nullptr;
> > -const char *flavor = nullptr;
> > -Disassembler::Disassemble(target->GetDebugger(),
> > target->GetArchitecture(),
> > - plugin_name, flavor, exe_ctx, 0, false, 0, 0,
> > - m_disassembly);
> > + if (m_disassembly.Empty()) {
> > +ExecutionContext exe_ctx(shared_from_this());
> > +Target *target = exe_ctx.GetTargetPtr();
> > +if (target) {
> > + const char *plugin_name = nullptr;
> > + const char *flavor = nullptr;
> > + Disassembler::Disassemble(target->GetDebugger(),
> > +target->GetArchitecture(), plugin_name,
> > flavor,
> > +exe_ctx, 0, false, 0, 0, m_disassembly);
> > +}
> > +if (m_disassembly.Empty())
> > + return nullptr;
> > }
> > +
> > return m_disassembly.GetData();
> > }
> >
> >
> >
> > ___
> > 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
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
Make that SBFrame::Disassemble...
Jim
> On Feb 28, 2017, at 10:21 AM, Jim Ingham wrote:
>
> SBStackFrame::Disassemble calls StackFrame::Disassemble to do its job. From
> the looks of it, your goof would cause lldb never to return any instructions
> when asked to disassemble a stack frame, since StackFrame does the work
> lazily and the error was to NOT do the work when the disassembly is empty,
> rather than to ONLY do the work... It should be straightforward to write a
> test that disassembles a stack frame with the SB API's and see if it gets ANY
> instructions. You can't do more than that without getting architecture
> specific, but testing that in the future somebody didn't break stack frame
> disassembly altogether should be pretty simple.
>
> Jim
>
>
>
>> On Feb 28, 2017, at 10:17 AM, Zachary Turner wrote:
>>
>> I'm not even sure how to exercise this code path. Granted, the reason it
>> broke at all is because of no test case, but I feel like someone who
>> understands this code should probably prepare a test case for it. (I know
>> in the past Jason has said that it was notoriously hard to write test cases
>> for disassembler)
>>
>> On Tue, Feb 28, 2017 at 10:13 AM Jim Ingham wrote:
>> No test case?
>>
>> Jim
>>
>>> On Feb 28, 2017, at 9:59 AM, Zachary Turner via lldb-commits
>>> wrote:
>>>
>>> Author: zturner
>>> Date: Tue Feb 28 11:59:59 2017
>>> New Revision: 296495
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=296495&view=rev
>>> Log:
>>> Fix incorrect logic in StackFrame::Disassemble.
>>>
>>> This had broken as the result of some previous cleanup.
>>>
>>> Modified:
>>> lldb/trunk/source/Target/StackFrame.cpp
>>>
>>> Modified: lldb/trunk/source/Target/StackFrame.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StackFrame.cpp?rev=296495&r1=296494&r2=296495&view=diff
>>> ==
>>> --- lldb/trunk/source/Target/StackFrame.cpp (original)
>>> +++ lldb/trunk/source/Target/StackFrame.cpp Tue Feb 28 11:59:59 2017
>>> @@ -221,18 +221,20 @@ bool StackFrame::ChangePC(addr_t pc) {
>>>
>>> const char *StackFrame::Disassemble() {
>>> std::lock_guard guard(m_mutex);
>>> - if (m_disassembly.Empty())
>>> -return nullptr;
>>> -
>>> - ExecutionContext exe_ctx(shared_from_this());
>>> - Target *target = exe_ctx.GetTargetPtr();
>>> - if (target) {
>>> -const char *plugin_name = nullptr;
>>> -const char *flavor = nullptr;
>>> -Disassembler::Disassemble(target->GetDebugger(),
>>> target->GetArchitecture(),
>>> - plugin_name, flavor, exe_ctx, 0, false, 0, 0,
>>> - m_disassembly);
>>> + if (m_disassembly.Empty()) {
>>> +ExecutionContext exe_ctx(shared_from_this());
>>> +Target *target = exe_ctx.GetTargetPtr();
>>> +if (target) {
>>> + const char *plugin_name = nullptr;
>>> + const char *flavor = nullptr;
>>> + Disassembler::Disassemble(target->GetDebugger(),
>>> +target->GetArchitecture(), plugin_name,
>>> flavor,
>>> +exe_ctx, 0, false, 0, 0, m_disassembly);
>>> +}
>>> +if (m_disassembly.Empty())
>>> + return nullptr;
>>> }
>>> +
>>> return m_disassembly.GetData();
>>> }
>>>
>>>
>>>
>>> ___
>>> 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] [lldb] r296504 - Fix a bug in r294611 w.r.t. Darwin Kernel debugging.
Author: jingham
Date: Tue Feb 28 12:57:54 2017
New Revision: 296504
URL: http://llvm.org/viewvc/llvm-project?rev=296504&view=rev
Log:
Fix a bug in r294611 w.r.t. Darwin Kernel debugging.
Modified:
lldb/trunk/include/lldb/Target/DynamicLoader.h
lldb/trunk/include/lldb/Target/Process.h
lldb/trunk/source/Core/DynamicLoader.cpp
lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
Modified: lldb/trunk/include/lldb/Target/DynamicLoader.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/DynamicLoader.h?rev=296504&r1=296503&r2=296504&view=diff
==
--- lldb/trunk/include/lldb/Target/DynamicLoader.h (original)
+++ lldb/trunk/include/lldb/Target/DynamicLoader.h Tue Feb 28 12:57:54 2017
@@ -331,6 +331,10 @@ protected:
// Read a pointer from memory at the given addr.
// Return LLDB_INVALID_ADDRESS if the read fails.
lldb::addr_t ReadPointer(lldb::addr_t addr);
+
+ // Calls into the Process protected method LoadOperatingSystemPlugin:
+ void LoadOperatingSystemPlugin(bool flush);
+
//--
// Member variables.
Modified: lldb/trunk/include/lldb/Target/Process.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Process.h?rev=296504&r1=296503&r2=296504&view=diff
==
--- lldb/trunk/include/lldb/Target/Process.h (original)
+++ lldb/trunk/include/lldb/Target/Process.h Tue Feb 28 12:57:54 2017
@@ -505,6 +505,7 @@ class Process : public std::enable_share
public PluginInterface {
friend class FunctionCaller; // For WaitForStateChangeEventsPrivate
friend class Debugger; // For PopProcessIOHandler and
ProcessIOHandlerIsActive
+ friend class DynamicLoader; // For LoadOperatingSystemPlugin
friend class ProcessEventData;
friend class StopInfo;
friend class Target;
Modified: lldb/trunk/source/Core/DynamicLoader.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/DynamicLoader.cpp?rev=296504&r1=296503&r2=296504&view=diff
==
--- lldb/trunk/source/Core/DynamicLoader.cpp (original)
+++ lldb/trunk/source/Core/DynamicLoader.cpp Tue Feb 28 12:57:54 2017
@@ -233,3 +233,10 @@ addr_t DynamicLoader::ReadPointer(addr_t
else
return value;
}
+
+void DynamicLoader::LoadOperatingSystemPlugin(bool flush)
+{
+if (m_process)
+m_process->LoadOperatingSystemPlugin(flush);
+}
+
Modified:
lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp?rev=296504&r1=296503&r2=296504&view=diff
==
---
lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
(original)
+++
lldb/trunk/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
Tue Feb 28 12:57:54 2017
@@ -25,6 +25,7 @@
#include "lldb/Host/Symbols.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Target/OperatingSystem.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/StackFrame.h"
#include "lldb/Target/Target.h"
@@ -1026,6 +1027,12 @@ void DynamicLoaderDarwinKernel::LoadKern
m_kernel.LoadImageAtFileAddress(m_process);
}
}
+
+// The operating system plugin gets loaded and initialized in
+// LoadImageUsingMemoryModule when we discover the kernel dSYM. For a
+// core file in particular, that's the wrong place to do this, since
+// we haven't fixed up the section addresses yet. So let's redo it here.
+LoadOperatingSystemPlugin(false);
if (m_kernel.IsLoaded() && m_kernel.GetModule()) {
static ConstString kext_summary_symbol("gLoadedKextSummaries");
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30457: [LLDB][MIPS] Core Dump Support
eugene added inline comments.
Comment at: source/Plugins/Process/Utility/RegisterContextFreeBSD_mips64.cpp:31
+// Number of register sets provided by this context.
+enum { k_num_register_sets = 1 };
+
Here and below why not use
constexpr size_t k_num_register_sets = 1;
it seems much more straightforward than enum-for-consts trick.
Comment at: source/Plugins/Process/Utility/RegisterContextFreeBSD_mips64.cpp:104
+ return &g_reg_sets_mips64[set];
+ return NULL;
+}
nullptr?
Comment at: source/Plugins/Process/elf-core/ThreadElfCore.h:75
+ else if (!abi.compare("o32"))
+return 96;
+ // N32 ABI
96, 72, 128... where do these number come from?
May be they should be put them into constants and document them somehow.
https://reviews.llvm.org/D30457
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r296516 - Wrap the call to UndecorateSymbolName in a mutex.
Author: zturner
Date: Tue Feb 28 14:29:20 2017
New Revision: 296516
URL: http://llvm.org/viewvc/llvm-project?rev=296516&view=rev
Log:
Wrap the call to UndecorateSymbolName in a mutex.
MSDN documents that this function is not thread-safe, so we
wrap it in a global mutex.
Modified:
lldb/trunk/source/Core/Mangled.cpp
Modified: lldb/trunk/source/Core/Mangled.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Mangled.cpp?rev=296516&r1=296515&r2=296516&view=diff
==
--- lldb/trunk/source/Core/Mangled.cpp (original)
+++ lldb/trunk/source/Core/Mangled.cpp Tue Feb 28 14:29:20 2017
@@ -41,6 +41,24 @@
using namespace lldb_private;
+#if defined(_MSC_VER)
+static DWORD safeUndecorateName(const char* Mangled, char *Demangled, DWORD
DemangledLength) {
+ static std::mutex M;
+ std::lock_guard Lock(M);
+ return ::UnDecorateSymbolName(
+ Mangled, Demangled, DemangledLength,
+UNDNAME_NO_ACCESS_SPECIFIERS | // Strip public, private, protected
+ // keywords
+UNDNAME_NO_ALLOCATION_LANGUAGE | // Strip __thiscall, __stdcall,
+ // etc keywords
+UNDNAME_NO_THROW_SIGNATURES |// Strip throw() specifications
+UNDNAME_NO_MEMBER_TYPE | // Strip virtual, static, etc
+ // specifiers
+UNDNAME_NO_MS_KEYWORDS // Strip all MS extension keywords
+);
+}
+#endif
+
static inline Mangled::ManglingScheme cstring_mangling_scheme(const char *s) {
if (s) {
if (s[0] == '?')
@@ -253,17 +271,7 @@ Mangled::GetDemangledName(lldb::Language
const size_t demangled_length = 2048;
demangled_name = static_cast(::malloc(demangled_length));
::ZeroMemory(demangled_name, demangled_length);
-DWORD result = ::UnDecorateSymbolName(
-mangled_name, demangled_name, demangled_length,
-UNDNAME_NO_ACCESS_SPECIFIERS | // Strip public, private, protected
- // keywords
-UNDNAME_NO_ALLOCATION_LANGUAGE | // Strip __thiscall,
__stdcall,
- // etc keywords
-UNDNAME_NO_THROW_SIGNATURES |// Strip throw()
specifications
-UNDNAME_NO_MEMBER_TYPE | // Strip virtual, static, etc
- // specifiers
-UNDNAME_NO_MS_KEYWORDS // Strip all MS extension keywords
-);
+DWORD result = safeUndecorateName(mangled_name, demangled_name,
demangled_length);
if (log) {
if (demangled_name && demangled_name[0])
log->Printf("demangled msvc: %s -> \"%s\"", mangled_name,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r296517 - clang-format the Mangled changes.
Author: zturner
Date: Tue Feb 28 14:30:31 2017
New Revision: 296517
URL: http://llvm.org/viewvc/llvm-project?rev=296517&view=rev
Log:
clang-format the Mangled changes.
Modified:
lldb/trunk/source/Core/Mangled.cpp
Modified: lldb/trunk/source/Core/Mangled.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Mangled.cpp?rev=296517&r1=296516&r2=296517&view=diff
==
--- lldb/trunk/source/Core/Mangled.cpp (original)
+++ lldb/trunk/source/Core/Mangled.cpp Tue Feb 28 14:30:31 2017
@@ -42,20 +42,21 @@
using namespace lldb_private;
#if defined(_MSC_VER)
-static DWORD safeUndecorateName(const char* Mangled, char *Demangled, DWORD
DemangledLength) {
+static DWORD safeUndecorateName(const char *Mangled, char *Demangled,
+DWORD DemangledLength) {
static std::mutex M;
std::lock_guard Lock(M);
return ::UnDecorateSymbolName(
- Mangled, Demangled, DemangledLength,
-UNDNAME_NO_ACCESS_SPECIFIERS | // Strip public, private, protected
- // keywords
-UNDNAME_NO_ALLOCATION_LANGUAGE | // Strip __thiscall, __stdcall,
- // etc keywords
-UNDNAME_NO_THROW_SIGNATURES |// Strip throw() specifications
-UNDNAME_NO_MEMBER_TYPE | // Strip virtual, static, etc
- // specifiers
-UNDNAME_NO_MS_KEYWORDS // Strip all MS extension keywords
-);
+ Mangled, Demangled, DemangledLength,
+ UNDNAME_NO_ACCESS_SPECIFIERS | // Strip public, private, protected
+ // keywords
+ UNDNAME_NO_ALLOCATION_LANGUAGE | // Strip __thiscall, __stdcall,
+ // etc keywords
+ UNDNAME_NO_THROW_SIGNATURES |// Strip throw() specifications
+ UNDNAME_NO_MEMBER_TYPE | // Strip virtual, static, etc
+ // specifiers
+ UNDNAME_NO_MS_KEYWORDS // Strip all MS extension keywords
+ );
}
#endif
@@ -271,7 +272,8 @@ Mangled::GetDemangledName(lldb::Language
const size_t demangled_length = 2048;
demangled_name = static_cast(::malloc(demangled_length));
::ZeroMemory(demangled_name, demangled_length);
-DWORD result = safeUndecorateName(mangled_name, demangled_name,
demangled_length);
+DWORD result =
+safeUndecorateName(mangled_name, demangled_name, demangled_length);
if (log) {
if (demangled_name && demangled_name[0])
log->Printf("demangled msvc: %s -> \"%s\"", mangled_name,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r296531 - Add additional areas I'm responsible for in the lldb codebase.
Author: jmolenda Date: Tue Feb 28 16:31:18 2017 New Revision: 296531 URL: http://llvm.org/viewvc/llvm-project?rev=296531&view=rev Log: Add additional areas I'm responsible for in the lldb codebase. Modified: lldb/trunk/CODE_OWNERS.txt Modified: lldb/trunk/CODE_OWNERS.txt URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/CODE_OWNERS.txt?rev=296531&r1=296530&r2=296531&view=diff == --- lldb/trunk/CODE_OWNERS.txt (original) +++ lldb/trunk/CODE_OWNERS.txt Tue Feb 28 16:31:18 2017 @@ -35,7 +35,8 @@ D: FreeBSD N: Jason Molenda E: jmole...@apple.com -D: ABI, Disassembler, Unwinding, iOS, debugserver, Platform +D: ABI, Disassembler, Unwinding, iOS, debugserver, Platform, ObjectFile, SymbolFile, +D: SymbolVendor, DWARF, gdb-remote N: Hafiz Abid Qadeer E: abidh@gmail.com ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
Writing SB API tests for the disassembler is easy, as Jim notes there is an
SBFrame::Disassemble() method that maps directly on to this call.
I've written unit tests that create a disassembler -- I do it for the unwind
tests. It's easy when you have an array of bytes to feed directly into the
disassembler. Writing a unit test for StackFrame::Disassemble is a little
trickier because the StackFrame has a reference to the thread which has a
reference to the process. And I'm guessing we use the StackFrame's
SymbolContext ivar to find the bounds of the function and read the bytes out of
the target or process to feed to the disassembler. That's a lot of classes
you'd need to set up to write a unit test for this function. But SB API would
be easy.
The complicated thing to test is UnwindLLDB / RegisterContextLLDB. They
operate on a stopped process and have stack memory, register context, and
information about all of the stack frames from the unwind plan importer
methods. It is easy to write an SB API test that exercises these code paths
(SBThread::GetNumFrames() will force a simple stack walk for a thread) but you
want to write very specific functions / prologues / epilogues / stack content
and you're basically writing assembly code to do it correctly, with a C wrapper
program around the assembly frame of interest. I wrote a testsuite in this
form for a previous unwinder I wrote, it works great, at least for a single
platform (e.g. macos).
Writing a unit test for UnwindLLDB / RegisterContextLLDB would be really great
but you need a way to fake up an entire process stopped at a point. I've had
ideas about creating a ProcessMock class which could read
heap/stack/thread/register context information from a file. But it's not
sufficient by itself - you need an ObjectFileMock where you can interject
symbols and the text of those functions as well. I think it's an interesting
idea that I'd like to pursue, but I won't have the time to work on this any
time soon. Even more exciting would be a ProcessMock that could represent
multiple program contexts so you could "step" in a ProcessMock. And an
automated way to gather all the information lldb touches while seeing an
example problem and automatically generating the ProcessMock/ObjectFileMock
information so it could be reproduced later? Hmm!
And ProcessMock/ObjectFileMock wouldn't just be limited to unit tests, SB API
tests could be written against this repo environment too.
Anyway, I'm getting off topic. There's no reason why this function can't be
tested. We could discuss whether it's a good idea to rewrite sections of code
that have little or no testing because it's always a risk, and the payoff for
making those changes must be commensurate (beyond "change the style") with that
risk. At it's core, lldb is a real world tool that thousands of people depend
on; breaking it or introducing bugs for little gain beyond aesthetics is a very
poor tradeoff. But once the function has been broken, in addition to fixing
it, clearly it's incumbent on any of us to do the right thing and write a test
so it doesn't happen again.
> On Feb 28, 2017, at 10:17 AM, Zachary Turner via lldb-commits
> wrote:
>
> I'm not even sure how to exercise this code path. Granted, the reason it
> broke at all is because of no test case, but I feel like someone who
> understands this code should probably prepare a test case for it. (I know in
> the past Jason has said that it was notoriously hard to write test cases for
> disassembler)
>
> On Tue, Feb 28, 2017 at 10:13 AM Jim Ingham wrote:
> No test case?
>
> Jim
>
> > On Feb 28, 2017, at 9:59 AM, Zachary Turner via lldb-commits
> > wrote:
> >
> > Author: zturner
> > Date: Tue Feb 28 11:59:59 2017
> > New Revision: 296495
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=296495&view=rev
> > Log:
> > Fix incorrect logic in StackFrame::Disassemble.
> >
> > This had broken as the result of some previous cleanup.
> >
> > Modified:
> >lldb/trunk/source/Target/StackFrame.cpp
> >
> > Modified: lldb/trunk/source/Target/StackFrame.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/StackFrame.cpp?rev=296495&r1=296494&r2=296495&view=diff
> > ==
> > --- lldb/trunk/source/Target/StackFrame.cpp (original)
> > +++ lldb/trunk/source/Target/StackFrame.cpp Tue Feb 28 11:59:59 2017
> > @@ -221,18 +221,20 @@ bool StackFrame::ChangePC(addr_t pc) {
> >
> > const char *StackFrame::Disassemble() {
> > std::lock_guard guard(m_mutex);
> > - if (m_disassembly.Empty())
> > -return nullptr;
> > -
> > - ExecutionContext exe_ctx(shared_from_this());
> > - Target *target = exe_ctx.GetTargetPtr();
> > - if (target) {
> > -const char *plugin_name = nullptr;
> > -const char *flavor = nullptr;
> > -Disassembler::Disassemble(target->GetDebugger(),
> > target->GetArchitecture(),
> >
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda wrote: > At it's core, lldb is a real world tool that thousands of people depend > on; breaking it or introducing bugs for little gain beyond aesthetics is a > very poor tradeoff. > Just for the record, I disagree with this assertion that there is little gain beyond aesthetics (as does I think almost everyone else in the LLVM/LLDB community). ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
> On Feb 28, 2017, at 3:14 PM, Zachary Turner wrote: > > On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda wrote: > At it's core, lldb is a real world tool that thousands of people depend on; > breaking it or introducing bugs for little gain beyond aesthetics is a very > poor tradeoff. > > Just for the record, I disagree with this assertion that there is little gain > beyond aesthetics (as does I think almost everyone else in the LLVM/LLDB > community). No doubt, early returns makes it easier to reason about complicated code. But you added an early return to a function that had maybe 10 lines of code in it and was trivial to read either way. There was pretty much zero chance somebody working on the code before this change would introduce a bug that they wouldn't because of the clarity provided by the early return. But in doing so you DID add a bug. In this case it seems clear that for the sake of very little more than aesthetics, you introduced a bug. That seems to me a very poor tradeoff. BTW, somebody at Apple tried to get the llvm version of gcov working on the lldb testsuite to see what kind of coverage we actually had. It didn't work right off the bat for reasons that weren't clear, and whoever did the initial effort lost the window of time they had to work on this. But that would be a useful exercise; then you could know whether the code you were touching was already well tested. Then we could gate any of these sorts of formal manipulations on there being adequate test coverage of the affected area in advance of that work. Jim ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
Another principle that seems useful here is not to make this sort of logic change unless this is code you are currently working on. When you have the actual purpose of the code in your head, it is fairly easy to make this sort of change correctly. You know what the code is supposed to be doing, and so it's easy to make sure the new form preserves that purpose. And if you are working on it you either know that it is tested or plan to add tests to it, which makes success even more likely. But when you are just skimming and happen to see something that you think isn't in the right form, you don't have adequate context, and you are much more likely to make a mistake. Jim > On Feb 28, 2017, at 3:22 PM, Jim Ingham via lldb-commits > wrote: > > >> On Feb 28, 2017, at 3:14 PM, Zachary Turner wrote: >> >> On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda wrote: >> At it's core, lldb is a real world tool that thousands of people depend on; >> breaking it or introducing bugs for little gain beyond aesthetics is a very >> poor tradeoff. >> >> Just for the record, I disagree with this assertion that there is little >> gain beyond aesthetics (as does I think almost everyone else in the >> LLVM/LLDB community). > > > No doubt, early returns makes it easier to reason about complicated code. > But you added an early return to a function that had maybe 10 lines of code > in it and was trivial to read either way. There was pretty much zero chance > somebody working on the code before this change would introduce a bug that > they wouldn't because of the clarity provided by the early return. But in > doing so you DID add a bug. In this case it seems clear that for the sake of > very little more than aesthetics, you introduced a bug. That seems to me a > very poor tradeoff. > > BTW, somebody at Apple tried to get the llvm version of gcov working on the > lldb testsuite to see what kind of coverage we actually had. It didn't work > right off the bat for reasons that weren't clear, and whoever did the initial > effort lost the window of time they had to work on this. But that would be a > useful exercise; then you could know whether the code you were touching was > already well tested. Then we could gate any of these sorts of formal > manipulations on there being adequate test coverage of the affected area in > advance of that work. > > > Jim > > ___ > 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
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
That patch was not really about early returns, it was about using StringRef. So my comment about the aesthetics was referring to the patch in general. The early return was more of a drive by, since I was already touching the code. In that respect I was just following the well documented and widely accepted LLVM policy. Regarding testing, I honestly don't see a reasonable path forward regarding increasing test coverage until we have a better testing infrastructure in place that is less platform-dependent, less flaky, and makes it much much easier to write small, targeted tests. On Tue, Feb 28, 2017 at 3:27 PM Jim Ingham wrote: > > > On Feb 28, 2017, at 3:14 PM, Zachary Turner wrote: > > > > On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda > wrote: > > At it's core, lldb is a real world tool that thousands of people depend > on; breaking it or introducing bugs for little gain beyond aesthetics is a > very poor tradeoff. > > > > Just for the record, I disagree with this assertion that there is little > gain beyond aesthetics (as does I think almost everyone else in the > LLVM/LLDB community). > > > No doubt, early returns makes it easier to reason about complicated code. > But you added an early return to a function that had maybe 10 lines of code > in it and was trivial to read either way. There was pretty much zero > chance somebody working on the code before this change would introduce a > bug that they wouldn't because of the clarity provided by the early > return. But in doing so you DID add a bug. In this case it seems clear > that for the sake of very little more than aesthetics, you introduced a > bug. That seems to me a very poor tradeoff. > > BTW, somebody at Apple tried to get the llvm version of gcov working on > the lldb testsuite to see what kind of coverage we actually had. It didn't > work right off the bat for reasons that weren't clear, and whoever did the > initial effort lost the window of time they had to work on this. But that > would be a useful exercise; then you could know whether the code you were > touching was already well tested. Then we could gate any of these sorts of > formal manipulations on there being adequate test coverage of the affected > area in advance of that work. > > > Jim > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
> On Feb 28, 2017, at 3:36 PM, Zachary Turner wrote:
>
> That patch was not really about early returns, it was about using StringRef.
> So my comment about the aesthetics was referring to the patch in general.
> The early return was more of a drive by, since I was already touching the
> code. In that respect I was just following the well documented and widely
> accepted LLVM policy.
I do not think that it is widely accepted LLVM policy to go changing logic in
code you don't really understand without testing the results of that change.
It is much better to leave a little bit of code looking not quite like you
would like it to be, than to add a bug to it.
>
> Regarding testing, I honestly don't see a reasonable path forward regarding
> increasing test coverage until we have a better testing infrastructure in
> place that is less platform-dependent, less flaky, and makes it much much
> easier to write small, targeted tests.
That seems to me to be a cop-out. Every time we've actually chased down a
"flakey" test on OS X we've found the origin in some race condition in lldb -
i.e. the test suite was not flakey, it was working quite correctly in showing
somewhere that lldb was flakey. There seem to still be instances of this - the
multi debugger test case - another one of the "flakey" ones - turns out to be a
race condition in accessing the section lists in the debugger. I doubt you are
going to get something that actually runs the debugger up to some point and
then does testing that is significantly simpler than the current test suite.
And I really doubt that the effort of doing that would end up having more
benefit than adding tests to the current suite.
And come on, these test are really not that hard to write. For instance,
here's the totality of an inline test:
> cat TestDumpDynamic.py
from __future__ import absolute_import
from lldbsuite.test import lldbinline
lldbinline.MakeInlineTest(
__file__, globals(), [
lldbinline.expectedFailureAll(
oslist=["windows"], bugnumber="llvm.org/pr24663")])
cat main.cpp
//===-- main.cpp *- C++ -*-===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===--===//
class Base {
public:
Base () = default;
virtual int func() { return 1; }
virtual ~Base() = default;
};
class Derived : public Base {
private:
int m_derived_data;
public:
Derived () : Base(), m_derived_data(0x0fedbeef) {}
virtual ~Derived() = default;
virtual int func() { return m_derived_data; }
};
int main (int argc, char const *argv[])
{
Base *base = new Derived();
return 0; //% stream = lldb.SBStream()
//% base = self.frame().FindVariable("base")
//% base.SetPreferDynamicValue(lldb.eDynamicDontRunTarget)
//% base.GetDescription(stream)
//% if self.TraceOn(): print(stream.GetData())
//% self.assertTrue(stream.GetData().startswith("(Derived *"))
}
And for smaller components that don't rely on having parts of the debugger up
and cooperating I thought you were happy with your gtests. Is that not true,
is there something wrong with the gtests that I've missed? I have only written
a few, but they seemed fine to me.
Jim
>
> On Tue, Feb 28, 2017 at 3:27 PM Jim Ingham wrote:
>
> > On Feb 28, 2017, at 3:14 PM, Zachary Turner wrote:
> >
> > On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda wrote:
> > At it's core, lldb is a real world tool that thousands of people depend on;
> > breaking it or introducing bugs for little gain beyond aesthetics is a very
> > poor tradeoff.
> >
> > Just for the record, I disagree with this assertion that there is little
> > gain beyond aesthetics (as does I think almost everyone else in the
> > LLVM/LLDB community).
>
>
> No doubt, early returns makes it easier to reason about complicated code.
> But you added an early return to a function that had maybe 10 lines of code
> in it and was trivial to read either way. There was pretty much zero chance
> somebody working on the code before this change would introduce a bug that
> they wouldn't because of the clarity provided by the early return. But in
> doing so you DID add a bug. In this case it seems clear that for the sake of
> very little more than aesthetics, you introduced a bug. That seems to me a
> very poor tradeoff.
>
> BTW, somebody at Apple tried to get the llvm version of gcov working on the
> lldb testsuite to see what kind of coverage we actually had. It didn't work
> right off the bat for reasons that weren't clear, and whoever did the initial
> effort lost the window of time they had to work on this. But that would be a
> useful exercise; then you could know whether the code you were touching was
> already well tested. Then we could gate any
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
It makes me uncomfortable to have these whole-project-ranging change sets going into the source, one example being const char * -> StringRef) for such small benefit. Yes there were a few methods where we were doing string parsing and tokenizing which became much smaller and easier to understand when switched to StringRef. But then the additional step of changing the entire codebase over to use StringRef was very hard for me to understand -- it's months of time, it's introducing bugs (some of which we've caught! Some of which I'm sure we haven't), and it leads to no benefit except that we're using a different model for passing strings around. I think there was discussion about how it might make the command line parser in lldb faster or more efficient, but that's a non-goal; if someone can show me the time for lldb to parse "lldb --attach-name a.out" takes a human detectable amount of time I'd be really interested to see it. When I am working in a method I'll often update the method to current best practices in the process of my necessary changes. I think this is a good use of time and it allows me to roll in the testing of my overall updates and the fixes I'm doing at the same time, both by testsuite and by manual testing. You mentioned that this kind of mechanical churn of the code is something everyone else in the LLVM/LLDB community supports. I don't know about the LLVM community, but I strongly argue that this is the wrong way to develop a stable, reliable product. I know I'm on the conservative side here, but these kinds of changes add no value to the product and add plenty of risk as they're being done while everyone is half paying attention to them going in. Maybe if there was a vastly more extensive testsuite we could make large scale changes without fear of breaking the debugger, but we don't have that, no one has had the time to add one yet, and it's definitely something that we need to be putting energy into. We don't need to be putting energy into switching to a different string representation across the board simply because it shows benefits in a handful of methods. As for the testsuite not being stable -- it's the debugger that isn't stable, not the testsuite. Jim mentioned that the multi process debugger test case is showing a SectionLoadList race condition that we need to fix. It also turned up the UnwindPlan locking bug that I fixed last week. I'm working on another problem right now where the C++ formatters fail ~15% of the time in the testsuite. The testsuite isn't the bug; lldb has the bug, the testsuite is showing us that it is happening. > On Feb 28, 2017, at 3:36 PM, Zachary Turner wrote: > > That patch was not really about early returns, it was about using StringRef. > So my comment about the aesthetics was referring to the patch in general. > The early return was more of a drive by, since I was already touching the > code. In that respect I was just following the well documented and widely > accepted LLVM policy. > > Regarding testing, I honestly don't see a reasonable path forward regarding > increasing test coverage until we have a better testing infrastructure in > place that is less platform-dependent, less flaky, and makes it much much > easier to write small, targeted tests. > > On Tue, Feb 28, 2017 at 3:27 PM Jim Ingham wrote: > > > On Feb 28, 2017, at 3:14 PM, Zachary Turner wrote: > > > > On Tue, Feb 28, 2017 at 3:07 PM Jason Molenda wrote: > > At it's core, lldb is a real world tool that thousands of people depend on; > > breaking it or introducing bugs for little gain beyond aesthetics is a very > > poor tradeoff. > > > > Just for the record, I disagree with this assertion that there is little > > gain beyond aesthetics (as does I think almost everyone else in the > > LLVM/LLDB community). > > > No doubt, early returns makes it easier to reason about complicated code. > But you added an early return to a function that had maybe 10 lines of code > in it and was trivial to read either way. There was pretty much zero chance > somebody working on the code before this change would introduce a bug that > they wouldn't because of the clarity provided by the early return. But in > doing so you DID add a bug. In this case it seems clear that for the sake of > very little more than aesthetics, you introduced a bug. That seems to me a > very poor tradeoff. > > BTW, somebody at Apple tried to get the llvm version of gcov working on the > lldb testsuite to see what kind of coverage we actually had. It didn't work > right off the bat for reasons that weren't clear, and whoever did the initial > effort lost the window of time they had to work on this. But that would be a > useful exercise; then you could know whether the code you were touching was > already well tested. Then we could gate any of these sorts of formal > manipulations on there being adequate test coverage of the affected are
[Lldb-commits] [lldb] r296548 - Greg Clayton is no longer working at Apple, he will continue to
Author: jmolenda Date: Tue Feb 28 18:00:45 2017 New Revision: 296548 URL: http://llvm.org/viewvc/llvm-project?rev=296548&view=rev Log: Greg Clayton is no longer working at Apple, he will continue to review patches etc from his clayborg email address. Modified: lldb/trunk/CODE_OWNERS.txt Modified: lldb/trunk/CODE_OWNERS.txt URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/CODE_OWNERS.txt?rev=296548&r1=296547&r2=296548&view=diff == --- lldb/trunk/CODE_OWNERS.txt (original) +++ lldb/trunk/CODE_OWNERS.txt Tue Feb 28 18:00:45 2017 @@ -13,8 +13,7 @@ E: scalla...@apple.com D: Expression evaluator, IR interpreter, Clang integration N: Greg Clayton -E: clayb...@gmail.com (Phabricator) -E: gclay...@apple.com (Direct) +E: clayb...@gmail.com D: Overall LLDB architecture, Host (common+macosx), Symbol, API, ABI, Mac-specific code, D: DynamicLoader, ObjectFile, IOHandler, EditLine, Core/Value*, Watchpoints, debugserver D: Build scripts, Test suite, Platform, gdb-remote, Anything not covered by this file ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
On Tue, Feb 28, 2017 at 3:49 PM Jim Ingham wrote: > > > On Feb 28, 2017, at 3:36 PM, Zachary Turner wrote: > > > > That patch was not really about early returns, it was about using > StringRef. So my comment about the aesthetics was referring to the patch > in general. The early return was more of a drive by, since I was already > touching the code. In that respect I was just following the well > documented and widely accepted LLVM policy. > > I do not think that it is widely accepted LLVM policy to go changing logic > in code you don't really understand without testing the results of that > change. It is much better to leave a little bit of code looking not quite > like you would like it to be, than to add a bug to it. > It seems like this is going to end up in a circular argument. Testing those changes happens automatically in LLVM, because there is good test coverage. So it's difficult to impossible to change logic in code without testing the results, because everywhere is tested. As a result, there is a widespread mentality that "if the tests pass, I didn't break anything". That is, in fact, the main benefit of having tests. Especially when changing a function like StackFrame::Disassemble, you would think that something, somewhere is testing disassembly right? I mean we can't expect everyone to have the entire code coverage of LLDB mapped out in their heads, so it's reasonable to make assumptions, especially about such core functionality as "is disassembly completely broken?" We should not be adopting an exclusionary policy of gating changes to the code based on how much code coverage there is or how much expertise you have in a particular area. We should instead be finding ways to make it easier to contribute. > > > > > Regarding testing, I honestly don't see a reasonable path forward > regarding increasing test coverage until we have a better testing > infrastructure in place that is less platform-dependent, less flaky, and > makes it much much easier to write small, targeted tests. > > That seems to me to be a cop-out. Every time we've actually chased down a > "flakey" test on OS X we've found the origin in some race condition in lldb > - i.e. the test suite was not flakey, it was working quite correctly in > showing somewhere that lldb was flakey. There seem to still be instances > of this - the multi debugger test case - another one of the "flakey" ones - > turns out to be a race condition in accessing the section lists in the > debugger. I doubt you are going to get something that actually runs the > debugger up to some point and then does testing that is significantly > simpler than the current test suite. And I really doubt that the effort of > doing that would end up having more benefit than adding tests to the > current suite. > Right, my point is just that tests are so all encompassing that if one fails it often gives you no clues about the reason for the failure. Because everything is a full scale integration test. On the other hand, that's the only type of test that it's practical to write. We have some gtests now, but it's still often very hard to write them because the system is not modular enough. Since A depends on B depends on C depends on D ad infinitum, you can't even create an instance of A to test it without spinning up 100 other things. There should be a system in place that allows one to write a test that tests *almost nothing* other than the specific functionality you are trying to test. You can still have the full scale tests, but you need a first line of defense, and integration tests should not be it. Anyway, this is starting to sound like a discussion we've had many times before. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
On Tue, Feb 28, 2017 at 4:09 PM Jason Molenda wrote: > > You mentioned that this kind of mechanical churn of the code is something > everyone else in the LLVM/LLDB community supports. I don't know about the > LLVM community, but I strongly argue that this is the wrong way to develop > a stable, reliable product. I know I'm on the conservative side here, but > these kinds of changes add no value to the product and add plenty of risk > as they're being done while everyone is half paying attention to them going > in. Maybe if there was a vastly more extensive testsuite we could make > large scale changes without fear of breaking the debugger, but we don't > have that, no one has had the time to add one yet, and it's definitely > something that we need to be putting energy into. We don't need to be > putting energy into switching to a different string representation across > the board simply because it shows benefits in a handful of methods. > The way to develop a stable, reliable product is with an extensive test suite. And the way to get an extensive test suite is to make a test infrastructure that encourages / enables low-footprint, low-overhead, fast, portable, and most importantly easy to write tests. I wish working on LLDB were still my primary responsibility, but sadly it's not. But rejecting obvious improvements to the codebase because there is insufficient test coverage for reasons outside the control of the person making the improvement is a quick way to send LLDB to the graveyard, not a way to make things better. I mean, Chris was 100% in agreement before he left Apple, as was the rest of the LLVM community, and I haven't heard anything about priorities changing amongst leadership (if anything I've heard the opposite), so it's a bit strange to see this disconnect here ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r296495 - Fix incorrect logic in StackFrame::Disassemble.
> On Feb 28, 2017, at 4:15 PM, Zachary Turner wrote:
>
>
>
> On Tue, Feb 28, 2017 at 3:49 PM Jim Ingham wrote:
>
> > On Feb 28, 2017, at 3:36 PM, Zachary Turner wrote:
> >
> > That patch was not really about early returns, it was about using
> > StringRef. So my comment about the aesthetics was referring to the patch
> > in general. The early return was more of a drive by, since I was already
> > touching the code. In that respect I was just following the well
> > documented and widely accepted LLVM policy.
>
> I do not think that it is widely accepted LLVM policy to go changing logic in
> code you don't really understand without testing the results of that change.
> It is much better to leave a little bit of code looking not quite like you
> would like it to be, than to add a bug to it.
>
> It seems like this is going to end up in a circular argument. Testing those
> changes happens automatically in LLVM, because there is good test coverage.
> So it's difficult to impossible to change logic in code without testing the
> results, because everywhere is tested. As a result, there is a widespread
> mentality that "if the tests pass, I didn't break anything". That is, in
> fact, the main benefit of having tests.
>
> Especially when changing a function like StackFrame::Disassemble, you would
> think that something, somewhere is testing disassembly right? I mean we
> can't expect everyone to have the entire code coverage of LLDB mapped out in
> their heads, so it's reasonable to make assumptions, especially about such
> core functionality as "is disassembly completely broken?"
>
> We should not be adopting an exclusionary policy of gating changes to the
> code based on how much code coverage there is or how much expertise you have
> in a particular area. We should instead be finding ways to make it easier to
> contribute.
You live in the world you have not the one you wished you had. The world you
have is that at present the lldb code coverage is good but not exhaustive,
which latter goal would be quite difficult to achieve BTW because by necessity
this is a tool with a huge surface area. More importantly, it is a tool that
many thousands of people rely on to get their jobs done. So if you are going
to responsibly edit lldb code, you can't pretend you can do so without ensuring
that there is testing for the change you made or adding it. By doing so you
are actively harming folks, and for this sort of change returning no real
benefit for that harm.
>
>
>
> >
> > Regarding testing, I honestly don't see a reasonable path forward regarding
> > increasing test coverage until we have a better testing infrastructure in
> > place that is less platform-dependent, less flaky, and makes it much much
> > easier to write small, targeted tests.
>
> That seems to me to be a cop-out. Every time we've actually chased down a
> "flakey" test on OS X we've found the origin in some race condition in lldb -
> i.e. the test suite was not flakey, it was working quite correctly in showing
> somewhere that lldb was flakey. There seem to still be instances of this -
> the multi debugger test case - another one of the "flakey" ones - turns out
> to be a race condition in accessing the section lists in the debugger. I
> doubt you are going to get something that actually runs the debugger up to
> some point and then does testing that is significantly simpler than the
> current test suite. And I really doubt that the effort of doing that would
> end up having more benefit than adding tests to the current suite.
> Right, my point is just that tests are so all encompassing that if one fails
> it often gives you no clues about the reason for the failure. Because
> everything is a full scale integration test.
I almost never find this to be the case, except for tests - like the multi
debugger test which it really did take some work to track down - where the
failure is at the integration level. Otherwise, the failure log points you to
exactly the statement that went wrong, and with a few printf's in the Python
code - or just running that test case in the debugger - you can easily figure
out what went wrong.
The one weakness in this sort of test is that if you break something
fundamental in the debugger you will get a huge cascade of failures because
none of the other tests can get to the point where they start to do their real
work. But given the way the debugger works, there are large parts of it that
just don't work if you break say the ability to stop at a breakpoint... That
was probably a pain when bringing up a new port, because you have to solve a
few hard problems first before you can begin to see any light. But even in
that case, it's pretty clear what doesn't work yet ("look, all the tests that
try to hit a breakpoint fail at the point where they try to do this.")
> On the other hand, that's the only type of test that it's practical to wr
