[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/tests/MessageObjects.cpp:83-88
+      if (endian == LITTLE) {
+        registers[register_id] = SwitchEndian(value_str);
+      }
+      else {
+        registers[register_id] = stoul(value_str, nullptr, 16);
+      }
----------------
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).


https://reviews.llvm.org/D32930