Date: Sun, 19 Jan 2025 19:41:32 -0600 From: Kyle Evans <kevans@FreeBSD.org> To: Brooks Davis <brooks@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, Warner Losh <imp@freebsd.org> Subject: Re: git: 9ded074e875c - main - Refactor makesyscalls.lua into a library Message-ID: <7371f49b-125a-47af-b36b-38f3a8ebfef1@FreeBSD.org> In-Reply-To: <Z4qi5cecDEZ0asv6@spindle.one-eyed-alien.net> References: <202410302108.49UL8tGl053622@gitrepo.freebsd.org> <25a28def-fbfd-49df-a2bf-dc4ef6609440@FreeBSD.org> <Z4mLe55Dpu9Jp_fi@spindle.one-eyed-alien.net> <a85ab469-0ed8-4c6d-b0e9-cb1f2e458595@FreeBSD.org> <Z4qi5cecDEZ0asv6@spindle.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On 1/17/25 12:35, Brooks Davis wrote: > 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 <sterspark@gmail.com> >>>>> AuthorDate: 2024-10-21 21:42:13 +0000 >>>>> Commit: Brooks Davis <brooks@FreeBSD.org> >>>>> 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 <imp@freebsd.org> >>>>> Co-authored-by: Kyle Evans <kevans@freebsd.org> >>>>> Co-authored-by: Brooks Davis <brooks@freebsd.org> >>>>> Sponsored by: Google (GSoC 24) >>>>> Pull Request: https://github.com/freebsd/freebsd-src/pull/1362 >>>>> Signed-off-by: agge3 <sterspark@gmail.com> >>>>> --- >>>>> 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'll take a look when I get a chance, thanks. > 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. I took a stab at this here: https://github.com/kevans91/freebsd/commit/083215d48541eb2be5e7725031c319f50f3881c8 Most of the uses are fairly trivial but, IMO, pretty reasonable (and generally of the same pattern). Even just a subset of CPP parsing to cover trivial #ifdef / #else / #endif sequences and tagging syscalls with a `condition`` that propagates to generated files would be sufficient, even if we aren't blindly propagating preprocessor conditions anymore. Thanks, Kyle Evans
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7371f49b-125a-47af-b36b-38f3a8ebfef1>