Support a wider history counter in pmcstat gmon output

[jtl]

Summary

pmcstat(8) contains an option to output sampling data in a gmon format
compatible with gprof(1). Currently, it uses the default histcounter,
which is an "unsigned short". With large sets of sampling data, it
is possible to overflow the maximum value provided by an unsigned
short.

This change adds the -e argument to pmcstat. If -e and -g are both
specified, pmcstat will use a histcounter type of uint64_t.

Sponsored by: Juniper Networks
Approved by: gnn (mentor) [Assuming he does...]
MFC after: 1 month

Test Plan

It compiles.

The usage output appears correct.

The man page output appears properly formatted (although, someone else
will probably be the final judge of that :-) ).

This sequence produces identical files:

root@:/usr/obj/usr/src/usr.sbin/pmcstat # pmcstat -D /var/tmp/gprof-new -R /var/tmp/sample.out -e -g
CONVERSION STATISTICS:
#exec/elf 2
#samples/total 4542
root@:/usr/obj/usr/src/usr.sbin/pmcstat # pmcstat -D /var/tmp/gprof-old -R /var/tmp/sample.out -g
CONVERSION STATISTICS:
#exec/elf 2
#samples/total 4542
root@:/usr/src # cd /var/tmp
root@:/var/tmp # gprof /boot/kernel/kernel gprof-new/CLOCK.PROF/kernel.gmon > gprof-new/gprof.out
time is in ticks, not seconds
root@:/var/tmp # gprof /boot/kernel/kernel gprof-old/CLOCK.PROF/kernel.gmon > gprof-old/gprof.out
time is in ticks, not seconds
root@:/var/tmp # diff gprof-???/gprof.out
root@:/var/tmp #

Running hd(1) on the gmon files shows the expected types in the gmon
header.

Finally, I see no complaints about overflows when presenting pmcstat
with a large data set and using the -e option.

[bjk]

Please update the date in the manual page, i.e., the .Dd entry.

[jhb]

Shame we can't just do this all the time.

[jtl]

In D4151#87272, @jhb wrote:

Shame we can't just do this all the time.

Agreed. Because the man page only promises compatibility with gprof(1), we could actually change the default and make a flag for backwards compatibility. But, we don't know who relies on the current behavior and would be impacted by the change.

[jtl]

Updated man page date, per @bjk's comment.

[bjk]

Thanks for the man page update; unfortunately I noticed something else since then.

usr.sbin/pmcstat/pmcpl_gprof.c

130 ↗

(On Diff #10168)

Should this be - 1 or - 2?
It looks that the 'if' branch is for an unsigned (WIDE)HISTCOUNTER and the 'else' branch should thus handle the case of a signed, but shifting a 32-bit signed integer '1' left by 31 bits will set the sign bit, which seems to be not the desired value.

[jtl]

usr.sbin/pmcstat/pmcpl_gprof.c

130 ↗

(On Diff #10168)

It is true that this sets the sign bit, but note that we use the inverse of this value. So, this should produce the maximum unsigned value, at least on platforms that only use the highest-order bit for a sign (which should be true for the only platforms that currently use pmcstat).

FWIW, I did test this and see that it produced the correct max value for both int64_t and uint64_t on an amd64 processor.

[jtl]

usr.sbin/pmcstat/pmcpl_gprof.c

130 ↗

(On Diff #10168)

this should produce the maximum unsigned value

Sorry, I didn't word that well. This should produce the maximum (positive) value.

[bjk]

usr.sbin/pmcstat/pmcpl_gprof.c

130 ↗

(On Diff #10168)

Sadly, even if it appears to function correctly in testing, I believe it still involves undefined behavior in the C language. See section 6.5.7 of n1256.pdf, paragraph 4:"If E1 has a signed type and a nonnegative value, and E1 * 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined". 1 * 2^31 is not representable in a twos-complement 32-bit signed integer, for example.

[jtl]

usr.sbin/pmcstat/pmcpl_gprof.c

130 ↗

(On Diff #10168)

1 * 2^31 is not representable in a twos-complement 32-bit signed integer, for example.

Ah, and I see that is also listed in J.2 as a "portability issue". Thanks for pointing that out!

If you have a suggestion to improve the automatic type detection, let me know. Or, I can hard-code the types for now. The automatic type detection is a "nice to have" feature intended to make it easier to maintain the code. It isn't essential for this change.

[wblock]

usr.sbin/pmcstat/pmcstat.8

266 ↗

(On Diff #10168)

Files are produced in a format compatible with
.Fx Ns 's

There might be a better way to do that. Is it necessary to specify that this is the FreeBSD gprof program?

Files are produced in a format compatible with
.Xr gprof 1 .

268 ↗

(On Diff #10168)

Avoid semicolons, it is almost always just splicing two sentences together.
Use "may" for permission, "might" for possibility.

However, other tools that cannot fully parse a BSD-style gprof might have trouble parsing these files.

[jtl]

usr.sbin/pmcstat/pmcstat.8

266 ↗

(On Diff #10168)

Is it necessary to specify that this is the FreeBSD gprof program?

I'm not sure. Is it? :-)

Linux gprof (for example) will not be able to parse these files, because it does not fully parse a BSD-style gmon header. (However, without this option, Linux gprof can read the file that is produced.) But, can we assume that readers will understand "gprof(1)" always refers to FreeBSD gprof?

[bjk]

usr.sbin/pmcstat/pmcpl_gprof.c

130 ↗

(On Diff #10168)

I don't know of a good way to do this, no. It may be easiest to just define a [WIDE]HISTCOUNTER_MAX macro at the typedef declaration point.

usr.sbin/pmcstat/pmcstat.8

266 ↗

(On Diff #10168)

It might be better to sidestep the issue and just say that "This will produce files that may not be compatible with gprof parsers that do not fully parse BSD-style gprof headers." possibly including "such as GNU gprof" (if true).

[jtl]

Incorporate feedback from @wblock and @bjk.

• Remove the automatic type detection.
• Try to incorporate a synthesis of the suggestions for man page rewording.

[bjk]

Thanks for being patient as we wordsmith things.

The wide counter type stuff looks good, now.

usr.sbin/pmcstat/pmcstat.8

265 ↗

(On Diff #10272)

"the" before "gprof", please.

266 ↗

(On Diff #10272)

And s/Files/These files/, as well.

[jtl]

Implement wording changes to the man page suggested by @bjk.

[jtl]

In D4151#88244, @bjk wrote:

Thanks for being patient as we wordsmith things.

No worries. I implemented the changes you requested.

[bjk]

Thanks!