Re: git: 9ded074e875c - main - Refactor makesyscalls.lua into a library
Date: Fri, 17 Jan 2025 18:35:17 UTC
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 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