This is an archive of the discontinued LLVM Phabricator instance.

[clangd] XPC transport layer, framework, test-client
ClosedPublic

Authored by jkorous on Nov 12 2018, 7:50 AM.

Diff Detail

Event Timeline

jkorous created this revision.Nov 12 2018, 7:50 AM

A question about the high-level build target setup (I don't know much about XPC services/frameworks, bear with me...):

This is set up so that the clangd binary (ClangdMain) can run unmodified as an XPC service, all flags and options are still respected etc.
At the same time, the framework plist seems like it is designed to invoke the clangd binary in a very specific configuration (no flags will be plumbed through).

Is it actually important that the clangd binary itself can be used with XPC, rather than defining another binary that spawns a ClangdServer+XPCTransport with the right config? If you strip out all of ClangdMain that's not related to flag parsing and options, there's almost nothing left...

tool/ClangdMain.cpp
30 ↗(On Diff #173674)

WDYT about putting newXPCTransport() in Transport.h behind an ifdef?

That will be consistent with JSON, allow the XPCTransport.h to be removed entirely, and remove one #ifdef here.

I find the #ifdefs easier to understand if they surround functional code, rather than #includes - might be just me.

328 ↗(On Diff #173674)

I'd consider putting this outside the #ifdef, so we can print an error and exit if it's requested but not built.

In fact, can this just be a regular flag instead? -transport={xpc|json}

329 ↗(On Diff #173674)

no support for -input-mirror or pretty-printing in the logs - deliberate?

xpc/CMakeLists.txt
21 ↗(On Diff #173674)

why is conversions a separate library from transport?

No problem with fine-grained targets per se, but it's inconsistent with much of the rest of clang-tools-extra.

xpc/Conversion.cpp
16 ↗(On Diff #173674)

nit:

using namespace llvm;
namespace clang {
namespace clangd {

(we're finally consistent about this)

22 ↗(On Diff #173674)

Key, PayloadString, etc

23 ↗(On Diff #173674)

nit: you can #include ScopedPrinter, and then this is just llvm::to_string(json)

28 ↗(On Diff #173674)

ah, this encoding is a little sad vs the "native" encoding of object -> xpc_dictionary, array -> xpc_array etc which looked promising...

Totally your call, but out of curiosity - why the change?

xpc/Conversion.h
19 ↗(On Diff #173674)

param name json -> JSON

xpc/XPCTransport.cpp
142 ↗(On Diff #173674)

lowercase namespace?

177 ↗(On Diff #173674)

I'm not clear on the relationship between XPC services and connections - if there are multiple connections, what happens? Do you want to check and close any subsequent ones? Can connections and/or messages arrive on multiple threads?

192 ↗(On Diff #173674)

you could consider asserting that they were previously null

xpc/XPCTransport.h
19 ↗(On Diff #173674)

as mentioned above, I think this may be more discoverable in Transport.h, with ifdef.
But if the layering/separate libraries are important, here seems OK too.

jkorous planned changes to this revision.Nov 15 2018, 9:50 AM

A question about the high-level build target setup (I don't know much about XPC services/frameworks, bear with me...):

This is set up so that the clangd binary (ClangdMain) can run unmodified as an XPC service, all flags and options are still respected etc.
At the same time, the framework plist seems like it is designed to invoke the clangd binary in a very specific configuration (no flags will be plumbed through).

Is it actually important that the clangd binary itself can be used with XPC, rather than defining another binary that spawns a ClangdServer+XPCTransport with the right config? If you strip out all of ClangdMain that's not related to flag parsing and options, there's almost nothing left...

I don't really expect you to be familiar with XPC - feel free to ask about anything and I'll do my best explaining.

It's more like that configuration isn't implemented in this initial patch but as we can't use command line options (launchd doesn't pass these to XPC service) we'll have to duplicate these as environment variables. We discusse internally and we prefer to have just single binary as that would make integration and testing easier for us.

tool/ClangdMain.cpp
30 ↗(On Diff #173674)

Ok, sounds reasonable.

328 ↗(On Diff #173674)

Unfortunately it's not possible to have launchd pass flags when spawning XPC services (otherwise it would be my first choice too).

329 ↗(On Diff #173674)

I was thinking this could be added later.

xpc/CMakeLists.txt
21 ↗(On Diff #173674)

Guilty of YAGNI violation here - was thinking about eventual use by XPC clients. I'll stick to consistency for now and would separate it if needed.

xpc/Conversion.cpp
16 ↗(On Diff #173674)

Ok. Thanks.

22 ↗(On Diff #173674)

Thanks. Somehow I was always working on projects with different naming conventions and still struggle with this.

23 ↗(On Diff #173674)

I'll take a look at ScopedPrinter. Thanks.

28 ↗(On Diff #173674)

I know. At first we didn't even think of anything else than 1:1 mapping between JSON and XPC dictionary as it's just natural. But later we learned about performance issues with construction of XPC dictionaries with lots of elements which would be a problem for code completion.

xpc/Conversion.h
19 ↗(On Diff #173674)

Good catch. Thanks.

xpc/XPCTransport.cpp
142 ↗(On Diff #173674)

Right. I should probably start looking for some tool checking this...

177 ↗(On Diff #173674)

Our use-case is an XPC service which is bundled to an application. It could be described as a "private daemon" - the service is launched on-demand for the application. There should be either zero or one connection at a time.

As for messages - we didn't plan for concurrent use. I'll add explicit synchronization.

192 ↗(On Diff #173674)

Good idea.

xpc/XPCTransport.h
19 ↗(On Diff #173674)

I agree, will move it.

jkorous updated this revision to Diff 175884.Nov 29 2018, 9:19 AM
jkorous marked 19 inline comments as done.
jkorous retitled this revision from [clangd] XPC transport layer, framework, test-client to [clangd][WIP] XPC transport layer, framework, test-client.
jkorous edited the summary of this revision. (Show Details)

Addressed most of the comments.

Since AFAIK Sam is off until the end of the year I am adding more reviewers.

tool/ClangdMain.cpp
329 ↗(On Diff #173674)

Just to clarify - since we'd like to have just a single binary for XPC and stdio transport and we sadly can't use command line options we'd have to duplicate probably all of them as env variables.
Would that be fine with you?

xpc/CMakeLists.txt
21 ↗(On Diff #173674)

Now that I actually took a better look we already have one client - our xpc-test-client. I assume that means - it's fine as it is, right?

xpc/XPCTransport.cpp
177 ↗(On Diff #173674)

I want to make the synchronous operation explicit here. Still need to work on this.

jkorous updated this revision to Diff 180640.Jan 8 2019, 4:10 AM
jkorous retitled this revision from [clangd][WIP] XPC transport layer, framework, test-client to [clangd] XPC transport layer, framework, test-client.
jkorous edited the summary of this revision. (Show Details)

added one more assert

jkorous updated this revision to Diff 180809.Jan 9 2019, 3:01 AM
jkorous edited the summary of this revision. (Show Details)

Fixed the synchronous handling of events.

@arphaman or @sammccall, could you please take the final look?

This revision is now accepted and ready to land.Jan 15 2019, 11:20 AM
This revision was automatically updated to reflect the committed changes.
tamur added a subscriber: tamur.Jan 15 2019, 10:33 PM

This patch seems to have broken the compilation. I get the following error on a linux platform:
[12/14] Linking CXX executable bin/clangd
FAILED: bin/clangd
: && /usr/local/google/home/tamur/src/llvm/2018_nov_12/llvm/Stable/bin/clang++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -fuse-ld=lld -Wl,--color-diagnostics -Wl,-allow-shlib-undefined -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -o bin/clangd -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.so.8svn -lpthread lib/libclangBasic.so.8svn lib/libclangDaemon.so.8svn lib/libclangFormat.so.8svn lib/libclangFrontend.so.8svn lib/libclangSema.so.8svn lib/libclangTooling.so.8svn lib/libclangToolingCore.so.8svn && :
ld.lld: error: undefined symbol: clang::clangd::newXPCTransport()

This patch seems to have broken the compilation. I get the following error on a linux platform:
[12/14] Linking CXX executable bin/clangd
FAILED: bin/clangd
: && /usr/local/google/home/tamur/src/llvm/2018_nov_12/llvm/Stable/bin/clang++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -fuse-ld=lld -Wl,--color-diagnostics -Wl,-allow-shlib-undefined -Wl,-O3 -Wl,--gc-sections tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -o bin/clangd -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.so.8svn -lpthread lib/libclangBasic.so.8svn lib/libclangDaemon.so.8svn lib/libclangFormat.so.8svn lib/libclangFrontend.so.8svn lib/libclangSema.so.8svn lib/libclangTooling.so.8svn lib/libclangToolingCore.so.8svn && :
ld.lld: error: undefined symbol: clang::clangd::newXPCTransport()

Fixed by rCTE351306

Sorry for not getting back to this, but it looks good.

I'm really glad about how minimal the changes are outside xpc/, because it makes it less likely we'll accidentally break something that can only be tested on mac.