Missing comma in man page.

Address @markj's comments.

In D41745#983815, @markj wrote:

If I compile usr.sbin/dtrace with this patch applied, I get:

In file included from /home/markj/src/freebsd/cddl/contrib/opensolaris/cmd/dtrace/dtrace.c:59:
/usr/include/libxo/xo.h:31:9: error: 'NORETURN' macro redefined [-Werror,-Wmacro-redefined]
   31 | #define NORETURN __dead2
      |         ^
/home/markj/src/freebsd/sys/contrib/openzfs/lib/libspl/include/assert.h:44:9: note: previous definition is here
   44 | #define NORETURN        __attribute__((__noreturn__))
      |         ^
1 error generated.
*** Error code 1

Stop.
make: stopped in /home/markj/src/freebsd/cddl/usr.sbin/dtrace

I tend to think that xo.h should undefine those symbols (NORETURN and PRINTFLIKE) at the end, or give them a XO_ namespace prefix. I'll fix it up locally and keep testing, but we need some solution for this before committing. (Or am I doing something wrong?)

Overall I think this is in good shape and can land soon. Thank you for writing test cases and thorough documentation. My inline comments are just nitpicking for the man page.

If I compile usr.sbin/dtrace with this patch applied, I get:

Update the diff to address a few output alignment issues when not using structured output via xo_emit(). Without the added xo_flush() calls, the output from xo_emit() would be printed after all the other output is printed via fprintf(). Also added a comment explaining why xo_flush() is called in those situations.

That's pretty terrible, given that all flavors of xo_emit* turn into the same codepath (xo_do_emit). Can you isolate this into something I can debug, or send me your current patch (phil@freebsd.org) and I'll see if I can do it?

$ sudo dtrace -n 'BEGIN { mod(0xffffffff8261a000); exit(0); }'
dtrace: description 'BEGIN ' matched 1 probe
CPU ID FUNCTION:NAME

6      1                           :BEGIN

opensolaris.ko

@phil I've tried to do a basic implementation using xo_create_to_file and then implementing dt_emit() by redirecting buffered output and sprintf output to the regular printf output (it becomes dt_vprintf since i have a va_list instead of variadic arguments). However, I've noticed that if I do regular text output using

Update the diff. This diff should address the following:

Thanks for the detailed reviews @markj and @phil! The libxo clarifications have been very helpful, much appreciated. I've added a few comments in places that can be discussed independently and will aim to update the diff addressing the comments sometime this week.

In D41745#951971, @domagoj.stolfa_gmail.com wrote:

In D41745#951858, @stephane.rochoy_stormshield.eu wrote:

I suggest to think about adding --libxo foo as an alias to -x oformat=foo to make things more inline with what is actually done for, e.g., netstat(1).

Thanks for the feedback! I don't take a strong view on whether or not xo_parse_args() should be called at the start, though it might require some finessing to make it work happily with libdtrace because xo_parse_args() sets a number of flags for libxo that I don't with -x oformat (such as colored output in some cases). Another thing worth pondering is that this is also an argument to libdtrace rather than dtrace(1), so one would inevitably need to still call dtrace_setopt(). Now, however libxo configuration would be performed twice: once in dtrace(1), and once in libdtrace. dtrace(1) itself would explicitly need to include libxo and link against it, but this isn't a big issue IMO, it just "feels" cleaner to me for it to only link against libdtrace and for libdtrace to provide all the libxo handling and configuration and for any consumer code (including dtrace(1)) to simply request it.

However I do agree with the general point that there is an inconsistency with the rest of the base system when it comes to libxo integration (though it's inconsistent in a fair bit of other ways too). One way to work around the issue without xo_parse_args() would be perhaps to add a - flag and manually parse out the -libxo ... that follows? I'm not sure what the best approach would be that doesn't get slightly messy.

In D41745#951971, @domagoj.stolfa_gmail.com wrote:

In D41745#951858, @stephane.rochoy_stormshield.eu wrote:

I suggest to think about adding --libxo foo as an alias to -x oformat=foo to make things more inline with what is actually done for, e.g., netstat(1).

Thanks for the feedback! I don't take a strong view on whether or not xo_parse_args() should be called at the start, though it might require some finessing to make it work happily with libdtrace because xo_parse_args() sets a number of flags for libxo that I don't with -x oformat (such as colored output in some cases).

Fix off->val type causing a build failure.

Fix struct indentation.

Add missing information to the man page and fix some bits in it (e.g. using .Fn for action names).

Updated the diff with documentation in the dtrace(1) man page, as well as a bug fix when it comes to naming aggregations. Namely, min, max, sum and count were all called count in the final output due to missing checks. These are now addressed.

Please address the comments from markj@ but otherwise this looks good, and more extensive than what we came up with on CADETS.

In D41745#951858, @stephane.rochoy_stormshield.eu wrote:

I suggest to think about adding --libxo foo as an alias to -x oformat=foo to make things more inline with what is actually done for, e.g., netstat(1).

I suggest to think about adding --libxo foo as an alias to -x oformat=foo to make things more inline with what is actually done for, e.g., netstat(1).

Attempt #2 at addressing @markj's comments... Forgot one gettimeofday

Address some comments by @markj. The man page comment is still true, as I will be updating that when all the documentation comes in.

I don't see a reason to avoid adding it to dtrace.1?

Perhaps its own man page is warranted in order to avoid polluting dtrace(1)?

Fix return offset handling.

Approved.

Address comments, fix stuff that was also fixed in riscv.

Update