Re: [PATCH v4 7/8] gdb/testsuite: Add unittest for qIsAddressTagged packet

[Luis Machado <luis.machado@arm.com>]

Hi,

A few comments, mostly dependent on the previous patch's (06/08) behavior.

On 4/16/24 15:07, Gustavo Romero wrote:
> Add unittests for testing qIsAddressTagged packet request creation and
> reply checks.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/remote.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 63799ac5e3f..42f34a03c95 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -15658,6 +15658,8 @@ test_memory_tagging_functions ()
>    scoped_restore restore_memtag_support_
>      = make_scoped_restore (&config->support);
>  
> +  struct gdbarch *gdbarch = current_inferior ()->arch ();
> +
>    /* Test memory tagging packet support.  */
>    config->support = PACKET_SUPPORT_UNKNOWN;
>    SELF_CHECK (remote.supports_memory_tagging () == false);
> @@ -15724,6 +15726,46 @@ test_memory_tagging_functions ()
>    create_store_memtags_request (packet, 0xdeadbeef, 255, 1, tags);
>    SELF_CHECK (memcmp (packet.data (), expected.c_str (),
>  		      expected.length ()) == 0);
> +
> +  /* Test creating a qIsAddressTagged request.  */
> +  expected = "qIsAddressTagged:deadbeef";
> +  create_is_address_tagged_request (gdbarch, packet, 0xdeadbeef);
> +  SELF_CHECK (strcmp (packet.data (), expected.c_str ()) == 0);
> +
> +  /* Test empty reply on qIsAddressTagged request.  */
> +  reply = "E00";

The comment seems to be out of sync with what's being tested. Should we be
testing an empty reply instead of E00?

> +  /* is_tagged must not changed, hence it's tested too.  */

s/must not changed/must not change

> +  bool is_tagged = false;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
> +  SELF_CHECK (is_tagged == false);
> +
> +  /* Test if only the first byte (01) is correctly extracted from a long
> +     numerical reply, with remaining garbage.  */
> +  reply = "0104A590001234006mC0fe";
> +  /* Because the first byte is 01, is_tagged should be set to true.  */
> +  is_tagged = false;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);

I think this particular reply (malformed) should make the packet check fail, as
we risk the remote returning garbage values and things working just because the
first byte was the expected number.

We should be more strict with the packet reply format and check its length.

> +  SELF_CHECK (is_tagged == true);
> +
> +  /* Test if only the first byte (00) is correctly extracted from a long
> +     numerical reply, with remaining garbage.  */
> +  reply = "0004A590001234006mC0fe";
> +  /* Because the first byte is 00, is_tagged should be set to false.  */
> +  is_tagged = true;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == true);
> +  SELF_CHECK (is_tagged == false);

Same case for the above test.

> +
> +  /* Test if only the first byte, 04, is correctly extracted and recognized
> +     as invalid (only 00 and 01 are valid replies).  */
> +  reply = "0404A590001234006mC0fe";
> +  /* Because the first byte is invalid is_tagged must not change.  */
> +  is_tagged = false;
> +  strcpy (packet.data (), reply.c_str ());
> +  SELF_CHECK (check_is_address_tagged_reply (packet, &is_tagged) == false);
> +  SELF_CHECK (is_tagged == false);
>  }
>  
>  static void

Also a similar situation.

We should exercise the following:

- Empty reply
- Not tagged - "00"
- Tagged - "01"
- Error - "E??"
- Malformed - packets of incorrect length and values.