Get rid of sa->narg. It serves no purpose; use sa->callp->sy_narg instead.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/amd64/include/proc.h | ||
|---|---|---|
| 97 ↗ | (On Diff #77125) | What is the point ? If this change triggers asserts in kern_thread.c then fix offsets there. |
| sys/arm64/arm64/elf32_machdep.c | ||
| 188 ↗ | (On Diff #77125) | The code cries for nargs local var. |
| sys/arm64/linux/linux_sysvec.c | ||
| 131 ↗ | (On Diff #77125) | Add a define for 8. Use it there and for native. |
| sys/amd64/include/proc.h | ||
|---|---|---|
| 97 ↗ | (On Diff #77125) | My plan was to do that in a subsequent commit, so I could eg MFC this part. |
| sys/amd64/amd64/trap.c | ||
|---|---|---|
| 1039 ↗ | (On Diff #77158) | I did not checked, but this line should be too long now (same for 1002). |
| sys/amd64/include/proc.h | ||
| 97 ↗ | (On Diff #77125) | We never handle it this way. Add padding to stable merge as needed. |
| sys/arm64/arm64/elf32_machdep.c | ||
| 190 ↗ | (On Diff #77158) | Remove excessive (). |
| sys/arm64/arm64/trap.c | ||
| 146 ↗ | (On Diff #77158) | Perhaps change 8 to MAXREGS. BTW amd64 names the constant NARGREGS. |
| sys/arm64/linux/linux_sysvec.c | ||
| 130 ↗ | (On Diff #77158) | 8 in the msg. |
| sys/arm64/arm64/trap.c | ||
|---|---|---|
| 146 ↗ | (On Diff #77158) | So you are still calling it MAXARGS. It should be NARGREGS, to be consistent with amd64 and riscv at least. |
| sys/arm64/arm64/trap.c | ||
|---|---|---|
| 146 ↗ | (On Diff #77158) | First of all, I'm not introducing those constants; renaming them for consistency would mix two otherwise unrelated changes. Also, those two have different meaning. MAXARGS, used by all architectures other than amd64, is the max number of arguments, ie the length of the args array. NARGREGS, which is 6 instead of 8, seems to be something quite different. |
| sys/arm64/arm64/trap.c | ||
|---|---|---|
| 146 ↗ | (On Diff #77158) | Then add NARGREGS and use it tere. Intent, same as for amd64 case, is quite obvious: right now functions only support register-passed args for syscalls. If we ever start supporting 9-arg syscalls, they would need to overflow args into stack and this requires a different code there. So it is not MAXARGS as sane limit, but NARGREGS since code does not support non-reg args. |
| sys/arm64/arm64/trap.c | ||
|---|---|---|
| 146 ↗ | (On Diff #77158) | Why would I add something that's basically an x86-specific workaround to architectures that don't need such a workaround? |
Comment Actions
To explain my view a bit better:
- This patch is unrelated to MAXARGS/NARGREG/8 issue at all. It doesn't introduce any of them, and doesn't use them, except using them to replace the '8' in existing code, as you requested.
- Even currently I see think of only one "long" syscall, and it's seven args. I don't see eight-arg syscalls being a thing in foreseeable future, given performance consequences and simple aesthetics.
Comment Actions
For me it is related. It fixes handling of number of arguments. So adding some consistency to one of the architectures is quite on topic. Do it as a followup if you prefer, but IMO it is good for this diff.
| sys/arm64/arm64/trap.c | ||
|---|---|---|
| 146 ↗ | (On Diff #77158) | Where do you see anything amd64-specific there ? Arm64 and amd64 are very similar that they only have the code written to fetch syscall args from registers in frame, and any overflow into stack is ignored (causes panic) on principle that we do not need it. This is why amd64 differentiate between MAXARGS and NARGREGS, and I do not see why arm64 needs to be different. |
Comment Actions
Ah, I think I get your point now. I do have some plans to do a bit of a cleanup in that area - MAXARGS should probably be moved into a MI header, individual handlers could use some cleanups etc - but I'd prefer to leave it for later. Let's make this patch do one thing: getting rid of sa->narg.