[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The largest issue I see here is the use of exceptions, which are banned in 
llvm. We'll need to get rid of those before we start discussing anything else. 
This means we'll need to propagate errors manually, and we should design it in 
a way that it is easy to ASSERT on them. One option is to use llvm::Expected 
for this, which is very good for this use, as it can actually contain a list of 
errors, which we can print out with a custom ASSERT_NO_ERROR macro. The usage 
would then be something like:

  Expected FooOr = getFoo();
  ASSERT_NO_ERROR(FooOr);
  // use FooOr.get()

However this has a disadvantage of the error message not containing the 
expression that failed, so it could be better having the functions just return 
llvm::Error and have the real return be in a by-ref argument:

  Foo foo;
  ASSERT_NO_ERROR(getFoo(foo));

Other options are possible as well.. with a sufficiently crazy assert macro we 
could also achieve syntax like `Foo foo = ASSERT_NO_ERROR(getFoo());`, but that 
may be too confusing.

I also highlighted a couple of other cases of violating the llvm coding 
standards, but that will need more attention once this is sorted. Sorry :/




Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:9
+
+using namespace std;
+using namespace lldb_private;

using namespace std goes against llvm coding standards 




Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:16
+  pid = stoi(elements["pid"], nullptr, 16);
+  parent_pid = stoi(elements["parent-pid"], nullptr, 16);
+  real_uid = stoi(elements["real-uid"], nullptr, 16);

StringRef::getAsInteger



Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:113
+  if (threads.size() != pcs.size()) {
+throw TestClientException("Size mismatch between threads and thread-pcs.");
+  }

Exceptions are not allowed in llvm, so this will need to be written 
differently. To get around the constructor-cannot-fail problem, you can make 
the constructor private, and have a static factory function (`Create`?) 
instead, which returns Expected. Then, all the operations that can fail are 
performed in the factory function (which can report failure), and the 
constructor is basically just used for initializing the fields).



Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:84
+std::unordered_map SplitPairList(const std::string& 
s);
+std::vector SplitList(const std::string& s, char delimeter);
+std::pair SplitPair(const std::string& s);

Delete and repace uses with StringRef::split



Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:50
+  Args args;
+  args.SetCommandString(LLDB_SERVER);
+  args.AppendArgument("gdbserver");

Using SetCommandString is wrong here, as that will actually attempt to parse 
the path as a full command (which will fail be wrong if it contains spaces, 
quotes, etc.). Just append the program name as well.



Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:65
+
+  sleep(5); // TODO: Sleep is bad. Can I wait for it to start?
+  SendAck(); // Send this as a handshake.

Since you're using reverse-connect (which is good), you should be able to 
detect that the server is ready by the fact it has connected to you.



Comment at: unittests/tools/lldb-server/tests/TestClient.cpp:70
+void TestClient::StopDebugger() {
+  Host::Kill(server_process_info.GetProcessID(), 15);
+}

This is not portable.


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.

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski added a comment.

GoogleTest is an already a part of LLVM. It is stripped down version of this 
software with patches for LLVM - at least with included NetBSD support that is 
still pending upstream.


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.

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski added inline comments.



Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+  asm volatile ("int3");
+  delay = false;

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.



Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:16
+  pid = stoi(elements["pid"], nullptr, 16);
+  parent_pid = stoi(elements["parent-pid"], nullptr, 16);
+  real_uid = stoi(elements["real-uid"], nullptr, 16);

labath wrote:
> StringRef::getAsInteger
`std::stoi` throws exceptions


https://reviews.llvm.org/D32930



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Zachary Turner via lldb-commits]

Take a look at ErrorChecking.h in llvm, it contains some useful macros for
using expected and error
On Mon, May 8, 2017 at 5:42 AM Kamil Rytarowski via Phabricator via
lldb-commits  wrote:

> krytarowski added inline comments.
>
>
> 
> Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
> +
> +  asm volatile ("int3");
> +  delay = false;
> 
> 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.
>
>
> 
> Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:16
> +  pid = stoi(elements["pid"], nullptr, 16);
> +  parent_pid = stoi(elements["parent-pid"], nullptr, 16);
> +  real_uid = stoi(elements["real-uid"], nullptr, 16);
> 
> labath wrote:
> > StringRef::getAsInteger
> `std::stoi` throws exceptions
>
>
> 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 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.

[Takuto Ikuta via Phabricator via lldb-commits]

takuto.ikuta added inline comments.



Comment at: unittests/CMakeLists.txt:57
 add_subdirectory(Breakpoint)
+add_subdirectory(tools)
 add_subdirectory(Core)

better to sort alphabetically?
Using tools instead of Tools intentionally?



Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:6
+
+using namespace std;
+

Need to remove this?



Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:133
+
+  for (auto i = elements.begin(); i != elements.end(); i++) {
+if (i->first[0] == 'T' && i->first.substr(3, 6) == "thread") {

Why not range based for?


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.

[Tamas Berghammer via Phabricator via lldb-commits]

tberghammer added a comment.

I few high level comments after a quick read through.




Comment at: unittests/tools/lldb-server/.gitignore:1
+thread_inferior

Why do we need this? I would expect cmake to *not* put anything into my source 
directory when doing an out-of-source build so I can have multiple build 
directory (e.g. for different targets) at the same time.



Comment at: unittests/tools/lldb-server/inferior/thread_inferior.cpp:21
+
+  asm volatile ("int3");
+  delay = false;

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.



Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:17
+
+class ProcessInfo {
+public:

Can we somehow reuse the logic in GDBRemoteCommunicationClient for parsing the 
packets (possibly after factoring out some of it into new standalone 
functions)? I think it would remove code duplication as well as increase the 
coverage provided by these tests.



Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:82-87
+// Common functions for parsing packet data.
+std::unordered_map SplitPairList(const std::string& 
s);
+std::vector SplitList(const std::string& s, char delimeter);
+std::pair SplitPair(const std::string& s);
+std::string HexDecode(const std::string& hex_encoded);
+unsigned long SwitchEndian(const std::string& little_endian);

Does these have to be global? Can we make them local to the .cc file (anonymous 
namespace or file static variable)?



Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:83
+// Common functions for parsing packet data.
+std::unordered_map SplitPairList(const std::string& 
s);
+std::vector SplitList(const std::string& s, char delimeter);

What if the key isn't unique?



Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:86-87
+std::pair SplitPair(const std::string& s);
+std::string HexDecode(const std::string& hex_encoded);
+unsigned long SwitchEndian(const std::string& little_endian);
+}

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)


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] D32823: Remove an expensive lock from Timer

[Scott Smith via Phabricator via lldb-commits]

scott.smith added a comment.

Can someone please commit this?  Thank you!


Repository:
  rL LLVM

https://reviews.llvm.org/D32823



___
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.

[Zachary Turner via Phabricator via lldb-commits]

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(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 element = SplitPair(s);
+pairs[element.first] = element.second;

`StringRef`



Comment at: unittests/tools/lldb-server/tests/MessageObjects.cpp:156-171
+vector SplitList(const string& s, char delimeter) {
+  size_t start = 0;
+  vector 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: unit

[Lldb-commits] [PATCH] D25886: [Test Suite] Properly respect --framework option

[Chris Bieneman via Phabricator via lldb-commits]

beanz added inline comments.



Comment at: lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py:1418
  "include")),
-'LD_EXTRAS': "-L%s -llldb" % lib_dir}
+'LD_EXTRAS': "-L%s/../lib -llldb -Wl,-rpath,%s/../lib" % 
(lib_dir, lib_dir)}
 elif sys.platform.startswith('win'):

fjricci wrote:
> Why do we need to make this `-L$(lib_dir)/../lib` instead of the original 
> `-L$(lib_dir)`? This breaks cases where `lib_dir` is `lib64` and not `lib`
Basically because this is hack on top of hack on top of hack...

Confusingly `lib_dir` is actually set to the `bin` dir not the correct setting 
for `lib`/`lib64`. Except if lldb supports the `-P` flag, and your host is 
FreeBSD or Linux, in which case we construct the lib path from the python 
path...

So this change was needed to make Darwin or NetBSD work, but broke FreeBSD and 
Linux in lib64 configurations... great...

We *really* need to clean up all of this because the garbled mess of 
platform-specific hacks is a really big problem.


Repository:
  rL LLVM

https://reviews.llvm.org/D25886



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D32597: Initiate loading of shared libraries in parallel

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm out of office this week. Could you hold  until I get back? Hopefully we 
will see some development on the llvm/lld front in the meanwhile.


Repository:
  rL LLVM

https://reviews.llvm.org/D32597



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits