Page MenuHomeFreeBSD

truss: split counting of syscalls and syscall calling convention
ClosedPublic

Authored by arichardson on Dec 16 2020, 6:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 3:06 PM
Unknown Object (File)
Fri, Mar 22, 3:06 PM
Unknown Object (File)
Fri, Mar 22, 3:06 PM
Unknown Object (File)
Fri, Mar 22, 3:06 PM
Unknown Object (File)
Fri, Mar 22, 3:06 PM
Unknown Object (File)
Fri, Mar 22, 3:06 PM
Unknown Object (File)
Fri, Mar 22, 3:05 PM
Unknown Object (File)
Fri, Mar 22, 3:05 PM
Subscribers

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
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 35453
Build 32363: arc lint + arc unit

Event Timeline

usr.bin/truss/syscall.h
224

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?

225

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; }?

230

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

usr.bin/truss/syscalls.c
1023

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

1030

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.

2991

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
224

syscall_decode sounds good to me.

225

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

usr.bin/truss/syscalls.c
1023

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

1030

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
225

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
1030

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

1086

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

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

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
1030

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
225

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
1030

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
234

Maybe call this decode instead?

usr.bin/truss/syscalls.c
1087

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
1030

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
484

I don't understand this change

usr.bin/truss/setup.c
484

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
206

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
1029
usr.bin/truss/main.c
206

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
226

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
1072

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
226

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

usr.bin/truss/syscalls.c
1072

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
1074

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

usr.bin/truss/syscall.h
226

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
92

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

1068–1076
jhb added inline comments.
usr.bin/truss/syscalls.c
1068–1076

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
1068–1076

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