This is an archive of the discontinued LLVM Phabricator instance.

Merge lldb-platform and lldb-gdbserver into a single binary.
ClosedPublic

Authored by flackr on Feb 10 2015, 2:05 PM.

Details

Summary

This commit merges lldb-platform and lldb-gdbserver into a single binary of the same size as each of the previous individual binaries. Execution mode is controlled by the first argument being either platform or gdbserver.

Diff Detail

Repository
rL LLVM

Event Timeline

flackr updated this revision to Diff 19708.Feb 10 2015, 2:05 PM
flackr retitled this revision from to Merge lldb-platform and lldb-gdbserver into a single binary..
flackr updated this object.
flackr edited the test plan for this revision. (Show Details)
flackr added reviewers: vharron, tberghammer.
flackr set the repository for this revision to rL LLVM.
flackr added a subscriber: Unknown Object (MLST).
silvas added a subscriber: silvas.Feb 10 2015, 4:24 PM

Can you maybe use the trick of looking at argv[0] instead of taking a command line option to know which program to be? (assuming it makes sense to do this change). See llvm-ar for an example.

emaste added a subscriber: emaste.Feb 10 2015, 5:31 PM

Can you maybe use the trick of looking at argv[0] instead of taking a command line option to know which program to be? (assuming it makes sense to do this change). See llvm-ar for an example.

Or look at lld as a good example. It chooses appropriate behaviour based on argv[0] but also has a -flavor argument for use when argv[0] does not match any of the known names.

% ./lld
Select the appropriate flavor
OVERVIEW: LLVM Linker

USAGE: lld [options] <inputs>

OPTIONS:
...
  -flavor <value> Flavor for linking, options are gnu/darwin/link


% ln -s lld ld
% ./ld
No input files
vharron edited edge metadata.Feb 10 2015, 5:42 PM

argv[0] would be more standard but requires a symlink on the target. This
just requires the binary.

vharron requested changes to this revision.Feb 10 2015, 9:40 PM
vharron edited edge metadata.
vharron added inline comments.
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
493 ↗(On Diff #19708)

I actually don't like lldb being removed from lldb-gdbserver. Then you just have something that doesn't make sense, because it isn't gdbserver. I think we should stick with lldb-gdbserver and also lldb-platform to differentiate the platform process from the platform command/concept.

This revision now requires changes to proceed.Feb 10 2015, 9:40 PM
tberghammer requested changes to this revision.Feb 11 2015, 6:01 AM
tberghammer edited edge metadata.

Please rebase it TOT as a big refactor was committed in what will conflict with this patch.

I would prefer to keep the source code for lldb-platform and lldb-gdbserver in two different cpp file and make lldb-server contain only a main method to distinguish between the two, but I am not sure how much code duplication it will cause.

tberghammer edited edge metadata.Feb 11 2015, 6:05 AM
tberghammer edited subscribers, added: Unknown Object (MLST); removed: Unknown Object (MLST).

I would prefer to keep the source code for lldb-platform and

lldb-gdbserver in two different cpp file

I was thinking the same thing but I'll speak up now that Thomas has
spoken.

flackr updated this revision to Diff 19783.Feb 11 2015, 2:01 PM
flackr edited edge metadata.

Keep lldb-platform.cpp and lldb-gdbserver.cpp separate but link and call into them from lldb-server.cpp. Also merges changes from ToT.

I've kept the two in separate cpp files with a new cpp file for lldb-server which calls the main on one or the other.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
493 ↗(On Diff #19708)

Undone.

tberghammer added a comment.EditedFeb 12 2015, 2:47 AM

Please update the project files for the xcode project.

tools/lldb-server/lldb-gdbserver.cpp
57–58

Please make the functions and the global variables static instead of using an anonymous namespace.

tools/lldb-server/lldb-platform.cpp
42–43

Please make the functions and the global variables static instead of using an anonymous namespace.

flackr updated this revision to Diff 19840.Feb 12 2015, 11:44 AM
flackr edited edge metadata.
  • Fixed the gdbserver test helper and verified no more test failures introduced by change.
  • Addressed review comments

I've verified the build on windows. It still does not build lldb-platform
Still working on getting mac to use to update the xcodeproj files.

I'll update as soon as I get a mac to test on.

tools/lldb-server/lldb-gdbserver.cpp
57–58

Done.

tools/lldb-server/lldb-platform.cpp
42–43

Done.

tberghammer accepted this revision.Feb 13 2015, 2:21 AM
tberghammer edited edge metadata.

LGTM

flackr updated this revision to Diff 20168.Feb 18 2015, 6:57 AM
flackr updated this object.
flackr edited edge metadata.

Updated xcode project and verified building and running lldb-server on OSX.

I've tested build on Windows, Linux, OSX and running on Linux and OSX (lldb-server is not built on Windows), if this is good can you land for me? I still don't have commit access. Thanks.

This revision was automatically updated to reflect the committed changes.

Hi Tamas/Robert,

We definitely need input from someone on the Apple side for CLs that affect them. UUIC, Apple is using lldb-platform.

Hi Tamas/Robert,

We definitely need input from someone on the Apple side for CLs that affect them. UUIC, Apple is using lldb-platform.

Sorry, I hadn't realized that. Should we revert and continue review? It landed as svn revision 229683 and 229691 (One of the tools taking diffs stripped deleting / adding empty files).

Let's wait for Greg to chime in.

Could you rename test/tools/lldb-gdbserver to test/tools/lldb-server as well?

tools/lldb-platform/Makefile