From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by sourceware.org (Postfix) with ESMTPS id 318CB3858D20 for ; Wed, 17 Apr 2024 19:11:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 318CB3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 318CB3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::62b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713381076; cv=none; b=DiuTpaQ5rmQ6rQnsN15b+ATuLHGVZWMxfzVcc/EKfmweoUQTZ+T54GbX+HWGQyyKM1zPnj8ixZIOUdRKQeJy2WPHVS5JMn+l74F/FAq7w9RCHtZx1lXnUkpUjNq7/zDeCHHdbyZArq5yGO582xGVARNzsPYB3wxxRFPtLiVJCTA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713381076; c=relaxed/simple; bh=buoQRJwPOvx23xDuk5ax2JigpE5swdeiMSNiLEig0Iw=; h=DKIM-Signature:Subject:From:To:Message-ID:Date:MIME-Version; b=t2AYp6g8JJ9ppMw4PVWe6bHB1Q+i89sBqZY4S4a6SUjQZzoaVpS2aCMHwZBflkxIfM6TYaeM7IZVuJVgNmra3C27NrHWm5HhhPw1Xy6gmNOgbwkM4aUa1D5gLgbRRt4xDBOpAQUL7vyfOXbi7heLFMKb8dUHyfWGGGuYD6TWVWY= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1e455b630acso773775ad.1 for ; Wed, 17 Apr 2024 12:11:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1713381073; x=1713985873; darn=sourceware.org; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:references:cc:to:from:subject:from:to:cc :subject:date:message-id:reply-to; bh=4Q6Wwls/1ntMUrQ18jwpyqyVImAwB/1IrHR53rjCpLA=; b=ZGAucdSM7Hq+vhQC6p6isT3ewj2+Ur6SnSNiwNUCgvH2E+2gz43UHwaS1NBXqGBWpz R1D30rEQFxDl4be7O0sxdQbPioCueUs0EY+a5oaR5mN8VEDOXpLpXEiyZvKPRApyDvU0 qfLe20xYevmSBIvpsfnl30pWSxGcqEBfsLMKSPQIWzPWPysYPPeyma58Epn85sbe6PuA B37UoC/TK9NXEagHj+q4RHfhgoQ6zIVv1l08cg0tQkOiixs70aOua57281nkcp3XbHCa WxxWIOqHjmKXK41Urxqb4U1hwLOj8DSSN56mSzXmbBR8FLn+Zi6PGvk7pgC+t5wpfuzi b37A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713381073; x=1713985873; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:references:cc:to:from:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=4Q6Wwls/1ntMUrQ18jwpyqyVImAwB/1IrHR53rjCpLA=; b=Cropt313CjI/5Qw3MQEo8kfTjG7IzyuVpCv8hkKyqS5VHLTtANQ7pF2CAH9ET06Kjx dPggfGbIWrQSwmzLyOB/FmvFozbe4OcM0euxaYlmvv+v2eNlaHxg1EAuXt66x46xdPDA TCOdqXDECujTNiqemsjCYlsIaa6eQEbOu0YhfioOF9d0M1h908TTy9HHwPpkhB/1PQ5E HYV90uiQPZnWMaNKLFyoi2QdpwYpF44ETCs+F8gcHZlkRupKsFRyP/YPebVvnPTLyj9E TsqTA+80mea3SyTMaN/7ynndldchJOjhouqANy4kUV+jmGTG7codzijPLYSKmc9bjfkU Sz/A== X-Forwarded-Encrypted: i=1; AJvYcCU316STxNx6HWzShzXrGSODxyIQ5jS1hT+YSkWrDLr+OWrlqd5uGmaqsTChBUvaxANU3Tt1e7p6KUt2Snj3ogEW/XbtmmFT5QGOJw== X-Gm-Message-State: AOJu0YxNSlESproG0YvHlMJlIo3K1Fo8mNYiP206iNdZvWeI55Tgd3E7 gEkNpg/PAQGmeMprEpyZVyesi0cY/OpMHte5Ne4gNhiSfpthchf94TjcCieyvko= X-Google-Smtp-Source: AGHT+IF8oiVN834MiWVp/dueyfjBPOeNhXLOSCE8WZR6FMOjV9BhSD9x6FMXXjY/Oj0nar48nEi7YQ== X-Received: by 2002:a17:903:228b:b0:1e4:55d8:dfae with SMTP id b11-20020a170903228b00b001e455d8dfaemr564021plh.4.1713381073126; Wed, 17 Apr 2024 12:11:13 -0700 (PDT) Received: from [192.168.0.102] ([177.139.3.230]) by smtp.gmail.com with ESMTPSA id j7-20020a170903024700b001e5572a99c3sm11805744plh.207.2024.04.17.12.11.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 17 Apr 2024 12:11:12 -0700 (PDT) Subject: Re: [PATCH v4 7/8] gdb/testsuite: Add unittest for qIsAddressTagged packet From: Gustavo Romero To: Luis Machado , gdb-patches@sourceware.org Cc: thiago.bauermann@linaro.org, eliz@gnu.org, tom@tromey.com References: <20240416140728.198163-1-gustavo.romero@linaro.org> <20240416140728.198163-8-gustavo.romero@linaro.org> <704f5b93-c0f8-4e50-8180-f57466833e35@arm.com> Message-ID: <4d6ae1b9-b184-e91f-dca3-64959091c5b9@linaro.org> Date: Wed, 17 Apr 2024 16:11:10 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.1 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 >>> --- >>>   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