OS-7639: ctfconvert -i option is mis-handled

Details

Issue Type:Bug
Priority:4 - Normal
Status:Resolved
Created at:2019-03-06T13:41:52.849Z
Updated at:2019-05-03T13:27:49.214Z

People

Created by:Former user
Reported by:Former user
Assigned to:Former user

Resolution

Fixed: A fix for this issue is checked into the tree and tested.
(Resolution Date: 2019-04-02T10:34:45.741Z)

Fix Versions

2019-04-11 Pete Hornberger (Release Date: 2019-04-11)

Description

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?

Comments

Comment by Former user
Created at 2019-03-06T13:49:05.036Z

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.


Comment by Former user
Created at 2019-03-06T20:03:07.999Z

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?


Comment by Former user
Created at 2019-03-13T14:01:56.363Z

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.


Comment by Former user
Created at 2019-03-14T10:10:36.634Z
Updated at 2019-03-14T10:12:54.021Z

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.


Comment by Former user
Created at 2019-03-15T12:41:04.583Z

I also tested a headnode reflash with these bits without issues


Comment by Former user
Created at 2019-03-22T10:54:35.122Z

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.


Comment by Former user
Created at 2019-04-01T11:10:57.316Z

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.


Comment by Former user
Created at 2019-04-01T11:17:54.182Z

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.


Comment by Jira Bot
Created at 2019-04-02T10:29:03.385Z

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>


Comment by Jira Bot
Created at 2019-04-02T10:29:06.410Z

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>


Comment by Jira Bot
Created at 2019-04-02T10:29:26.711Z

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>