[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[David Spickett via lldb-commits]

https://github.com/DavidSpickett approved this pull request.


https://github.com/llvm/llvm-project/pull/90533
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[David Spickett via lldb-commits]

DavidSpickett wrote:

Seems logical to me, keep the 99% use case simple and folks who know they need 
a high mem mask have to put in the extra effort to really consider it.

https://github.com/llvm/llvm-project/pull/90533
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[Jason Molenda via lldb-commits]

jasonmolenda wrote:

> On the face of it
> 
> > When we have an unset high memory address mask, and we are told to set low- 
> > and high-memory to the same new address mask, maintain the high memory mask 
> > as unset in Process. The same thing is done with the 
> > SBProcess::SetAddressMask API when the user specifies 
> > lldb.eAddressMaskRangeAll.
> 
> Seems to itself be confusing and if you're doing this to address
> 
> > When high memory has a different set of masks, those become active in 
> > Process and the user can use highmem-virtual-addressable-bits to override 
> > only the highmem value, if it is wrong.
> 
> You're adding one gotcha to avoid another.
> 
> However, I think the result of the highmem mask being unset is the same as 
> setting both masks to the same value. Except that `virtual-addressable-bits` 
> will now effectively apply to both masks, until the user sets _only_ the high 
> mask to some value. At that point, `highmem-virtual-addressable-bits` must be 
> used to modify the high mem value.
> 
> Correct?
> 
> So this doesn't actually change anything for an API user that was setting 
> address masks for high and low all at once. They'll still think both are the 
> same value, but _internally_, lldb tries high, sees that it's unset, and 
> falls back to the low mask.

I'm not thrilled about where I'm ending up. 

For the vast majority of our users, the separate address masks for high and low 
memory will never occur, it's an unusual environment that sets up the MMU this 
way, and operates in both halves of memory.  The goal of this patch is to keep 
the high memory masks unset (set to LLDB_INVALID_MASK) unless someone sets them 
to a value distinct from the "low memory" masks.

I almost think about changing these masks in Process to be a class which holds 
either {code, data} or {low code, low data, high code, high data} internally, 
with methods to get/set the masks and the requirement that high and low be set 
simultaneously when they're both going to be specified so we can detect if it's 
genuinely a split-address space situation.  

https://github.com/llvm/llvm-project/pull/90533
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[Jason Molenda via lldb-commits]

@@ -1465,6 +1465,20 @@ class Process : public 
std::enable_shared_from_this,
   /// platforms where there is a difference (only Arm Thumb at this time).
   lldb::addr_t FixAnyAddress(lldb::addr_t pc);
 
+  /// Retrieve the actual address masks for high memory code/data,
+  /// without the normal fallbacks of returning the low-memory masks
+  /// or the user settings.  Needed by SBProcess to determine if
+  /// the high address masks have actually been set, and should
+  /// be updated when "set all masks" is requested.  In most cases,
+  /// the highmem masks should be left to have LLDB_INVALID_ADDRESS_MASK,
+  /// unset.
+  lldb::addr_t GetConcreteHighmemCodeAddressMask() {

jasonmolenda wrote:

I think I also thought about moving to a std::optional for all the masks, but 
in the SB API I need a way to say "no mask available" and without creating an 
SBAddressMask object with an IsValid() method, I can't represent that without a 
magic invalid value.

https://github.com/llvm/llvm-project/pull/90533
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[Jason Molenda via lldb-commits]

@@ -1465,6 +1465,20 @@ class Process : public 
std::enable_shared_from_this,
   /// platforms where there is a difference (only Arm Thumb at this time).
   lldb::addr_t FixAnyAddress(lldb::addr_t pc);
 
+  /// Retrieve the actual address masks for high memory code/data,
+  /// without the normal fallbacks of returning the low-memory masks
+  /// or the user settings.  Needed by SBProcess to determine if
+  /// the high address masks have actually been set, and should
+  /// be updated when "set all masks" is requested.  In most cases,
+  /// the highmem masks should be left to have LLDB_INVALID_ADDRESS_MASK,
+  /// unset.
+  lldb::addr_t GetConcreteHighmemCodeAddressMask() {

jasonmolenda wrote:

Yeah this is a weird API I almost thought to make it a protected method because 
the only user should be SBFrame where we need to distinguish between the 
"currently in-effect high memory mask" and "the current Process mask setting, 
which may be unset".  For the effective high memory mask, if the user hasn't 
specified a high memory addressable bits setting, and the process doesn't have 
a mask set, then we are in a "one-mask-for-all-addresses" mode and we return 
the low memory address mask.

https://github.com/llvm/llvm-project/pull/90533
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[David Spickett via lldb-commits]

DavidSpickett wrote:

On the face of it

> When we have an unset high memory address mask, and we are told to set low- 
> and high-memory to the same new address mask, maintain the high memory mask 
> as unset in Process. The same thing is done with the 
> SBProcess::SetAddressMask API when the user specifies 
> lldb.eAddressMaskRangeAll.

Seems to itself be confusing and if you're doing this to address

> When high memory has a different set of masks, those become active in Process 
> and the user can use highmem-virtual-addressable-bits to override only the 
> highmem value, if it is wrong.

You're adding one gotcha to avoid another.

However, I think the result of the highmem mask being unset is the same as 
setting both masks to the same value. Except that `virtual-addressable-bits` 
will now effectively apply to both masks, until the user sets *only* the high 
mask to some value. At that point, `highmem-virtual-addressable-bits` must be 
used to modify the high mem value.

Correct?

So this doesn't actually change anything for an API user that was setting 
address masks for high and low all at once. They'll still think both are the 
same value, but *internally*, lldb tries high, see's that it's unset, and falls 
back to the low mask.

https://github.com/llvm/llvm-project/pull/90533
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[David Spickett via lldb-commits]

@@ -1465,6 +1465,20 @@ class Process : public 
std::enable_shared_from_this,
   /// platforms where there is a difference (only Arm Thumb at this time).
   lldb::addr_t FixAnyAddress(lldb::addr_t pc);
 
+  /// Retrieve the actual address masks for high memory code/data,
+  /// without the normal fallbacks of returning the low-memory masks
+  /// or the user settings.  Needed by SBProcess to determine if
+  /// the high address masks have actually been set, and should
+  /// be updated when "set all masks" is requested.  In most cases,
+  /// the highmem masks should be left to have LLDB_INVALID_ADDRESS_MASK,
+  /// unset.
+  lldb::addr_t GetConcreteHighmemCodeAddressMask() {

DavidSpickett wrote:

All this makes me wonder if this can be `optional` but we're 
already knee deep in the `INVALID_` style and it would only obscure the intent 
of this patch.

https://github.com/llvm/llvm-project/pull/90533
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda updated 
https://github.com/llvm/llvm-project/pull/90533

>From 3c272e99326a287f0a61c1f8c2c7ee790e1aeb48 Mon Sep 17 00:00:00 2001
From: Jason Molenda 
Date: Mon, 29 Apr 2024 16:45:36 -0700
Subject: [PATCH 1/2] [lldb] Be conversative about setting highmem address
 masks

The most common case with address masks/addressable bits is that
we have one value/mask that applies across the entire address space,
for code and data.  On AArch64, we can have separate masks for high
and low address spaces.  In the normal "one mask for all memory"
case, we use the low memory masks in Process, and we use the
`virtual-addressable-bits` setting for the user to override that
value.  When high memory has a different set of masks, those become
active in Process and the user can use `highmem-virtual-addressable-bits`
to override only the highmem value, if it is wrong.

This patch is handling a case where a gdb stub sent incorrect address
masks, for both high and low memory:

```
(lldb) process plugin packet send qHostInfo
  packet: qHostInfo
response: 
cputype:16777228;cpusubtype:0;endian:little;ptrsize:8;low_mem_addressing_bits:64;high_mem_addressing_bits:64;
```

When in fact this target was using 47 bits of addressing.  This
qHostInfo response set the Process low and high address masks for
code and data so that no bit-clearing is performed.  The user tried
to override this with the virtual-addressable-bits setting, and was
surprised when it had no affect on code running in high memory.
Because the high memory code mask now has a setting (all bits are
addressable) and they needed to use the very-uncommon
highmem-virtual-addressable-bits setting.

When we have an *unset* high memory address mask, and we are told
to set low- and high-memory to the same new address mask, maintain
the high memory mask as unset in Process.  The same thing is done
with the SBProcess::SetAddressMask API when the user specifies
lldb.eAddressMaskRangeAll.

Added a new test case to TestAddressMasks.py to test that the
low-memory override works correctly.  Also fixed a bug in the
testsuite that I did where I added `@skipIf(archs=["arm"])`
to avoid a problem on the Linaro 32-bit arm Ubuntu CI bot.  I
forgot that this is a regex, and so the tests were being skipped
entirely on any arm.* system.
---
 lldb/source/API/SBProcess.cpp | 26 ---
 lldb/source/Target/Process.cpp| 20 +++---
 .../process/address-masks/TestAddressMasks.py | 21 ---
 3 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp
index c37c111c5a58e0..a0654d23e67efd 100644
--- a/lldb/source/API/SBProcess.cpp
+++ b/lldb/source/API/SBProcess.cpp
@@ -1298,7 +1298,12 @@ void SBProcess::SetAddressMask(AddressMaskType type, 
addr_t mask,
 case eAddressMaskTypeCode:
   if (addr_range == eAddressMaskRangeAll) {
 process_sp->SetCodeAddressMask(mask);
-process_sp->SetHighmemCodeAddressMask(mask);
+// If the highmem mask is the default-invalid,
+// don't change it, keep everything consistently
+// using the lowmem all-address-space mask.
+if (process_sp->GetHighmemCodeAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK)
+  process_sp->SetHighmemCodeAddressMask(mask);
   } else if (addr_range == eAddressMaskRangeHigh) {
 process_sp->SetHighmemCodeAddressMask(mask);
   } else {
@@ -1308,7 +1313,12 @@ void SBProcess::SetAddressMask(AddressMaskType type, 
addr_t mask,
 case eAddressMaskTypeData:
   if (addr_range == eAddressMaskRangeAll) {
 process_sp->SetDataAddressMask(mask);
-process_sp->SetHighmemDataAddressMask(mask);
+// If the highmem mask is the default-invalid,
+// don't change it, keep everything consistently
+// using the lowmem all-address-space mask.
+if (process_sp->GetHighmemDataAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK)
+  process_sp->SetHighmemDataAddressMask(mask);
   } else if (addr_range == eAddressMaskRangeHigh) {
 process_sp->SetHighmemDataAddressMask(mask);
   } else {
@@ -1319,8 +1329,16 @@ void SBProcess::SetAddressMask(AddressMaskType type, 
addr_t mask,
   if (addr_range == eAddressMaskRangeAll) {
 process_sp->SetCodeAddressMask(mask);
 process_sp->SetDataAddressMask(mask);
-process_sp->SetHighmemCodeAddressMask(mask);
-process_sp->SetHighmemDataAddressMask(mask);
+// If the highmem masks are the default-invalid,
+// don't change them, keep everything consistently
+// using the lowmem all-address-space masks.
+if (process_sp->GetHighmemDataAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK ||
+process_sp->GetHighmemCodeAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK) {
+  process_sp->SetHighmemCodeAddressMask(mask);
+  

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)


Changes

The most common case with address masks/addressable bits is that we have one 
value/mask that applies across the entire address space, for code and data.  On 
AArch64, we can have separate masks for high and low address spaces.  In the 
normal "one mask for all memory" case, we use the low memory masks in Process, 
and we use the `virtual-addressable-bits` setting for the user to override that 
value.  When high memory has a different set of masks, those become active in 
Process and the user can use `highmem-virtual-addressable-bits` to override 
only the highmem value, if it is wrong.

This patch is handling a case where a gdb stub sent incorrect address masks, 
for both high and low memory:

```
(lldb) process plugin packet send qHostInfo
  packet: qHostInfo
response: 
cputype:16777228;cpusubtype:0;endian:little;ptrsize:8;low_mem_addressing_bits:64;high_mem_addressing_bits:64;
```

When in fact this target was using 47 bits of addressing.  This qHostInfo 
response set the Process low and high address masks for code and data so that 
no bit-clearing is performed.  The user tried to override this with the 
virtual-addressable-bits setting, and was surprised when it had no affect on 
code running in high memory. Because the high memory code mask now has a 
setting (all bits are addressable) and they needed to use the very-uncommon 
highmem-virtual-addressable-bits setting.

When we have an *unset* high memory address mask, and we are told to set low- 
and high-memory to the same new address mask, maintain the high memory mask as 
unset in Process.  The same thing is done with the SBProcess::SetAddressMask 
API when the user specifies lldb.eAddressMaskRangeAll.

Added a new test case to TestAddressMasks.py to test that the low-memory 
override works correctly.  Also fixed a bug in the testsuite that I did where I 
added `@skipIf(archs=["arm"])` to avoid a problem on the Linaro 32-bit 
arm Ubuntu CI bot.  I forgot that this is a regex, and so the tests were being 
skipped entirely on any arm.* system.

---
Full diff: https://github.com/llvm/llvm-project/pull/90533.diff


3 Files Affected:

- (modified) lldb/source/API/SBProcess.cpp (+22-4) 
- (modified) lldb/source/Target/Process.cpp (+16-4) 
- (modified) lldb/test/API/python_api/process/address-masks/TestAddressMasks.py 
(+18-3) 


``diff
diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp
index c37c111c5a58e0..a0654d23e67efd 100644
--- a/lldb/source/API/SBProcess.cpp
+++ b/lldb/source/API/SBProcess.cpp
@@ -1298,7 +1298,12 @@ void SBProcess::SetAddressMask(AddressMaskType type, 
addr_t mask,
 case eAddressMaskTypeCode:
   if (addr_range == eAddressMaskRangeAll) {
 process_sp->SetCodeAddressMask(mask);
-process_sp->SetHighmemCodeAddressMask(mask);
+// If the highmem mask is the default-invalid,
+// don't change it, keep everything consistently
+// using the lowmem all-address-space mask.
+if (process_sp->GetHighmemCodeAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK)
+  process_sp->SetHighmemCodeAddressMask(mask);
   } else if (addr_range == eAddressMaskRangeHigh) {
 process_sp->SetHighmemCodeAddressMask(mask);
   } else {
@@ -1308,7 +1313,12 @@ void SBProcess::SetAddressMask(AddressMaskType type, 
addr_t mask,
 case eAddressMaskTypeData:
   if (addr_range == eAddressMaskRangeAll) {
 process_sp->SetDataAddressMask(mask);
-process_sp->SetHighmemDataAddressMask(mask);
+// If the highmem mask is the default-invalid,
+// don't change it, keep everything consistently
+// using the lowmem all-address-space mask.
+if (process_sp->GetHighmemDataAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK)
+  process_sp->SetHighmemDataAddressMask(mask);
   } else if (addr_range == eAddressMaskRangeHigh) {
 process_sp->SetHighmemDataAddressMask(mask);
   } else {
@@ -1319,8 +1329,16 @@ void SBProcess::SetAddressMask(AddressMaskType type, 
addr_t mask,
   if (addr_range == eAddressMaskRangeAll) {
 process_sp->SetCodeAddressMask(mask);
 process_sp->SetDataAddressMask(mask);
-process_sp->SetHighmemCodeAddressMask(mask);
-process_sp->SetHighmemDataAddressMask(mask);
+// If the highmem masks are the default-invalid,
+// don't change them, keep everything consistently
+// using the lowmem all-address-space masks.
+if (process_sp->GetHighmemDataAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK ||
+process_sp->GetHighmemCodeAddressMask() !=
+LLDB_INVALID_ADDRESS_MASK) {
+  process_sp->SetHighmemCodeAddressMask(mask);
+  process_sp->SetHighmemDataAddressMask(mask);
+}
   } else if (addr_range == eAddressMaskRangeHigh) {
 

[Lldb-commits] [lldb] [lldb] Be conversative about setting highmem address masks (PR #90533)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda created 
https://github.com/llvm/llvm-project/pull/90533

The most common case with address masks/addressable bits is that we have one 
value/mask that applies across the entire address space, for code and data.  On 
AArch64, we can have separate masks for high and low address spaces.  In the 
normal "one mask for all memory" case, we use the low memory masks in Process, 
and we use the `virtual-addressable-bits` setting for the user to override that 
value.  When high memory has a different set of masks, those become active in 
Process and the user can use `highmem-virtual-addressable-bits` to override 
only the highmem value, if it is wrong.

This patch is handling a case where a gdb stub sent incorrect address masks, 
for both high and low memory:

```
(lldb) process plugin packet send qHostInfo
  packet: qHostInfo
response: 
cputype:16777228;cpusubtype:0;endian:little;ptrsize:8;low_mem_addressing_bits:64;high_mem_addressing_bits:64;
```

When in fact this target was using 47 bits of addressing.  This qHostInfo 
response set the Process low and high address masks for code and data so that 
no bit-clearing is performed.  The user tried to override this with the 
virtual-addressable-bits setting, and was surprised when it had no affect on 
code running in high memory. Because the high memory code mask now has a 
setting (all bits are addressable) and they needed to use the very-uncommon 
highmem-virtual-addressable-bits setting.

When we have an *unset* high memory address mask, and we are told to set low- 
and high-memory to the same new address mask, maintain the high memory mask as 
unset in Process.  The same thing is done with the SBProcess::SetAddressMask 
API when the user specifies lldb.eAddressMaskRangeAll.

Added a new test case to TestAddressMasks.py to test that the low-memory 
override works correctly.  Also fixed a bug in the testsuite that I did where I 
added `@skipIf(archs=["arm"])` to avoid a problem on the Linaro 32-bit arm 
Ubuntu CI bot.  I forgot that this is a regex, and so the tests were being 
skipped entirely on any arm.* system.

>From 3c272e99326a287f0a61c1f8c2c7ee790e1aeb48 Mon Sep 17 00:00:00 2001
From: Jason Molenda 
Date: Mon, 29 Apr 2024 16:45:36 -0700
Subject: [PATCH] [lldb] Be conversative about setting highmem address masks

The most common case with address masks/addressable bits is that
we have one value/mask that applies across the entire address space,
for code and data.  On AArch64, we can have separate masks for high
and low address spaces.  In the normal "one mask for all memory"
case, we use the low memory masks in Process, and we use the
`virtual-addressable-bits` setting for the user to override that
value.  When high memory has a different set of masks, those become
active in Process and the user can use `highmem-virtual-addressable-bits`
to override only the highmem value, if it is wrong.

This patch is handling a case where a gdb stub sent incorrect address
masks, for both high and low memory:

```
(lldb) process plugin packet send qHostInfo
  packet: qHostInfo
response: 
cputype:16777228;cpusubtype:0;endian:little;ptrsize:8;low_mem_addressing_bits:64;high_mem_addressing_bits:64;
```

When in fact this target was using 47 bits of addressing.  This
qHostInfo response set the Process low and high address masks for
code and data so that no bit-clearing is performed.  The user tried
to override this with the virtual-addressable-bits setting, and was
surprised when it had no affect on code running in high memory.
Because the high memory code mask now has a setting (all bits are
addressable) and they needed to use the very-uncommon
highmem-virtual-addressable-bits setting.

When we have an *unset* high memory address mask, and we are told
to set low- and high-memory to the same new address mask, maintain
the high memory mask as unset in Process.  The same thing is done
with the SBProcess::SetAddressMask API when the user specifies
lldb.eAddressMaskRangeAll.

Added a new test case to TestAddressMasks.py to test that the
low-memory override works correctly.  Also fixed a bug in the
testsuite that I did where I added `@skipIf(archs=["arm"])`
to avoid a problem on the Linaro 32-bit arm Ubuntu CI bot.  I
forgot that this is a regex, and so the tests were being skipped
entirely on any arm.* system.
---
 lldb/source/API/SBProcess.cpp | 26 ---
 lldb/source/Target/Process.cpp| 20 +++---
 .../process/address-masks/TestAddressMasks.py | 21 ---
 3 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp
index c37c111c5a58e0..a0654d23e67efd 100644
--- a/lldb/source/API/SBProcess.cpp
+++ b/lldb/source/API/SBProcess.cpp
@@ -1298,7 +1298,12 @@ void SBProcess::SetAddressMask(AddressMaskType type, 
addr_t mask,
 case eAddressMaskTypeCode:
   if (addr_range == eAddressMaskRangeAll) {