From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DFA12C3DA4A for ; Thu, 8 Aug 2024 09:33:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=IJnjWxXTZVsWNlkG2+poyduCb6IkMW4+aALvzkEQB6E=; b=kO1Tcgq2zGNUACBe3iF8BBpkKc IuvboTVv72t9Aw9YdomXuaKpqM7sbpzoX9DdwwPoe1cyAK7UZRVIUILqM3GpL527PCZBzXLMYlaDl c6/MinP1kA/JggyIrLEjTy5MgsuuissVh2KZfbyMxpgwRWiMWClQkOYMdrrTjSQvjUWVhreoOSF2z v2rDA+2hEgyrjgVpe552KjTOhMdEnFDHW55RyRcZ3PXLnLcE2faTx260QQTNhtqZluFI0zgQb4eDI RQFV2H3EEPRGQfbRGfRvklKD44yOmrBQHPFn1SK1tMLmLkuYEG1BEk/l+uWh75Me8bOTTiSx2zumf gE5AERBg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sbzVr-00000007jLy-2Pva; Thu, 08 Aug 2024 09:32:51 +0000 Received: from mail-wr1-x42f.google.com ([2a00:1450:4864:20::42f]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sbzVI-00000007jGs-3JsA for linux-arm-kernel@lists.infradead.org; Thu, 08 Aug 2024 09:32:18 +0000 Received: by mail-wr1-x42f.google.com with SMTP id ffacd0b85a97d-369f68f63b1so391208f8f.2 for ; Thu, 08 Aug 2024 02:32:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1723109534; x=1723714334; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=IJnjWxXTZVsWNlkG2+poyduCb6IkMW4+aALvzkEQB6E=; b=snEY/85S0u0feJJ4aVqfawLatK5sFH2buKf5DwoMXF+OGX5MkD+LUmqo1U0t/7nAbs piDcATBNBLmcnuZIGD9jHxYbbSB2ptQvEO0mshZfWpI8e48+hMjN3C9n3W+utrDHoaYN xO17/vAtsj0u+rGD9AVPpVKmrjeXDmSQnUWddQ18r+afo2GUTQrFcRJc/IvCUrOjII+F 3mX0F/5OGLp0E/S73BC9E/OH7qJmAckNoWJYy42HROpeNRwFwGkxZwaQnWI4BwdptSSl /Xn3ms++3yL+hssXnIGjuY3Zzmn6DLn2D1ab5d43D+2jY+R2ri8ulacj5N1qFEulFIhn WXKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723109534; x=1723714334; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=IJnjWxXTZVsWNlkG2+poyduCb6IkMW4+aALvzkEQB6E=; b=iYTL1WtIwxuOxVcDT01BERUYrImhcZ4QqbswV+OGj9emmcW3l/wboAl/+t1jphJO5E Xxgiov8Q+1M6/bZsbNZMSHly9B+3tRmnXLxTNh1L2BUhdcGRy73LUwSf1LP2xhtlqD6j U2YIbQ5ajUIu3N9+/f84aun4qEQ1lb6f/CXqN6kiuBAwdKDxhovNnVQ+EdcniFr0w9Jo YYt/uQk9Wc05yIM5SHN3+s0yxh7QCFdy0xb+NhdwDfFj1dq/CKMJK55ptkniTWjr/fvP smWMYGjOxFO3HbXkqiHHOaKGb9mv+EVRCpooFBCiU2HmbPicanuSfOTkz4wdbLK/qfl5 cjKw== X-Forwarded-Encrypted: i=1; AJvYcCWS5E7hSQwJQy9xhBP1vMKW0kPjmXz0fbo02arL20bpVxvp52f8hGcHY/ig2CkN2t1n97CFNSsb1y1Z+NW6hzym/iVnE6TJHkF1tjKQCl1SvH9Oc/g= X-Gm-Message-State: AOJu0Yzumytib/I9GKZ9fYGqzAw1P+t4LLZ10/PPSWCLo8EKTcmSx4I7 z38fsLn4Fq9tiCCU3XFPKLBLbtG2Pw0C6Jeu8hVTlYXhnyes7wG0k8pjZ+Ux6BI= X-Google-Smtp-Source: AGHT+IFszfgATope1Jfc17yIw/mR6BUNSIZsl3orgqABYh+bWAVC2/1IHHu9XGbwTgIhQflBab5i6w== X-Received: by 2002:a5d:618a:0:b0:36b:357a:bfee with SMTP id ffacd0b85a97d-36d273c8a46mr1012106f8f.1.1723109534296; Thu, 08 Aug 2024 02:32:14 -0700 (PDT) Received: from [192.168.1.3] ([89.47.253.130]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36d2718f1b4sm1281952f8f.61.2024.08.08.02.32.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 08 Aug 2024 02:32:13 -0700 (PDT) Message-ID: Date: Thu, 8 Aug 2024 10:32:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] perf scripts python arm-cs-trace-disasm.py: Skip disasm if address continuity is broken To: Leo Yan , Ganapatrao Kulkarni , scclevenger@os.amperecomputing.com Cc: acme@redhat.com, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, darren@os.amperecomputing.com, james.clark@arm.com, suzuki.poulose@arm.com, Al.Grant@arm.com, Mike Leach References: <20240719092619.274730-1-gankulkarni@os.amperecomputing.com> <8f6f221b-4c9a-42e1-b8ce-1f492caee184@linaro.org> <0a697a54-5dd8-4351-a651-991724690db2@os.amperecomputing.com> <543813f6-cb1f-4759-b26f-75246750814d@linaro.org> <00fac24c-d664-4ebb-8c60-f4697b7f76c1@linaro.org> <8b53a424-19f7-4042-a2db-e1c5d051f9cc@os.amperecomputing.com> <6adf84fa-b755-4d7a-957a-9bf01e442238@linaro.org> <6f535bb6-2cee-48e6-93f1-ea19887bae74@os.amperecomputing.com> <027c76a9-9bd4-43e9-a170-8391a0037291@linaro.org> <3d7a6f93-0555-48fa-99cb-bf26b53c2da5@os.amperecomputing.com> <4dd7f210-c03e-4203-b8e9-1c26a7f8fe79@arm.com> Content-Language: en-US From: James Clark In-Reply-To: <4dd7f210-c03e-4203-b8e9-1c26a7f8fe79@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240808_023216_860740_7D361BA8 X-CRM114-Status: GOOD ( 28.88 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 07/08/2024 5:48 pm, Leo Yan wrote: > Hi all, > > On 8/7/2024 3:53 PM, James Clark wrote: > > A minor suggestion: if the discussion is too long, please delete the > irrelevant message ;) > > [...] > >>> --- a/tools/perf/scripts/python/arm-cs-trace-disasm.py >>> +++ b/tools/perf/scripts/python/arm-cs-trace-disasm.py >>> @@ -257,6 +257,11 @@ def process_event(param_dict): >>>                  print("Stop address 0x%x is out of range [ 0x%x .. 0x%x >>> ] for dso %s" % (stop_addr, int(dso_start), int(dso_end), dso)) >>>                  return >>> >>> +       if (stop_addr < start_addr): >>> +               if (options.verbose == True): >>> +                       print("Packet Dropped, Discontinuity detected >>> [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr, >>> dso)) >>> +               return >>> + >> >> I suppose my only concern with this is that it hides real errors and >> Perf shouldn't be outputting samples that go backwards. Considering that >> fixing this in OpenCSD and Perf has a much wider benefit I think that >> should be the ultimate goal. I'm putting this on my todo list for now >> (including Steve's merging idea). > > In the perf's util/cs-etm.c file, it handles DISCONTINUITY with: > > case CS_ETM_DISCONTINUITY: > /* > * The trace is discontinuous, if the previous packet is > * instruction packet, set flag PERF_IP_FLAG_TRACE_END > * for previous packet. > */ > if (prev_packet->sample_type == CS_ETM_RANGE) > prev_packet->flags |= PERF_IP_FLAG_BRANCH | > PERF_IP_FLAG_TRACE_END; > > I am wandering if OpenCSD has passed the correct info so Perf decoder can > detect the discontinuity. If yes, then the flag 'PERF_IP_FLAG_TRACE_END' will > be set (it is a general flag in branch sample), then we can consider use it in > the python script to handle discontinuous data. No OpenCSD isn't passing the correct info here. Higher up in the thread I suggested an OpenCSD patch that makes it detect the error earlier and fixes the issue. It also needs to output a discontinuity when the address goes backwards. So two fixes and then the script works without modifications. > >> >> But in the mean time what about having a force option? >> >>> +       if (stop_addr < start_addr): >>> +               if (options.verbose == True or not options.force): >>> +                       print("Packet Dropped, Discontinuity detected >>> [stop_add:0x%x start_addr:0x%x ] for dso %s" % (stop_addr, start_addr, >>> dso)) >>> +               if (not options.force): >>> +                       return > > If the stop address is less than the start address, it must be something > wrong. In this case, we can report a warning for discontinuity and directly > return (also need to save the `addr` into global variable for next parsing). > > I prefer to not add force option for this case - eventually, this will consume > much time for reporting this kind of failure and need to root causing it. A > better way is we just print out the reasoning in the log and continue to dump. But in this case we've identified all the known issues that would cause the script to fail and we can fix them in Perf and OpenCSD. There may not even be any more issues that will cause the script to fail in the future so there's no point in softening the error IMO. That will only hide future issues (of which there may be none) and make root causing harder when it hits some other tool.