Issue Type: | Bug |
---|---|
Priority: | 4 - Normal |
Status: | Resolved |
Created at: | 2019-03-06T13:41:52.849Z |
Updated at: | 2019-05-03T13:27:49.214Z |
Created by: | Former user |
---|---|
Reported by: | Former user |
Assigned to: | Former user |
Fixed: A fix for this issue is checked into the tree and tested.
(Resolution Date: 2019-04-02T10:34:45.741Z)
2019-04-11 Pete Hornberger (Release Date: 2019-04-11)
This issue is a combination of problems specific to the new CTF tools, and existing dubious behavior.
I noticed that in a Makefile that was using ctfconvert i on a .so file, I was not getting CTF out of the end silently. If I didn't specify -i, the same happened. The first bug is in the new tools:
134 ctf_convert_ftypes(elf, &type); 135 printf("got types: %d\n", type); 136 if (flags & CTF_CONVERT_F_IGNNONC) { 137 if (type == CTFCONV_SOURCE_NONE || 138 (type & CTFCONV_SOURCE_UNKNOWN)) { 139 *errp = ECTF_CONVNOCSRC; 140 return (NULL); 141 } 142 }
The check on line :136 is reversed: we should be reporting an error if -i is not specified. As it happens it always is under illumos so we probably didn't notice this.
The second issue is when we use ctfconvert in "merge" mode as we are in this case:
.../ctfconvert -L VERSION -i sysinfo_mod.so
In this mode, the -i option really doesn't make any sense as it's currently implemented. We're going to see STT_FILE
entries for sysinfo_mod.so
, consider it CTFCONV_SOURCE_UNKNOWN
, and we would fail with an error if the above bug was fixed and we didn't specify -i. That is, it's always required.
The original code made a little more sense when we were only converted one .o file at a time: there, we typically only see the one source file foo.c
But this does make me question what the semantics of this check really should be. What bug or issue are we trying to protect against with this code?
If we want to keep the non-i behaviour around, I believe we should change it such that it complains only if it finds NO C source files in the object we're CTF converting. And if so, we should evaluate where in illumos actually needs the -i flag. One example right now is uts/'s rule for firmware bins, which incorrectly calls ctfconvert.
rm pointed me to OS-6486 and OS-6488 which are somewhat related. OS-6428 may also be relevant still.
Taking a step back from what's there right now, there's a few different scenarios that are not
really implemented very well. It seems to me that we probably want the following functionality:
We should remove the usage of -i from illumos, and fix the few places that still try to ctfconvert a C++ source file.
We should make ctfmerge -t the default both for old style ctfmerge, and ctfconvert with multiple DIEs in merge mode.
We should fix the DWARF code such that it finds no DWARF or an empty DIE, it errors out.
We should provide an -i flag for ctfconvert/ctfmerge which over-rides both of these, but in general we wouldn't
want to use it. Perhaps this should still be two flags?
I picked up a few other old CTF bugs along with this. I've ended up with the following:
1. ctfmerge -t is ignored
2. ctfmerge will complain if a C-source file does not have CTF. An -m option will over-ride this check and forge on
3. ctfconvert will complain unless at least one C-source file is found. If -i is specified, it keeps quiet and does nothing in this case (and now does the right thing). In general there should be little call for -i.
4. ctfconvert will complain if a C-source file does not have DWARF (we do this by correlating STT_FILE entries against the CUs we find). This includes empty DIEs and missing DIEs.
5. All of illumos-joyent is fixed to not require either -i or -m.
As the options changes are incompatible (and the CTF tools are stricter by default), I took a look at out-of-tree users. The majority are using jlclulow's old ctftools tarball, either via eng.git or separately, so won't be directly affected. A refresh of that could deal with any issues at that time.
zfs_snapshot_tar uses /opt/onbld but builds fine with the new tools.
v8plus by default uses /opt/onbld, and is built into several components. Obviously, the smartos-live build itself is OK, which coveres node-zonename and node-kstat. Several others, such as node-kstat, sdc-manta, mola, marlin, mako, and madtom, over-ride the ctf tools with /bin/true, so won't be affected.
This leaves node-contract, which appears to be unused.
I also tested a headnode reflash with these bits without issues
I took the opportunity to remove the need for ctfconvert -i from all of illumos-joyent. The few places that were trying to do $(POST_PROCESS_O) on a non-C file (C++ or asm) have been replaced with alternatives instead. In fact, these alternatives don't do anything, but if we ever want them to, they should be in place there.
I did an exact compare against all ctf-dump-able objects in smartos-live/proto. The diffs fell into a few categories:
IOW, they looked fine.
In terms of more general testing, I threw all of build system's /opt/ against the new tools and verified we didn't behave unexpectedly. I also ran a smaller version of pkgsrc using these tools, and the only problems I hit were known ones not yet fixed in this wad such as OS-6457
I also checked MDB and dtrace were basically happy.
I also checked that a KVM VM still started up w/o problems.
I checked that mdb_v8 still seemed to work. I got ENOTSUP when trying to demangle in ::jsstack, but that isn't new in this set of changes.
illumos-kvm-cmd commit 1e9a87038f5bc0a65a32da476e324864c23fcc8d (branch master, by John Levon)
OS-7566 Clean up kvm-cmd usage of ctf tools
OS-7639 ctfconvert -i option is mis-handled
Reviewed by: Robert Mustacchi <rm@joyent.com>
Reviewed by: Patrick Mooney <patrick.mooney@joyent.com>
Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
Approved by: Jerry Jelinek <jerry.jelinek@joyent.com>
illumos-extra commit 3fa6c35b518057ce2a05093c5d969f6c6d379783 (branch master, by John Levon)
OS-7639 ctfconvert -i option is mis-handled
OS-7659 mdb_v8 build is not CTF-complete
Reviewed by: Robert Mustacchi <rm@joyent.com>
Approved by: Jerry Jelinek <jerry.jelinek@joyent.com>
illumos-joyent commit 495dfd0ac49c1326af24c839ba967d1e8264782d (branch master, by John Levon)
OS-6428 bad free in ctf_dwarf_init_die
OS-6486 ctfconvert -i never converts
OS-6488 ctfconvert should handle empty dies
OS-6505 Improve ctfconvert error messages
OS-7639 ctfconvert -i option is mis-handled
OS-7663 ctf_dwarf_convert_type() relies on un-initialized id
OS-7688 shouldn't build gcore.c as part of kmdb
Reviewed by: Robert Mustacchi <rm@joyent.com>
Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
Approved by: Robert Mustacchi <rm@joyent.com>