Hi Carlos, first, I want to say thanks for putting this together. I know there's a lot of comments below, but there's a lot of things here that are good. Pleae let me know how I can help you work through the feedback. If anything's not clear, please let me know and thanks for taking a first stab at this!

Please make sure to run cstyle as part of git pbchk. The way that these functions are put together isn't correct, it should look like:

static int
pct(ushort_t pct)


I haven't commented on this for the other occurrences in this file.

If we're calculating this as a uint_t, then why are we returning a signed value? Shouldn't it be an unsigned value?

I'm not sure that I follow all the calculations that you're doing here. I get that you're trying to conver the 16-bit integer floating point value to something else, but it's not clear where 0x7000 comes from or why you do the check on value below. Would you mind writing a comment that better explains the conversion and what you're expecting as output?

This should be replaced with a call to proc_sysname(3PROC). Please don't encode the table here.

This should be replaced with a call to proc_sysname(3PROC). Longer term we should put the name in the note, since the numbers aren't stable.

I'm not sure it's worth bothering with leading zeros. Importantly, in a 64-bit process, this can be a 64-bit value so the choice of 8 seems a bit arbitrary, as that only makes sense for a 32-bit process.

tv_sec and tv_nsec are signed values, not unsigned. Also, both of these are technically long values.

See other notes on the choice of 8 leading zeros.

I think with dmodel strings we should do the same with the libproc bits I mention below.

I think in all the cases where we're trying to add new tables, we should look to other ways to do this. For example, a lot of the various signal and fault codes are in libproc, so we should add those there so we don't duplicate these again. If that's something you'd like some help with, let me know. I'd be happy to help with that part. Though maybe we should add the errno to error string to libc.

Note, this structure should generally be constructed as:

...
case ILL_ILLOPC:
        code = "ILL_ILLOPC";
        break;
...

With all of these, I think we should be printing out the actual name of the signal we received, not just the subcode. We can get the name of the signal via proc_signame(3PROC).

cstyle nit: The indentation here (and with others) is incorrect, the continuation should be indented four spaces, you have it right with the following line, though the last two lines can be joined together.

I think it's worth a comment describing where you get these values from and how you figured them out.

So, a general note on this. Right now, this has been implemented in a way that it exists for mdb on a process, core file, live kernel, kernel crash dump, and the kernel debugger. However, the process information and having this only makes sense in the first two cases. To deal with this, the dcmd needs to be defined as part of the process target.
While there's not a great place for this, I'd probalby use the libc module as a place to implement this.

I would probably consider structuring the arguments for this in a different way. I'd probalby have ::psinfo show the summary version by default and then add a '-v' flag that describes having verbosity. I don't think it makes sense to have to specify an argument to get something.

I would probably declare these variables at the top, with everything else.

So, mdb_warn() has an interesting behavior. If you don't have a new line character at the end ('\n'), then it will append the most recent mdb error. If you were trying to get it in this particular case, then you have to keep in mind that it might have been generated by the first one, but was clobbered by the second call.

Have you tested what happens when you have a core file of a different bitness in mdb? It seems like this is relying on the sizing of these structures being the same as the underlying host type. Also, what if you're debugging a core file where the elf note is larger that the underlying structure? Wouldn't that end up clobbering memory on the stack?

I suspect that this will impact how many of the members and offsets are printed.

Minor stylistic thing, but since these are exclusive options, I'd probably suggest that you use an 'else if' for the second case.

The help message here is supposed to be a short summary of the options. However, you can supply a function that is run when someone asks for full help (via the ::help dcmd) which is where this additional explanation should be.

Unused variable.

Thanks, this looks pretty good. I only have a few inor follow ups.

Thanks for describing the usage here.

Could remove the extra blank?

The static uint_t value should be on one line and pct_value should begin the next (see all the other functions in this file for an example).

Same thing about the function declaration cstyle as with pct_value.

A couple of notes on this:

Shouldn't we be usying the buff that we used up above to print the data? Otherwise don't we still have the same problem with printing 8 digits in a 64-bit program case?

Also, there should be a space between the '10' and '?'.

It appears that we set up buff, but then don't do anything with it.

It seems we're filling in buff, but then not using it.

A couple of style notes: you don't need parens around this line and there should be a space before the '?'.

In this case, I think you actually don't want the '\n' character so that way mdb_warn can include the more detailed error reason that mdb_get_xdata() failed automatically.

I'd still recommend adding a more detailed help message if you'd like to. The help information you had before was useful, just that it should be in the usage function callback that you can define with a dcmd.

Duplicated copyright notice.

Please use SYS2STR_MAX preprocessor constant instead of arbitrary value to set the size of this array.
This line should be:
char sysname[SYS2STR_MAX];

Format string signedness mismatch, pr_uid and pr_gid are unsigned values.

This line should look like:
    mdb_printf("\tpr_uid:     %u\t\t\tpr_gid:     %u\n",

pr_sid is signed.
    mdb_printf("\tpr_pgid:    %u\t\t\tpr_sid:     %d\n",

Type of pr_size is size_t, should be casted to ulong_t:
    mdb_printf("\tpr_addr:    %s%*spr_size:    %#lx\n",
        buff, strlen(buff) > spbcols ? minspaces : (spbcols - bufflen), " ",
        (ulong_t)psinfo.pr_size);

pr_rssize also should be casted to ulong_t, and length modifiers are missing.
    mdb_printf("\tpr_rssize:  %#lx\t\tpr_wchan:   %#lx\n",
        (ulong_t)psinfo.pr_rssize, (ulong_t)psinfo.pr_lwp.pr_wchan);

Type of tv_nsec is long and tv_sec also derived from that type.
(Just to make these type-correct. Of course, I'm aware the practical limit of tv_nsec. In the case of tv_sec, maybe somebody after this date and time: Tue, 19 Jan 2038 03:14:07 GMT [without leap seconds] want to see correct value :) .)
    mdb_printf("\tpr_start:\n\t    tv_sec: %ld\t\ttv_nsec:    %ld\n",

    mdb_printf("\tpr_time:\n\t    tv_sec: %ld\t\t\ttv_nsec:    %ld\n",
        psinfo.pr_time.tv_sec, psinfo.pr_time.tv_nsec);

    mdb_printf("\tpr_ctime:\n\t    tv_sec: %ld\t\t\ttv_nsec:    %ld\n",
        psinfo.pr_ctime.tv_sec, psinfo.pr_ctime.tv_nsec);

    mdb_printf("\tpr_argc:    %d\t\t\tpr_argv:    0x%lx\n",
        psinfo.pr_argc, (ulong_t)psinfo.pr_argv);

    mdb_printf("\tpr_euid:    %u\t\t\tpr_egid:    %u\n",

        "UID:     %4u  (real user id)\n",

    mdb_printf("PGID:   %6d  (process group id)\tGID:     %4u"

    mdb_printf("SID:    %6d  (session id)\t\tEGID:    %4u"

    mdb_snprintf(buff, sizeof (buff), "%ld.%d seconds",

Please remove one from the two whitespace after the = operator.

    mdb_snprintf(buff, sizeof (buff), "%ld.%d seconds",

Typo in comment and reference to SIG2STR_MAX was removed to make this comment shorter.

(I think this reference should be in the block comment, a few lines above.)
Trailing comment separated by the rest of the line with a tab, see here: C  Style  and  Coding  Standards  for  SunOS, Section 7.3. Trailing Comments, Page 8 (the rules that cstyle (1onbld) try to check, comes from this document).
#define DMODELSTR_MAX 32    /* max. string length of data model */

This should be between proc_content2str (line 208) and proc_finistdio (should be in alphabetical order).

See the style of how systable[] initializied (one line - one value, and line indentation).

Typo and should be in in alphabetical order.
.It Sy Perror_printf Ta Sy proc_arg_grab
.It Sy proc_arg_psinfo Ta Sy proc_arg_xgrab
.It Sy proc_arg_xpsinfo Ta Sy proc_content2str
.It Sy proc_dmodelname Ta Sy proc_finistdio
.It Sy proc_fltname Ta Sy proc_fltset2str
.It Sy proc_flushstdio Ta Sy proc_get_auxv
.It Sy proc_get_cred Ta Sy proc_get_priv
.It Sy proc_get_psinfo Ta Sy proc_get_status
.It Sy proc_initstdio Ta Sy proc_lwp_in_set
.It Sy proc_lwp_range_valid Ta Sy proc_signame
.It Sy proc_sigset2str Ta Sy proc_str2content
.It Sy proc_str2flt Ta Sy proc_str2fltset
.It Sy proc_str2sig Ta Sy proc_str2sigset
.It Sy proc_str2sys Ta Sy proc_str2sysset
.It Sy proc_sysname Ta Sy proc_sysset2str
.It Sy proc_unctrl_psinfo Ta Sy proc_walk

This should be in line 1223 instead of 1236 (should be listed in alphabetical order).

A typo in the comment still persist:
's/syscalls/data models/'


(DMODELSTR_MAX is for data model strings and not for syscalls, even if these constans at this moment expands to the same value.)

You don't have to duplicate the same file again.
Instead, you have to create links to the common manual page (in this case it's proc_fltname.3proc).
Please, edit these files:

usr/src/man/man3proc/Makefile:

395 395   proc_flushstdio.3proc     := LINKSRC = proc_initstdio.3proc
396 396   proc_finistdio.3proc      := LINKSRC = proc_initstdio.3proc
397 397   
    398 + proc_dmodelname.3proc     := LINKSRC = proc_fltname.3proc
398 399   proc_signame.3proc        := LINKSRC = proc_fltname.3proc
399 400   proc_sysname.3proc        := LINKSRC = proc_fltname.3proc
400 401   
401 402   proc_sigset2str.3proc     := LINKSRC = proc_fltset2str.3proc



usr/src/pkg/manifests/system-library.man3proc.inc:

230 230   link path=usr/share/man/man3proc/proc_arg_xgrab.3proc target=proc_arg_grab.3proc
231 231   link path=usr/share/man/man3proc/proc_arg_xpsinfo.3proc target=proc_arg_psinfo.3proc
    232 + link path=usr/share/man/man3proc/proc_dmodelname.3proc target=proc_fltname.3proc
232 233   link path=usr/share/man/man3proc/proc_finistdio.3proc target=proc_initstdio.3proc
233 234   link path=usr/share/man/man3proc/proc_flushstdio.3proc target=proc_initstdio.3proc
234 235   link path=usr/share/man/man3proc/proc_free_priv.3proc target=proc_get_priv.3proc


See in the illumos Developer's Guide, Adding a new manual page section:


A single manual page may be used for multiple manuals. For example, the manual page...

Thanks, this looks pretty good. Thank you for writing the manual page for the new libproc function. I have a few minor notes, some stylistic, some correctness based. Thank you for all the work on this.

I don't think you want the '\n' on this, that way you can get the underlying mdb error printed out that occurred automatically for you.

Is there a reason this isn't joined to the previous line like others?

So, I think we have an interesting question here. The kernel represents the model usually as a uint_t (see ddi_model_convert_from(9F) for example). However, in procfs we end up representing it as a signed character. That's kind of unfortunate. I guess we should probably keep it as a char to match the procfs version. I might actually increase it to an int to match the other versions as we can always place a char argument in an int.

A couple of notes here and some questions. In new code, I would pretty much avoid strcpy and always use strlcpy even when it means doing extra work as it'll end up keeping the code safer against refactoring.
However, is there a reason that you're copying the string into the name buffer to then copy it out to the user buffer? Rather than copying it to an intermediate buffer, you can just use a pointer to index into dmdls, or just keep track of the offset and pass that to strncpy. I think you're basing this off of proc_signame(); however, that case actually is modifying the string to make sure that "SIG" is there where as here we don't have to. Instead, I would model this more off of proc_fltname().
Finally, I don't think it's correct to use PR_MODEL_UNKNOWN in the case where we don't recognize the value. I would probably treat this like proc_fltname and proc_signame and instead of using dmdls[0], do something like:

len = snprintf(buf, bufsz, "DMODEL#%d", dmodel);

Thanks for all your work on this, it looks good.

Ship It!