[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.
Zachary Turner via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon May 8 10:51:07 PDT 2017
zturner added inline comments.
================
Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+ asm volatile ("int3");
+ delay = false;
----------------
tberghammer wrote:
> krytarowski wrote:
> > int3 is specific to x86.
> >
> > Some instructions to emit software breakpoint in other architectures:
> > https://github.com/NetBSD/src/commit/c215d1b7d6c1385720b27fd45189b5dea6d6e4aa
> >
> > Also there is a support for threads, this is not as portable as it could be. The simplest example could be without them, and threads in debuggee could be tested in dedicated regression tests. This will help out to sort bugs with threads from other features.
> I think there should be a compiler intrinsic available as well.
Depends on the compiler. See `llvm/Support/Compiler.h`. There is a macro `LLVM_BUILTIN_DEBUGTRAP`. I would suggest improving the definition of this macro to include those cases for compilers which don't have the intrinsic (e.g. everything but MSVC).
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:13
+namespace CommunicationTests {
+ProcessInfo::ProcessInfo(const string& response) {
+ auto elements = SplitPairList(response);
----------------
`StringRef`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:22-30
+ if (elements["endian"] == "little") {
+ endian = LITTLE;
+ }
+ else if (elements["endian"] == "big") {
+ endian = BIG;
+ }
+ else {
----------------
```
endian = llvm::StringSwitch<endianness>(elements["endian"])
.Case("little", support::little)
.Case("big", support::big)
.Default(support::native);
```
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:32
+
+ ptrsize = stoi(elements["ptrsize"], nullptr, 10);
+}
----------------
`StringRef::getAsInteger()`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:61-63
+ string name;
+ thread_info->GetValueForKeyAsString("name", name);
+ string reason;
----------------
`StringRef name, reason;`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:64-71
+ thread_info->GetValueForKeyAsString("reason", reason);
+ unsigned long signal;
+ thread_info->GetValueForKeyAsInteger("signal", signal);
+ unsigned long tid;
+ thread_info->GetValueForKeyAsInteger("tid", tid);
+
+ StructuredData::Dictionary* register_dict;
----------------
Either handle `nullptr` or assert that they're not null.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:77-79
+ string key_str;
+ keys->GetItemAtIndexAsString(i, key_str);
+ string value_str;
----------------
`StringRef key_str, value_str;`
================
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);
+ }
----------------
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.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:122-123
+ while (true) {
+ stringstream ss;
+ ss << hex << setw(2) << setfill('0') << register_id;
+ string hex_id = ss.str();
----------------
Use `llvm::raw_string_ostream` or `raw_svector_ostream` instead of `stringstream`.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:126
+ if (elements.find(hex_id) != elements.end()) {
+ registers[register_id++] = SwitchEndian(elements[hex_id]);
+ }
----------------
Same as above.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:135-136
+ if (i->first[0] == 'T' && i->first.substr(3, 6) == "thread") {
+ thread = stoul(i->second, nullptr, 16);
+ signal = stoul(i->first.substr(1, 2), nullptr, 16);
+ }
----------------
```
i->second.getAsInteger(16, thread);
i->first.slice(1, 2).getAsInteger(16, signal);
```
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:149
+ for (string& s : SplitList(s, ';')) {
+ pair<string, string> element = SplitPair(s);
+ pairs[element.first] = element.second;
----------------
`StringRef`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:156-171
+vector<string> SplitList(const string& s, char delimeter) {
+ size_t start = 0;
+ vector<string> elements;
+ while (start < s.size()) {
+ size_t end = s.find_first_of(delimeter, start);
+ elements.push_back(s.substr(start, end - start));
+ if (end == string::npos) {
----------------
Delete this function and use `StringRef::split()` instead.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:173-183
+pair<string, string> SplitPair(const string& s) {
+ pair<string, string> element;
+ size_t colon = s.find_first_of(':');
+ if (colon == string::npos) {
+ return element;
+ }
+
----------------
Delete this function and use `s.split(':')` instead.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:185-197
+string HexDecode(const string& hex_encoded) {
+ string decoded;
+ if (hex_encoded.size() % 2 == 1) {
+ return decoded;
+ }
+
+ for (size_t i = 0; i < hex_encoded.size(); i += 2) {
----------------
Delete and use `llvm::fromHex()`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:199-206
+unsigned long SwitchEndian(const string& little_endian) {
+ string big_endian;
+ for (int i = little_endian.size() - 2; i >= 0; i -= 2) {
+ big_endian += little_endian.substr(i, 2);
+ }
+
+ return stoul(big_endian, nullptr, 16);
----------------
Delete and use `llvm::support::endian::write()`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:8-9
+ class ThreadInfo;
+ typedef std::unordered_map<unsigned long, ThreadInfo> ThreadInfoMap;
+ typedef std::unordered_map<unsigned long, unsigned long> ULongMap;
+
----------------
Can you use a `DenseMap` here? `unordered_map` is junk for most use cases.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:11-15
+enum Endian {
+ LITTLE,
+ BIG,
+ UNKNOWN
+};
----------------
We can delete this enum. Use `llvm::support::endianness` instead.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:19
+public:
+ ProcessInfo(const std::string& response);
+ unsigned int GetPid() const;
----------------
`StringRef`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:24
+private:
+ unsigned int pid;
+ unsigned int parent_pid;
----------------
`pid_t`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:26
+ unsigned int parent_pid;
+ unsigned int real_uid;
+ unsigned int real_gid;
----------------
`uid_t`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:30-31
+ unsigned int effective_gid;
+ std::string triple;
+ std::string ostype;
+ Endian endian;
----------------
`llvm::SmallString<16>`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:39
+ ThreadInfo();
+ ThreadInfo(const std::string& name, const std::string& reason,
+ const ULongMap& registers, unsigned int signal);
----------------
`StringRef`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:53
+public:
+ JThreadsInfo(const std::string& response, Endian endian);
+
----------------
`StringRef`, `llvm::support::endianness`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:63
+public:
+ JStopInfo(const std::string& response);
+};
----------------
`Constructor should be explicit. Also this class seems to do nothing, delete?
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:68
+public:
+ StopReply(const std::string& response);
+
----------------
`StringRef`. Also can you make the constructor explicit?
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:74
+ unsigned int signal;
+ unsigned long thread;
+ std::string name;
----------------
`lldb::tid_t`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:76
+ std::string name;
+ std::shared_ptr<JStopInfo> jstopinfo;
+ ULongMap thread_pcs;
----------------
This does not appear to be used. Can you delete?
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:83
+// Common functions for parsing packet data.
+std::unordered_map<std::string, std::string> SplitPairList(const std::string& s);
+std::vector<std::string> SplitList(const std::string& s, char delimeter);
----------------
tberghammer wrote:
> What if the key isn't unique?
Return an `llvm::StringMap<StringRef>` here. Also the argument should be a `StringRef`.
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:86
+std::pair<std::string, std::string> SplitPair(const std::string& s);
+std::string HexDecode(const std::string& hex_encoded);
+unsigned long SwitchEndian(const std::string& little_endian);
----------------
tberghammer wrote:
> I would hope we have functions in LLVM/LLDB doing these sort of things (might have to be changed slightly to make them easily accessible)
`llvm::fromHex`
================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:87
+std::string HexDecode(const std::string& hex_encoded);
+unsigned long SwitchEndian(const std::string& little_endian);
+}
----------------
`llvm::sys::SwapByteOrder_32(N)`
Note however that the real solution is to just delete this function entirely. See below where I provide an alternative.
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:73
+
+void TestClient::SetInferior(const vector<string>& inferior_args) {
+ stringstream command;
----------------
`llvm::ArrayRef<string> inferior_args`
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:74
+void TestClient::SetInferior(const vector<string>& inferior_args) {
+ stringstream command;
+ command << "A";
----------------
`llvm::raw_string_ostream`
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:76-80
+ for (size_t i = 0; i < inferior_args.size(); i++) {
+ if (i > 0) command << ',';
+ string hex_encoded = HexEncode(inferior_args[i]);
+ command << hex_encoded.size() << ',' << i << ',' << hex_encoded;
+ }
----------------
`for (const auto &arg : inferior_args)`
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:157
+unsigned int TestClient::GetPcRegisterId() {
+ if (pc_register == UINT_MAX) {
+ for (unsigned int register_id = 0; ; register_id++) {
----------------
```
if (pc_register != UINT_MAX)
return pc_register;
```
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:183
+
+ stringstream ss;
+ ss << LOCALHOST << ":" << listening_port << ends;
----------------
`raw_string_ostream`
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:189
+string TestClient::GenerateLogFileName(const ArchSpec& arch) const {
+ stringstream log_file;
+ log_file << "lldb-test-traces/lldb-" << test_case_name << '-' << test_name
----------------
`raw_string_ostream`
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:195-202
+string TestClient::HexEncode(const string& s) const {
+ stringstream encoded;
+ for (const char& c : s) {
+ encoded << hex << (int)c;
+ }
+
+ return encoded.str();
----------------
Delete this function and use `llvm::toHex()`
================
Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:205
+ PacketResult result) {
+ stringstream ss;
+ ss << "Failure sending message: " << message << " Result: ";
----------------
`raw_string_ostream`
================
Comment at: unittests/tools/lldb-server/tests/TestClientException.h:13-21
+namespace CommunicationTests {
+class TestClientException : public std::exception {
+public:
+ TestClientException(const std::string& message);
+ const char* what() const noexcept;
+private:
+ std::string message;
----------------
Delete as exceptions are banned. Use `llvm::Error` instead.
================
Comment at: unittests/tools/lldb-server/tests/gtest_common.h:19-26
+#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
+// Due to a bug in <thread>, when _HAS_EXCEPTIONS == 0 the header will try to
+// call
+// uncaught_exception() without having a declaration for it. The fix for this
+// is
+// to manually #include <eh.h>, which contains this declaration.
+#include <eh.h>
----------------
This is no longer necessary, I'm curious how you found this code? All references to this hack were removed a couple of months ago.
https://reviews.llvm.org/D32930
More information about the lldb-commits
mailing list