libctf: Handle CTFv3 containers

[markj]

Summary

In general, the patch adds indirection to minimize the amount of code
that needs to know about differences between v2 and v3. Specifically,
some new ctf_get_ctt_* functions are added, and new LCTF_* macros are
added to use the underlying container's version to do the right thing.

CTF containers can have parent/child relationships, wherein a type ID in
one container refers to a type in the parent. It is permitted for the
parent and child to have different versions.

[domagoj.stolfa_gmail.com]

Overall the patch looks good to me. I've run all of this under ASAN + UBSAN (with the full patch set applied) and nothing was flagged in this code as problematic when running the DTrace test suite and the FreeBSD build. I feel like it might be a good idea to properly restructure the code around CTFv2 and CTFv3 to account for other versions in the API rather than relying on the "else" case always being v3, but aside from that LGTM. @markj: what do you think about the alignment warning/error propagation?

cddl/contrib/opensolaris/common/ctf/ctf_create.c

114

What about version 1 and future versions? Perhaps this should be a switch instead of an if/else?

157

See first comment

319

See first comment

349

Same issue

418

Same issue

459

Same issue

485

Same issue

cddl/contrib/opensolaris/common/ctf/ctf_lookup.c

206

Singling this one out for the sake of discussion, but there are a number of them from various patches: I wonder if it would be worth to add alignment checks for anything we dereference that originates from a file that contains CTF data. If everything is done correctly, these seem fine, however I feel like if a bug does crop up somewhere and for whatever reason the CTF data does not end up aligned, on some architectures we might simply crash due to an unaligned load, while others would silently work.

I suppose my question is: would be be worth to simply fail or perhaps warn on architectures that allow it both in the kernel and userspace instead of having an inconsistency?

cddl/contrib/opensolaris/common/ctf/ctf_open.c

527

Same as first issue

586

Same as first issue

cddl/contrib/opensolaris/common/ctf/ctf_types.c

43

Same as first issue

72

Same as first issue

104–105

Same as first issue

151

Same as first issue

468

Same as first issue

873

Same as first issue

[markj]

cddl/contrib/opensolaris/common/ctf/ctf_create.c

114

I had removed support for v1 as it's never been used on FreeBSD and just complicates the code.

This could be a switch statement I guess but I don't expect to introduce a CTFv4. And even with support for a hypothetical v4 this code may or may not change depending on how the format changes. I think it's ok to assume either v2 or v3 for now.

cddl/contrib/opensolaris/common/ctf/ctf_lookup.c

206

Well, the problem exists already, no? We were assuming 16-bit alignment instead of 32-bit alignment. I'm not aware of any alignment bugs having appeared in the past. I agree that it'd be good to have assertions verifying alignment but I'd be inclined to add that in a follow-up commit. (Really I would rather spend time finishing a rewrite of ctfconvert/ctfmerge to make them simpler and faster.)

I'm also not sure how concerned we are with unaligned accesses now that mips is gone? I think most if not all supported platforms can emulate unaligned accesses...?

313

Should be "two uint_t's".

[domagoj.stolfa_gmail.com]

Both seem reasonable to me. I'll aim to run some more tests with this and look it over one more time in detail by Friday and flag if I find anything, but overall looks good to me. Thanks for working on this!

cddl/contrib/opensolaris/common/ctf/ctf_create.c

114

That sounds reasonable enough to me.

cddl/contrib/opensolaris/common/ctf/ctf_lookup.c

206

The problem indeed already exists, but wanted to bring it up to get your thoughts on it since the code is being touched now. I'm fine with this being low priority and something that can be done as a follow-up commit as it's not really the point of this patch.