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
