Page MenuHomeFreeBSD

truss: split counting of syscalls and syscall calling convention
ClosedPublic

Authored by arichardson on Dec 16 2020, 6:30 PM.

Details

Summary

This change is a refactoring cleanup to improve support for compat32
syscalls (and compat64 on CHERI systems). Each process ABI now has it's
own struct sycall instead of using one global list. The list of all
syscalls is replaced with a list of seen syscalls. Looking up the syscall
argument passing convention now interates over the fixed-size array instead
of using a link-list that's populated on startup so we no longer need the
init_syscall() function.
The actual functional changes are in D27625.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

usr.bin/truss/syscall.h
225

Personally not a huge fan of syscall_cconv as a name, maybe syscall_decode to match the fact that these come from the decoded_syscalls array?

226

I don't think this really belongs in here. You only need it for lookups, so why not make the table an array of struct { char *; struct syscall_cconv; }?

231

This belongs in struct syscall IMO (and I don't think you need it here).

usr.bin/truss/syscalls.c
1027

Or just leave it as void as it just returns sc?

1033

Hm, might be a little cleaner if there were a separate fixup_syscall, then this could just take a const struct syscall *. It's not obvious that adding a syscall modifies it.

3007

Why does this new argument exist? The function only gets called once AFAICT with the value true. It'd probably just be nicer to split out a separate function, or just forego it entirely and note that it's fine because we're about to return from main anyway.

arichardson added inline comments.
usr.bin/truss/syscall.h
225

syscall_decode sounds good to me.

226

I wanted to avoid having to change all entries in the table.

usr.bin/truss/syscalls.c
1027

I originally had this function doing the allocation, now the return value is pointless.

1033

I could move the fixup to the caller, but then I'd need to do it twice. Also sc can't be const anyway due to the assignment.

  • address jrtc27 review comments
  • 32-bit fix after latest changes
usr.bin/truss/syscall.h
226

Hm. The reason I don't like it is that it's not a useful field to have for syscalls that don't have special decoding, since it's the ABI-prefixed name (with D27625), not the unprefixed name like some like you'll get for known freebsd32 syscalls, and so if we can instead limit it to only where it's needed (the lookup) then it's consistent and there's no risk of being confused and using the wrong thing. Which also means you can rename syscall's kernel_name back to name, as that's not the best name for it either currently.

usr.bin/truss/syscalls.c
1033

Right you need to deal with the slist in it... I'd still prefer to not have a hidden fixup though.

1089–1091

Don't need the variable just check name == NULL.

arichardson added inline comments.
usr.bin/truss/syscall.h
226

I agree it's not useful, but I don't want to edit the entire table. I think changing the table should probably be a separate commit that sorts it to allow use of bsearch.

usr.bin/truss/syscalls.c
1033

The fixup can only be done once we know the ABI, so I think this is probably the right place to do it.

usr.bin/truss/syscall.h
226

It'd probably be best to decouple sorting from restructuring, that way it's clear they're NFCs (well, at least git will tell you the sorting was just moving lines). Also neither are really dependent on the other except for the fact that they conflict if done independently. Would you consider leaving kernel_name as name so that a follow-up doesn't need to revert that once the restructuring has happened?

usr.bin/truss/syscalls.c
1033

Well just like you have void add_syscall(struct procabi *abi, u_int number, struct syscall *sc) you'd have void fixup_syscall(struct procabi *abi, struct syscall *sc).

usr.bin/truss/syscall.h
241

Maybe call this decode instead?

usr.bin/truss/syscalls.c
1089

Maybe we can just leave this NULL in this case rather than filling it with a potentially-wrong value given using this field is an error?

usr.bin/truss/syscalls.c
1033

Yes that would also work, but since both will done at the same time we might as well merge them?

arichardson marked 2 inline comments as done.

rename variable. Don't bother initializing name for unknown syscalls

usr.bin/truss/setup.c
483

I don't understand this change

usr.bin/truss/setup.c
483

Yeah I guess that doesn't need to be upstreamed and only makes sense if args is not a long...

arichardson marked an inline comment as done.

revert unnecessary name->display_name and unnecessary debug output change.

usr.bin/truss/main.c
205

We're not freeing on error paths, and we're about to exit here. I don't think we need to bother with this.

usr.bin/truss/syscalls.c
1032
usr.bin/truss/main.c
205

Sure, happy to drop this. I'm not sure if any allocations have happened though if we hit the error return above.

arichardson marked 2 inline comments as done.
  • fix typo
  • free on exit instead of explicitly
usr.bin/truss/syscall.h
227

It's not the number of return values, but the types. 0 means "doesn't return" (for exit()), 1 means "long" (or int), and 2 means off_t. It could possibly stand to be an enum instead of an int, but not in this commit.

usr.bin/truss/syscalls.c
1069

Why does this have to be allocated? There's no reason to ever free() these things vs just letting exit() reclaim them that I can think of?

arichardson added inline comments.
usr.bin/truss/syscall.h
227

I just moved this line from below, should I fix the comment too?

usr.bin/truss/syscalls.c
1069

Good catch, it was being free()'d unconditionally in a previous version of the patch, so it had to match asprintf(). This is no longer the case so I'll drop the strdup().

  • Avoid strdup() since we don't free() memory
usr.bin/truss/syscalls.c
1071

This comment probably wants to cover the asprintf vs assigning as non-owned string literal case above too?

usr.bin/truss/syscall.h
227

I just moved this line from below, should I fix the comment too?

Oh cute. Might as well update the comment, yes.

usr.bin/truss/syscalls.c
91

Note that it is mostly sorted, it's just sorted by ABI first, then sorted alphabetically within the ABI.

1066–1079
jhb added inline comments.
usr.bin/truss/syscalls.c
1066–1079

I think you probably want this still as style(9) otherwise wants a blank line before the comment, and moving the comment up below the existing blank line seems simpler.

This revision is now accepted and ready to land.Mar 1 2021, 6:35 PM
usr.bin/truss/syscalls.c
1066–1079

Sorry, missed that suggestion when updating the review. Will fix before committing.