This is an archive of the discontinued LLVM Phabricator instance.

Implement vAttachOrWait
ClosedPublic

Authored by augusto2112 on Jan 14 2021, 4:36 AM.

Details

Summary

Implements the required functions on gdb-remote so the '--include-existing' flag of process attach works correctly on Linux.

Diff Detail

Event Timeline

augusto2112 requested review of this revision.Jan 14 2021, 4:36 AM
augusto2112 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 4:36 AM
augusto2112 added inline comments.Jan 14 2021, 4:44 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
372

Is there an easier way to format a StringRef such that if I have a StringRef "foo" I get a new StringRef "/foo"?

377

There was an oversight on my last patch, where we could attach to a different process if it ended with the name of the process we want to attach to. For example if we want to attach to "bar", it could also attach to "foobar". I changed it so we exclude processes whose names aren't a perfect match ("bar") or end with a slash + process name ("path/to/bar"). Is this enough, or could there be other false positives?

3314

Handle_vAttachOrWait and Handle_vAttachWait are pretty much the same. The only differences are the argument we pass to packet.SetFilePos and to AttachWaitProcess. Since they're so small I decided to keep them as separate functions, but I could make one common function for both.

labath added inline comments.Jan 14 2021, 5:45 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
372

Not exactly, since StringRef always needs a persistent materialized string to refer to. But in the cases you don't have one handy, you can use a std::string. You could create it as ("/" + foo).str(), but per the comment below, I don't think this is the right solution.

377

That is odd, because GetNameAsStringRef should already strip the path:

llvm::StringRef ProcessInfo::GetNameAsStringRef() const {
  return m_executable.GetFilename().GetStringRef();
}

If that's not working for you, then I guess you need to figure out why is that function failing.

augusto2112 added inline comments.Jan 14 2021, 6:07 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
377

Oh I see. I just assumed that it could potentially return the full path, my bad. I believe simply comparing info_name != process_name will be sufficient then.

clayborg added inline comments.Jan 14 2021, 12:33 PM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
359–366

Reverse this logic and switch the if/else contents?

371

Do we need to get the correct directory delimiter for the current platform here? This will work for everything except windows. Just wanted to bring this up in case we do.

Addresses comments, includes tests.

augusto2112 marked 4 inline comments as done.Jan 14 2021, 2:28 PM
augusto2112 added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
371

Turns out that was unnecessary since match_infos are compared by file name and not full path.

clayborg accepted this revision.Jan 15 2021, 11:29 AM

LGTM. Pavel?

This revision is now accepted and ready to land.Jan 15 2021, 11:29 AM
labath accepted this revision.Jan 18 2021, 10:58 PM

@labath, @clayborg since both of you approved of it, I think this patch is ready to be merged, right? Could one of you do it? I don't have commit access.

@labath sorry for the repeated messages. I believe this is ready to be merged, right?

This revision was landed with ongoing or failed builds.Jan 24 2021, 12:39 PM
Closed by commit rG2afaf072f5c1: Implement vAttachOrWait (authored by augusto2112, committed by labath). · Explain Why
This revision was automatically updated to reflect the committed changes.