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

[Greg Clayton via Phabricator via lldb-commits]

clayborg 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();
+
----------------
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.


================
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
----------------
Is this comment needed for the set accessors? These functions don't return anything


================
Comment at: lldb/include/lldb/API/SBProcess.h:451-453
+  lldb::addr_t FixCodeAddress(lldb::addr_t addr);
+  lldb::addr_t FixDataAddress(lldb::addr_t addr);
+  lldb::addr_t FixAnyAddress(lldb::addr_t addr);
----------------
Should the parameter be named "load_addr" to ensure people know it is a load address?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155905