[lldb] [gdb-remote] Also handle high/low memory addressable bits setting in the stop info packet

[jasonmolenda]

Summary

A small follow on patch to https://reviews.llvm.org/D157667 to handle the new low_mem_addressing_bits and high_mem_addressing_bits keys in the stop reply packet too, not just qHostInfo.

I changed AddressableBits so that the low memory and high memory bits can be set separately, and we can query whether any addressing bits were seen while parsing the packet, and update the Process address masks if they were.

[JDevlieghere]

lldb/source/Utility/AddressableBits.cpp

81–83

Please implement operator bool() instead.

85–87

Where is this called?

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

81–83

ok.

85–87

Hm. When I thought of this patch last night, I was sure I needed a Clear method for some reason, but you're right I never ended up using it.

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

85–87

AHA I did use it, but it's in the previous patch. It's in the base class method of ObjectFile.

virtual bool GetAddressableBits(lldb_private::AddressableBits &address_bits) {
  address_bits.Clear();
  return false;
}

[jasonmolenda]

Update patch to change the AddressableBits::IsValid to AddressableBits::HasValue which is a clearer description. The IsValid methods are used across the lldb_private classes everywhere. I tried changing this to operator bool but I thought that was less clear in use. I compromised by changing it to HasValue - this is a POD class so "IsValid" seemed a little questionable, but I didn't like operator bool. Let's try this out.

[JDevlieghere]

lldb/source/Utility/AddressableBits.cpp

85–87

Why is this returning a bool and not AddressableBits?

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

85–87

The addressable_bits argument is a [out] variable to indicate if it has a value. You're right, now that there's a HasValue method, ObjectFile::GetAddressableBits can return an AddressableBits and the caller can call HasValue.

[JDevlieghere]

In D158041#4593205, @jasonmolenda wrote:

Update patch to change the AddressableBits::IsValid to AddressableBits::HasValue which is a clearer description. The IsValid methods are used across the lldb_private classes everywhere. I tried changing this to operator bool but I thought that was less clear in use. I compromised by changing it to HasValue - this is a POD class so "IsValid" seemed a little questionable, but I didn't like operator bool. Let's try this out.

Yeah that's fine. I don't feel strongly about operator bool(). My main concern is to not create another class that can be in an "invalid state" and requires you to check whether or not it's valid before doing an operation. As you point out, that's not really the case here anyway, and using different terminology helps convey that.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

2314–2316

I would inline the check for HasValue in SetProcessMasks. You already do some kind of sanity checking there, might as well make that sound and do it unconditionally.

[jasonmolenda]

Update patch to remove the HasValue/IsValue and Clear methods from AddressableBits. Change methods that may fill in an AddressableBits object with values to return an AddressableBits object unconditionally. Change AddressableBits::SetProcessMasks so it can be called unconditionally, and it only sets mask values in Process if values were set.

I wanted to allow for someone to create an AddressableBits object with only the high memory addressing bits specified (for instance) and the low memory address mask will not be changed. I updated AddressableBits::SetProcessMasks to use the current Process mask values if it doesn't have a value specified. This will be more important when I add the SB API to SBProcess to set address masks in https://reviews.llvm.org/D155905 I expect.

[JDevlieghere]

Thanks, this LGTM.