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

[Valentina Giusti via Phabricator via lldb-commits]

valentinagiusti marked 3 inline comments as done.
valentinagiusti added inline comments.


================
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 =
----------------
clayborg wrote:
> 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);
> ```
same as above.


================
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()) {
----------------
clayborg wrote:
> 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); 
> ```
This also doesn't work, it returns the error: "could not resolve value"., which is why I opted for using SBData when I first wrote this code.


================
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:359
+
+      if (!GetInitInfo(debugger, target, arch, frame, bndcfgu, arg, ptr, result,
+                       error))
----------------
clayborg wrote:
> 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().
I just wanted to avoid repeating the code to initialize the target and have it only inside GetInitInfo.


https://reviews.llvm.org/D29078