[Lldb-commits] [PATCH] D29078: This patch implements a command to access and manipulate the Intel(R) MPX Boundary Tables.

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

There are a few places where you are reading memory and then wanting to decode data from it. Right now, you first read memory into a local buffer and then we create an SBData and set the data to the bytes. It would be nice to have the ability to read memory and get an SBData back:

  class SBProcess
  {
    SBData ReadData(lldb::addr_t addr, size_t size, SBError &error);
  };

Since the process can fill in the byte order and address byte size it can return an SBData that is ready to use. If you are interested in adding this in this patch, let me know.



================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:35-36
+
+  std::string ptr_str(cptr);
+  ptr_str.insert(0, "&");
+  lldb::SBValue ptr_addr = frame.GetValueForVariablePath(ptr_str.c_str());
----------------
```
std:string ptr_str("&");
ptr_str += cptr;
```


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:47
+
+static inline uint32_t toU32(uint8_t b0, uint8_t b1, uint8_t b2, uint8_t b3) {
+  return ((uint32_t)b0 << 24) | ((uint32_t)b1 << 16) | ((uint32_t)b2 << 8) |
----------------
Remove this and use SBData::GetUnsignedInt32(...) or SBData::GetAddress(...).


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:52
+
+static inline uint64_t toU64(uint8_t b0, uint8_t b1, uint8_t b2, uint8_t b3,
+                             uint8_t b4, uint8_t b5, uint8_t b6, uint8_t b7) {
----------------
Remove this and use SBData::GetUnsignedInt64(...) or SBData::GetAddress(...).


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:143-150
+  if (arch == llvm::Triple::ArchType::x86_64) {
+    bd_entry =
+        toU64(bd_entry_v[7], bd_entry_v[6], bd_entry_v[5], bd_entry_v[4],
+              bd_entry_v[3], bd_entry_v[2], bd_entry_v[1], bd_entry_v[0]);
+  } else {
+    bd_entry =
+        toU32(bd_entry_v[3], bd_entry_v[2], bd_entry_v[1], bd_entry_v[0]);
----------------
No need to do manual byte order stuff here, we can use SBData and you don't need the "if (arch == ...)" since SBData knows the address byte size:

```
SBError error;
SBData data;
data.SetData(error, bd_entry_v.data(), bd_entry_v.size(), target.GetByteOrder(), target.GetAddressByteSize());
lldb::addr_t bd_entry = data.GetAddress(error, 0);
```


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:199-218
+  if (arch == llvm::Triple::ArchType::x86_64) {
+    lbound = toU64(bt_entry_v[7], bt_entry_v[6], bt_entry_v[5], bt_entry_v[4],
+                   bt_entry_v[3], bt_entry_v[2], bt_entry_v[1], bt_entry_v[0]);
+    ubound =
+        ~toU64(bt_entry_v[15], bt_entry_v[14], bt_entry_v[13], bt_entry_v[12],
+               bt_entry_v[11], bt_entry_v[10], bt_entry_v[9], bt_entry_v[8]);
+    value =
----------------
Use SBData and you don't need the "if (arch == ...)" since SBData knows the address byte size:

```
SBError error;
SBData data;
uint32_t addr_size = target.GetAddressByteSize();
data.SetData(error, bt_entry_v.data(), bt_entry_v.size(), target.GetByteOrder(), addr_size);

lldb::addr_t lbound = data.GetAddress(error, 0 * addr_size);
lldb::addr_t ubound  = data.GetAddress(error, 1 * addr_size);
uint64_t value = data.GetAddress(error, 2 * addr_size);
uint64_t meta = data.GetAddress(error, 3 * addr_size);
```


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:313-314
+
+  lldb::SBData bndcfgu_data = bndcfgu_val.GetData();
+  bndcfgu = bndcfgu_data.GetUnsignedInt64(error, 0);
+  if (!error.Success()) {
----------------
No need to use SBData here, the SBValue can provide what you need. You use SBValue::GetValueAsUnsigned() and specify the invalid value to return as the argument (zero for nullptr):
```
bndcfgu = bndcfgu_val.GetValueAsUnsigned(0); 
```


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:354
+      llvm::Triple::ArchType arch;
+      lldb::SBFrame frame;
+      lldb::SBError error;
----------------
No need for the "frame" variable as it is only used within GetInitInfo. Remove this?


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:359
+
+      if (!GetInitInfo(debugger, target, arch, frame, bndcfgu, arg, ptr, result,
+                       error))
----------------
I would either extract the target here before passing to any other functions like this one and only pass the target. There is no need to pass both the debugger and the target since you can do "target.GetDebugger()" anytime you need the debugger from the target. You can also remove the "frame" variable from this since it is only used in GetInitInfo().


https://reviews.llvm.org/D29078