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

[Gustavo Romero]

On 4/17/24 4:03 PM, Gustavo Romero wrote:
> Hi Luis,
> 
> On 4/17/24 6:38 AM, Luis Machado wrote:
>> 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?
> 
> Right, just noticed that too. Fixed in v5, thanks.
> 
> 
>>> +  /* 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.
> 
> I agree, but regarding the malformed case due to an incorrect length,
> in check_is_address_tagged_reply we explicitly request to convert
> only one byte in hex format (2 hex digits):
> 
>    /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
>    hex2bin (packet.data (), &reply, 1);
> 
> so the rest is truncated. I can add a test to verify if truncation is right,
> for instance: "0104A590001234006mC0fe" should returned "Tagged". I don't
> think it's worth checking for length in the code.
> 
> So, how about:
> 
> Empty reply
> Not tagged - "00"
> Tagged - "01"
> Error - "E00"
> Malformed, length truncation - "0104A590001234006"
> Malformed, values - "0m" (not a hex value)

ah, and an invalid reply as well, like 04 (which is neither 00 nor 01), so:

Empty reply - ""
Not tagged - "00"
Tagged - "01"
Invalid - "04"
Error - "E00"
Malformed, length truncation - "0104A590001234006"
Malformed, values - "0m" (not a hex value)


Cheers,
Gustavo