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

[Jason Majors via Phabricator via lldb-commits]

jmajors marked 12 inline comments as done.
jmajors added inline comments.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:52
+
+  unsigned long ReadRegister(unsigned long register_id) const;
+
----------------
labath wrote:
> tberghammer wrote:
> > "unsigned long" is only 32 bit on some systems but a register can be arbitrary large so the return value should be something more generic. This is true for every location you are working with registers.
> changing to uint64_t does not really solve the issue Tamas was pointing out, it possibly even makes it worse. The reason I did not bring it up until now is that `long` happens match the size of general purpose registers, which are the ones that we care about for the moment, which was kinda nice.
> 
> However, intel sse registers can be up to 512 bits wide, so we will need to have something more generic sooner or later. lldb has a `RegisterValue` class for this purpose, so we should probably use that.  If it shows that it makes the code too clunky, we can add some accessor functions which assert that the size of register is e.g. sizeof(void*) and return a simple integer. They can then be used it cases where you know that the register must contain a pointer-sized value.
Rather than trying to test and convert them all, I just left them as strings. I only access one value (the PC), so I'm converting it as I need it.


================
Comment at: unittests/tools/lldb-server/tests/MessageObjects.h:33-42
+  lldb::pid_t pid;
+  lldb::pid_t parent_pid;
+  uint32_t real_uid;
+  uint32_t real_gid;
+  uint32_t effective_uid;
+  uint32_t effective_gid;
+  std::string triple;
----------------
tberghammer wrote:
> A large part of these variables are never read by anybody. Do we want to keep them around just in case or should we remove them?
I figured I might as well parse the whole message at once, in case new tests need the pieces.



https://reviews.llvm.org/D32930