This is an archive of the discontinued LLVM Phabricator instance.

Patch to Attach pid successfully from different dir
ClosedPublic

Authored by emaste on Apr 19 2017, 10:33 PM.

Details

Summary

While using lldb we found the small bug with attaching to the running process.
"attach" works fine when we run the lldb on the same dir where we ran the executable but not if the dir changes.

cd lldb_temp/
./test &

[2] 39044

cd ../
/b/vignesh/lldb_daily_build/20170419_bkup/binaries/bin/lldb

(lldb) attach 39044
error: attach failed: unable to find executable for './test'

I see the lldb using the relative path instead of absolute path of the exe.
Modified the way sysctl gets the exe name to get the absolute path.

Diff Detail

Repository
rL LLVM

Event Timeline

vbalu created this revision.Apr 19 2017, 10:33 PM
labath added a subscriber: lldb-commits.
emaste edited edge metadata.Apr 21 2017, 7:01 PM

Sorry I've been away from LLDB for a bit, I will take a look at this soon.

emaste commandeered this revision.Apr 23 2017, 5:41 PM
emaste updated this revision to Diff 96334.
emaste edited reviewers, added: vbalu; removed: emaste.

Slightly simpler approach after early returns added in rL301100

vbalu accepted this revision.Apr 23 2017, 10:04 PM

Fine. This will work. Thanks for the update.

This revision is now accepted and ready to land.Apr 23 2017, 10:04 PM

I have plan to revisit corresponding files in NetBSD and switch from kvm(3) to sysctl(3). But this is lower priority than Process Plugin right now.

I have plan to revisit corresponding files in NetBSD and switch from kvm(3) to sysctl(3). But this is lower priority than Process Plugin right now.

Thanks for the note. I wasn't aware that you had that planned, but I incorporated the improvements you made in NetBSD's Host.cpp (early returns etc.) in order to reduce diffs and facilitate shared effort on these bits at some point.

I have plan to revisit corresponding files in NetBSD and switch from kvm(3) to sysctl(3). But this is lower priority than Process Plugin right now.

Thanks for the note. I wasn't aware that you had that planned, but I incorporated the improvements you made in NetBSD's Host.cpp (early returns etc.) in order to reduce diffs and facilitate shared effort on these bits at some point.

At the moment at least attach to pid isn't functional on NetBSD.

However good that the NetBSD code helped to improve FreeBSD.

emaste added a subscriber: dim.May 19 2017, 5:55 AM
dim added a comment.May 19 2017, 6:05 AM

As I found out in rL303015, the KERN_PROC_PATHNAME has one drawback: if an executable file has multiple hard links, you will get just one of its filenames as the result. Since that filename is more or less randomly chosen, it does *not* have to correspond to the actual argv[0] the executable was invoked with. If that does not matter, this approach is fine, though.

If we end up with hard links I think we should change them to symbolic ones. This is a common policy in some open source distributions.

In D32271#759361, @dim wrote:

As I found out in rL303015, the KERN_PROC_PATHNAME has one drawback: if an executable file has multiple hard links, you will get just one of its filenames as the result. Since that filename is more or less randomly chosen, it does *not* have to correspond to the actual argv[0] the executable was invoked with. If that does not matter, this approach is fine, though.

Thanks @dim, it's because of rL303015 that I added you to CC.

Thinking about this further, I think that in this (LLDB) case it's acceptable (even if it might prove slightly surprising). If we attach via -p <pid> this code is used to find the corresponding binary, and even if we open the file by the "wrong" name we still have the correct content.

vbalu requested changes to this revision.Aug 28 2017, 11:48 PM
if (::sysctl(mib, 4, pathname, &len, NULL, 0) == 0)

"len" is not declared. Please change to "pathname_len".

This revision now requires changes to proceed.Aug 28 2017, 11:48 PM
emaste updated this revision to Diff 113664.Sep 2 2017, 4:30 PM
emaste edited edge metadata.

correct variable

This revision was automatically updated to reflect the committed changes.