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

[Jason Majors via Phabricator via lldb-commits]

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(&registers[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