Page MenuHomeFreeBSD

truss: minor cleanup and pedantic warning fixes
ClosedPublic

Authored by arichardson on May 11 2021, 10:59 AM.

Details

Summary

Noticed while porting the recent truss compat32 changes to CheriBSD.

Test Plan

Fewer warnings with -Wpedantic

Diff Detail

Repository
rG 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/syscalls.c
866–877

Maybe fix this while here?

1894

I kind of think intptr_t is better here than int64_t? For CheriBSD I'd guess this would need the signed variant of ptraddr_t (hah!, another reason for sptraddr_t).

Also, I'm not quite sure if sign-extending is always correct (it is for MIPS maybe, but not i386).

1900
arichardson marked 2 inline comments as done.

Address review comments

arichardson added inline comments.
usr.bin/truss/syscalls.c
1894

The latest version uses (u)intptr_t. I've changed it to zero-extend for i386 and sign-extend otherwise.

usr.bin/truss/syscalls.c
1119

So kvaddr_t is in theory for kernel virtual addresses, not necessarily userland. However, there is a psaddr_t in <sys/procfs.h> that you could use as that is "process addresses" and is the type used in things like the libthread_db interface for user addresses. psaddr_t is uint64_t.

arichardson marked 2 inline comments as done.
  • use psaddr_t instead

I think this is fine. I'm still not fully certain of the right answer for sign extension vs not.

usr.bin/truss/syscalls.c
1721

I don't know what the right choice is for sign-extending vs not. I suspect it's actually wrong to sign-extend on just about every architecture. The question is what effective virtual address is used by the CPU when running in 32-bit mode? Perhaps for MIPS we have to sign extend, but it's probably wrong for every other platform. I would probably be tempted to invert this to sign extend only on MIPS and zero-extend everywhere else.

Hmm, indeed, truss is broken today for i386 due to the sign extending:

70294: vfork()                                   = 70295 (0x11297)
70295: <new process>
70295: freebsd32_execve("/sbin/echo",[ "echo", "I'm in the child
" ],[ "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(nu
ll)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(nu
ll)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(nu
ll)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)", "(null)" ]) ERR#2 'No such file or directory'

With sign extension disabled:

70371: <new process>
70370: vfork()                                   = 70371 (0x112e3)
70371: freebsd32_execve("/sbin/echo",[ "echo", "I'm in the child
" ],[ "ALTERNATE_EDITOR=vi", "BLOCKSIZE=K", "COLORFGBG=0;15", "COLORTERM=truecolor", "CSCOPE_EDITOR=emacsclient", "CVS_RSH=ssh", "DISPLAY=:0",
...
1728–1730
This revision is now accepted and ready to land.Jun 15 2021, 4:32 PM
usr.bin/truss/syscalls.c
1721

Sounds good I'll change it to be MIPS only.