Page MenuHomeFreeBSD

drop libelf dependency for USDT probes
ClosedPublic

Authored by markj on Feb 2 2015, 8:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 12:50 AM
Unknown Object (File)
Sat, Nov 23, 12:50 AM
Unknown Object (File)
Sat, Nov 23, 12:50 AM
Unknown Object (File)
Sat, Nov 23, 12:50 AM
Unknown Object (File)
Sat, Nov 23, 12:49 AM
Unknown Object (File)
Sat, Nov 23, 12:28 AM
Unknown Object (File)
Fri, Nov 15, 8:05 AM
Unknown Object (File)
Wed, Nov 13, 4:05 AM
Subscribers

Details

Summary

See https://lists.freebsd.org/pipermail/freebsd-dtrace/2015-January/000342.html

I've changed libdtrace to create DOF in a section named _SUNW_dof rather than .SUNW_dof so that we can find the section address easily. drti.o now uses the startSUNW_dof and stopSUNW_dof variables (emitted by ld) for this purpose.

I've also changed sys.mk to add a new variable, DFLAGS, which is a generic flags variable for use with dtrace(1) in the build. Rationale: users might wish to add -x lazyload to DFLAGS, which tells dtrace(1) not to link drti.o into the output executable/library. In this case, dtrace(1) discovers and loads the DOF when it attaches to the target process, rather than having the process load the DOF itself during startup. I plan to commit the DFLAGS change in a separate commit than the rest of this revision.

N.B. lazyload mode currently doesn't work on FreeBSD; it'll be fixed in a separate change.

Test Plan

DTrace test suite passes, ruby 2.1 builds and creates probes during startup.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

markj retitled this revision from to drop libelf dependency for USDT probes.
markj updated this object.
markj edited the test plan for this revision. (Show Details)
markj added reviewers: emaste, gnn, rpaulo, DTrace.

Overall I like it, and using _sunw_DOF is nice since I could see a change to replace a leading . with _ in creating the __start_<foo> symbol being accepted upstream. I'm happy with the ELF parts but didn't look into the DFLAGS in detail yet.

We should try to get changes like shp->sh_addralign = DTRACE_DOFSCN_ALIGN; incorporated into illumos.

gnn edited edge metadata.
This revision is now accepted and ready to land.Feb 4 2015, 12:27 AM
rpaulo edited edge metadata.
rpaulo added inline comments.
cddl/contrib/opensolaris/lib/libdtrace/common/drti.c
150–153

I think the FreeBSD-specific code should be under #ifdef FreeBSD.

markj edited edge metadata.
  • ifndef illumos -> ifdef FreeBSD
This revision now requires review to proceed.Feb 5 2015, 3:59 AM
jhibbits added a reviewer: jhibbits.
jhibbits added a subscriber: jhibbits.

Ship it!

This revision is now accepted and ready to land.Feb 5 2015, 7:23 AM
rpaulo edited edge metadata.
rpaulo added inline comments.
cddl/contrib/opensolaris/lib/libdtrace/common/drti.c
211

Missed this one.

markj edited edge metadata.

This revision is a substantial update of the previous one.

It turns out that we don't need the linker to emit special symbols for us:
dt_link.c already goes through the trouble of putting the SUNW_dof symbol at
the beginning of the .SUNW_dof section, so we can find the address of the DOF
section simply by taking the address of
SUNW_dof. No linker magic needed. In
fact, I'm now totally unclear as to why drti.o needed to be modified to use
libelf in the first place. For some reason I was convinced that we were missing
a feature of the Sun linker needed to make this trick work.

So this revision mostly just restores upstream code in drti.o without having
to change the DOF section name. The major complication is the case where
multiple dtrace-generated object files are linked into the same object. Since
the SUNW_dof symbol is global (it has to be, since it's referenced in drti.o),
linking multiple object files causes a problem, since they all contain
definitions of
SUNW_dof.

I fixed this by running objcopy --localize-hidden on the generated object file,
and by giving the SUNW_dof symbol hidden visibility (which it should have
anyway). So now each generated object file has its own local
SUNW_dof symbol
for its corresponding DOF section, so we no longer need to loop over the DOF
sections in drti.o (we added that because Node.js uses multiple object files).
Most of the new code is needed to run objcopy in dtrace -G, which is very
straightforward.

Testing: all usdt tests pass, ruby and postgres successfully create probes.

This revision now requires review to proceed.Feb 17 2015, 3:39 AM
rpaulo edited edge metadata.

The only reason it needed libelf was because I thought it required our linker to patch the symbols when linking. It appears that's not the case, which is good. Regarding the code, you keep writing "#ifndef illumos", so I suspect that's your preferred version and I won't argue.

This revision is now accepted and ready to land.Feb 17 2015, 7:21 AM

Looks much nicer.

Are you interested in having a look at elftoolchain's elfcopy and implementing --localize-hidden there? It supports --localize-symbol already.

share/mk/sys.mk
75–78

This looks like Phabricator confusion -- not part of your change

Just FYI: I found a way

In D1757#25, @emaste wrote:

Looks much nicer.

Are you interested in having a look at elftoolchain's elfcopy and implementing --localize-hidden there? It supports --localize-symbol already.

Sure, I'll do that.

share/mk/sys.mk
75–78

No, I should have omitted this from the review, but it was intentional - it bugged me that CTFFLAGS wasn't grouped with CTFCONVERT and CTFMERGE.

In D1757#22, @rpaulo wrote:

The only reason it needed libelf was because I thought it required our linker to patch the symbols when linking. It appears that's not the case, which is good. Regarding the code, you keep writing "#ifndef illumos", so I suspect that's your preferred version and I won't argue.

No, #ifdef FreeBSD is better for the objcopy bits I think. I'll fix it.

markj updated this revision to Diff 3831.

Closed by commit rS278934 (authored by @markj).