Re: [PATCH v3 04/24] Unit testing for GDB-side remote memory tagging handling

Luis Machado via Gdb-patches
2020-12-28T17:17:25Z

On 12/25/20 2:34 AM, Simon Marchi wrote:
>
>
> On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote:
>> Updated in v2:
>>
>> - Adjusted unit tests to cope with new type field in the remote packets.
>>
>> --
>>
>> Include some unit testing for the functions handling the new qMemTags and
>> QMemTags packets.
>>
>> gdb/ChangeLog:
>>
>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org>
>>
>> * remote: Include gdbsupport/selftest.h.
>> (test_memory_tagging_functions): New function.
>> (_initialize_remote): Register test_memory_tagging_functions.
>> ---
>> gdb/remote.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 89 insertions(+)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index e45eef4bee..6db7a74542 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -79,6 +79,7 @@
>> #include <algorithm>
>> #include <unordered_map>
>> #include "async-event.h"
>> +#include "gdbsupport/selftest.h"
>>
>> /* The remote target. */
>>
>> @@ -14529,6 +14530,89 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>> return 0;
>> }
>>
>> +#if GDB_SELF_TEST
>> +
>> +namespace selftests {
>> +
>> +static void
>> +test_memory_tagging_functions (void)
>
> Remove "void".
>

Fixed.

>> +{
>> + remote_target remote;
>> +
>> + struct packet_config *config
>> + = &remote_protocol_packets[PACKET_memory_tagging_feature];
>> +
>> + /* Test memory tagging packet support. */
>> + config->support = PACKET_SUPPORT_UNKNOWN;
>> + SELF_CHECK (remote.supports_memory_tagging () == false);
>> + config->support = PACKET_DISABLE;
>> + SELF_CHECK (remote.supports_memory_tagging () == false);
>> + config->support = PACKET_ENABLE;
>> + SELF_CHECK (remote.supports_memory_tagging () == true);
>
> This seems to set the global flag of whether the remote side supports
> memory tagging. Even though it's not likely to be a real problem, IWBN
> to restore the global state that you touch.
>

Good catch. I've fixed this with a scoped_restore.

> Otherwise, LGTM.
>
> Simon
>