From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f178.google.com (mail-pf1-f178.google.com [209.85.210.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4AA891E48A for ; Wed, 18 Dec 2024 00:30:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.178 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734481823; cv=none; b=k5QHC8HuUNP2FaHj5XhpbFcUHOIVxIiP8neseNnUlcJGgBXhKEOSMe8y/tt7Id93fRaYhI89oDSbKWcg0KjWlU3ByWBglbVLOXALQmMKWHHb6ybmQj8cIV53dLGgl2Xh9IizzrEdm8LJy274gcdg4diuWOquV/fi0JFT5gW/B1A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734481823; c=relaxed/simple; bh=8aoJfoRE7+15dJoExHzjdMJUpg8MUKDQ4A8VV/wLmbg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=mWQ4PZHze5PENuJKDdy20QwVmO1s0KttF6KO0nz/SNxLrbUGS9Jw64sqYhX7CcAzyIHwOlGyxpY0bU5cPY2CemBmbG+rWnR+yk6Aopm7DfnxuLmhUYPsfBHZJgZBkboOopKwA/gaZEX24jJedfoAdKSUlRxVB6UG9qzBDrqa4IA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com; spf=pass smtp.mailfrom=rivosinc.com; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b=aICEC9yK; arc=none smtp.client-ip=209.85.210.178 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rivosinc.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b="aICEC9yK" Received: by mail-pf1-f178.google.com with SMTP id d2e1a72fcca58-728f28744c5so5500487b3a.1 for ; Tue, 17 Dec 2024 16:30:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1734481819; x=1735086619; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=Cw55AKdZY3Iyzi7xSZCsR45xkksNMr90DAnnsKl2gF4=; b=aICEC9yK7zwnV7201rTX1CgWg1iJ5jJG79666ogVeZPZ4sUwOTAdq9rmbzbbpvaMdk KZlL7pAWaUjOTEFLo6SKKXdLIjWJqOpnUzRXsC2pR2YbWTFXd7/9d+OGYxLBzr8LNpQh hW5lXKxLghGmmdANro8oSXfhoG7Zo0zT7w8b24sypXfhrPTeiRfqimArFQWHCY7bjURB At4X/aUjKQjnj3Uq/GiAYymNbEhDk3YB29gEZaXttZg7dRhQUBRU3QrJTpUXGMv/qGUb Rr8Ul8rN3iTZtkwNFSwECHqYOFwo/UbfiJIJf/sjmh+5CGQSbOxj3vqHpE4IyrhrWH0q Fg6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734481819; x=1735086619; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Cw55AKdZY3Iyzi7xSZCsR45xkksNMr90DAnnsKl2gF4=; b=mjxSqW+y0mFt6jZ+7HvHV75Fxb9ucIYpU++idsyzVOxJ0Iz03jTZaAzZtKo/rOm8Bg vAH2pUnVbvYUXrUMgGwa0LjznWz/2GDnFiDeypKV4rSQCIKLNu8UEK+bIRPicMB/1xxC SxTGWKGcsTvYoxdHH/g5MbywoLGyBaSSvBXmIHHdwBnHNSIl1QVWGJH7nhAc997SGAcv XP0hHG56Hns7wyGDW04ucYmurYONILBJunF2ZZs3IcFsbU/vUkzL1wBae7ibhgKrsYLY +PviFps6DeqInLNPnzcNvF32M6hgVvXcTM6Ek2BNlr8CUo28/92vjkrIyAJJ7m580uFq LNSw== X-Forwarded-Encrypted: i=1; AJvYcCV1XTygOTmvk2Yb4fO5e/Uw7tX2rsTrvi/jGqUppgi5t1sNtbWGwynjiqf5v2KwmK3Q6gKFENIdK37gzBk=@vger.kernel.org X-Gm-Message-State: AOJu0Yx9hB28ulydgCDR1QQy296kfyI2W3/U/rPFCVNMwfhVYDLP4nRT mApO5v9a49OdO2KxtFFCNanSaAPw6MYEB9mIqiFkL+TuebD0+iL5rVHxkgy8b9k= X-Gm-Gg: ASbGncsC1N7CwnpwKjqveI3JNkk2lW+xNsI5EFVEY0RwJ7nGKkPAZ0Mp3IV6Qg1KxFJ 7QubiJZGsOrXV45j8bdsTWl5PNGzmtsva5iDx8reuP/Cr5drC0B9IW7GXQ/JEdoMVNrZwJcSeVd ppjTWwx7p2AosnofKKBkDDR2Jd6LgKVtENJgRBL9wdq7QYUxJKAa/ISbRq1AWbjSy/7ByEPPJut H5YaK6/051YHAzzMLiw6U9Lbu9CLYEfomelyy1W8vc+yRA= X-Google-Smtp-Source: AGHT+IG5Cx6yPttPbYg5wTTofbNuEwnlOQqyTrXYd/ruQPCVe05gCE7tcoBeRNxbyUyUCgfsg6Vu2g== X-Received: by 2002:a05:6a20:a103:b0:1e1:9f57:eac3 with SMTP id adf61e73a8af0-1e5b480f77bmr1790673637.18.1734481819597; Tue, 17 Dec 2024 16:30:19 -0800 (PST) Received: from ghost ([50.145.13.30]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-801d5aaea84sm5297831a12.26.2024.12.17.16.30.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Dec 2024 16:30:18 -0800 (PST) Date: Tue, 17 Dec 2024 16:30:15 -0800 From: Charlie Jenkins To: Ian Rogers Cc: Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Namhyung Kim , Mark Rutland , Alexander Shishkin , Jiri Olsa , Adrian Hunter , Paul Walmsley , Palmer Dabbelt , Albert Ou , Nathan Chancellor , Nick Desaulniers , Bill Wendling , Justin Stitt , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org Subject: Re: [PATCH v2] tools: perf: tests: Fix code reading for riscv Message-ID: References: <20241217-perf_fix_riscv_obj_reading-v2-1-58f81b7b4c7d@rivosinc.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Tue, Dec 17, 2024 at 04:18:32PM -0800, Ian Rogers wrote: > On Tue, Dec 17, 2024 at 3:52 PM Charlie Jenkins wrote: > > > > After binutils commit e43d876 which was first included in binutils 2.41, > > riscv no longer supports dumping in the middle of instructions. Increase > > the objdump window by 2-bytes to ensure that any instruction that sits > > on the boundary of the specified stop-address is not cut in half. > > > > Signed-off-by: Charlie Jenkins > > Reviewed-by: Ian Rogers > > > --- > > A binutils patch has been sent as well to fix this in objdump [1]. > > > > Link: > > https://sourceware.org/pipermail/binutils/2024-December/138139.html [1] > > --- > > Changes in v2: > > - Do objdump version detection at runtime (Ian) > > - Link to v1: https://lore.kernel.org/r/20241216-perf_fix_riscv_obj_reading-v1-0-b75962660a9b@rivosinc.com > > --- > > tools/perf/tests/code-reading.c | 84 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 83 insertions(+), 1 deletion(-) > > > > diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c > > index 27c82cfb7e7de42284bf5af9cf7594a3a963052e..7e24d10a543ac18ac2be70b829d088874e0edfd5 100644 > > --- a/tools/perf/tests/code-reading.c > > +++ b/tools/perf/tests/code-reading.c > > @@ -1,5 +1,6 @@ > > // SPDX-License-Identifier: GPL-2.0 > > #include > > +#include > > #include > > #include > > #include > > @@ -176,6 +177,66 @@ static int read_objdump_output(FILE *f, void *buf, size_t *len, u64 start_addr) > > return err; > > } > > > > +/* > > + * Only gets GNU objdump version. Returns 0 for llvm-objdump. > > + */ > > +static int objdump_version(void) > > +{ > > + size_t line_len; > > + char cmd[PATH_MAX * 2]; > > + char *line = NULL; > > + const char *fmt; > > + FILE *f; > > + int ret; > > + > > + int version_tmp, version_num = 0; > > + char *version = 0, *token; > > + > > + fmt = "%s --version"; > > + ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path); > > + if (ret <= 0 || (size_t)ret >= sizeof(cmd)) > > + return -1; > > + /* Ignore objdump errors */ > > + strcat(cmd, " 2>/dev/null"); > > + f = popen(cmd, "r"); > > + if (!f) { > > + pr_debug("popen failed\n"); > > + return -1; > > + } > > + /* Get first line of objdump --version output */ > > + ret = getline(&line, &line_len, f); > > + pclose(f); > > + if (ret < 0) { > > + pr_debug("getline failed\n"); > > + return -1; > > + } > > + > > + token = strsep(&line, " "); > > + if (token != NULL && !strcmp(token, "GNU")) { > > + // version is last part of first line of objdump --version output. > > + while ((token = strsep(&line, " "))) > > + version = token; > > + > > + // Convert version into a format we can compare with > > + token = strsep(&version, "."); > > + version_num = atoi(token); > > + if (version_num) > > + version_num *= 10000; > > + > > + token = strsep(&version, "."); > > + version_tmp = atoi(token); > > + if (token) > > + version_num += version_tmp * 100; > > + > > + token = strsep(&version, "."); > > + version_tmp = atoi(token); > > + if (token) > > + version_num += version_tmp; > > + } > > + > > + return version_num; > > +} > > + > > static int read_via_objdump(const char *filename, u64 addr, void *buf, > > size_t len) > > { > > @@ -183,9 +244,30 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf, > > const char *fmt; > > FILE *f; > > int ret; > > + u64 stop_address = addr + len; > > + > > + if (IS_ENABLED(__riscv)) { > > Not sure if there is a consistency issue here. Elsewhere we're just > using ifdef, such as: > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/include/dwarf-regs.h?h=perf-tools-next#n69 I don't have any strong feelings about that. I can change it to be an ifdef. On other lists I have been told to use IS_ENABLED whenever possible, but it's only a small difference. - Charlie > > Thanks, > Ian > > > + int version = objdump_version(); > > + > > + /* Default to this workaround if version parsing fails */ > > + if (version < 0 || version > 24100) { > > + /* > > + * Starting at riscv objdump version 2.41, dumping in > > + * the middle of an instruction is not supported. riscv > > + * instructions are aligned along 2-byte intervals and > > + * can be either 2-bytes or 4-bytes. This makes it > > + * possible that the stop-address lands in the middle of > > + * a 4-byte instruction. Increase the stop_address by > > + * two to ensure an instruction is not cut in half, but > > + * leave the len as-is so only the expected number of > > + * bytes are collected. > > + */ > > + stop_address += 2; > > + } > > + } > > > > fmt = "%s -z -d --start-address=0x%"PRIx64" --stop-address=0x%"PRIx64" %s"; > > - ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, addr + len, > > + ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, stop_address, > > filename); > > if (ret <= 0 || (size_t)ret >= sizeof(cmd)) > > return -1; > > > > --- > > base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4 > > change-id: 20241213-perf_fix_riscv_obj_reading-cabf02be3c85 > > -- > > - Charlie > >