[Lldb-commits] [PATCH] D32732: "target variable" not showing all global variable if we print any global variable before execution starts
vbalu updated this revision to Diff 98762.
vbalu added a comment.
Modified as per suggested. I used the flag option "flagsParsedVariables" to
monitor if global variable are completely parsed or not.
Looks like it never used. For a compile unit global variable will be parsed
only through the method "GetVariableList" so i set the flag there.
Repository:
rL LLVM
https://reviews.llvm.org/D32732
Files:
packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
packages/Python/lldbsuite/test/functionalities/target_command/globals.c
source/Symbol/CompileUnit.cpp
Index: source/Symbol/CompileUnit.cpp
===
--- source/Symbol/CompileUnit.cpp
+++ source/Symbol/CompileUnit.cpp
@@ -241,11 +241,16 @@
}
VariableListSP CompileUnit::GetVariableList(bool can_create) {
- if (m_variables.get() == nullptr && can_create) {
+ // Return NULL if m_variables are partially parsed
+ if (!can_create && m_flags.IsClear(flagsParsedVariables))
+return NULL;
+ if ((m_variables.get() == nullptr || m_flags.IsClear(flagsParsedVariables)) &&
+ can_create) {
SymbolContext sc;
CalculateSymbolContext(&sc);
assert(sc.module_sp);
sc.module_sp->GetSymbolVendor()->ParseVariablesForContext(sc);
+m_flags.Set(flagsParsedVariables);
}
return m_variables;
Index: packages/Python/lldbsuite/test/functionalities/target_command/globals.c
===
--- packages/Python/lldbsuite/test/functionalities/target_command/globals.c
+++ packages/Python/lldbsuite/test/functionalities/target_command/globals.c
@@ -12,6 +12,7 @@
const char* my_global_str = "abc";
const char **my_global_str_ptr = &my_global_str;
static int my_static_int = 228;
+int my_global_int = 10;
int main (int argc, char const *argv[])
{
Index: packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
===
--- packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
+++ packages/Python/lldbsuite/test/functionalities/target_command/TestTargetCommand.py
@@ -59,6 +59,24 @@
self.do_target_variable_command_no_fail('globals')
+@expectedFailureAndroid(archs=['aarch64'])
+def test_target_variable_after_print_command(self):
+"""Test 'print' command before and 'target var' after starting the inferior."""
+d = {'C_SOURCES': 'globals.c', 'EXE': 'globals'}
+self.build(dictionary=d)
+self.addTearDownCleanup(dictionary=d)
+
+self.do_target_variable_after_print_command('globals')
+
+@expectedFailureAndroid(archs=['aarch64'])
+def test_target_variable_with_regex(self):
+"""Test 'target var -r' command before and 'target var' after starting the inferior."""
+d = {'C_SOURCES': 'globals.c', 'EXE': 'globals'}
+self.build(dictionary=d)
+self.addTearDownCleanup(dictionary=d)
+
+self.do_target_variable_with_regex('globals')
+
def do_target_command(self):
"""Exercise 'target create', 'target list', 'target select' commands."""
exe_a = os.path.join(os.getcwd(), "a.out")
@@ -245,7 +263,8 @@
substrs=['my_global_char',
'my_global_str',
'my_global_str_ptr',
- 'my_static_int'])
+ 'my_static_int',
+ 'my_global_int'])
self.expect(
"target variable my_global_str",
@@ -273,3 +292,49 @@
substrs=[
"my_global_char",
"'X'"])
+
+def do_target_variable_after_print_command(self, exe_name):
+"""Exercise 'print' command before and 'target var' after starting the inferior."""
+self.runCmd("file " + exe_name, CURRENT_EXECUTABLE_SET)
+
+self.expect(
+"target variable my_global_char",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+"my_global_char",
+"'X'"])
+
+self.runCmd("b main")
+self.runCmd("run")
+
+# To test whether all varaible are shown if we print only one variable before
+# starting inferior
+self.expect("target variable",
+substrs=['my_global_char',
+ 'my_global_str',
+ 'my_global_str_ptr',
+ 'my_static_int',
+ 'my_global_int'])
+
+def do_target_variable_with_regex(self, exe_name):
+"""Exercise 'target var -r' command before and 'target var' after starting the inferior."""
+self.runCmd("file " + exe_name, CURRENT_EXECUTABLE_SET)
+
+self.expect(
+"target variable -r my_global_ch*",
+VARIABLES_DISPLAYED_CORRECTLY,
+substrs=[
+
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
krytarowski added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:83-88
+ if (endian == LITTLE) {
+registers[register_id] = SwitchEndian(value_str);
+ }
+ else {
+registers[register_id] = stoul(value_str, nullptr, 16);
+ }
zturner wrote:
> jmajors wrote:
> > zturner wrote:
> > > jmajors wrote:
> > > > zturner wrote:
> > > > > This code block is unnecessary.
> > > > >
> > > > > ```
> > > > > unsigned long X;
> > > > > if (!value_str.getAsInteger(X))
> > > > > return some error;
> > > > > llvm::support::endian::write(®isters[register_id], X, endian);
> > > > > ```
> > > > >
> > > > > By using llvm endianness functions you can just delete the
> > > > > `SwitchEndian()` function entirely, as it's not needed.
> > > > These endian functions don't work here. getAsInteger() assumes the
> > > > number is in human reading order (i.e. big endian). So it's converting
> > > > the little endian string as if it's big, then converting that little
> > > > endian long to another little endian long, which does nothing.
> > > > I need something that reverses the string first.
> > > >
> > > > What do you think about adding a version of StringRef::getAsInteger
> > > > that accepts an endianness parameter?
> > > Hmm.. What about this?
> > >
> > > ```
> > > unsigned long X;
> > > if (!value_str.getAsInteger(X))
> > > return some error;
> > > sys::swapByteOrder(X);
> > > ```
> > >
> > > It shouldn't matter if you swap the bytes in the string before doing the
> > > translation, or swap the bytes in the number after doing the translation
> > > should it?
> > It doesn't matter when I do it. The issue with the other functions was they
> > were converting target to host, when both were little. For string parsing,
> > it needs target to big to string, or target to string to big.
> Maybe I'm just missing something, but if you've got the string "ABCDEF" which
> is a little endian string, then it is supposed to represent the number
> 0x00EFCDAB or 15,715,755 (assuming a 4 byte number). If you parse it as big
> endian, which is what `getAsInteger` does, you're going to end up with the
> number 0xABCDEF00 which is 2,882,400,000, which is |AB|CD|EF|00| (on a big
> endian machine) or |00|EF|CD|AB| (on a little endian machine). If you then
> swap the bytes, you're going to end up with |00|EF|CD|AB| (on a big endian
> machine) or |AB|CD|EF|00| on a little endian machine. In both cases, these
> represent the number 15,715,755 as desired.
>
> It's possible I'm just getting confused here, but I don't see why the above
> code sample doesn't work.
We are also technically supposed to support PDP endian. It's part of the GDB
protocol and there are scratches for it in the LLDB code. Currently we don't
support these targets in LLVM, but this might change in future (GCC actively
maintains these targets).
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32732: "target variable" not showing all global variable if we print any global variable before execution starts
jingham added a comment. That looks like the right way to do it. What was your thinking behind returning null rather than the partial list of variables already parsed if can_create is false? That doesn't seem like what somebody who is bothering to pass in can_create false intends. Repository: rL LLVM https://reviews.llvm.org/D32732 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors marked 17 inline comments as done.
jmajors added a comment.
More feedback changes.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ LLVM_BUILTIN_DEBUGTRAP;
+ delay = false;
krytarowski wrote:
> jmajors wrote:
> > zturner wrote:
> > > This will work on MSVC and presumably clang. I'm not sure about gcc. Is
> > > that sufficient for your needs? Do you know if gcc has the
> > > `__builtin_debugtrap` intrinsic?
> > Do we use gcc to build/test lldb? If not, then it shouldn't be an issue. If
> > we ever change our compiler of choice, we can always change this to match.
> Yes, we use and support GCC with libstdc++ to build LLDB including tests. At
> least on NetBSD.
Is there a gcc equivalent, that I could wrap in some #ifdefs?
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:83-88
+ if (endian == LITTLE) {
+registers[register_id] = SwitchEndian(value_str);
+ }
+ else {
+registers[register_id] = stoul(value_str, nullptr, 16);
+ }
krytarowski wrote:
> zturner wrote:
> > jmajors wrote:
> > > zturner wrote:
> > > > jmajors wrote:
> > > > > zturner wrote:
> > > > > > This code block is unnecessary.
> > > > > >
> > > > > > ```
> > > > > > unsigned long X;
> > > > > > if (!value_str.getAsInteger(X))
> > > > > > return some error;
> > > > > > llvm::support::endian::write(®isters[register_id], X, endian);
> > > > > > ```
> > > > > >
> > > > > > By using llvm endianness functions you can just delete the
> > > > > > `SwitchEndian()` function entirely, as it's not needed.
> > > > > These endian functions don't work here. getAsInteger() assumes the
> > > > > number is in human reading order (i.e. big endian). So it's
> > > > > converting the little endian string as if it's big, then converting
> > > > > that little endian long to another little endian long, which does
> > > > > nothing.
> > > > > I need something that reverses the string first.
> > > > >
> > > > > What do you think about adding a version of StringRef::getAsInteger
> > > > > that accepts an endianness parameter?
> > > > Hmm.. What about this?
> > > >
> > > > ```
> > > > unsigned long X;
> > > > if (!value_str.getAsInteger(X))
> > > > return some error;
> > > > sys::swapByteOrder(X);
> > > > ```
> > > >
> > > > It shouldn't matter if you swap the bytes in the string before doing
> > > > the translation, or swap the bytes in the number after doing the
> > > > translation should it?
> > > It doesn't matter when I do it. The issue with the other functions was
> > > they were converting target to host, when both were little. For string
> > > parsing, it needs target to big to string, or target to string to big.
> > Maybe I'm just missing something, but if you've got the string "ABCDEF"
> > which is a little endian string, then it is supposed to represent the
> > number 0x00EFCDAB or 15,715,755 (assuming a 4 byte number). If you parse
> > it as big endian, which is what `getAsInteger` does, you're going to end up
> > with the number 0xABCDEF00 which is 2,882,400,000, which is |AB|CD|EF|00|
> > (on a big endian machine) or |00|EF|CD|AB| (on a little endian machine).
> > If you then swap the bytes, you're going to end up with |00|EF|CD|AB| (on a
> > big endian machine) or |AB|CD|EF|00| on a little endian machine. In both
> > cases, these represent the number 15,715,755 as desired.
> >
> > It's possible I'm just getting confused here, but I don't see why the above
> > code sample doesn't work.
> We are also technically supposed to support PDP endian. It's part of the GDB
> protocol and there are scratches for it in the LLDB code. Currently we don't
> support these targets in LLVM, but this might change in future (GCC actively
> maintains these targets).
I might have been unclear.
Using swapByteOrder, when the target is little endian works. I was explaining
why the read & write functions didn't work for this case.
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:70
+void TestClient::StopDebugger() {
+ Host::Kill(server_process_info.GetProcessID(), 15);
+}
krytarowski wrote:
> jmajors wrote:
> > labath wrote:
> > > This is not portable.
> > Is there a portable way of stopping?
> `15` on my platform (NetBSD) is `SIGTERM`.
>
> I've implemented similar feature in `NativeProcessNetBSD::Halt()`. Perhaps
> you can call `Halt()` as well?
I changed it to send $k and let the lldb-server figure out platform issues.
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors updated this revision to Diff 98826.
jmajors marked an inline comment as done.
jmajors added a comment.
Feedback changes.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unittests/tools/lldb-server/inferior/thread_inferior.cpp
unittests/tools/lldb-server/tests/CMakeLists.txt
unittests/tools/lldb-server/tests/MessageObjects.cpp
unittests/tools/lldb-server/tests/MessageObjects.h
unittests/tools/lldb-server/tests/TestClient.cpp
unittests/tools/lldb-server/tests/TestClient.h
unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
Index: unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
@@ -0,0 +1,50 @@
+//===-- ThreadsInJstopinfoTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include
+
+#include "gtest/gtest.h"
+#include "TestClient.h"
+
+using namespace CommunicationTests;
+
+TEST(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) {
+ std::vector inferior_args;
+ // This inferior spawns N threads, then forces a break.
+ inferior_args.push_back(THREAD_INFERIOR);
+ inferior_args.push_back("4");
+
+ auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
+
+ TestClient client(test_info->name(), test_info->test_case_name());
+ client.StartDebugger();
+ client.SetInferior(inferior_args);
+ client.ListThreadsInStopReply();
+ client.Continue();
+ unsigned int pc_reg = client.GetPcRegisterId();
+
+ auto jthreads_info = client.GetJThreadsInfo();
+ ASSERT_TRUE(jthreads_info.get()) << "Error reading JThreadInfo.";
+
+ auto stop_reply = client.GetLatestStopReply();
+ auto stop_reply_pcs = stop_reply->GetThreadPcs();
+ auto thread_infos = jthreads_info->GetThreadInfos();
+ ASSERT_EQ(stop_reply_pcs.size(), thread_infos.size())
+<< "Thread count mismatch.";
+
+ for (auto stop_reply_pc : stop_reply_pcs) {
+unsigned long tid = stop_reply_pc.first;
+ASSERT_TRUE(thread_infos.find(tid) != thread_infos.end())
+ << "Thread ID: " << tid << " not in JThreadsInfo.";
+ASSERT_EQ(stop_reply_pcs[tid], thread_infos[tid].ReadRegister(pc_reg))
+ << "Mismatched PC for thread: " << tid;
+ }
+
+ client.StopDebugger();
+}
Index: unittests/tools/lldb-server/tests/TestClient.h
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/TestClient.h
@@ -0,0 +1,57 @@
+//===-- TestClient.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include
+#include
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Target/ProcessLaunchInfo.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
+
+#include "MessageObjects.h"
+
+namespace CommunicationTests {
+// TODO: Make the test client an abstract base class, with different children
+// for different types of connections: llgs v. debugserver
+class TestClient :
+ public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
+public:
+ TestClient(const std::string& test_name, const std::string& test_case_name);
+ virtual ~TestClient();
+ void StartDebugger();
+ void StopDebugger();
+ void SetInferior(llvm::ArrayRef inferior_args);
+ void ListThreadsInStopReply();
+ void SetBreakpoint(unsigned long address);
+ void Continue(unsigned long thread_id = 0);
+ std::shared_ptr GetProcessInfo();
+ std::shared_ptr GetJThreadsInfo();
+ std::shared_ptr GetLatestStopReply();
+ void SendMessage(const std::string& message);
+ void SendMessage(const std::string& message, std::string& response_string);
+ void SendMessage(const std::string& message, std::string& response_string,
+ PacketResult expected_result);
+ unsigned int GetPcRegisterId();
+
+private:
+ void GenerateConnectionAddress(std::string& address);
+ std::string GenerateLogFileName(const lldb_private::ArchSpec& arch) const;
+ std::string FormatFailedResult(const std::string& message,
+ lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult
+ result);
+
+ std::shared_ptr process_info;
+ std::shared_ptr stop_reply;
+ lldb_private::ProcessLaunchInfo server_process_info;
+ std::string test_name;
+ std::string test_case_name;
+ unsigned int pc_register;
+};
+}
Index: unittests/tools/lldb-server/tests/
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
zturner added a comment.
Getting pretty close. Sorry, still have a few more nits.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:25-30
+ elements["pid"].getAsInteger(16, pid);
+ elements["parent-pid"].getAsInteger(16, parent_pid);
+ elements["real-uid"].getAsInteger(16, real_uid);
+ elements["real-gid"].getAsInteger(16, real_gid);
+ elements["effective-uid"].getAsInteger(16, effective_uid);
+ elements["effective-gid"].getAsInteger(16, effective_gid);
Since this is in a constructor, what happens if you get a malformed response?
You don't have a way to return an error here, and none of these errors are
checked. Do you want to `assert` if this happens? Or are you ok silently
dropping this kind of failure?
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:109
+ if (value_str.getAsInteger(16, register_value)) {
+return std::shared_ptr(nullptr);
+ }
You don't need to explicitly construct a `std::shared_ptr`. You can just write
`return nullptr;` There are a couple of other places above this that do the
same thing.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+llvm::support::endianness endian) {
+ auto stop_reply = std::shared_ptr(new StopReply());
+
Can you use `llvm::make_shared()` here? Probably doesn't matter
much, but it's good practice to use `make_shared<>` wherever possible since it
uses less memory.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:159-160
+unsigned long pc;
+threads[i].getAsInteger(16, thread_id);
+pcs[i].getAsInteger(16, pc);
+stop_reply->thread_pcs[thread_id] = pc;
Is Error checking needed on these two calls?
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:167
+std::stringstream ss;
+ss << std::hex << std::setw(2) << std::setfill('0') << register_id;
+std::string hex_id = ss.str();
Is this the same as `std::string hex_id = llvm::utostr(register_id);`?
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+ val.getAsInteger(16, stop_reply->thread);
+ key.substr(1, 2).getAsInteger(16, stop_reply->signal);
Is error checking needed here?
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:105-108
+ std::stringstream message;
+ message << "vCont;c:" << std::hex << thread_id;
+ std::string response;
+ SendMessage(message.str(), response);
Can you use `llvm` formatting methods instead of `std::stringstream` here?
`std::string message = llvm::formatv("vCont;c:{0:x-}", thread_id).str();`
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:155-156
+ for (unsigned int register_id = 0; ; register_id++) {
+std::stringstream message;
+message << "qRegisterInfo" << std::hex << register_id;
+// This will throw if we scan all registers and never get the
`std::string message = llvm::formatv("qRegisterInfo{0:x-}", register_id).str();`
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
krytarowski added inline comments. Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21 + + LLVM_BUILTIN_DEBUGTRAP; + delay = false; jmajors wrote: > krytarowski wrote: > > jmajors wrote: > > > zturner wrote: > > > > This will work on MSVC and presumably clang. I'm not sure about gcc. > > > > Is that sufficient for your needs? Do you know if gcc has the > > > > `__builtin_debugtrap` intrinsic? > > > Do we use gcc to build/test lldb? If not, then it shouldn't be an issue. > > > If we ever change our compiler of choice, we can always change this to > > > match. > > Yes, we use and support GCC with libstdc++ to build LLDB including tests. > > At least on NetBSD. > Is there a gcc equivalent, that I could wrap in some #ifdefs? No, there is no equivalent and I'm trying to convince that we should not try to use this `__builtin_debugtrap()` in the code. We should ask the debugger to set and handle the trap. Otherwise we will need to handle this on per-cpu and per-os matrix. In the SPARC/NetBSD example we would need to ask the debugger to set PC after the trap manually. https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
krytarowski added inline comments. Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:33 + uid_t real_uid; + unsigned int real_gid; + uid_t effective_uid; Is there an option to use `gid_t`? https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r302948 - Fix build on Mac OS.
Author: eugene Date: Fri May 12 15:44:09 2017 New Revision: 302948 URL: http://llvm.org/viewvc/llvm-project?rev=302948&view=rev Log: Fix build on Mac OS. Modified: lldb/trunk/source/Host/macosx/Host.mm Modified: lldb/trunk/source/Host/macosx/Host.mm URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Host/macosx/Host.mm?rev=302948&r1=302947&r2=302948&view=diff == --- lldb/trunk/source/Host/macosx/Host.mm (original) +++ lldb/trunk/source/Host/macosx/Host.mm Fri May 12 15:44:09 2017 @@ -1328,8 +1328,7 @@ Status Host::ShellExpandArguments(Proces if (!str_sp) continue; - launch_info.GetArguments().AppendArgument( - llvm::StringRef(str_sp->GetValue().c_str())); + launch_info.GetArguments().AppendArgument(str_sp->GetValue()); } } ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r302954 - Fixed the OS X build after Error -> Status rename.
Author: spyffe
Date: Fri May 12 16:53:44 2017
New Revision: 302954
URL: http://llvm.org/viewvc/llvm-project?rev=302954&view=rev
Log:
Fixed the OS X build after Error -> Status rename.
Modified:
lldb/trunk/lldb.xcodeproj/project.pbxproj
Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=302954&r1=302953&r2=302954&view=diff
==
--- lldb/trunk/lldb.xcodeproj/project.pbxproj (original)
+++ lldb/trunk/lldb.xcodeproj/project.pbxproj Fri May 12 16:53:44 2017
@@ -333,7 +333,6 @@
26744EF11338317700EF765A /* GDBRemoteCommunicationClient.cpp in
Sources */ = {isa = PBXBuildFile; fileRef = 26744EED1338317700EF765A /*
GDBRemoteCommunicationClient.cpp */; };
26744EF31338317700EF765A /* GDBRemoteCommunicationServer.cpp in
Sources */ = {isa = PBXBuildFile; fileRef = 26744EEF1338317700EF765A /*
GDBRemoteCommunicationServer.cpp */; };
26764C971E48F482008D3573 /* ConstString.cpp in Sources */ =
{isa = PBXBuildFile; fileRef = 26764C961E48F482008D3573 /* ConstString.cpp */;
};
- 26764C991E48F4D2008D3573 /* Error.cpp in Sources */ = {isa =
PBXBuildFile; fileRef = 26764C981E48F4D2008D3573 /* Error.cpp */; };
26764C9E1E48F51E008D3573 /* Stream.cpp in Sources */ = {isa =
PBXBuildFile; fileRef = 26764C9D1E48F51E008D3573 /* Stream.cpp */; };
26764CA01E48F528008D3573 /* RegularExpression.cpp in Sources */
= {isa = PBXBuildFile; fileRef = 26764C9F1E48F528008D3573 /*
RegularExpression.cpp */; };
26764CA21E48F547008D3573 /* StreamString.cpp in Sources */ =
{isa = PBXBuildFile; fileRef = 26764CA11E48F547008D3573 /* StreamString.cpp */;
};
@@ -691,6 +690,8 @@
3FDFED2D19C257A0009756A7 /* HostProcess.cpp in Sources */ =
{isa = PBXBuildFile; fileRef = 3FDFED2C19C257A0009756A7 /* HostProcess.cpp */;
};
490A36C0180F0E6F00BA31F8 /* PlatformWindows.cpp in Sources */ =
{isa = PBXBuildFile; fileRef = 490A36BD180F0E6F00BA31F8 /* PlatformWindows.cpp
*/; };
490A966B1628C3BF00F0002E /* SBDeclaration.h in Headers */ =
{isa = PBXBuildFile; fileRef = 9452573816262CEF00325455 /* SBDeclaration.h */;
settings = {ATTRIBUTES = (Public, ); }; };
+ 492DB7E71EC662B100B9E9AF /* Status.h in CopyFiles */ = {isa =
PBXBuildFile; fileRef = 492DB7E61EC662B100B9E9AF /* Status.h */; };
+ 492DB7EB1EC662E200B9E9AF /* Status.cpp in Sources */ = {isa =
PBXBuildFile; fileRef = 492DB7E81EC662D100B9E9AF /* Status.cpp */; };
4939EA8D1BD56B6D00084382 /* REPL.cpp in Sources */ = {isa =
PBXBuildFile; fileRef = 4939EA8C1BD56B6D00084382 /* REPL.cpp */; };
494260DA14579144003C1C78 /* VerifyDecl.cpp in Sources */ = {isa
= PBXBuildFile; fileRef = 494260D914579144003C1C78 /* VerifyDecl.cpp */; };
4959511F1A1BC4BC00F6F8FC /* ClangModulesDeclVendor.cpp in
Sources */ = {isa = PBXBuildFile; fileRef = 4959511E1A1BC4BC00F6F8FC /*
ClangModulesDeclVendor.cpp */; };
@@ -1193,6 +1194,7 @@
dstPath = "$(DEVELOPER_INSTALL_DIR)/usr/share/man/man1";
dstSubfolderSpec = 0;
files = (
+ 492DB7E71EC662B100B9E9AF /* Status.h in
CopyFiles */,
AF90106515AB7D3600FF120D /* lldb.1 in CopyFiles
*/,
);
runOnlyForDeploymentPostprocessing = 1;
@@ -1853,8 +1855,6 @@
2675F6FF1332BE690067997B /* PlatformRemoteiOS.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path =
PlatformRemoteiOS.h; sourceTree = ""; };
26764C951E48F46F008D3573 /* ConstString.h */ = {isa =
PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ConstString.h;
path = include/lldb/Utility/ConstString.h; sourceTree = ""; };
26764C961E48F482008D3573 /* ConstString.cpp */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
name = ConstString.cpp; path = source/Utility/ConstString.cpp; sourceTree =
""; };
- 26764C981E48F4D2008D3573 /* Error.cpp */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp;
name = Error.cpp; path = source/Utility/Error.cpp; sourceTree = ""; };
- 26764C9A1E48F4DD008D3573 /* Error.h */ = {isa =
PBXFileReference; lastKnownFileType = sourcecode.c.h; name = Error.h; path =
include/lldb/Utility/Error.h; sourceTree = ""; };
26764C9B1E48F50C008D3573 /* Stream.h */ = {isa =
PBXFileReference; lastKnownFileType = sourcecode.c.h; name = Stream.h; path =
include/lldb/Utility/Stream.h; sourceTree = ""; };
26764C9C1E48F516008D3573 /* RegularExpression.h */ = {isa =
PBXFileReference; lastKnownFileType = sourcecode.c.h;
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
labath added a comment.
next batch of comments from me (I expect to have more on monday). :)
In https://reviews.llvm.org/D32930#752843, @krytarowski wrote:
> I can build locally with `make thread_inferior`, how to run it?
run the `check-lldb-unit` target.
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ LLVM_BUILTIN_DEBUGTRAP;
+ delay = false;
krytarowski wrote:
> jmajors wrote:
> > krytarowski wrote:
> > > jmajors wrote:
> > > > zturner wrote:
> > > > > This will work on MSVC and presumably clang. I'm not sure about gcc.
> > > > > Is that sufficient for your needs? Do you know if gcc has the
> > > > > `__builtin_debugtrap` intrinsic?
> > > > Do we use gcc to build/test lldb? If not, then it shouldn't be an
> > > > issue. If we ever change our compiler of choice, we can always change
> > > > this to match.
> > > Yes, we use and support GCC with libstdc++ to build LLDB including tests.
> > > At least on NetBSD.
> > Is there a gcc equivalent, that I could wrap in some #ifdefs?
> No, there is no equivalent and I'm trying to convince that we should not try
> to use this `__builtin_debugtrap()` in the code. We should ask the debugger
> to set and handle the trap.
>
> Otherwise we will need to handle this on per-cpu and per-os matrix. In the
> SPARC/NetBSD example we would need to ask the debugger to set PC after the
> trap manually.
The thing with the lldb-server tests is that there is no "debugger" which can
set the figure out where to set the breakpoint. Lldb-server operates at a much
lower level, and it knows nothing about dwarf, or even symbol tables, and I
think the tests shouldn't either (mainly to keep the tests more targetted, but
also because it would be quite tricky to wire that up at this level). The
existing lldb-server tests don't do debug info parsing either.
BTW, this test doesn't actually need the int3 breakpoint for its work -- all we
need is for the inferior to stop so that the debugger can take a look at this
state. Any stopping event will do the trick, and the most "portable" would
probably be dereferencing a null pointer.
However, we will get back to this soon enough, when we start talking about
breakpoint-setting tests. Since the client doesn't know anything about debug
info, the best way to figure out where to set a breakpoint is for the inferior
to tell us. The way existing lldb-server tests do that is via printf, but that
has some issues (intercepting stdio is hard or impossible on windows and stdio
comes asynchronously on linux so it is hard to write race-free tests). The most
reliable way I came up for that was to put that value in a register. Now this
requires a bit of assembly (eg., `movq %rax, $function_I_want_to_break_in;
int3` in x86 case), but that little snippet can be tucked away in a utility
function (plus a similar utility function on the recieving side to read out the
value) and noone has to look at it again, and the rest of the test can be
architecture-independent.
The assembly will obviously be architecture-dependent, but I don't think we
will really need an OS dimension. I am not sure if we currently support on os
where the PC fixup would be necessary, but even if we do, the implementation of
that would be quite simple.
I am open to other ideas on how to pass information between the inferior and
the test though.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:91
+if (!register_dict) {
+ return std::shared_ptr(nullptr);
+}
return nullptr; (I.e., a shared pointer is implicitly constructible from
nullptr).
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+llvm::support::endianness endian) {
+ auto stop_reply = std::shared_ptr(new StopReply());
+
zturner wrote:
> Can you use `llvm::make_shared()` here? Probably doesn't matter
> much, but it's good practice to use `make_shared<>` wherever possible since
> it uses less memory.
I raise the bet to `llvm::make_unique` ;). shared pointers lead to messy
ownership semantics, we should only use them when absolutely necessary, and I
don't think that is the case here.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+ val.getAsInteger(16, stop_reply->thread);
+ key.substr(1, 2).getAsInteger(16, stop_reply->signal);
zturner wrote:
> Is error checking needed here?
More error checking definitely needed (here and everywhere else). If I break
lldb-server while working on it, I'd like to see a clear error message from the
test rather than an obscure crash. This should return `Expected` if
you don't care about copying or `Expected>` if you
do (*), so that the test can ASSERT that the message was received and parsed
correctly and print out the error otherwis
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors marked 6 inline comments as done.
jmajors added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:25-30
+ elements["pid"].getAsInteger(16, pid);
+ elements["parent-pid"].getAsInteger(16, parent_pid);
+ elements["real-uid"].getAsInteger(16, real_uid);
+ elements["real-gid"].getAsInteger(16, real_gid);
+ elements["effective-uid"].getAsInteger(16, effective_uid);
+ elements["effective-gid"].getAsInteger(16, effective_gid);
zturner wrote:
> Since this is in a constructor, what happens if you get a malformed response?
> You don't have a way to return an error here, and none of these errors are
> checked. Do you want to `assert` if this happens? Or are you ok silently
> dropping this kind of failure?
I changed it to a Create with tests for success.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+llvm::support::endianness endian) {
+ auto stop_reply = std::shared_ptr(new StopReply());
+
zturner wrote:
> Can you use `llvm::make_shared()` here? Probably doesn't matter
> much, but it's good practice to use `make_shared<>` wherever possible since
> it uses less memory.
I wanted to keep my constructors protected, to force the use of Create().
make_shared doesn't work with protected constructors.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:167
+std::stringstream ss;
+ss << std::hex << std::setw(2) << std::setfill('0') << register_id;
+std::string hex_id = ss.str();
zturner wrote:
> Is this the same as `std::string hex_id = llvm::utostr(register_id);`?
No. The register IDs are all two nibbles wide, so I need the setw & setfill (or
equivalent).
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors updated this revision to Diff 98858.
jmajors marked an inline comment as done.
jmajors added a comment.
More feedback changes.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unittests/tools/lldb-server/inferior/thread_inferior.cpp
unittests/tools/lldb-server/tests/CMakeLists.txt
unittests/tools/lldb-server/tests/MessageObjects.cpp
unittests/tools/lldb-server/tests/MessageObjects.h
unittests/tools/lldb-server/tests/TestClient.cpp
unittests/tools/lldb-server/tests/TestClient.h
unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
Index: unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
@@ -0,0 +1,50 @@
+//===-- ThreadsInJstopinfoTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include
+
+#include "gtest/gtest.h"
+#include "TestClient.h"
+
+using namespace CommunicationTests;
+
+TEST(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) {
+ std::vector inferior_args;
+ // This inferior spawns N threads, then forces a break.
+ inferior_args.push_back(THREAD_INFERIOR);
+ inferior_args.push_back("4");
+
+ auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
+
+ TestClient client(test_info->name(), test_info->test_case_name());
+ client.StartDebugger();
+ client.SetInferior(inferior_args);
+ client.ListThreadsInStopReply();
+ client.Continue();
+ unsigned int pc_reg = client.GetPcRegisterId();
+
+ auto jthreads_info = client.GetJThreadsInfo();
+ ASSERT_TRUE(jthreads_info.get()) << "Error reading JThreadInfo.";
+
+ auto stop_reply = client.GetLatestStopReply();
+ auto stop_reply_pcs = stop_reply->GetThreadPcs();
+ auto thread_infos = jthreads_info->GetThreadInfos();
+ ASSERT_EQ(stop_reply_pcs.size(), thread_infos.size())
+<< "Thread count mismatch.";
+
+ for (auto stop_reply_pc : stop_reply_pcs) {
+unsigned long tid = stop_reply_pc.first;
+ASSERT_TRUE(thread_infos.find(tid) != thread_infos.end())
+ << "Thread ID: " << tid << " not in JThreadsInfo.";
+ASSERT_EQ(stop_reply_pcs[tid], thread_infos[tid].ReadRegister(pc_reg))
+ << "Mismatched PC for thread: " << tid;
+ }
+
+ client.StopDebugger();
+}
Index: unittests/tools/lldb-server/tests/TestClient.h
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/TestClient.h
@@ -0,0 +1,57 @@
+//===-- TestClient.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include
+#include
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Target/ProcessLaunchInfo.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
+
+#include "MessageObjects.h"
+
+namespace CommunicationTests {
+// TODO: Make the test client an abstract base class, with different children
+// for different types of connections: llgs v. debugserver
+class TestClient :
+ public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
+public:
+ TestClient(const std::string& test_name, const std::string& test_case_name);
+ virtual ~TestClient();
+ void StartDebugger();
+ void StopDebugger();
+ void SetInferior(llvm::ArrayRef inferior_args);
+ void ListThreadsInStopReply();
+ void SetBreakpoint(unsigned long address);
+ void Continue(unsigned long thread_id = 0);
+ std::shared_ptr GetProcessInfo();
+ std::shared_ptr GetJThreadsInfo();
+ std::shared_ptr GetLatestStopReply();
+ void SendMessage(const std::string& message);
+ void SendMessage(const std::string& message, std::string& response_string);
+ void SendMessage(const std::string& message, std::string& response_string,
+ PacketResult expected_result);
+ unsigned int GetPcRegisterId();
+
+private:
+ void GenerateConnectionAddress(std::string& address);
+ std::string GenerateLogFileName(const lldb_private::ArchSpec& arch) const;
+ std::string FormatFailedResult(const std::string& message,
+ lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult
+ result);
+
+ std::shared_ptr process_info;
+ std::shared_ptr stop_reply;
+ lldb_private::ProcessLaunchInfo server_process_info;
+ std::string test_name;
+ std::string test_case_name;
+ unsigned int pc_register;
+};
+}
Index: unittests/tools/lldb-server/t
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:167
+std::stringstream ss;
+ss << std::hex << std::setw(2) << std::setfill('0') << register_id;
+std::string hex_id = ss.str();
jmajors wrote:
> zturner wrote:
> > Is this the same as `std::string hex_id = llvm::utostr(register_id);`?
> No. The register IDs are all two nibbles wide, so I need the setw & setfill
> (or equivalent).
Bear with me here, I'm //determined// to kill this `stringstream` ;-)
How about `std::string hex_id = llvm::formatv("{0:x-2}", register_id);`
```
x = lowercase hex.
- means don't print the `0x` prefix.
2 means print at least 2 digits.
```
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
krytarowski added a comment. In https://reviews.llvm.org/D32930#753916, @labath wrote: > next batch of comments from me (I expect to have more on monday). :) > > In https://reviews.llvm.org/D32930#752843, @krytarowski wrote: > > > I can build locally with `make thread_inferior`, how to run it? > > > run the `check-lldb-unit` target. OK, thanks! https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
krytarowski added inline comments. Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21 + + LLVM_BUILTIN_DEBUGTRAP; + delay = false; labath wrote: > krytarowski wrote: > > jmajors wrote: > > > krytarowski wrote: > > > > jmajors wrote: > > > > > zturner wrote: > > > > > > This will work on MSVC and presumably clang. I'm not sure about > > > > > > gcc. Is that sufficient for your needs? Do you know if gcc has > > > > > > the `__builtin_debugtrap` intrinsic? > > > > > Do we use gcc to build/test lldb? If not, then it shouldn't be an > > > > > issue. If we ever change our compiler of choice, we can always change > > > > > this to match. > > > > Yes, we use and support GCC with libstdc++ to build LLDB including > > > > tests. At least on NetBSD. > > > Is there a gcc equivalent, that I could wrap in some #ifdefs? > > No, there is no equivalent and I'm trying to convince that we should not > > try to use this `__builtin_debugtrap()` in the code. We should ask the > > debugger to set and handle the trap. > > > > Otherwise we will need to handle this on per-cpu and per-os matrix. In the > > SPARC/NetBSD example we would need to ask the debugger to set PC after the > > trap manually. > The thing with the lldb-server tests is that there is no "debugger" which can > set the figure out where to set the breakpoint. Lldb-server operates at a > much lower level, and it knows nothing about dwarf, or even symbol tables, > and I think the tests shouldn't either (mainly to keep the tests more > targetted, but also because it would be quite tricky to wire that up at this > level). The existing lldb-server tests don't do debug info parsing either. > > BTW, this test doesn't actually need the int3 breakpoint for its work -- all > we need is for the inferior to stop so that the debugger can take a look at > this state. Any stopping event will do the trick, and the most "portable" > would probably be dereferencing a null pointer. > > However, we will get back to this soon enough, when we start talking about > breakpoint-setting tests. Since the client doesn't know anything about debug > info, the best way to figure out where to set a breakpoint is for the > inferior to tell us. The way existing lldb-server tests do that is via > printf, but that has some issues (intercepting stdio is hard or impossible on > windows and stdio comes asynchronously on linux so it is hard to write > race-free tests). The most reliable way I came up for that was to put that > value in a register. Now this requires a bit of assembly (eg., `movq %rax, > $function_I_want_to_break_in; int3` in x86 case), but that little snippet can > be tucked away in a utility function (plus a similar utility function on the > recieving side to read out the value) and noone has to look at it again, and > the rest of the test can be architecture-independent. > > The assembly will obviously be architecture-dependent, but I don't think we > will really need an OS dimension. I am not sure if we currently support on os > where the PC fixup would be necessary, but even if we do, the implementation > of that would be quite simple. > > I am open to other ideas on how to pass information between the inferior and > the test though. Can we go for `raise(SIGTRAP)`? https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
labath added inline comments. Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21 + + LLVM_BUILTIN_DEBUGTRAP; + delay = false; krytarowski wrote: > labath wrote: > > krytarowski wrote: > > > jmajors wrote: > > > > krytarowski wrote: > > > > > jmajors wrote: > > > > > > zturner wrote: > > > > > > > This will work on MSVC and presumably clang. I'm not sure about > > > > > > > gcc. Is that sufficient for your needs? Do you know if gcc has > > > > > > > the `__builtin_debugtrap` intrinsic? > > > > > > Do we use gcc to build/test lldb? If not, then it shouldn't be an > > > > > > issue. If we ever change our compiler of choice, we can always > > > > > > change this to match. > > > > > Yes, we use and support GCC with libstdc++ to build LLDB including > > > > > tests. At least on NetBSD. > > > > Is there a gcc equivalent, that I could wrap in some #ifdefs? > > > No, there is no equivalent and I'm trying to convince that we should not > > > try to use this `__builtin_debugtrap()` in the code. We should ask the > > > debugger to set and handle the trap. > > > > > > Otherwise we will need to handle this on per-cpu and per-os matrix. In > > > the SPARC/NetBSD example we would need to ask the debugger to set PC > > > after the trap manually. > > The thing with the lldb-server tests is that there is no "debugger" which > > can set the figure out where to set the breakpoint. Lldb-server operates at > > a much lower level, and it knows nothing about dwarf, or even symbol > > tables, and I think the tests shouldn't either (mainly to keep the tests > > more targetted, but also because it would be quite tricky to wire that up > > at this level). The existing lldb-server tests don't do debug info parsing > > either. > > > > BTW, this test doesn't actually need the int3 breakpoint for its work -- > > all we need is for the inferior to stop so that the debugger can take a > > look at this state. Any stopping event will do the trick, and the most > > "portable" would probably be dereferencing a null pointer. > > > > However, we will get back to this soon enough, when we start talking about > > breakpoint-setting tests. Since the client doesn't know anything about > > debug info, the best way to figure out where to set a breakpoint is for the > > inferior to tell us. The way existing lldb-server tests do that is via > > printf, but that has some issues (intercepting stdio is hard or impossible > > on windows and stdio comes asynchronously on linux so it is hard to write > > race-free tests). The most reliable way I came up for that was to put that > > value in a register. Now this requires a bit of assembly (eg., `movq %rax, > > $function_I_want_to_break_in; int3` in x86 case), but that little snippet > > can be tucked away in a utility function (plus a similar utility function > > on the recieving side to read out the value) and noone has to look at it > > again, and the rest of the test can be architecture-independent. > > > > The assembly will obviously be architecture-dependent, but I don't think we > > will really need an OS dimension. I am not sure if we currently support on > > os where the PC fixup would be necessary, but even if we do, the > > implementation of that would be quite simple. > > > > I am open to other ideas on how to pass information between the inferior > > and the test though. > Can we go for `raise(SIGTRAP)`? Doesn't work on windows (so it is not more portable than null dereference) and it has no payload (so you cannot use it for passing data to the test) :) https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
zturner added inline comments. Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21 + + LLVM_BUILTIN_DEBUGTRAP; + delay = false; labath wrote: > krytarowski wrote: > > labath wrote: > > > krytarowski wrote: > > > > jmajors wrote: > > > > > krytarowski wrote: > > > > > > jmajors wrote: > > > > > > > zturner wrote: > > > > > > > > This will work on MSVC and presumably clang. I'm not sure > > > > > > > > about gcc. Is that sufficient for your needs? Do you know if > > > > > > > > gcc has the `__builtin_debugtrap` intrinsic? > > > > > > > Do we use gcc to build/test lldb? If not, then it shouldn't be an > > > > > > > issue. If we ever change our compiler of choice, we can always > > > > > > > change this to match. > > > > > > Yes, we use and support GCC with libstdc++ to build LLDB including > > > > > > tests. At least on NetBSD. > > > > > Is there a gcc equivalent, that I could wrap in some #ifdefs? > > > > No, there is no equivalent and I'm trying to convince that we should > > > > not try to use this `__builtin_debugtrap()` in the code. We should ask > > > > the debugger to set and handle the trap. > > > > > > > > Otherwise we will need to handle this on per-cpu and per-os matrix. In > > > > the SPARC/NetBSD example we would need to ask the debugger to set PC > > > > after the trap manually. > > > The thing with the lldb-server tests is that there is no "debugger" which > > > can set the figure out where to set the breakpoint. Lldb-server operates > > > at a much lower level, and it knows nothing about dwarf, or even symbol > > > tables, and I think the tests shouldn't either (mainly to keep the tests > > > more targetted, but also because it would be quite tricky to wire that up > > > at this level). The existing lldb-server tests don't do debug info > > > parsing either. > > > > > > BTW, this test doesn't actually need the int3 breakpoint for its work -- > > > all we need is for the inferior to stop so that the debugger can take a > > > look at this state. Any stopping event will do the trick, and the most > > > "portable" would probably be dereferencing a null pointer. > > > > > > However, we will get back to this soon enough, when we start talking > > > about breakpoint-setting tests. Since the client doesn't know anything > > > about debug info, the best way to figure out where to set a breakpoint is > > > for the inferior to tell us. The way existing lldb-server tests do that > > > is via printf, but that has some issues (intercepting stdio is hard or > > > impossible on windows and stdio comes asynchronously on linux so it is > > > hard to write race-free tests). The most reliable way I came up for that > > > was to put that value in a register. Now this requires a bit of assembly > > > (eg., `movq %rax, $function_I_want_to_break_in; int3` in x86 case), but > > > that little snippet can be tucked away in a utility function (plus a > > > similar utility function on the recieving side to read out the value) and > > > noone has to look at it again, and the rest of the test can be > > > architecture-independent. > > > > > > The assembly will obviously be architecture-dependent, but I don't think > > > we will really need an OS dimension. I am not sure if we currently > > > support on os where the PC fixup would be necessary, but even if we do, > > > the implementation of that would be quite simple. > > > > > > I am open to other ideas on how to pass information between the inferior > > > and the test though. > > Can we go for `raise(SIGTRAP)`? > Doesn't work on windows (so it is not more portable than null dereference) > and it has no payload (so you cannot use it for passing data to the test) :) Just use `LLVM_BUILTIN_TRAP` or a null pointer dereference, so it works everywhere. https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
krytarowski added inline comments. Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21 + + LLVM_BUILTIN_DEBUGTRAP; + delay = false; zturner wrote: > labath wrote: > > krytarowski wrote: > > > labath wrote: > > > > krytarowski wrote: > > > > > jmajors wrote: > > > > > > krytarowski wrote: > > > > > > > jmajors wrote: > > > > > > > > zturner wrote: > > > > > > > > > This will work on MSVC and presumably clang. I'm not sure > > > > > > > > > about gcc. Is that sufficient for your needs? Do you know > > > > > > > > > if gcc has the `__builtin_debugtrap` intrinsic? > > > > > > > > Do we use gcc to build/test lldb? If not, then it shouldn't be > > > > > > > > an issue. If we ever change our compiler of choice, we can > > > > > > > > always change this to match. > > > > > > > Yes, we use and support GCC with libstdc++ to build LLDB > > > > > > > including tests. At least on NetBSD. > > > > > > Is there a gcc equivalent, that I could wrap in some #ifdefs? > > > > > No, there is no equivalent and I'm trying to convince that we should > > > > > not try to use this `__builtin_debugtrap()` in the code. We should > > > > > ask the debugger to set and handle the trap. > > > > > > > > > > Otherwise we will need to handle this on per-cpu and per-os matrix. > > > > > In the SPARC/NetBSD example we would need to ask the debugger to set > > > > > PC after the trap manually. > > > > The thing with the lldb-server tests is that there is no "debugger" > > > > which can set the figure out where to set the breakpoint. Lldb-server > > > > operates at a much lower level, and it knows nothing about dwarf, or > > > > even symbol tables, and I think the tests shouldn't either (mainly to > > > > keep the tests more targetted, but also because it would be quite > > > > tricky to wire that up at this level). The existing lldb-server tests > > > > don't do debug info parsing either. > > > > > > > > BTW, this test doesn't actually need the int3 breakpoint for its work > > > > -- all we need is for the inferior to stop so that the debugger can > > > > take a look at this state. Any stopping event will do the trick, and > > > > the most "portable" would probably be dereferencing a null pointer. > > > > > > > > However, we will get back to this soon enough, when we start talking > > > > about breakpoint-setting tests. Since the client doesn't know anything > > > > about debug info, the best way to figure out where to set a breakpoint > > > > is for the inferior to tell us. The way existing lldb-server tests do > > > > that is via printf, but that has some issues (intercepting stdio is > > > > hard or impossible on windows and stdio comes asynchronously on linux > > > > so it is hard to write race-free tests). The most reliable way I came > > > > up for that was to put that value in a register. Now this requires a > > > > bit of assembly (eg., `movq %rax, $function_I_want_to_break_in; int3` > > > > in x86 case), but that little snippet can be tucked away in a utility > > > > function (plus a similar utility function on the recieving side to read > > > > out the value) and noone has to look at it again, and the rest of the > > > > test can be architecture-independent. > > > > > > > > The assembly will obviously be architecture-dependent, but I don't > > > > think we will really need an OS dimension. I am not sure if we > > > > currently support on os where the PC fixup would be necessary, but even > > > > if we do, the implementation of that would be quite simple. > > > > > > > > I am open to other ideas on how to pass information between the > > > > inferior and the test though. > > > Can we go for `raise(SIGTRAP)`? > > Doesn't work on windows (so it is not more portable than null dereference) > > and it has no payload (so you cannot use it for passing data to the test) :) > Just use `LLVM_BUILTIN_TRAP` or a null pointer dereference, so it works > everywhere. I'm for NULL pointer dereference. ``` volatile char *p = NULL; *p = 'a'; ``` or similar. https://reviews.llvm.org/D32930 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors marked 7 inline comments as done.
jmajors added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:142
+llvm::support::endianness endian) {
+ auto stop_reply = std::shared_ptr(new StopReply());
+
labath wrote:
> jmajors wrote:
> > zturner wrote:
> > > Can you use `llvm::make_shared()` here? Probably doesn't
> > > matter much, but it's good practice to use `make_shared<>` wherever
> > > possible since it uses less memory.
> > I wanted to keep my constructors protected, to force the use of Create().
> > make_shared doesn't work with protected constructors.
> I raise the bet to `llvm::make_unique` ;). shared pointers lead to messy
> ownership semantics, we should only use them when absolutely necessary, and I
> don't think that is the case here.
Since I'm returning copies of the pointer container in getter functions, I
think I need shared, not unique.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+ val.getAsInteger(16, stop_reply->thread);
+ key.substr(1, 2).getAsInteger(16, stop_reply->signal);
labath wrote:
> zturner wrote:
> > Is error checking needed here?
> More error checking definitely needed (here and everywhere else). If I break
> lldb-server while working on it, I'd like to see a clear error message from
> the test rather than an obscure crash. This should return
> `Expected` if you don't care about copying or
> `Expected>` if you do (*), so that the test can
> ASSERT that the message was received and parsed correctly and print out the
> error otherwise.
>
> (*) Or the out argument idea I wrote about earlier.
I converted my pointer containers to Expected>.
I noticed that ASSERT_* doesn't fail the test, it returns, so I need to make
the functions in TestClient return Error or Expected<> objects and check them
in the test body.
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
jmajors updated this revision to Diff 98869.
jmajors marked 2 inline comments as done.
jmajors added a comment.
Changed shared_ptr returns to Expected>.
Some other error processing additions.
https://reviews.llvm.org/D32930
Files:
unittests/CMakeLists.txt
unittests/tools/CMakeLists.txt
unittests/tools/lldb-server/CMakeLists.txt
unittests/tools/lldb-server/inferior/thread_inferior.cpp
unittests/tools/lldb-server/tests/CMakeLists.txt
unittests/tools/lldb-server/tests/MessageObjects.cpp
unittests/tools/lldb-server/tests/MessageObjects.h
unittests/tools/lldb-server/tests/TestClient.cpp
unittests/tools/lldb-server/tests/TestClient.h
unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
Index: unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/ThreadIdsInJstopinfoTest.cpp
@@ -0,0 +1,50 @@
+//===-- ThreadsInJstopinfoTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include
+
+#include "gtest/gtest.h"
+#include "TestClient.h"
+
+using namespace CommunicationTests;
+
+TEST(ThreadsInJstopinfoTest, TestStopReplyContainsThreadPcsLlgs) {
+ std::vector inferior_args;
+ // This inferior spawns N threads, then forces a break.
+ inferior_args.push_back(THREAD_INFERIOR);
+ inferior_args.push_back("4");
+
+ auto test_info = ::testing::UnitTest::GetInstance()->current_test_info();
+
+ TestClient client(test_info->name(), test_info->test_case_name());
+ client.StartDebugger();
+ client.SetInferior(inferior_args);
+ client.ListThreadsInStopReply();
+ client.Continue();
+ unsigned int pc_reg = client.GetPcRegisterId();
+
+ auto jthreads_info = client.GetJThreadsInfo();
+ ASSERT_TRUE(jthreads_info.get()) << "Error reading JThreadInfo.";
+
+ auto stop_reply = client.GetLatestStopReply();
+ auto stop_reply_pcs = stop_reply.GetThreadPcs();
+ auto thread_infos = jthreads_info->GetThreadInfos();
+ ASSERT_EQ(stop_reply_pcs.size(), thread_infos.size())
+<< "Thread count mismatch.";
+
+ for (auto stop_reply_pc : stop_reply_pcs) {
+unsigned long tid = stop_reply_pc.first;
+ASSERT_TRUE(thread_infos.find(tid) != thread_infos.end())
+ << "Thread ID: " << tid << " not in JThreadsInfo.";
+ASSERT_EQ(stop_reply_pcs[tid], thread_infos[tid].ReadRegister(pc_reg))
+ << "Mismatched PC for thread: " << tid;
+ }
+
+ client.StopDebugger();
+}
Index: unittests/tools/lldb-server/tests/TestClient.h
===
--- /dev/null
+++ unittests/tools/lldb-server/tests/TestClient.h
@@ -0,0 +1,57 @@
+//===-- TestClient.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include
+#include
+
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Target/ProcessLaunchInfo.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
+
+#include "MessageObjects.h"
+
+namespace CommunicationTests {
+// TODO: Make the test client an abstract base class, with different children
+// for different types of connections: llgs v. debugserver
+class TestClient :
+ public lldb_private::process_gdb_remote::GDBRemoteCommunicationClient {
+public:
+ TestClient(const std::string& test_name, const std::string& test_case_name);
+ virtual ~TestClient();
+ void StartDebugger();
+ void StopDebugger();
+ void SetInferior(llvm::ArrayRef inferior_args);
+ void ListThreadsInStopReply();
+ void SetBreakpoint(unsigned long address);
+ void Continue(unsigned long thread_id = 0);
+ const ProcessInfo& GetProcessInfo();
+ std::unique_ptr GetJThreadsInfo();
+ const StopReply& GetLatestStopReply();
+ void SendMessage(const std::string& message);
+ void SendMessage(const std::string& message, std::string& response_string);
+ void SendMessage(const std::string& message, std::string& response_string,
+ PacketResult expected_result);
+ unsigned int GetPcRegisterId();
+
+private:
+ void GenerateConnectionAddress(std::string& address);
+ std::string GenerateLogFileName(const lldb_private::ArchSpec& arch) const;
+ std::string FormatFailedResult(const std::string& message,
+ lldb_private::process_gdb_remote::GDBRemoteCommunication::PacketResult
+ result);
+
+ std::unique_ptr process_info;
+ std::unique_ptr stop_reply;
+ lldb_private::ProcessLaunchInfo server_process_info;
+ std::string test_name;
+ std::string test_case_name;
+ unsigned
[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
zturner added inline comments.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:189
+if (key.size() >= 9 && key[0] == 'T' && key.substr(3, 6) == "thread") {
+ val.getAsInteger(16, stop_reply->thread);
+ key.substr(1, 2).getAsInteger(16, stop_reply->signal);
jmajors wrote:
> labath wrote:
> > zturner wrote:
> > > Is error checking needed here?
> > More error checking definitely needed (here and everywhere else). If I
> > break lldb-server while working on it, I'd like to see a clear error
> > message from the test rather than an obscure crash. This should return
> > `Expected` if you don't care about copying or
> > `Expected>` if you do (*), so that the test can
> > ASSERT that the message was received and parsed correctly and print out the
> > error otherwise.
> >
> > (*) Or the out argument idea I wrote about earlier.
> I converted my pointer containers to Expected>.
> I noticed that ASSERT_* doesn't fail the test, it returns, so I need to make
> the functions in TestClient return Error or Expected<> objects and check them
> in the test body.
You'll want to use something like I've got in D33059 for checking the values of
`Expected`s and `Error`s. That's pending code review though. You can put
the logic in the body as you suggested, but it's really cumbersome.
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:26-27
+Expected> ProcessInfo::Create(StringRef response)
{
+ std::unique_ptr process_info =
+std::unique_ptr(new ProcessInfo);
+ auto elements = SplitPairList(response);
How about `std::unique_ptr process_info(new ProcessInfo);`
https://reviews.llvm.org/D32930
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
