[PATCH] D54428: [clangd] XPC transport layer, framework, test-client

Thu Nov 15 09:50:25 PST 2018

jkorous planned changes to this revision.
jkorous added a comment.

In https://reviews.llvm.org/D54428#1297147, @sammccall wrote:

> 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.

================
Comment at: tool/ClangdMain.cpp:30
+#ifdef CLANGD_BUILD_XPC
+#include "xpc/XPCTransport.h"
+#endif
----------------
sammccall wrote:
> 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.
Ok, sounds reasonable.

================
Comment at: tool/ClangdMain.cpp:328
+#ifdef CLANGD_BUILD_XPC
+    if (getenv("CLANGD_AS_XPC_SERVICE"))
+      return newXPCransport();
----------------
sammccall wrote:
> 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}`
Unfortunately it's not possible to have launchd pass flags when spawning XPC services (otherwise it would be my first choice too).

================
Comment at: tool/ClangdMain.cpp:329
+    if (getenv("CLANGD_AS_XPC_SERVICE"))
+      return newXPCransport();
+#endif
----------------
sammccall wrote:
> no support for `-input-mirror` or pretty-printing in the logs - deliberate?
I was thinking this could be added later.

================
Comment at: xpc/CMakeLists.txt:21
+
+add_clang_library(clangdXpcJsonConversions
+  Conversion.cpp
----------------
sammccall wrote:
> 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.
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.

================
Comment at: xpc/Conversion.cpp:16
+
+using namespace clang;
+using namespace clangd;
----------------
sammccall wrote:
> nit:
> ```
> using namespace llvm;
> namespace clang {
> namespace clangd {
> ```
> (we're finally consistent about this)
Ok. Thanks.

================
Comment at: xpc/Conversion.cpp:22
+xpc_object_t clangd::jsonToXpc(const json::Value &json) {
+  const char *const key = "LSP";
+  std::string payload_string;
----------------
sammccall wrote:
> Key, PayloadString, etc
Thanks. Somehow I was always working on projects with different naming conventions and still struggle with this.

================
Comment at: xpc/Conversion.cpp:23
+  const char *const key = "LSP";
+  std::string payload_string;
+  raw_string_ostream payload_stream(payload_string);
----------------
sammccall wrote:
> nit: you can #include ScopedPrinter, and then this is just llvm::to_string(json)
I'll take a look at ScopedPrinter. Thanks.

================
Comment at: xpc/Conversion.cpp:28
+  xpc_object_t payload_obj = xpc_string_create(payload_string.c_str());
+  return xpc_dictionary_create(&key, &payload_obj, 1);
+}
----------------
sammccall wrote:
> 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?
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.

================
Comment at: xpc/Conversion.h:19
+
+xpc_object_t jsonToXpc(const llvm::json::Value &json);
+llvm::json::Value xpcToJson(const xpc_object_t &json);
----------------
sammccall wrote:
> param name json -> JSON
Good catch. Thanks.

================
Comment at: xpc/XPCTransport.cpp:142
+
+namespace XPCClosure {
+// "owner" of this "closure object" - necessary for propagating connection to
----------------
sammccall wrote:
> lowercase namespace?
Right. I should probably start looking for some tool checking this...

================
Comment at: xpc/XPCTransport.cpp:177
+    if (!TransportObject->handleMessage(std::move(Doc), *HandlerPtr)) {
+      log("Received exit notification - cancelling connection.");
+      xpc_connection_cancel(xpc_dictionary_get_remote_connection(message));
----------------
sammccall wrote:
> 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?
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.

================
Comment at: xpc/XPCTransport.cpp:192
+  // exit of xpc_main().
+  XPCClosure::TransportObject = this;
+  XPCClosure::HandlerPtr = &Handler;
----------------
sammccall wrote:
> you could consider asserting that they were previously null
Good idea.

================
Comment at: xpc/XPCTransport.h:19
+
+std::unique_ptr<Transport> newXPCransport();
+
----------------
sammccall wrote:
> 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.
I agree, will move it.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54428