Re: git: 9ded074e875c - main - Refactor makesyscalls.lua into a library
- In reply to: Brooks Davis : "Re: git: 9ded074e875c - main - Refactor makesyscalls.lua into a library"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 20 Jan 2025 01:41:32 UTC
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