ctf: Add definitions for CTFv3

[markj]

Summary

These are based on definitions added to binutils' libctf. Specifically:

- Type IDs are now encoded in 32 bits rather than 16, changing the
layout of ctf_type_t, ctf_array_t, ctf_member_t and ctf_lmember_t.
- Type info is encoded in 32 bits rather than 16. The type "kind" is
kept to 5 bits, and the type "vlen" (which typically encodes the
size of an instance of the type) is extended from 10 bits to 26.

The main upside is that we remove the current limit of 2^{16-1} distinct
types in the main kernel executable imposed by CTFv2. Other limits,
such as that on the number of elements in an enum, imposed by the vlen
limit, are also raised.

This change adds v2 and v3 flavours of macros and type definitions which
differ between the two versions. Compatibility is preserved for now by
having generic names refer to the v2 definitions, so, e.g., ctf_type_t
is still a v2 type.

No functional change intended.

[debdrup]

What about changes to src/share/man/man5/ctf.5?

I don't see them here or in the stack.

[emaste]

This warrants adding a copyright line perhaps

[domagoj.stolfa_gmail.com]

The code looks good to me, but it probably needs man page changes like @debdrup mentioned. I also wonder if adding a comment at the top of the header somewhere simply stating what CTFv2 and CTFv3 are and how they differ at a high level makes sense. Perhaps in one or two short sentences so that if someone unfamiliar with CTF wants to simply use the header knows what they're working with without needing to read through a fairly detailed man page? Do you have any thoughts on this?

[domagoj.stolfa_gmail.com]

sys/sys/ctf.h

152–153

Curious... Have you tried to change this to CTF_VERSION_3 to see what happens with this patch stack applied? Do things just work?

192

Since there's already a diff against these, I wonder if adding some clarification on what k, r and l are? The other macros are fairly obvious, but this one had me puzzled until I looked at the man page and found:

#define CTF_TYPE_INFO(kind, isroot, vlen) \
        (((kind) << 11) | (((isroot) ? 1 : 0) << 10) | ((vlen) & CTF_MAX_VLEN))

which is a much clearer description of the macro than this version.

196

Same as above

281

Have you tried changing this to v3? Does it break anything? In other words, could we at some point in the future change these typedefs and have things "magically" just be better?

[markj]

sys/sys/ctf.h

152–153

One of the subsequent patches in this series does exactly this. It does not work at this point in the series since libctf etc. are not yet patched.

281

At the end of this series, these typedefs are not used at all and could possibly be removed. There's no sensible way to use them; it's better to provide accessors which know the backing CTF version and can cast a void * to the versioned type. For instance, at the end of the series, libctf's ctf_lookup_by_id() returns a void * that can be passed to ctf_get_ctt_size() etc.

[markj]

• Add copyright line.
• Update man page.
• Try to make the definitions of CTF_*_TYPE_INFO more clear.

[markj]

Fix a typo.

[domagoj.stolfa_gmail.com]

I've read through the man page changes twice and couldn't find anything wrong. LGTM but maybe a read through by someone from docs would be a good idea.

[emaste]

cddl/contrib/opensolaris/lib/libctf/common/ctf.5

174

vers 0x03 now?

199

Should we make these 0x0000 0x0042 0x0036 now?
And similar elsewhere.

414

32767 or 32768?

[pauamma_gundo.com]

Keeping my changes to the minimum I feel necessary since this is contributed software:

• obvious errors
• accessibility (looking at you, ASCII art)

both everywhere, not just in the changed spots of the manual page. (I was going to stick to those for errors, but I started noticing inconsistencies and typos in identifiers elsewhere).

This needs a usefulness and relevance pass later, though. Documenting Illumos conventions is less useful than the equivalent FreeBSD conventions would be, for instance.

(And as always, my review is limited to the manual page.)

cddl/contrib/opensolaris/lib/libctf/common/ctf.5

68

preamble and header are separate sections, consistent with ASCII art below and sections numbering 7.

72

149

Please remove the whole ASCII art segment and make sure the description under it using C declarations is complete by itself. You can't reasonably expect a screenreader user to tolerate 4K of ASCII, let alone make sense of it.

153

What does the 0t prefix mean anyway?

337

367

If this change or a stacked one embiggen the preamble or the header, "thirty-six" needs to be updated.

374

388–393

Isn't that the other way? Subtract the offset of the earlier section (cth_typeoff) from the offset of the latter section (cth_stroff)?

424–425

31-bit signed integers? (Otherwise, 0x7fffffff isn't -1.)

468–471

Most significant bit is 31, not 32, since bit number start at 0. Likewise, most significant bit of vlen is 24, since bit 25 is root.

Also, should kind, root, and vlen have .Sy (or maybe .Em as used elsewhere for struct fields)?

1022

[pauamma_gundo.com]

Misclick

[markj]

In D34360#780625, @pauamma_gundo.com wrote:

This needs a usefulness and relevance pass later, though. Documenting Illumos conventions is less useful than the equivalent FreeBSD conventions would be, for instance.

I amended a couple of lingering references to illumos. Overall the page does document FreeBSD conventions. The uniquification mechanism described here is not used on FreeBSD, which is the main reason the illumos references persist.

cddl/contrib/opensolaris/lib/libctf/common/ctf.5

149

It's been helpful in the past, so I prefer to keep it. This document describes a file format that's useful to a vanishingly small number of developers who work on kernel tracing utilities.

Having worked in the past for a few years for someone who relied a screenreader and wrote some elaborate system software based on dense Infiniband specs replete with tables, diagrams, etc., please forgive me for being skeptical that deleting this diagram represents much of an accessibility improvement. Nonetheless, feel free to submit a patch which does as you request, I will merge it.

153

It's a prefix sometimes used to indicate octal representation. I'm not sure why it's used here.

199

Well, to be pedantic it should really be 0x00000000, 0x00000042, 0x00000036, no? I don't know that it's really useful to expand the sample values to the proper width when the offsets are provided as well.

367

The header is unchanged.

424–425

From the description of child containers above, there are two type identifier spaces [0x1, 0x7ffffffe] and [0x80000001, 0xfffffffe] which have a one-to-one correspondence. 0xffffffff is not a valid type identifier, so 0x7fffffff cannot be valid either.

[markj]

Man page updates.

[emaste]

cddl/contrib/opensolaris/lib/libctf/common/ctf.5

199

Well, to be pedantic it should really be 0x00000000, 0x00000042, 0x00000036, no? I don't know that it's really useful to expand the sample values to the proper width when the offsets are provided as well.

Oh, indeed, it should have been 0x0000, 0x0042 already if my comment was to hold. Fine.

[pauamma_gundo.com]

Second review when I can.

cddl/contrib/opensolaris/lib/libctf/common/ctf.5

149

They may have found it possible. That doesn't mean it was at all easy or that others may have succeeded, and I believe this kind of difficulty is the same as what Fred Brooks calls "accidental complexity" in one of the essays in The Mythical Man Month. The gist of that essay is that every change that lowers accidental complexity (as opposed to essential complexity) is worth making for that very reason.

So if no one beats me to that, I will indeed submit an accessibility patch as soon as I have time for that.

[pauamma_gundo.com]

Question so the speech friendliness change reflects reality.

cddl/contrib/opensolaris/lib/libctf/common/ctf.5

153

Looking at the 0t36+ctf_*off offsets in this diagram and the text below, for instance;

Therefore, something with an offset of 0 is at an offset of thirty-six
bytes relative to the start of the
.Nm
file.

implies those offsets aren't octal at all (which is one interpretation of "not sure why", another being "the offsets should use decimal instead"). Can you confirm that 36 is actually decimal 36, not octal 36 = decimal 30?

[markj]

cddl/contrib/opensolaris/lib/libctf/common/ctf.5

153

Yes, it looks like it should indeed be decimal. Apparently in Solaris "0t" is supposed to denote a decimal number, not an octal number, at least based on their printf man page: https://docs.oracle.com/cd/E19683-01/806-6545/api-21/index.html

So the man page is correct, but uses some rather unfamiliar notation.