Page MenuHomeFreeBSD

truss: minor cleanup and pedantic warning fixes
ClosedPublic

Authored by arichardson on May 11 2021, 10:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 17 2024, 2:11 PM
Unknown Object (File)
Dec 12 2023, 12:10 AM
Unknown Object (File)
Nov 27 2023, 10:39 AM
Unknown Object (File)
Nov 26 2023, 3:21 AM
Unknown Object (File)
Nov 23 2023, 5:20 PM
Unknown Object (File)
Sep 17 2023, 5:39 AM
Unknown Object (File)
Sep 6 2023, 8:17 AM
Unknown Object (File)
Sep 5 2023, 6:45 PM
Subscribers

Details

Summary

Noticed while porting the recent truss compat32 changes to CheriBSD.

Test Plan

Fewer warnings with -Wpedantic

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 39914
Build 36803: arc lint + arc unit

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).

1901
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
1722

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",
...
1729–1731
This revision is now accepted and ready to land.Jun 15 2021, 4:32 PM
usr.bin/truss/syscalls.c
1722

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