| Message ID | 20240328224850.2785280-3-gustavo.romero@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add another way to check for MTE-tagged addresses on remote targets | expand |
Gustavo Romero <gustavo.romero@linaro.org> writes: > Move MTE address check out of set_memtag and add this check to the > upper layer, before set_memtag is called. This is a preparation for > using a target hook instead of a gdbarch hook MTE address checks. gdbarch_set_memtags is also called from memory_tag_with_logical_tag_command. Shouldn't the same check be added there? > > Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org> > --- > gdb/aarch64-linux-tdep.c | 4 ---- > gdb/printcmd.c | 6 ++++++ > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c > index 50055ac3f48..8e6e63d4dcb 100644 > --- a/gdb/aarch64-linux-tdep.c > +++ b/gdb/aarch64-linux-tdep.c > @@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address, > /* Remove the top byte. */ > addr = gdbarch_remove_non_address_bits (gdbarch, addr); > > - /* Make sure we are dealing with a tagged address to begin with. */ > - if (!aarch64_linux_tagged_address_p (gdbarch, address)) > - return false; > - > /* With G being the number of tag granules and N the number of tags > passed in, we can have the following cases: > > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index cb0d32aa4bc..ae4d640ccf2 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -3127,6 +3127,12 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty) > /* Parse the input. */ > parse_set_allocation_tag_input (args, &val, &length, tags); > > + /* If the address is not in a region memory mapped with a memory tagging > + flag, it is no use trying to manipulate its allocation tag. */ > + if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) { > + show_addr_not_tagged (value_as_address(val)); > + } GNU style doesn't use curly braces in if blocks with only one statement. > + > if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags, > memtag_type::allocation)) > gdb_printf (_("Could not update the allocation tag(s).\n")); -- Thiago
On 3/29/24 9:47 PM, Thiago Jung Bauermann wrote: > Gustavo Romero <gustavo.romero@linaro.org> writes: > >> Move MTE address check out of set_memtag and add this check to the >> upper layer, before set_memtag is called. This is a preparation for >> using a target hook instead of a gdbarch hook MTE address checks. > > gdbarch_set_memtags is also called from > memory_tag_with_logical_tag_command. Shouldn't the same check be added > there? In aarch64_linux_set_memtags, the memory check is inside the else {}, which is only executed when tag_type == memtag_type::allocation, but not in the if {}, which is executed when memtag_type::logical. Because memory_tag_with_logical_tag_command always calls set_memtags with the argument for tag_type param == memtag_type::logical there is no need to check the address. In other words, memory_tag_with_logical_tag_command is about logical tags only, so it's a local operation that does not touch any memory in the target, it just changes a local pointer, so it's ok to change the pointer, being it tagged or not, hence not memory checks are needed. These comments try to clarify a bit it: In memory_tag_with_logical_tag_command: /* Setting the logical tag is just a local operation that does not touch any memory from the target. Given an input value, we modify the value to include the appropriate tag. <-- In memory_tag_print_tag_command: /* If the address is not in a region memory mapped with a memory tagging flag, it is no use trying to access/manipulate its allocation tag. It is OK to manipulate the logical tag though. */ <-- >> >> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org> >> --- >> gdb/aarch64-linux-tdep.c | 4 ---- >> gdb/printcmd.c | 6 ++++++ >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c >> index 50055ac3f48..8e6e63d4dcb 100644 >> --- a/gdb/aarch64-linux-tdep.c >> +++ b/gdb/aarch64-linux-tdep.c >> @@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address, >> /* Remove the top byte. */ >> addr = gdbarch_remove_non_address_bits (gdbarch, addr); >> >> - /* Make sure we are dealing with a tagged address to begin with. */ >> - if (!aarch64_linux_tagged_address_p (gdbarch, address)) >> - return false; >> - >> /* With G being the number of tag granules and N the number of tags >> passed in, we can have the following cases: >> >> diff --git a/gdb/printcmd.c b/gdb/printcmd.c >> index cb0d32aa4bc..ae4d640ccf2 100644 >> --- a/gdb/printcmd.c >> +++ b/gdb/printcmd.c >> @@ -3127,6 +3127,12 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty) >> /* Parse the input. */ >> parse_set_allocation_tag_input (args, &val, &length, tags); >> >> + /* If the address is not in a region memory mapped with a memory tagging >> + flag, it is no use trying to manipulate its allocation tag. */ >> + if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) { >> + show_addr_not_tagged (value_as_address(val)); >> + } > > GNU style doesn't use curly braces in if blocks with only one statement. Fixed in v3. Cheers, Gustavo
Gustavo Romero <gustavo.romero@linaro.org> writes: > On 3/29/24 9:47 PM, Thiago Jung Bauermann wrote: >> Gustavo Romero <gustavo.romero@linaro.org> writes: >> >>> Move MTE address check out of set_memtag and add this check to the >>> upper layer, before set_memtag is called. This is a preparation for >>> using a target hook instead of a gdbarch hook MTE address checks. >> gdbarch_set_memtags is also called from >> memory_tag_with_logical_tag_command. Shouldn't the same check be added >> there? > > In aarch64_linux_set_memtags, the memory check is inside the else {}, which > is only executed when tag_type == memtag_type::allocation, but not in the if {}, > which is executed when memtag_type::logical. Because > memory_tag_with_logical_tag_command always calls set_memtags with the argument > for tag_type param == memtag_type::logical there is no need to check the address. > > In other words, memory_tag_with_logical_tag_command is about logical tags only, > so it's a local operation that does not touch any memory in the target, it just > changes a local pointer, so it's ok to change the pointer, being it tagged or not, > hence not memory checks are needed. > > These comments try to clarify a bit it: > > In memory_tag_with_logical_tag_command: > > /* Setting the logical tag is just a local operation that does not touch > any memory from the target. Given an input value, we modify the value > to include the appropriate tag. <-- > > > In memory_tag_print_tag_command: > > /* If the address is not in a region memory mapped with a memory tagging > flag, it is no use trying to access/manipulate its allocation tag. > > It is OK to manipulate the logical tag though. */ <-- Thanks for the explanation, makes sense! -- Thiago
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c index 50055ac3f48..8e6e63d4dcb 100644 --- a/gdb/aarch64-linux-tdep.c +++ b/gdb/aarch64-linux-tdep.c @@ -2525,10 +2525,6 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address, /* Remove the top byte. */ addr = gdbarch_remove_non_address_bits (gdbarch, addr); - /* Make sure we are dealing with a tagged address to begin with. */ - if (!aarch64_linux_tagged_address_p (gdbarch, address)) - return false; - /* With G being the number of tag granules and N the number of tags passed in, we can have the following cases: diff --git a/gdb/printcmd.c b/gdb/printcmd.c index cb0d32aa4bc..ae4d640ccf2 100644 --- a/gdb/printcmd.c +++ b/gdb/printcmd.c @@ -3127,6 +3127,12 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty) /* Parse the input. */ parse_set_allocation_tag_input (args, &val, &length, tags); + /* If the address is not in a region memory mapped with a memory tagging + flag, it is no use trying to manipulate its allocation tag. */ + if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) { + show_addr_not_tagged (value_as_address(val)); + } + if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags, memtag_type::allocation)) gdb_printf (_("Could not update the allocation tag(s).\n"));
Move MTE address check out of set_memtag and add this check to the upper layer, before set_memtag is called. This is a preparation for using a target hook instead of a gdbarch hook MTE address checks. Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org> --- gdb/aarch64-linux-tdep.c | 4 ---- gdb/printcmd.c | 6 ++++++ 2 files changed, 6 insertions(+), 4 deletions(-)