From nobody Fri Jan 17 18:35:17 2025 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4YZT1f2Rghz5lJMm; Fri, 17 Jan 2025 18:35:18 +0000 (UTC) (envelope-from brooks@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4YZT1f1fZ3z3kDp; Fri, 17 Jan 2025 18:35:18 +0000 (UTC) (envelope-from brooks@freebsd.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1737138918; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ljykCQvyffKJuxA0ENBzGxqQ3dx+MEJs9IeIvV7ft/w=; b=rxydgm1UJGrKUzQwSczUtU2ZQVe+kjxwHGuZTMn+8Dy9ia1qPiJCUbbh/vH3KVRQkswoxe f2ZohSciESR4LDeWN253bqJ7N0b5HfyavfZ7tv6RBgCh6zyOXfTHtRLSRbVOXppXxKvYuP KnNmIPBY1zo3g81SRWdGdAJF2kwJhx0zcbj6B/+L2tCC+wcwslPBGPemvHMYyPTZffWX3a 09+Mvx3o9EE8hAuCz0m1E4/rfeRGT5WoYT3zKN2HDXHmZ6/IDwn48DKLZYeqk8M7/bWS3l EJVb2/FeIpg1aqlwMmOXgy3U3jb2GORx7Mf2EC5fB4iWlHH2ms8wm3qT3z04aQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1737138918; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ljykCQvyffKJuxA0ENBzGxqQ3dx+MEJs9IeIvV7ft/w=; b=XpDKMOmkDdmrtDtRYCV7FlUjrxxF13c6nadd0Kbi9ZJ+LjKGlVX8ys4TWICaRv9b927l4Y rcpyPy7LuVD7Smk3/l61Wq5a1Tn4IGp/8azBFOONM3LA5/wllo5RXXQ/qvBPf/EsMxVoy3 pSxLQBV4AeY54BrDULPSVbU7hOSvlcmDo720WTMbnLRli0BRphtcch4xYVM+0r5oH9H0O/ JSr1oOXvMirXPDf19b89SYkgQWLodd9mEtKADu+CsJSiYn+pwV+lXZbW0/Q5QaF5g/Sayu i5VFcXB5opdeWVUHPgI7Q8pjunXoO/HqWEpCJ8uCowiZ7rfuabSipATo5tTNBA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1737138918; a=rsa-sha256; cv=none; b=JPEOM0zA7DvKjy0M9xw81HO78b8Hn/VPkomXp0QsRaMok9CgMkAHM87y3gaXjUffiNxnrH uA51J9KHetQfp35ZHEZyHFtgkcPNTjQYvX3dTYpjBo5vjKH0huPaVhCXUQmBtH1wUTPUnB TR8DidLifTTfCk5b8n6vFqRWUvCsNy72kX0Mv3vAKlmVP0cIIAIC/Jw4oEKONgEWd7F9ek 1i5eQhtnCgbHM0iGSddCOKmq2KdmBsyHWIhjiLndhEpebLbgkE/IP5djhz878SOGMXMxr1 66/PfZnK1iT1CEStiwYR4S0CA1diBTOrBTOHjBF59W4DwhlvcE++pDgfIhjwcQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: brooks/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id 4YZT1f0X4pzCSN; Fri, 17 Jan 2025 18:35:18 +0000 (UTC) (envelope-from brooks@freebsd.org) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 7B5513C01A0; Fri, 17 Jan 2025 18:35:17 +0000 (UTC) Date: Fri, 17 Jan 2025 18:35:17 +0000 From: Brooks Davis To: Kyle Evans Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, Warner Losh Subject: Re: git: 9ded074e875c - main - Refactor makesyscalls.lua into a library Message-ID: References: <202410302108.49UL8tGl053622@gitrepo.freebsd.org> <25a28def-fbfd-49df-a2bf-dc4ef6609440@FreeBSD.org> List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Thu, Jan 16, 2025 at 08:53:07PM -0600, Kyle Evans wrote: > On 1/16/25 16:43, Brooks Davis wrote: > > On Thu, Jan 16, 2025 at 03:52:22PM -0600, Kyle Evans wrote: > > > On 10/30/24 16:08, Brooks Davis wrote: > > > > The branch main has been updated by brooks: > > > > > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=9ded074e875c29cb92d5f643801990d7bb23cca4 > > > > > > > > commit 9ded074e875c29cb92d5f643801990d7bb23cca4 > > > > Author: agge3 > > > > AuthorDate: 2024-10-21 21:42:13 +0000 > > > > Commit: Brooks Davis > > > > CommitDate: 2024-10-30 21:04:30 +0000 > > > > > > > > Refactor makesyscalls.lua into a library > > > > * main.lua replicates the functionality of makesyscalls.lua > > > > * Individual files are generated by their associated module > > > > * Modules can be called as standalone scripts to generate a specific > > > > file > > > > * Data and procedures are performed by objects instead of procedual code > > > > * Bitmasks are replaced by declarative types > > > > * Temporary files are no longer produced, writing is stored in memory > > > > * Comments provide explanation to functions and semantics > > > > Google Summer of Code 2024 Final Work Product > > > > Co-authored-by: Warner Losh > > > > Co-authored-by: Kyle Evans > > > > Co-authored-by: Brooks Davis > > > > Sponsored by: Google (GSoC 24) > > > > Pull Request: https://github.com/freebsd/freebsd-src/pull/1362 > > > > Signed-off-by: agge3 > > > > --- > > > > sys/tools/syscalls/README.md | 49 +++ > > > > sys/tools/syscalls/config.lua | 312 +++++++++++++++++ > > > > sys/tools/syscalls/core/freebsd-syscall.lua | 147 ++++++++ > > > > sys/tools/syscalls/core/scarg.lua | 163 +++++++++ > > > > sys/tools/syscalls/core/scret.lua | 45 +++ > > > > sys/tools/syscalls/core/syscall.lua | 497 +++++++++++++++++++++++++++ > > > > sys/tools/syscalls/main.lua | 64 ++++ > > > > sys/tools/syscalls/scripts/init_sysent.lua | 193 +++++++++++ > > > > sys/tools/syscalls/scripts/libsys_h.lua | 111 ++++++ > > > > sys/tools/syscalls/scripts/syscall_h.lua | 97 ++++++ > > > > sys/tools/syscalls/scripts/syscall_mk.lua | 90 +++++ > > > > sys/tools/syscalls/scripts/syscalls.lua | 109 ++++++ > > > > sys/tools/syscalls/scripts/syscalls_map.lua | 74 ++++ > > > > sys/tools/syscalls/scripts/sysproto_h.lua | 242 +++++++++++++ > > > > sys/tools/syscalls/scripts/systrace_args.lua | 268 +++++++++++++++ > > > > sys/tools/syscalls/tools/generator.lua | 113 ++++++ > > > > sys/tools/syscalls/tools/util.lua | 194 +++++++++++ > > > > 17 files changed, 2768 insertions(+) > > > > > > > > [...] > > > > diff --git a/sys/tools/syscalls/core/freebsd-syscall.lua b/sys/tools/syscalls/core/freebsd-syscall.lua > > > > new file mode 100644 > > > > index 000000000000..193b1e43563c > > > > --- /dev/null > > > > +++ b/sys/tools/syscalls/core/freebsd-syscall.lua > > > > @@ -0,0 +1,147 @@ > > > > [...] > > > > +function FreeBSDSyscall:parseSysfile() > > > > + local file = self.sysfile > > > > + local config = self.config > > > > + local commentExpr = "^%s*;.*" > > > > + > > > > + if file == nil then > > > > + return nil, "No file given" > > > > + end > > > > + > > > > + self.syscalls = {} > > > > + > > > > + local fh, msg = io.open(file) > > > > + if fh == nil then > > > > + return nil, msg > > > > + end > > > > + > > > > + local incs = "" > > > > + local defs = "" > > > > + local s > > > > + for line in fh:lines() do > > > > + line = line:gsub(commentExpr, "") -- Strip any comments. > > > > + -- NOTE: Can't use pure pattern matching here because of > > > > + -- the 's' test and this is shorter than a generic pattern > > > > + -- matching pattern. > > > > + if line == nil or line == "" then > > > > + goto skip -- Blank line, skip this line. > > > > + elseif s ~= nil then > > > > + -- If we have a partial system call object s, > > > > + -- then feed it one more line. > > > > + if s:add(line) then > > > > + -- Append to system call list. > > > > + for t in s:iter() do > > > > + if t:validate(t.num - 1) then > > > > + table.insert(self.syscalls, t) > > > > + else > > > > + util.abort(1, > > > > + "Skipped system call " .. > > > > + "at number " .. t.num) > > > > + end > > > > + end > > > > + s = nil > > > > + end > > > > + elseif line:match("^#%s*include") then > > > > + incs = incs .. line .. "\n" > > > > + elseif line:match("%%ABI_HEADERS%%") then > > > > + local h = self.config.abi_headers > > > > + if h ~= nil and h ~= "" then > > > > + incs = incs .. h .. "\n" > > > > + end > > > > + elseif line:match("^#%s*define") then > > > > + defs = defs .. line.. "\n" > > > > + elseif line:match("^#") then > > > > + util.abort(1, "Unsupported cpp op " .. line) > > > > > > This specifically is kind of a huge regression, and I don't really know how to > > > cope with it. We've guaranteed for years that we'll copy preprocessor > > > directives through to all output files. We don't use that upstream in > > > FreeBSD, but we work with downstreams/vendors that make extensive use of it in > > > their syscall definitions. > > > > > > I don't really know what the answer is to this, but we probably shouldn't have > > > dropped it without some discussion first. This is going to be a bit of a > > > headache... > > > > This response seems rather hyperbolic. This change was up for review > > for months and the feature is unused in tree so there was no way to know > > it was important. > > > > Re-reading, yes, this was a bit dramatic; my apologies. There's plenty of > frustration here, mostly amplified by the fact that I was on the review just > as much as you folks and have worked in environments that use it- it certainly > should have stuck out to me, but I just didn't have the time into it that I'd > hoped I would. > > I would've also really liked to see an "XXX" comment at a minimum drawing > attention to it or a call-out in the commit message, given that the syscall > definition documentation isn't that lengthy and this is one of the few > guarantees we make it. I think there's some compromise to be had, but... > > > It would be helpful to work through some examples understand what people > > need here and if it really has to be a refactor to pass things through > > or if adding some new tags and config values could do the job. > > > > ... I'll respond to this this weekend, hopefully. I'd like to condense what > I'm aware of into some formal test cases in sys/tools that I can point to so > that we have something less abstract to debate the merits of, and also so that > we have something we can verify the functionality against. I've implemented simple support for ifdef's syscall variants in https://github.com/freebsd/freebsd-src/pull/1575. It's not robust at all and may be missing some bits in newly generated files, but it's probably not much worse than what's in 14. I agree some tests would be good. I think I'd deviate from the norm and run them as part of the top level `make sysent` for ease of develoment. > > The quickest workaround would probably to revert these commits locally > > (I took care to isolate them for exactly this reason): > > > > 5212b9500116 sysent: GC sys/tools/makesyscalls.lua > > 204d065dac81 sysent: switch to refactored makesyscalls.lua > > > > You might also need to revert this commit or implement it in > > makesyscalls.lua: > > > > bbc0f33b1317 sysent: add a NOLIB modifer to prevent stub generation > > > > If we need be, I'd even be ok with temporarily restoring makesyscalls.lua > > and back porting easy features, but maintaining two is certainly not > > viable indefinitely. > > > > This isn't urgent enough to warrant any of that- it's just something that I'd > like to work out how we can solve (preferably in the short-to-mid term), > because there good uses for at least a subset of what we supported before. I > don't think we need to go as far as supporting arbitrary preprocessor, but if > we could at least devise something for conditional syscall slots that'd > probably be sufficient and workable. I think the right midterm solution is probably a combination of generating the files at buildtime (one of the goals of the refactoring) and some sort of overlay framework so you can replace syscalls on a per-arch basis. -- Brooks