| Message ID | 20240416140728.198163-8-gustavo.romero@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add another way to check tagged addresses on remote targets | expand |
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.
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) Cheers, Gustavo
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
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"; + /* is_tagged must not changed, hence it's tested too. */ + 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); + 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); + + /* 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
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(+)