Track kernel function definition locations more precisely - #6427
Track kernel function definition locations more precisely#6427fingolfin wants to merge 8 commits into
Conversation
fingolfin
added
kind: enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6427 +/- ##
==========================================
- Coverage 79.08% 79.08% -0.01%
==========================================
Files 685 685
Lines 293736 293895 +159
Branches 8664 8705 +41
==========================================
+ Hits 232315 232427 +112
- Misses 59621 59648 +27
- Partials 1800 1820 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI unfortunately fails. I can have a look once this is green |
f30b6bd to
c9458f6
Compare
| return FALSE; | ||
| } | ||
|
|
||
| static const char * SkipCTrivia(const char * p, UInt * line) |
There was a problem hiding this comment.
I'm not opposed if people really want it, but is this basically writing an entire mini C'ish' parser to find the line numbers of functions in the kernel? How hard has this been tested?
There was a problem hiding this comment.
Reply generated by Claude (with my review) — @fingolfin
Fair question. It's deliberately not a C parser — it never touches the preprocessor and doesn't try to understand declarations. It tokenizes just enough to tell a definition apart from the other places a name can appear: skip comments and string/char literals, accept the symbol only as a whole token, then require a balanced parameter list followed by {. Prototypes, call sites and mentions in the *F FuncFOO( ... ) comment blocks are therefore skipped, and macro-generated functions are simply never found — in which case we fall back to the old behaviour of naming the C function (src/macfloat.c:FuncSIN_MACFLOAT). It's ~120 lines and it fails closed.
On testing: tst/teststandard/startline.tst now checks the result for every kernel function GAP knows about against the C sources, without using the kernel scanner. Currently that's 944 globals bound to a kernel function with a known C name; 915 resolve to a line, 29 don't. It asserts:
- the reported line starts the definition of that C function (return type optional, possibly on the preceding line) — 0 mismatches;
- distinct C functions get distinct lines — 804 distinct (file, symbol) pairs → 804 distinct (file, line) pairs;
- 573 of the 915 have their name occurring earlier in the same file — comment blocks, forward declarations, calls — so the discrimination is exercised heavily rather than incidentally. The test asserts this stays above 100, so it can't silently degenerate into checking nothing;
- the 29 unresolved ones are exactly the
MAKEMATHPRIMITIVE-generated ones inmacfloat.c; the test asserts their names occur only inside comments, and thatLocationFuncfalls back to the C function name for them.
This was also verified in-tree, out-of-tree with an absolute srcdir, and with --enable-debug --enable-Werror.
Resolve C definition lines lazily for standard GVAR-backed kernel
functions. The result is cached, and used for StartlineFunc and
PageSource.
Before:
gap> LocationFunc(RETURN_FIRST);
"src/gap.c:RETURN_FIRST"
After:
gap> LocationFunc(RETURN_FIRST);
"GAPROOT/src/gap.c:448"
This then allows us to drop the now unused LOCATION_BODY
accessors, rename the shared body field to startline, and remove
the leftover empty-string initialization for undefined global
functions.
AI-assisted: OpenAI Codex was used to implement the change and
prepare the regression coverage.
Co-authored-by: Codex <codex@openai.com>
Also tweak PrintKernelFunction
c9458f6 to
dd17143
Compare
Rework the preceding commits so that they no longer change any API or output that others rely on: * `SET_LOCATION_BODY` is kept, but as a deprecated shim which appends the location to the body's filename. GAP.jl and Semigroups (via gapbind14) both call it to label a function with the name of the C resp. Julia function implementing it; folding that into the filename produces exactly the same output, while still letting us drop the `startline_or_location` union. Without this, GAP.jl does not build against GAP master, which is why the "CI with GAP.jl" jobs failed. * `LOCATION_FUNC` is restored, but is now derived from the handler cookie instead of being stored in the function body. It serves as a fallback for kernel functions whose definition cannot be located in the C source -- e.g. those generated by the `MAKEMATHPRIMITIVE` macro in `macfloat.c` -- and lets the test suite check what the scanner reports. * `SetupFuncInfo` no longer prefixes file names with `GAPROOT/`. CI runs `$SRCDIR/configure` with an absolute path, so `__FILE__` is absolute there and the prefix was only ever added for in-tree builds; this made both out-of-tree jobs fail. The source file is now instead located by looking up the shortened name (e.g. `src/calls.c`) in the GAP root directories, which works in either case. * Only function bodies which still look exactly like the ones `SetupFuncInfo` creates are considered, so that bodies made up by kernel extensions are left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Check the line numbers reported for all 900+ kernel functions GAP knows about against the C sources, without relying on the kernel's own scanner: the reported line must start the definition of the C function, distinct C functions must be reported at distinct lines, and each function whose definition is not found must be one whose name never occurs outside of comments, i.e. one generated by a macro. For over half of the functions checked, the name also occurs earlier in the file -- in the `*F FuncFOO( ... )` comment blocks, in forward declarations, or in calls -- so this exercises exactly the cases the scanner has to tell apart. The test asserts that this is so, to make sure it stays meaningful. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`LocationFunc` now reports the plain file name if neither a line number nor the name of a C function is available. That is the case for function bodies made up by kernel extensions, which put a description into the file name: GAP.jl's `Display(Julia.Base.parse)` expects to see a `# Julia:parse` header, which `Display` obtains via `LocationFunc`. Also move the new test to `teststandard`. It iterates over all global variables, which under `testinstall-loadall` both triggers output from packages loading data on demand, and turns up kernel functions of packages whose sources we cannot locate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Had to make (or rather, let an AI make) some major changes to let CI pass (if it will, we'll see), need to review those myself and am not sure I am happy with overall compromises we had to make to get things to pass... sigh Therefore, I've changed this PR to a draft. |
3 participants
Resolve C definition lines lazily for standard GVAR-backed kernel functions. The result is cached, and used for
StartlineFuncandPageSource.Before:
After:
This then allows us to drop the now unused
LOCATION_BODYaccessors, rename the shared body field tostartline, and remove the leftover empty-string initialization for undefined global functions.AI-assisted: OpenAI Codex was used to implement the change and prepare the regression coverage.
I actually wrote this back in April but then didn't want to bother spending time to get this merged.