From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x430.google.com (mail-pf1-x430.google.com [IPv6:2607:f8b0:4864:20::430]) by sourceware.org (Postfix) with ESMTPS id 1E25D3858D20 for ; Wed, 17 Apr 2024 19:03:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1E25D3858D20 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 1E25D3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::430 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713380613; cv=none; b=wc2K03pXbJwswtH02t18Xpa07jSZUl1wXNh/gxDo0QpcCpSLbu0841mywOFLtuuFONK37U8vV0HSQXxuD7UXeVLmanYTSERzuYmft3ixrNxNMl626G91U2fENgpz/Bopv8qNJJl5Xpt8egT+GkFGhWAURuRuJTCGnB07YxsRoTk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713380613; c=relaxed/simple; bh=HonkNg/OSwcLXtSr1BBfMLJS2Qj0VqY3eiJDiQiGB/w=; h=DKIM-Signature:Subject:To:From:Message-ID:Date:MIME-Version; b=tiwY/aorhOQ2BgR/CtJyXtIM02zviyVO3036Lb1qm8SDyauln/qdtst0Ma/MCKacLgZK8VnLMi8B5CUXa76uQfkMxksbqEGGNgijmSv+ScfNnnWnKG+7cX+PJpqDpq9um6JmfDAYBM0sHse+hbLwKZhhlhye4u4Nds0xZPk4PEs= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x430.google.com with SMTP id d2e1a72fcca58-6ead4093f85so121105b3a.3 for ; Wed, 17 Apr 2024 12:03:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1713380601; x=1713985401; darn=sourceware.org; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject:from:to:cc :subject:date:message-id:reply-to; bh=DdaoF8M8mwP7AgSIQuVYjUgCR4JSBedtA3ULLIHTrCw=; b=Nb+LHI2QXG1agJhPVZJS6SbApcfrAGZdemxwDmTrAymxkBEjn2h9jW8iaMTihWdsh/ IGrLQOoyR2p6wsWzOTD0lwaNdSGBlGXUOBtLKTi2shSaz1KGQkK4xDrqMPCAlsUp4+PA YFdMojiem8HGzRzJYcZcLU4iXvx9B+9rtZYsVfPIVdEPZ3SXIY/bACmCqtQ910SltMYb kF88hyqJwI4GHERIJpW6yQL5n478gurO7X5vdPy7dR9DrRH0OSMCVip7hH4wDDBs9IcV SNm7ZuG62jTapAYIqmWq8h3U/8yQT9i5cHMGFQwT1JTTmwVGpiECrMb4SUwPlqwCGpht MpIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713380601; x=1713985401; h=content-transfer-encoding:content-language:in-reply-to:mime-version :user-agent:date:message-id:from:references:cc:to:subject :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=DdaoF8M8mwP7AgSIQuVYjUgCR4JSBedtA3ULLIHTrCw=; b=iDCcOKAy2VjOZxv+ZnHEJxHLv3wOjEdQFQYLhvj7MhpGMKqkS8q/LHjt8GR/10S/yv C8qyEE9XSTjbRx8fTVjvhk2G3r6ocRqD56OE/an3p5NXjBUB1GTdxQGiKjjJMJQHmdHl z/txZClYsi78oF27Wh3+6t7HfnAAFHUEOn9YtwB+MORzS0v68BfKNQuVR52r5xPymDqt FfgJJzv2mEvF7gTnyHcIXK4C0i4XdLspwOx+O1n4ew2WqiYobTBlGlchpHYOXLjVYQTl JlvevEp846bHi40hatWKdjSYiIxo6/ciLkQp3Syf+iM32p4AiIKoZDSCNi8kCPR6CQId yqaQ== X-Forwarded-Encrypted: i=1; AJvYcCVvH6GlY9vqO2CvVqRKTz6gJvYpTLvg/4ijAHLW5dnf862ZeNRdpEBGWj9dyHPnMxqAgCtsBQvs8rbO6/x7zxJQeeh7lmSDIEhBmA== X-Gm-Message-State: AOJu0YyQZDMcINnnjRl+GUd7mPcrKS2QgbW84TrobmnxZd0+VQvp3SSK 8GzxtBRKMAdzOzQSoI6hW9TilCIqeOwG3OLuDjxAa+mK23fUkLYxbl09dcp+wc8= X-Google-Smtp-Source: AGHT+IEPoD0Khof3f9aBeyWK9d16JT3pObfNYvI10+1rUBdk1HsdswXoG2axpcWMBZyYnmB4H8ItsQ== X-Received: by 2002:a17:902:b78a:b0:1e6:40f1:93be with SMTP id e10-20020a170902b78a00b001e640f193bemr351099pls.27.1713380600969; Wed, 17 Apr 2024 12:03:20 -0700 (PDT) Received: from [192.168.0.102] ([177.139.3.230]) by smtp.gmail.com with ESMTPSA id y8-20020a17090264c800b001e47972a2casm12142147pli.96.2024.04.17.12.03.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 17 Apr 2024 12:03:20 -0700 (PDT) Subject: Re: [PATCH v4 7/8] gdb/testsuite: Add unittest for qIsAddressTagged packet 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> From: Gustavo Romero Message-ID: Date: Wed, 17 Apr 2024 16:03:17 -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: <704f5b93-c0f8-4e50-8180-f57466833e35@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.4 required=5.0 tests=BAYES_00,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: 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) Cheers, Gustavo