Skip to content

[lldb][RPC] Upstream RPC server interface emitters #138032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chelcassanova
Copy link
Contributor

This commit upstreams the LLDB RPC server interface emitters. These emitters generate the server-side API interfaces for RPC, which communicate directly with liblldb itself out of process using the SB API.

https://discourse.llvm.org/t/rfc-upstreaming-lldb-rpc/85804

@llvmbot
Copy link
Member

llvmbot commented Apr 30, 2025

@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

Changes

This commit upstreams the LLDB RPC server interface emitters. These emitters generate the server-side API interfaces for RPC, which communicate directly with liblldb itself out of process using the SB API.

https://discourse.llvm.org/t/rfc-upstreaming-lldb-rpc/85804


Patch is 30.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/138032.diff

4 Files Affected:

  • (added) lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.cpp (+73)
  • (added) lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.h (+46)
  • (added) lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerSourceEmitter.cpp (+592)
  • (added) lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerSourceEmitter.h (+80)
diff --git a/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.cpp b/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.cpp
new file mode 100644
index 0000000000000..a2818ada1f323
--- /dev/null
+++ b/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.cpp
@@ -0,0 +1,73 @@
+//===-- RPCServerHeaderEmitter.cpp ----------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RPCServerHeaderEmitter.h"
+#include "RPCCommon.h"
+
+#include "clang/AST/AST.h"
+#include "clang/AST/Mangle.h"
+#include "clang/Frontend/CompilerInstance.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang;
+using namespace lldb_rpc_gen;
+
+void RPCServerHeaderEmitter::EmitMethod(const Method &method) {
+  const std::string &MangledName = method.MangledName;
+
+  EmitLine("class " + MangledName +
+           " : public rpc_common::RPCFunctionInstance {");
+  EmitLine("public:");
+  IndentLevel++;
+  EmitConstructor(MangledName);
+  EmitDestructor(MangledName);
+  EmitHandleRPCCall();
+  IndentLevel--;
+  EmitLine("};");
+}
+
+void RPCServerHeaderEmitter::EmitHandleRPCCall() {
+  EmitLine("bool HandleRPCCall(rpc_common::Connection &connection, "
+           "rpc_common::RPCStream &send, rpc_common::RPCStream &response) "
+           "override;");
+}
+
+void RPCServerHeaderEmitter::EmitConstructor(const std::string &MangledName) {
+  EmitLine(MangledName + "() : RPCFunctionInstance(\"" + MangledName +
+           "\") {}");
+}
+
+void RPCServerHeaderEmitter::EmitDestructor(const std::string &MangledName) {
+  EmitLine("~" + MangledName + "() override {}");
+}
+
+std::string RPCServerHeaderEmitter::GetHeaderGuard() {
+  const std::string UpperFilenameNoExt =
+      llvm::sys::path::stem(
+          llvm::sys::path::filename(OutputFile->getFilename()))
+          .upper();
+  return "GENERATED_LLDB_RPC_SERVER_" + UpperFilenameNoExt + "_H";
+}
+
+void RPCServerHeaderEmitter::Begin() {
+  const std::string HeaderGuard = GetHeaderGuard();
+  EmitLine("#ifndef " + HeaderGuard);
+  EmitLine("#define " + HeaderGuard);
+  EmitLine("");
+  EmitLine("#include <lldb-rpc/common/RPCFunction.h>");
+  EmitLine("");
+  EmitLine("namespace rpc_server {");
+}
+
+void RPCServerHeaderEmitter::End() {
+  EmitLine("} // namespace rpc_server");
+  EmitLine("#endif // " + GetHeaderGuard());
+}
diff --git a/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.h b/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.h
new file mode 100644
index 0000000000000..cdf201090b7c8
--- /dev/null
+++ b/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerHeaderEmitter.h
@@ -0,0 +1,46 @@
+//===-- RPCServerHeaderEmitter.h ----------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_RPC_GEN_RPCSERVERHEADEREMITTER_H
+#define LLDB_RPC_GEN_RPCSERVERHEADEREMITTER_H
+
+#include "RPCCommon.h"
+
+#include "clang/AST/AST.h"
+#include "llvm/Support/ToolOutputFile.h"
+
+using namespace clang;
+
+namespace lldb_rpc_gen {
+class RPCServerHeaderEmitter : public FileEmitter {
+public:
+  RPCServerHeaderEmitter(std::unique_ptr<llvm::ToolOutputFile> &&OutputFile)
+      : FileEmitter(std::move(OutputFile)) {
+    Begin();
+  }
+
+  ~RPCServerHeaderEmitter() { End(); }
+
+  void EmitMethod(const Method &method);
+
+private:
+  void EmitHandleRPCCall();
+
+  void EmitConstructor(const std::string &MangledName);
+
+  void EmitDestructor(const std::string &MangledName);
+
+  std::string GetHeaderGuard();
+
+  void Begin();
+
+  void End();
+};
+} // namespace lldb_rpc_gen
+
+#endif // LLDB_RPC_GEN_RPCSERVERHEADEREMITTER_H
diff --git a/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerSourceEmitter.cpp b/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerSourceEmitter.cpp
new file mode 100644
index 0000000000000..645ddd11662a7
--- /dev/null
+++ b/lldb/tools/lldb-rpc/lldb-rpc-gen/server/RPCServerSourceEmitter.cpp
@@ -0,0 +1,592 @@
+//===-- RPCServerSourceEmitter.cpp ----------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "RPCServerSourceEmitter.h"
+#include "RPCCommon.h"
+
+#include "clang/AST/AST.h"
+#include "clang/Frontend/CompilerInstance.h"
+
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include <map>
+
+using namespace clang;
+using namespace lldb_rpc_gen;
+
+// For methods with pointer return types, it's important that we know how big
+// the type of the pointee is. We must correctly size a buffer (in the form of a
+// Bytes object) before we can actually use it.
+static const std::map<llvm::StringRef, size_t> MethodsWithPointerReturnTypes = {
+    {"_ZN4lldb12SBModuleSpec12GetUUIDBytesEv", 16}, // sizeof(uuid_t) -> 16
+    {"_ZNK4lldb8SBModule12GetUUIDBytesEv", 16},     // sizeof(uuid_t) -> 16
+};
+
+void RPCServerSourceEmitter::EmitMethod(const Method &method) {
+  if (method.ContainsFunctionPointerParameter)
+    EmitCallbackFunction(method);
+
+  EmitCommentHeader(method);
+  EmitFunctionHeader(method);
+  EmitFunctionBody(method);
+  EmitFunctionFooter();
+}
+
+void RPCServerSourceEmitter::EmitCommentHeader(const Method &method) {
+  std::string CommentLine;
+  llvm::raw_string_ostream CommentStream(CommentLine);
+
+  CommentStream << "// " << method.QualifiedName << "("
+                << method.CreateParamListAsString(eServer) << ")";
+  if (method.IsConst)
+    CommentStream << " const";
+
+  EmitLine("//------------------------------------------------------------");
+  EmitLine(CommentLine);
+  EmitLine("//------------------------------------------------------------");
+}
+
+void RPCServerSourceEmitter::EmitFunctionHeader(const Method &method) {
+  std::string FunctionHeader;
+  llvm::raw_string_ostream FunctionHeaderStream(FunctionHeader);
+  FunctionHeaderStream
+      << "bool rpc_server::" << method.MangledName
+      << "::HandleRPCCall(rpc_common::Connection &connection, RPCStream "
+         "&send, RPCStream &response) {";
+  EmitLine(FunctionHeader);
+  IndentLevel++;
+}
+
+void RPCServerSourceEmitter::EmitFunctionBody(const Method &method) {
+  EmitLine("// 1) Make local storage for incoming function arguments");
+  EmitStorageForParameters(method);
+  EmitLine("// 2) Decode all function arguments");
+  EmitDecodeForParameters(method);
+  EmitLine("// 3) Call the method and encode the return value");
+  EmitMethodCallAndEncode(method);
+}
+
+void RPCServerSourceEmitter::EmitFunctionFooter() {
+  EmitLine("return true;");
+  IndentLevel--;
+  EmitLine("}");
+}
+
+void RPCServerSourceEmitter::EmitStorageForParameters(const Method &method) {
+  // If we have an instance method and it isn't a constructor, we'll need to
+  // emit a "this" pointer.
+  if (method.IsInstance && !method.IsCtor)
+    EmitStorageForOneParameter(method.ThisType, "this_ptr", method.Policy,
+                               /* IsFollowedByLen = */ false);
+  for (auto Iter = method.Params.begin(); Iter != method.Params.end(); Iter++) {
+    EmitStorageForOneParameter(Iter->Type, Iter->Name, method.Policy,
+                               Iter->IsFollowedByLen);
+    // Skip over the length parameter, we don't emit it.
+    if (!lldb_rpc_gen::TypeIsConstCharPtrPtr(Iter->Type) &&
+        Iter->IsFollowedByLen)
+      Iter++;
+  }
+}
+
+void RPCServerSourceEmitter::EmitStorageForOneParameter(
+    QualType ParamType, const std::string &ParamName,
+    const PrintingPolicy &Policy, bool IsFollowedByLen) {
+  // First, we consider `const char *`, `const char **`. They have special
+  // server-side types.
+  if (TypeIsConstCharPtr(ParamType)) {
+    EmitLine("rpc_common::ConstCharPointer " + ParamName + ";");
+    return;
+  } else if (TypeIsConstCharPtrPtr(ParamType)) {
+    EmitLine("rpc_common::StringList " + ParamName + ";");
+    return;
+  }
+
+  QualType UnderlyingType =
+      lldb_rpc_gen::GetUnqualifiedUnderlyingType(ParamType);
+  const bool IsSBClass = lldb_rpc_gen::TypeIsSBClass(UnderlyingType);
+
+  if (ParamType->isPointerType() && !IsSBClass) {
+    // Void pointer with no length is usually a baton for a callback. We're
+    // going to hold onto the pointer value so we can send it back to the
+    // client-side when we implement callbacks.
+    if (ParamType->isVoidPointerType() && !IsFollowedByLen) {
+      EmitLine("void * " + ParamName + " = nullptr;");
+      return;
+    }
+
+    if (!ParamType->isFunctionPointerType()) {
+      EmitLine("Bytes " + ParamName + ";");
+      return;
+    }
+
+    assert(ParamType->isFunctionPointerType() && "Unhandled pointer type");
+    EmitLine("rpc_common::function_ptr_t " + ParamName + " = nullptr;");
+    return;
+  }
+
+  std::string StorageDeclaration;
+  llvm::raw_string_ostream StorageDeclarationStream(StorageDeclaration);
+
+  UnderlyingType.print(StorageDeclarationStream, Policy);
+  StorageDeclarationStream << " ";
+  if (IsSBClass)
+    StorageDeclarationStream << "*";
+  StorageDeclarationStream << ParamName;
+  if (IsSBClass)
+    StorageDeclarationStream << " = nullptr";
+  else
+    StorageDeclarationStream << " = {}";
+  StorageDeclarationStream << ";";
+  EmitLine(StorageDeclaration);
+}
+
+void RPCServerSourceEmitter::EmitDecodeForParameters(const Method &method) {
+  if (method.IsInstance && !method.IsCtor)
+    EmitDecodeForOneParameter(method.ThisType, "this_ptr", method.Policy);
+  for (auto Iter = method.Params.begin(); Iter != method.Params.end(); Iter++) {
+    EmitDecodeForOneParameter(Iter->Type, Iter->Name, method.Policy);
+    if (!lldb_rpc_gen::TypeIsConstCharPtrPtr(Iter->Type) &&
+        Iter->IsFollowedByLen)
+      Iter++;
+  }
+}
+
+void RPCServerSourceEmitter::EmitDecodeForOneParameter(
+    QualType ParamType, const std::string &ParamName,
+    const PrintingPolicy &Policy) {
+  QualType UnderlyingType =
+      lldb_rpc_gen::GetUnqualifiedUnderlyingType(ParamType);
+
+  if (TypeIsSBClass(UnderlyingType)) {
+    std::string DecodeLine;
+    llvm::raw_string_ostream DecodeLineStream(DecodeLine);
+    DecodeLineStream << ParamName << " = "
+                     << "RPCServerObjectDecoder<";
+    UnderlyingType.print(DecodeLineStream, Policy);
+    DecodeLineStream << ">(send, rpc_common::RPCPacket::ValueType::Argument);";
+    EmitLine(DecodeLine);
+    EmitLine("if (!" + ParamName + ")");
+    IndentLevel++;
+    EmitLine("return false;");
+    IndentLevel--;
+  } else {
+    EmitLine("if (!RPCValueDecoder(send, "
+             "rpc_common::RPCPacket::ValueType::Argument, " +
+             ParamName + "))");
+    IndentLevel++;
+    EmitLine("return false;");
+    IndentLevel--;
+  }
+}
+
+std::string RPCServerSourceEmitter::CreateMethodCall(const Method &method) {
+  std::string MethodCall;
+  llvm::raw_string_ostream MethodCallStream(MethodCall);
+  if (method.IsInstance) {
+    if (!method.IsCtor)
+      MethodCallStream << "this_ptr->";
+    MethodCallStream << method.BaseName;
+  } else
+    MethodCallStream << method.QualifiedName;
+
+  std::vector<std::string> Args;
+  std::string FunctionPointerName;
+  for (auto Iter = method.Params.begin(); Iter != method.Params.end(); Iter++) {
+    std::string Arg;
+    // We must check for `const char *` and `const char **` first.
+    if (TypeIsConstCharPtr(Iter->Type)) {
+      // `const char *` is stored server-side as rpc_common::ConstCharPointer
+      Arg = Iter->Name + ".c_str()";
+    } else if (TypeIsConstCharPtrPtr(Iter->Type)) {
+      // `const char **` is stored server-side as rpc_common::StringList
+      Arg = Iter->Name + ".argv()";
+    } else if (lldb_rpc_gen::TypeIsSBClass(Iter->Type)) {
+      Arg = Iter->Name;
+      if (!Iter->Type->isPointerType())
+        Arg = "*" + Iter->Name;
+    } else if (Iter->Type->isPointerType() &&
+               !Iter->Type->isFunctionPointerType() &&
+               (!Iter->Type->isVoidPointerType() || Iter->IsFollowedByLen)) {
+      // We move pointers between the server and client as 'Bytes' objects.
+      // Pointers with length arguments will have their length filled in below.
+      // Pointers with no length arguments are assumed to behave like an array
+      // with length of 1, except for void pointers which are handled
+      // differently.
+      Arg = "(" + Iter->Type.getAsString(method.Policy) + ")" + Iter->Name +
+            ".GetData()";
+    } else if (Iter->Type->isFunctionPointerType()) {
+      // If we have a function pointer, we only want to pass something along if
+      // we got a real pointer.
+      Arg = Iter->Name + " ? " + method.MangledName + "_callback : nullptr";
+      FunctionPointerName = Iter->Name;
+    } else if (Iter->Type->isVoidPointerType() && !Iter->IsFollowedByLen &&
+               method.ContainsFunctionPointerParameter) {
+      // Assumptions:
+      //  - This is assumed to be the baton for the function pointer.
+      //  - This is assumed to come after the function pointer parameter.
+      // We always produce this regardless of the value of the baton argument.
+      Arg = "new CallbackInfo(" + FunctionPointerName + ", " + Iter->Name +
+            ", connection.GetConnectionID())";
+    } else
+      Arg = Iter->Name;
+
+    if (Iter->Type->isRValueReferenceType())
+      Arg = "std::move(" + Arg + ")";
+    Args.push_back(Arg);
+
+    if (!lldb_rpc_gen::TypeIsConstCharPtrPtr(Iter->Type) &&
+        Iter->IsFollowedByLen) {
+      std::string LengthArg = Iter->Name + ".GetSize()";
+      if (!Iter->Type->isVoidPointerType()) {
+        QualType UUT = lldb_rpc_gen::GetUnqualifiedUnderlyingType(Iter->Type);
+        LengthArg += " / sizeof(" + UUT.getAsString(method.Policy) + ")";
+      }
+      Args.push_back(LengthArg);
+      Iter++;
+    }
+  }
+  MethodCallStream << "(" << llvm::join(Args, ", ") << ")";
+
+  return MethodCall;
+}
+
+std::string RPCServerSourceEmitter::CreateEncodeLine(const std::string &Value,
+                                                     bool IsEncodingSBClass) {
+  std::string EncodeLine;
+  llvm::raw_string_ostream EncodeLineStream(EncodeLine);
+
+  if (IsEncodingSBClass)
+    EncodeLineStream << "RPCServerObjectEncoder(";
+  else
+    EncodeLineStream << "RPCValueEncoder(";
+
+  EncodeLineStream
+      << "response, rpc_common::RPCPacket::ValueType::ReturnValue, ";
+  EncodeLineStream << Value;
+  EncodeLineStream << ");";
+  return EncodeLine;
+}
+
+// There are 4 cases to consider:
+// - const SBClass &: No need to do anything.
+// - const foo &: No need to do anything.
+// - SBClass &: The server and the client hold on to IDs to refer to specific
+//   instances, so there's no need to send any information back to the client.
+// - foo &: The client is sending us a value over the wire, but because the type
+//   is mutable, we must send the changed value back in case the method call
+//   mutated it.
+//
+// Updating a mutable reference is done as a return value from the RPC
+// perspective. These return values need to be emitted after the method's return
+// value, and they are emitted in the order in which they occur in the
+// declaration.
+void RPCServerSourceEmitter::EmitEncodesForMutableParameters(
+    const std::vector<Param> &Params) {
+  for (auto Iter = Params.begin(); Iter != Params.end(); Iter++) {
+    // No need to manually update an SBClass
+    if (lldb_rpc_gen::TypeIsSBClass(Iter->Type))
+      continue;
+
+    if (!Iter->Type->isReferenceType() && !Iter->Type->isPointerType())
+      continue;
+
+    // If we have a void pointer with no length, there's nothing to update. This
+    // is likely a baton for a callback. The same goes for function pointers.
+    if (Iter->Type->isFunctionPointerType() ||
+        (Iter->Type->isVoidPointerType() && !Iter->IsFollowedByLen))
+      continue;
+
+    // No need to update pointers and references to const-qualified data.
+    QualType UnderlyingType = lldb_rpc_gen::GetUnderlyingType(Iter->Type);
+    if (UnderlyingType.isConstQualified())
+      continue;
+
+    const std::string EncodeLine =
+        CreateEncodeLine(Iter->Name, /* IsEncodingSBClass = */ false);
+    EmitLine(EncodeLine);
+  }
+}
+
+// There are 3 possible scenarios that this method can encounter:
+// 1. The method has no return value and is not a constructor.
+//    Only the method call itself is emitted.
+// 2. The method is a constructor.
+//    The call to the constructor is emitted in the encode line.
+// 3. The method has a return value.
+//    The method call is emitted and the return value is captured in a variable.
+//    After that, an encode call is emitted with the variable that captured the
+//    return value.
+void RPCServerSourceEmitter::EmitMethodCallAndEncode(const Method &method) {
+  // FIXME: The hand-written lldb-rpc-server currently doesn't emit destructors
+  // for LocalObjectRefs... even if the type is an ObjectRef. What should we do
+  // here?
+
+  const std::string MethodCall = CreateMethodCall(method);
+
+  // If this function returns nothing, we just emit the call and update any
+  // mutable references. Note that constructors have return type `void` so we
+  // must explicitly check for that here.
+  if (!method.IsCtor && method.ReturnType->isVoidType()) {
+    EmitLine(MethodCall + ";");
+    EmitEncodesForMutableParameters(method.Params);
+    return;
+  }
+
+  static constexpr llvm::StringLiteral ReturnVariableName("__result");
+
+  // If this isn't a constructor, we'll need to store the result of the method
+  // call in a result variable.
+  if (!method.IsCtor) {
+    // We need to determine what the appropriate return type is. Here is the
+    // strategy:
+    // 1.) `SBFoo` -> `SBFoo &&`
+    // 2.) If the type is a pointer other than `const char *` or `const char **`
+    //     or `void *`, the return type will be `Bytes` (e.g. `const uint8_t *`
+    //     -> `Bytes`).
+    // 3.) Otherwise, emit the exact same return type.
+    std::string ReturnTypeName;
+    std::string AssignLine;
+    llvm::raw_string_ostream AssignLineStream(AssignLine);
+    if (method.ReturnType->isPointerType() &&
+        !lldb_rpc_gen::TypeIsConstCharPtr(method.ReturnType) &&
+        !lldb_rpc_gen::TypeIsConstCharPtrPtr(method.ReturnType) &&
+        !method.ReturnType->isVoidPointerType()) {
+      llvm::StringRef MangledNameRef(method.MangledName);
+      auto Pos = MethodsWithPointerReturnTypes.find(MangledNameRef);
+      assert(Pos != MethodsWithPointerReturnTypes.end() &&
+             "Unable to determine the size of the return buffer");
+      if (Pos == MethodsWithPointerReturnTypes.end()) {
+        EmitLine(
+            "// Intentionally inserting a compiler error. lldb-rpc-gen "
+            "was unable to determine how large the return buffer should be.");
+        EmitLine("ThisShouldNotCompile");
+        return;
+      }
+      AssignLineStream << "Bytes " << ReturnVariableName << "(" << MethodCall
+                       << ", " << Pos->second << ");";
+    } else {
+      if (lldb_rpc_gen::TypeIsSBClass(method.ReturnType)) {
+        // We want to preserve constness, so we don't strip qualifications from
+        // the underlying type
+        QualType UnderlyingReturnType =
+            lldb_rpc_gen::GetUnderlyingType(method.ReturnType);
+        ReturnTypeName =
+            UnderlyingReturnType.getAsString(method.Policy) + " &&";
+      } else
+        ReturnTypeName = method.ReturnType.getAsString(method.Policy);
+
+      AssignLineStream << ReturnTypeName << " " << ReturnVariableName << " = "
+                       << MethodCall << ";";
+    }
+    EmitLine(AssignLine);
+  }
+
+  const bool IsEncodingSBClass =
+      lldb_rpc_ge...
[truncated]

Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK in isolation, a bit like looking at a TableGen backend, easier to judge the output than the emitters.

Please look over the FIXMEs and see if anything can be done right now, I highlighted one that is potentially a problem but the rest don't seem important. See if you agree.

Also please remove any that are no longer relevant.

void RPCServerSourceEmitter::EmitMethodCallAndEncode(const Method &method) {
// FIXME: The hand-written lldb-rpc-server currently doesn't emit destructors
// for LocalObjectRefs... even if the type is an ObjectRef. What should we do
// here?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the impact of this and, to repeat the comment, what should we do here?

Is this a nice to have or a memory leak waiting to happen if someone uses a LocalObjectRef in the right/wrong way?

(whatever a LocalObjectRef is :) )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this FIXME can be removed. There should be no impact because a LocalObjectRef's only member is a shared pointer to an ObjectRef (which does have a proper destructor).

Future PRs should have the proper definitions for ObjectRef and LocalObjectRef, but effectively every SB class inherits from one of them. ObjectRef holds onto a connection, a class ID (so you know what kind of object it is), and an object ID (so you know which object it refers to).

Copy link
Contributor Author

@chelcassanova chelcassanova May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to add on to this, ObjectRefs are held on to by the server (they're usually used to store larger amounts of information about an RPC object and its connection) while LocalObjectRefs are used locally for smaller classes (like Lists and Specs if I'm not mistaken).

EmitLine(
"// Intentionally inserting a compiler error. lldb-rpc-gen "
"was unable to determine how large the return buffer should be.");
EmitLine("ThisShouldNotCompile");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#error <something vaguely useful> would be a bit more standard and provide some help to debug this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this with my latest push.

@DavidSpickett DavidSpickett changed the title [lldb[RPC] Upstream RPC server interface emitters [lldb][RPC] Upstream RPC server interface emitters May 6, 2025
@chelcassanova chelcassanova force-pushed the upstream-lldb-rpc/add-lldb-rpc-server-emitters branch 2 times, most recently from e037e27 to 8a96a26 Compare May 14, 2025 16:17
@chelcassanova
Copy link
Contributor Author

Pushed again to address David's comments and remove most of the FIXMEs. There's one FIXME I've kept in for now:

// FIXME: SB class server references are stored as non-const references so
// that we can actually change them as needed. If a parameter is marked
// const, we will fail to compile because we cannot make an
// SBFooServerReference from a `const SBFoo &`.
// To work around this issue, we'll apply a `const_cast` if needed so we
// can continue to generate callbacks for now, but we really should
// rethink the way we store object IDs server-side to support
// const-qualified parameters.

@bulbazord This is referring to how we emit storage for const SB parameters in callback functions. Currently it looks like it only gets applied to one function that gets generated. Any thoughts here?

@bulbazord
Copy link
Member

Pushed again to address David's comments and remove most of the FIXMEs. There's one FIXME I've kept in for now:

// FIXME: SB class server references are stored as non-const references so
// that we can actually change them as needed. If a parameter is marked
// const, we will fail to compile because we cannot make an
// SBFooServerReference from a `const SBFoo &`.
// To work around this issue, we'll apply a `const_cast` if needed so we
// can continue to generate callbacks for now, but we really should
// rethink the way we store object IDs server-side to support
// const-qualified parameters.

@bulbazord This is referring to how we emit storage for const SB parameters in callback functions. Currently it looks like it only gets applied to one function that gets generated. Any thoughts here?

Given it only applies to one function, it's probably okay to keep it around for now. The fact that we need a const_cast at all is concerning but I think we've done a reasonably alright job at explaining why it exists (though I am a partial author, so maybe my perspective is biased).

Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments to be addressed but otherwise I have no problems with this.

@chelcassanova chelcassanova force-pushed the upstream-lldb-rpc/add-lldb-rpc-server-emitters branch from 8a96a26 to a6c3566 Compare May 27, 2025 18:35
Copy link

github-actions bot commented May 27, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@chelcassanova chelcassanova force-pushed the upstream-lldb-rpc/add-lldb-rpc-server-emitters branch 2 times, most recently from 8732082 to b6edf90 Compare May 27, 2025 19:39
@chelcassanova
Copy link
Contributor Author

I added some small shell tests to this patch that check the output of the emitter mainly for sanity checking. @bulbazord I think having basic tests like what I added would be good for the server-side emitter, but if there's anything other thing you can think of test here please let me know!

This commit upstreams the LLDB RPC server interface emitters. These
emitters generate the server-side API interfaces for RPC, which
communicate directly with liblldb itself out of process using the SB
API.

https://discourse.llvm.org/t/rfc-upstreaming-lldb-rpc/85804
@chelcassanova chelcassanova force-pushed the upstream-lldb-rpc/add-lldb-rpc-server-emitters branch from b6edf90 to c209f35 Compare May 27, 2025 19:58
@chelcassanova
Copy link
Contributor Author

@DavidSpickett I think I've address any previous comments to this point and that this patch is ready to land (contingent on the rpc-gen tool itself landing first). If possible, could you give this another once over?

Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the UNSUPPORTED changed to apply to any system.

@@ -0,0 +1,15 @@
# Disabling until the lldb-rpc-gen tool lands.
UNSUPPORTED: system-windows, system-linux, system-darwin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNSUPPORTED: * should disable it everywhere.

Saves someone on FreeBSD getting an unpleasant surprise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do that for the rest of the tests as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants