Page MenuHomeFreeBSD

Add RISC-V support for truss(1)
ClosedPublic

Authored by lwhsu on Jan 19 2017, 6:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 12, 8:01 AM
Unknown Object (File)
Fri, Nov 29, 11:09 AM
Unknown Object (File)
Wed, Nov 27, 2:35 PM
Unknown Object (File)
Tue, Nov 26, 4:51 AM
Unknown Object (File)
Nov 23 2024, 1:41 AM
Unknown Object (File)
Nov 20 2024, 11:34 AM
Unknown Object (File)
Nov 4 2024, 6:16 PM
Unknown Object (File)
Nov 2 2024, 6:38 AM
Subscribers

Details

Summary

Add RISC-V support for truss(1)

While here, extract NARGREG as a definition.

Test Plan

I've tested this with several commands under /bin, compared with outputs from
amd64 and all seems reasonable. Is there any further tests suggested?

Diff Detail

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

Event Timeline

lwhsu retitled this revision from to Add RISC-V support for truss(1).
lwhsu updated this object.
lwhsu edited the test plan for this revision. (Show Details)
lwhsu added reviewers: br, jhb.
usr.bin/truss/riscv64-freebsd.c
67

I think we have 8 argument registers in total: a[0] to a[7]

usr.bin/truss/riscv64-freebsd.c
67

That's right, which is copied in line 74-75.
I was referencing https://svnweb.freebsd.org/base/head/sys/riscv/riscv/trap.c?revision=308731&view=markup#l102 and I guess this line should be:

syscall_num = regs.t[0];

right?

Correct syscall_num source.

Replace one more '8' to NARGREG

usr.bin/truss/riscv64-freebsd.c
67

yes according to lib/libc/riscv/SYS.h we have set syscall number to t0

usr.bin/truss/riscv64-freebsd.c
50

sort lines by length:

struct current_syscall *cs;
u_int i, reg, syscall_num;
struct reg regs;
lwpid_t tid;

77

leave empty line before return

95

leave empty line before return

usr.bin/truss/riscv64-freebsd.c
50

What does the length here mean? line length?
Although it's not kernel code, but I think it's ok to follow style(9):
"When declaring variables in functions declare them sorted by size, then in alphabetical order;"
Is there any standard I missed?

Style:

  • dash after the star in copyright header
  • leave empty line before return
usr.bin/truss/riscv64-freebsd.c
50

so I think ordering by size means "order it by line length". Am I wrong ?

usr.bin/truss/riscv64-freebsd.c
50

I guess it means the size of variables resident in the memory. The example after that section in style(9) looks like that. Could you check it?

Aside from the minor style nits, this looks fine to me.

usr.bin/truss/riscv64-freebsd.c
50

Yes, it means by the size of the variable in memory. I think your best bet here is to just match whatever style is used in the other MD truss backends. (I think they don't have the blank line before return, etc.) They are generally consistent with each other so I think it's best to match those.

usr.bin/truss/riscv64-freebsd.c
50

@br , how do you think if we keep this order and remove the blank lines before return? I referenced what is in aarch64-freebsd.c .

usr.bin/truss/riscv64-freebsd.c
50

OK lets keep that order used in other files.

Lets add empty line before return to all the arches by the same or separate commit?

usr.bin/truss/riscv64-freebsd.c
50

I don't have a strong opinion of this, but if we are going to add empty lines, probably should do in a separate commit since it is a white space change.

BTW, I did not find "an empty line before return", there is only "values in return statements should be enclosed in parentheses." so I don't know if it's good to do such change on all other files.

@jhb , how do you think?

Aside from the minor style nits, this looks fine to me.

(Sorry last comment was a misfire due to Safari confusion)

usr.bin/truss/riscv64-freebsd.c
50

For now I would say just match the existing files. I'm not sure if I've seen a consistent preference within FreeBSD for or against blank lines before return().

lwhsu edited edge metadata.

remove empty line before return

br edited edge metadata.
This revision is now accepted and ready to land.Jan 23 2017, 9:51 AM
This revision was automatically updated to reflect the committed changes.