From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f178.google.com (mail-pl1-f178.google.com [209.85.214.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 92F3D1494C9 for ; Fri, 3 Jan 2025 01:45:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.178 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735868702; cv=none; b=FMdjtN0mNJDRn5LEjIeKGDvOhPqrUR/Y4KKqyI5gGYk/yg12V/0g6674xqPs+mGbfbExD0B6zTq62PcQsxe3T1ndI8tpacvdklgQhbiTzAUBILOOnY3hzhtVo6rpV2U3stY+2vCxnSMU/jxRkWHx9OPi579ewlGJv7AqtNtBpGA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735868702; c=relaxed/simple; bh=8DXRkjKiMXyKKlr3S0B4rCNSO4zHo1bxFapnvPyVoTU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=JIzdz6SIh7wLWBmg79lm/q2Gn7dzvpTOzRfyszG0bS7hniCSofEq1JAlTUBYNdaLYWHeIiKvS6Zg8aZ/UbI8hMRHsHigVafO/4/jj1WHF+2hkl0f5R19okWXp9mTeZbEKPv4JW6/rxYSk7J59TFwLBOU4aGOJUuLho38HAIpWR8= 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=k54mYGDo; arc=none smtp.client-ip=209.85.214.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="k54mYGDo" Received: by mail-pl1-f178.google.com with SMTP id d9443c01a7336-21649a7bcdcso159096915ad.1 for ; Thu, 02 Jan 2025 17:45:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1735868700; x=1736473500; 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=pJ3eKF6usIayHtABB82uMYDq9+VNdHrqEKVZK5a0SvE=; b=k54mYGDoQrnvc9W7givm587O+CNnoeSHTOGs7VYXW694O29NSvz/UsAy5f+oj/wLql /athqu7ZAFs284J0474qVOIM41myBXXyOkl4HpSnvdsbDiFfZHyA50nwN69ikvnqoc16 ZwxlEeM53Fqck1KPgtaHz1M5kxm9t/6+K4pjD2h2UKTDWTSMVqQpeQUsd2pGFA+SFaWM 0Dkyok3l82XpFNuU+wP1UhLvOVgy2K1QT8lQrJ/fzi/+//M4hCHvFW5NOutZuhl+dysB y9jJEEhr/pPXZImYvGXpRw8EY7Y/CJrT4xJxvnHtov+Qasy6pz+o3cv9sfOS8z8eEmhY xfsw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735868700; x=1736473500; 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=pJ3eKF6usIayHtABB82uMYDq9+VNdHrqEKVZK5a0SvE=; b=NfQcmC19ZuFXZ+VyR57j/WQkq5vBGDdTAZliaiO3p7tUb1ohae+HxKKe1fw/XytD1s HOCYhfM0jYFhI5GuLYw2iX+S4X6dt4WX697A9PhUBLTMrQuxlKi0Vw9HILtfMI+4tvT9 gas8jpCCzH1U67x0pwD9vJscVxhnUNU6HllfqUHzEPwVskNBmnId7X52APBjg+QgffOM B0SohyjvLD5Z8ZlaoStYcPmCkqaqHEMFRhzODixI7xMJ7APbuf01Qidt/80Np2cVZAUZ zQTfdeBPUrxxZz5HMSSz8Zt0YT1kDwquDijiqf+CX1MRUqqso9UW1DQOyh7JJunPk+l3 0v9A== X-Forwarded-Encrypted: i=1; AJvYcCVwLkcKLYaIW/aqWnmEtC1ylPOkPUdUku6XRK8i9njXuOlRFS9U3EOs/Ul0VWQMCsKodeZ3U35JaFMc3nE=@vger.kernel.org X-Gm-Message-State: AOJu0Yy63aMKUFR7dKQwqfFk6amTk9f7AyUtnC3e2JKtyzhiSFEnwQ/e mSA+Q+/Er93qenFXqHlN1DtJiqeiiaYxLfaEe14q2KS8UOoN//0QYPWRpkCWqKs= X-Gm-Gg: ASbGncvzXoCMcJz6YzKLQAFUNikebSOukTIWNU4nNEpd9rJ7VnjotYb9h7gbmyPBlQ5 +xj/Rmk+HRHCQl2eDmC4ahshMq13SkdTF71Gl/XMN8ez+QMv0LcDImDUpCEZ6pxYlkXyqJOSJ47 2T6kxTSKxSu+jkGPNY8UMjXYtlXm8Ct9y8Hx1hfNX6A+3vjYtHwS9Cn5y2nzMwSiQb4eHbDGe07 LZtuyIQIKe25wn4+gGvSj1lTHS66edTOQ+3ZYOjT0WFnRuFoBh0 X-Google-Smtp-Source: AGHT+IGlJOK8lsWm8Cmc19QjQZCDFg/Q+5Dom/f2Np8/qC3K9ofC60FF2YnXzD02V2GYbVMkVLFEug== X-Received: by 2002:a05:6a00:124a:b0:725:f4c6:6b68 with SMTP id d2e1a72fcca58-72abde404e9mr65911625b3a.4.1735868699801; Thu, 02 Jan 2025 17:44:59 -0800 (PST) Received: from ghost ([2601:647:6700:64d0:e7aa:b727:e049:3265]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-842b20b3fbbsm19449947a12.24.2025.01.02.17.44.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Jan 2025 17:44:59 -0800 (PST) Date: Thu, 2 Jan 2025 17:44:56 -0800 From: Charlie Jenkins To: Ian Rogers Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Ingo Molnar , 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 Wed, Dec 18, 2024 at 05:52:24PM -0800, Charlie Jenkins wrote: > On Wed, Dec 18, 2024 at 05:20:15PM -0800, Ian Rogers wrote: > > On Wed, Dec 18, 2024 at 2:32 PM Charlie Jenkins wrote: > > > > > > On Wed, Dec 18, 2024 at 02:13:20PM -0800, Ian Rogers wrote: > > > > On Wed, Dec 18, 2024 at 1:02 PM Charlie Jenkins wrote: > > > > > > > > > > On Wed, Dec 18, 2024 at 11:23:51AM -0800, Ian Rogers wrote: > > > > > > On Wed, Dec 18, 2024 at 10:41 AM Arnaldo Carvalho de Melo > > > > > > wrote: > > > > > > > > > > > > > > On Tue, Dec 17, 2024 at 04:30:15PM -0800, Charlie Jenkins wrote: > > > > > > > > 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 > > > > > > > > > > > > > > > > > --- a/tools/perf/tests/code-reading.c > > > > > > > > > > @@ -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. > > > > > > > > > > > > > > Can't we just use uname here? > > > > > > > > > > > > > > So that we don't use kconfig.h since its not used in tools/perf/ and > > > > > > > makes it looks like perf is in lockstep with the kernel source tree > > > > > > > version it was compiled from? > > > > > > > > > > > > > > $ git grep kconfig.h tools/perf/ > > > > > > > $ > > > > > > > > > > > > > > BTW, what would happen if I collected a perf.data file on x86_64 and > > > > > > > would read it in a RiscV machine with such a objdump version? The same > > > > > > > problem? > > > > > > > > > > > > This code is in tests hence thinking that a separate fix is needed for > > > > > > that problem. Hopefully the use of elf machine/flags tackles it: > > > > > > 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#n25 > > > > > > We are getting somewhat disassembler heavy. We have llvm as a library, > > > > > > capstone as a library, binutils objdump and llvm objdump. Given the > > > > > > pain with parsing text, could we lose the objdumps? Similarly for > > > > > > addr2line? > > > > > > > > > > Are you suggesting to remove this test case entirely to get rid of the > > > > > objdump dependency? The goal of this test case seems to be to check > > > > > objdump and perf return the same data, so it doesn't seem like there > > > > > would be an alternative to using objdump. > > > > > > > > I can imagine having an objdump dependency for a test but not for some > > > > more core like `perf annotate`. We have to do weird things when > > > > parsing text, like this code I'm not proud of: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/srcline.c?h=perf-tools-next#n523 > > > > The issue with that code is that LLVM objdump has changed its output > > > > in newer versions to be closer to binutils objdump. Did that break > > > > perf? Maybe it just broke what our variables think is an LLVM objdump, > > > > but things aren't really broken. This kind of issue doesn't occur with > > > > a library, although the differing needs of library versions is a real > > > > thing. > > > > > > Yeah doing the parsing of the text output is not ideal... For this test > > > case it should be possible to dynamically link against libbfd. > > > > I need to write the patch set to delete libbfd from perf. IANAL but > > the issue is that libbfd is part of binutils and GPLv3, while perf is > > part of the Linux kernel and largely GPLv2. GPLv3 is incompatible with > > GPLv2: > > https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility > > While using dlopen means we're not linking against libbfd, we may > > effectively be using it as a plugin which again GPLv3 (in my IANAL > > opinion) wouldn't allow: > > https://www.gnu.org/licenses/gpl-faq.en.html#GPLPlugins > > Currently to get libbfd support in perf you need to be building the > > binary yourself and add to the build BUILD_NONDISTRO=1. We do this as > > part of our build testing but having all the #ifdef-ed libbfd code if > > nothing else makes the code harder to understand. > > Licensing is fun ;) > > > > > > I would > > > guess something similar could be done with llvm-objdump but I am less > > > familiar with that. I don't know if that's a good path to go down > > > though. > > > > In the past I perceived there was hostility toward LLVM from the Linux > > kernel community. I guess GPL was considered the special sauce as to > > why Linux won and the BSDs hadn't, so the preference was to favor a > > compiler that used the same license. I don't think that's true any > > more and I think there's a lot of sense in using LLVM's libraries > > rather than reinventing them in the perf tool, or using perhaps less > > orthodox sources like libcapstone. I'm not a fan of the text output > > processing stuff so getting rid of objdump and llvm-objdump support > > would be good imo. > > Yeah I agree. This test case did end up being interesting though as it > unconvered this change in behavior of objdump on riscv, but that's > tangential to the purpose of this test case. We need this patch on riscv > to stop this test from failing, but it is also reasonable to approach > this differently and not use objdump at all. What's the next step here? Would you prefer to get rid of this test entirely? I sent out a v3 that uses uname [1]. Link https://lore.kernel.org/lkml/20241219-perf_fix_riscv_obj_reading-v3-1-a7d644dcfa50@rivosinc.com/ [1] - Charlie > > > > > Another area where I think we could lose a lot of code baggage is with > > libunwind, as BPF support requires libelf which brings with it > > libdwarves which when present means we don't use libunwind. I've heard > > reports that libdwarves is slower, but I'm sure we can add caches to > > speed it up which would likely benefit a range of people. I was kind > > of hoping with all that deleted we may be able to get rid of the > > majority of the arch directory, but the syscalltbl work is adding to > > that directory :-) > > Yeah... The syscalltbl work will add some extra parts to the arch > directory. A lot of the additions are generic and a chunk of > arch-specific ifdefs were able to be removed, but we still have the > "problem" that not every architecture uses the shared syscall table and > supports different syscalls. > > - Charlie > > > > > Thanks, > > Ian