[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/include/lldb/API/SBProcess.h:415-416
+  /// (unsigned long long) 0xfffffffffffffc00
+  lldb::addr_t GetCodeAddressMask();
+  lldb::addr_t GetDataAddressMask();
+
----------------
clayborg wrote:
> Will there only ever be two variants of this for code and data? If we think there might be another address mask type later it might be a good idea to use an enumeration for these two functions and all functions below? I was thinking of something like:
> ```
> enum AddressMaskType {
>   eTypeCode = 0,
>   eTypeData
> };
> lldb::addr_t GetAddressMask(AddressMaskType mask_type);
> ```
> 
> This enum could be used in all subsequent calls. I am just thinking of any needed extra API changes needed in the future. If we need another address mask type in the future we just add another enum and all functions stay the same if we do it as suggested above. Else we will need to add many more API functions if we need a new type later.
That's a very good idea.  The HighCode/HighData can be rolled into the same list of enums.


================
Comment at: lldb/include/lldb/API/SBProcess.h:430-434
+  /// If the low ten bits are the only valid bits for addressing,
+  /// this would be the mask returned:
+  ///
+  /// (lldb) p/x ~((1ULL << 10) - 1)
+  /// (unsigned long long) 0xfffffffffffffc00
----------------
clayborg wrote:
> Is this comment needed for the set accessors? These functions don't return anything
I wanted to include this concrete example on both the Get and Set methods because the Linux address masks are the opposite of what I would have expected if someone told me it was an address mask.  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155905/new/

https://reviews.llvm.org/D155905