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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
