[lldb][RPC] Upstream lldb-rpc-gen tool #138031
Conversation
|
@llvm/pr-subscribers-lldb Author: Chelsea Cassanova (chelcassanova) ChangesThis commit upstreams the https://discourse.llvm.org/t/rfc-upstreaming-lldb-rpc/85804 Patch is 45.25 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/138031.diff 3 Files Affected:
diff --git a/lldb/tools/lldb-rpc/lldb-rpc-gen/RPCCommon.cpp b/lldb/tools/lldb-rpc/lldb-rpc-gen/RPCCommon.cpp
new file mode 100644
index 0000000000000..2059ada774ccb
--- /dev/null
+++ b/lldb/tools/lldb-rpc/lldb-rpc-gen/RPCCommon.cpp
@@ -0,0 +1,535 @@
+//===-- RPCCommon.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 "RPCCommon.h"
+
+#include "clang/AST/AST.h"
+#include "clang/AST/Mangle.h"
+#include "clang/Lex/Lexer.h"
+
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang;
+
+// We intentionally do not generate some classes because they are currently
+// inconvenient, they aren't really used by most consumers, or we're not sure
+// why they exist.
+static constexpr llvm::StringRef DisallowedClasses[] = {
+ "SBCommunication", // What is this used for?
+ "SBInputReader", // What is this used for?
+ "SBCommandPluginInterface", // This is hard to support, we can do it if
+ // really needed though.
+ "SBCommand", // There's nothing too difficult about this one, but many of
+ // its methods take a SBCommandPluginInterface pointer so
+ // there's no reason to support this.
+};
+
+// We intentionally avoid generating certain methods either because they are
+// difficult to support correctly or they aren't really used much from C++.
+// FIXME: We should be able to annotate these methods instead of maintaining a
+// list in the generator itself.
+static constexpr llvm::StringRef DisallowedMethods[] = {
+ // The threading functionality in SBHostOS is deprecated and thus we do not
+ // generate them. It would be ideal to add the annotations to the methods
+ // and then support not generating deprecated methods. However, without
+ // annotations the generator generates most things correctly. This one is
+ // problematic because it returns a pointer to an "opaque" structure
+ // (thread_t) that is not `void *`, so special casing it is more effort than
+ // it's worth.
+ "_ZN4lldb8SBHostOS10ThreadJoinEP17_opaque_pthread_tPPvPNS_7SBErrorE",
+ "_ZN4lldb8SBHostOS12ThreadCancelEP17_opaque_pthread_tPNS_7SBErrorE",
+ "_ZN4lldb8SBHostOS12ThreadCreateEPKcPFPvS3_ES3_PNS_7SBErrorE",
+ "_ZN4lldb8SBHostOS12ThreadDetachEP17_opaque_pthread_tPNS_7SBErrorE",
+ "_ZN4lldb8SBHostOS13ThreadCreatedEPKc",
+};
+
+static constexpr llvm::StringRef ClassesWithoutDefaultCtor[] = {
+ "SBHostOS",
+ "SBReproducer",
+};
+
+static constexpr llvm::StringRef ClassesWithoutCopyOperations[] = {
+ "SBHostOS",
+ "SBReproducer",
+ "SBStream",
+ "SBProgress",
+};
+
+static constexpr llvm::StringRef MethodsWithPointerPlusLen[] = {
+ "_ZN4lldb6SBData11ReadRawDataERNS_7SBErrorEyPvm",
+ "_ZN4lldb6SBData7SetDataERNS_7SBErrorEPKvmNS_9ByteOrderEh",
+ "_ZN4lldb6SBData20SetDataWithOwnershipERNS_7SBErrorEPKvmNS_9ByteOrderEh",
+ "_ZN4lldb6SBData25CreateDataFromUInt64ArrayENS_9ByteOrderEjPym",
+ "_ZN4lldb6SBData25CreateDataFromUInt32ArrayENS_9ByteOrderEjPjm",
+ "_ZN4lldb6SBData25CreateDataFromSInt64ArrayENS_9ByteOrderEjPxm",
+ "_ZN4lldb6SBData25CreateDataFromSInt32ArrayENS_9ByteOrderEjPim",
+ "_ZN4lldb6SBData25CreateDataFromDoubleArrayENS_9ByteOrderEjPdm",
+ "_ZN4lldb6SBData22SetDataFromUInt64ArrayEPym",
+ "_ZN4lldb6SBData22SetDataFromUInt32ArrayEPjm",
+ "_ZN4lldb6SBData22SetDataFromSInt64ArrayEPxm",
+ "_ZN4lldb6SBData22SetDataFromSInt32ArrayEPim",
+ "_ZN4lldb6SBData22SetDataFromDoubleArrayEPdm",
+ "_ZN4lldb10SBDebugger22GetDefaultArchitectureEPcm",
+ "_ZN4lldb10SBDebugger13DispatchInputEPvPKvm",
+ "_ZN4lldb10SBDebugger13DispatchInputEPKvm",
+ "_ZN4lldb6SBFile4ReadEPhmPm",
+ "_ZN4lldb6SBFile5WriteEPKhmPm",
+ "_ZNK4lldb10SBFileSpec7GetPathEPcm",
+ "_ZN4lldb10SBFileSpec11ResolvePathEPKcPcm",
+ "_ZN4lldb8SBModule10GetVersionEPjj",
+ "_ZN4lldb12SBModuleSpec12SetUUIDBytesEPKhm",
+ "_ZNK4lldb9SBProcess9GetSTDOUTEPcm",
+ "_ZNK4lldb9SBProcess9GetSTDERREPcm",
+ "_ZNK4lldb9SBProcess19GetAsyncProfileDataEPcm",
+ "_ZN4lldb9SBProcess10ReadMemoryEyPvmRNS_7SBErrorE",
+ "_ZN4lldb9SBProcess11WriteMemoryEyPKvmRNS_7SBErrorE",
+ "_ZN4lldb9SBProcess21ReadCStringFromMemoryEyPvmRNS_7SBErrorE",
+ "_ZNK4lldb16SBStructuredData14GetStringValueEPcm",
+ "_ZN4lldb8SBTarget23BreakpointCreateByNamesEPPKcjjRKNS_"
+ "14SBFileSpecListES6_",
+ "_ZN4lldb8SBTarget10ReadMemoryENS_9SBAddressEPvmRNS_7SBErrorE",
+ "_ZN4lldb8SBTarget15GetInstructionsENS_9SBAddressEPKvm",
+ "_ZN4lldb8SBTarget25GetInstructionsWithFlavorENS_9SBAddressEPKcPKvm",
+ "_ZN4lldb8SBTarget15GetInstructionsEyPKvm",
+ "_ZN4lldb8SBTarget25GetInstructionsWithFlavorEyPKcPKvm",
+ "_ZN4lldb8SBThread18GetStopDescriptionEPcm",
+ // The below mangled names are used for dummy methods in shell tests
+ // that test the emitters' output. If you're adding any new mangled names
+ // from the actual SB API to this list please add them above.
+ "_ZN4lldb33SBRPC_"
+ "CHECKCONSTCHARPTRPTRWITHLEN27CheckConstCharPtrPtrWithLenEPPKcm",
+ "_ZN4lldb19SBRPC_CHECKARRAYPTR13CheckArrayPtrEPPKcm",
+ "_ZN4lldb18SBRPC_CHECKVOIDPTR12CheckVoidPtrEPvm",
+};
+
+// These methods should take a connection parameter according to our logic in
+// RequiresConnectionParameter() but in the handwritten version they
+// don't take a connection. These methods need to have their implementation
+// changed but for now, we just have an exception list of functions that will
+// never be a given connection parameter.
+//
+// FIXME: Change the implementation of these methods so that they can be given a
+// connection parameter.
+static constexpr llvm::StringRef
+ MethodsThatUnconditionallyDoNotNeedConnection[] = {
+ "_ZN4lldb16SBBreakpointNameC1ERNS_8SBTargetEPKc",
+ "_ZN4lldb10SBDebugger7DestroyERS0_",
+ "_ZN4lldb18SBExecutionContextC1ENS_8SBThreadE",
+};
+
+// These classes inherit from rpc::ObjectRef directly (as opposed to
+// rpc::LocalObjectRef). Changing them from ObjectRef to LocalObjectRef is ABI
+// breaking, so we preserve that compatibility here.
+//
+// lldb-rpc-gen emits classes as LocalObjectRefs by default.
+//
+// FIXME: Does it matter which one it emits by default?
+static constexpr llvm::StringRef ClassesThatInheritFromObjectRef[] = {
+ "SBAddress",
+ "SBBreakpointName",
+ "SBCommandInterpreter",
+ "SBCommandReturnObject",
+ "SBError",
+ "SBExecutionContext",
+ "SBExpressionOptions",
+ "SBFileSpec",
+ "SBFileSpecList",
+ "SBFormat",
+ "SBFunction",
+ "SBHistoricalFrame",
+ "SBHistoricalLineEntry",
+ "SBHistoricalLineEntryList",
+ "SBLineEntry",
+ "SBStream",
+ "SBStringList",
+ "SBStructuredData",
+ "SBSymbolContext",
+ "SBSymbolContextList",
+ "SBTypeMember",
+ "SBTypeSummaryOptions",
+ "SBValueList",
+};
+
+static llvm::StringMap<llvm::SmallVector<llvm::StringRef>>
+ ClassName_to_ParameterTypes = {
+ {"SBLaunchInfo", {"const char *"}},
+ {"SBPlatformConnectOptions", {"const char *"}},
+ {"SBPlatformShellCommand", {"const char *", "const char *"}},
+ {"SBBreakpointList", {"SBTarget"}},
+};
+
+QualType lldb_rpc_gen::GetUnderlyingType(QualType T) {
+ QualType UnderlyingType;
+ if (T->isPointerType())
+ UnderlyingType = T->getPointeeType();
+ else if (T->isReferenceType())
+ UnderlyingType = T.getNonReferenceType();
+ else
+ UnderlyingType = T;
+
+ return UnderlyingType;
+}
+
+QualType lldb_rpc_gen::GetUnqualifiedUnderlyingType(QualType T) {
+ QualType UnderlyingType = GetUnderlyingType(T);
+ return UnderlyingType.getUnqualifiedType();
+}
+
+std::string lldb_rpc_gen::GetMangledName(ASTContext &Context,
+ CXXMethodDecl *MDecl) {
+ std::string Mangled;
+ llvm::raw_string_ostream MangledStream(Mangled);
+
+ GlobalDecl GDecl;
+ if (const auto *CtorDecl = dyn_cast<CXXConstructorDecl>(MDecl))
+ GDecl = GlobalDecl(CtorDecl, Ctor_Complete);
+ else if (const auto *DtorDecl = dyn_cast<CXXDestructorDecl>(MDecl))
+ GDecl = GlobalDecl(DtorDecl, Dtor_Deleting);
+ else
+ GDecl = GlobalDecl(MDecl);
+
+ MangleContext *MC = Context.createMangleContext();
+ MC->mangleName(GDecl, MangledStream);
+ return Mangled;
+}
+
+bool lldb_rpc_gen::TypeIsFromLLDBPrivate(QualType T) {
+
+ auto CheckTypeForLLDBPrivate = [](const Type *Ty) {
+ if (!Ty)
+ return false;
+ const auto *CXXRDecl = Ty->getAsCXXRecordDecl();
+ if (!CXXRDecl)
+ return false;
+ const auto *NSDecl =
+ llvm::dyn_cast<NamespaceDecl>(CXXRDecl->getDeclContext());
+ if (!NSDecl)
+ return false;
+ return NSDecl->getName() == "lldb_private";
+ };
+
+ // First, get the underlying type (remove qualifications and strip off any
+ // pointers/references). Then we'll need to desugar this type. This will
+ // remove things like typedefs, so instead of seeing "lldb::DebuggerSP" we'll
+ // actually see something like "std::shared_ptr<lldb_private::Debugger>".
+ QualType UnqualifiedUnderlyingType = GetUnqualifiedUnderlyingType(T);
+ const Type *DesugaredType =
+ UnqualifiedUnderlyingType->getUnqualifiedDesugaredType();
+ assert(DesugaredType && "DesugaredType from a valid Type is nullptr!");
+
+ // Check the type itself.
+ if (CheckTypeForLLDBPrivate(DesugaredType))
+ return true;
+
+ // If that didn't work, it's possible that the type has a template argument
+ // that is an lldb_private type.
+ if (const auto *TemplateSDecl =
+ llvm::dyn_cast_or_null<ClassTemplateSpecializationDecl>(
+ DesugaredType->getAsCXXRecordDecl())) {
+ for (const TemplateArgument &TA :
+ TemplateSDecl->getTemplateArgs().asArray()) {
+ if (TA.getKind() != TemplateArgument::Type)
+ continue;
+ if (CheckTypeForLLDBPrivate(TA.getAsType().getTypePtr()))
+ return true;
+ }
+ }
+ return false;
+}
+
+bool lldb_rpc_gen::TypeIsSBClass(QualType T) {
+ QualType UnqualifiedUnderlyingType = GetUnqualifiedUnderlyingType(T);
+ const auto *CXXRDecl = UnqualifiedUnderlyingType->getAsCXXRecordDecl();
+ if (!CXXRDecl)
+ return false; // SB Classes are always C++ classes
+
+ return CXXRDecl->getName().starts_with("SB");
+}
+
+bool lldb_rpc_gen::TypeIsConstCharPtr(QualType T) {
+ if (!T->isPointerType())
+ return false;
+
+ QualType UnderlyingType = T->getPointeeType();
+ if (!UnderlyingType.isConstQualified())
+ return false;
+
+ // FIXME: We should be able to do `UnderlyingType->isCharType` but that will
+ // return true for `const uint8_t *` since that is effectively an unsigned
+ // char pointer. We currently do not support pointers other than `const char
+ // *` and `const char **`.
+ return UnderlyingType->isSpecificBuiltinType(BuiltinType::Char_S) ||
+ UnderlyingType->isSpecificBuiltinType(BuiltinType::SChar);
+}
+
+bool lldb_rpc_gen::TypeIsConstCharPtrPtr(QualType T) {
+ if (!T->isPointerType())
+ return false;
+
+ return TypeIsConstCharPtr(T->getPointeeType());
+}
+
+bool lldb_rpc_gen::TypeIsDisallowedClass(QualType T) {
+ QualType UUT = GetUnqualifiedUnderlyingType(T);
+ const auto *CXXRDecl = UUT->getAsCXXRecordDecl();
+ if (!CXXRDecl)
+ return false;
+
+ llvm::StringRef DeclName = CXXRDecl->getName();
+ for (const llvm::StringRef DisallowedClass : DisallowedClasses)
+ if (DeclName == DisallowedClass)
+ return true;
+ return false;
+}
+
+bool lldb_rpc_gen::TypeIsCallbackFunctionPointer(QualType T) {
+ return T->isFunctionPointerType();
+}
+
+bool lldb_rpc_gen::MethodIsDisallowed(const std::string &MangledName) {
+ llvm::StringRef MangledNameRef(MangledName);
+ return llvm::is_contained(DisallowedMethods, MangledNameRef);
+}
+
+bool lldb_rpc_gen::HasCallbackParameter(CXXMethodDecl *MDecl) {
+ bool HasCallbackParameter = false;
+ bool HasBatonParameter = false;
+ auto End = MDecl->parameters().end();
+ for (auto Iter = MDecl->parameters().begin(); Iter != End; Iter++) {
+ if ((*Iter)->getType()->isFunctionPointerType()) {
+ HasCallbackParameter = true;
+ continue;
+ }
+
+ if ((*Iter)->getType()->isVoidPointerType())
+ HasBatonParameter = true;
+ }
+
+ return HasCallbackParameter && HasBatonParameter;
+}
+
+// FIXME: Find a better way to do this. Here is why it is written this way:
+// By the time we have already created a `Method` object, we have extracted the
+// `QualifiedName` and the relevant QualTypes for parameters/return types, many
+// of which contains "lldb::" in them. To change it in a way that would be
+// friendly to liblldbrpc, we would need to have a way of replacing that
+// namespace at the time of creating a Method, and only for liblldbrpc methods.
+// IMO this would complicate Method more than what I'm doing here, and not
+// necessarily for any more benefit.
+// In clang-tools-extra, there is a ChangeNamespaces tool which tries to do
+// something similar to this. It also operates primarily on string replacement,
+// but uses more sophisticated clang tooling to do so.
+// For now, this will do what we need it to do.
+std::string
+lldb_rpc_gen::ReplaceLLDBNamespaceWithRPCNamespace(std::string Name) {
+ auto Pos = Name.find("lldb::");
+ while (Pos != std::string::npos) {
+ constexpr size_t SizeOfLLDBNamespace = 4;
+ Name.replace(Pos, SizeOfLLDBNamespace, "lldb_rpc");
+ Pos = Name.find("lldb::");
+ }
+ return Name;
+}
+
+std::string lldb_rpc_gen::StripLLDBNamespace(std::string Name) {
+ auto Pos = Name.find("lldb::");
+ if (Pos != std::string::npos) {
+ constexpr size_t SizeOfLLDBNamespace = 6;
+ Name = Name.substr(Pos + SizeOfLLDBNamespace);
+ }
+ return Name;
+}
+
+bool lldb_rpc_gen::SBClassRequiresDefaultCtor(const std::string &ClassName) {
+ return !llvm::is_contained(ClassesWithoutDefaultCtor, ClassName);
+}
+
+bool lldb_rpc_gen::SBClassRequiresCopyCtorAssign(const std::string &ClassName) {
+ return !llvm::is_contained(ClassesWithoutCopyOperations, ClassName);
+}
+
+bool lldb_rpc_gen::SBClassInheritsFromObjectRef(const std::string &ClassName) {
+ return llvm::is_contained(ClassesThatInheritFromObjectRef, ClassName);
+}
+
+std::string lldb_rpc_gen::GetSBClassNameFromType(QualType T) {
+ assert(lldb_rpc_gen::TypeIsSBClass(T) &&
+ "Cannot get SBClass name from non-SB class type!");
+
+ QualType UnqualifiedUnderlyingType = GetUnqualifiedUnderlyingType(T);
+ const auto *CXXRDecl = UnqualifiedUnderlyingType->getAsCXXRecordDecl();
+ assert(CXXRDecl && "SB class was not CXXRecordDecl!");
+ if (!CXXRDecl)
+ return std::string();
+
+ return CXXRDecl->getName().str();
+}
+lldb_rpc_gen::Method::Method(CXXMethodDecl *MDecl, const PrintingPolicy &Policy,
+ ASTContext &Context)
+ : Policy(Policy), Context(Context),
+ QualifiedName(MDecl->getQualifiedNameAsString()),
+ BaseName(MDecl->getNameAsString()),
+ MangledName(lldb_rpc_gen::GetMangledName(Context, MDecl)),
+ ReturnType(MDecl->getReturnType()), IsConst(MDecl->isConst()),
+ IsInstance(MDecl->isInstance()), IsCtor(isa<CXXConstructorDecl>(MDecl)),
+ IsCopyAssign(MDecl->isCopyAssignmentOperator()),
+ IsMoveAssign(MDecl->isMoveAssignmentOperator()),
+ IsDtor(isa<CXXDestructorDecl>(MDecl)),
+ IsConversionMethod(isa<CXXConversionDecl>(MDecl)) {
+ uint8_t UnnamedArgIdx = 0;
+ bool PrevParamWasPointer = false;
+ for (const auto *ParamDecl : MDecl->parameters()) {
+ Param param;
+ if (ParamDecl->hasDefaultArg())
+ param.DefaultValueText =
+ Lexer::getSourceText(
+ CharSourceRange::getTokenRange(
+ ParamDecl->getDefaultArg()->getSourceRange()),
+ Context.getSourceManager(), Context.getLangOpts())
+ .str();
+
+ param.IsFollowedByLen = false;
+ param.Name = ParamDecl->getNameAsString();
+ // If the parameter has no name, we'll generate one
+ if (param.Name.empty()) {
+ param.Name = "arg" + std::to_string(UnnamedArgIdx);
+ UnnamedArgIdx++;
+ }
+ param.Type = ParamDecl->getType();
+
+ // FIXME: Instead of using this heuristic, the ideal thing would be to add
+ // annotations to the SBAPI methods themselves. For now, we have a list of
+ // methods that we know will need this.
+ if (PrevParamWasPointer) {
+ PrevParamWasPointer = false;
+ const bool IsIntegerType = param.Type->isIntegerType() &&
+ !param.Type->isBooleanType() &&
+ !param.Type->isEnumeralType();
+ if (IsIntegerType && llvm::is_contained(MethodsWithPointerPlusLen,
+ llvm::StringRef(MangledName)))
+ Params.back().IsFollowedByLen = true;
+ }
+
+ if (param.Type->isPointerType() &&
+ !lldb_rpc_gen::TypeIsConstCharPtr(param.Type) &&
+ !param.Type->isFunctionPointerType())
+ PrevParamWasPointer = true;
+
+ if (param.Type->isFunctionPointerType())
+ ContainsFunctionPointerParameter = true;
+
+ Params.push_back(param);
+ }
+
+ if (IsInstance)
+ ThisType = MDecl->getThisType();
+
+ if (const auto *CtorDecl = dyn_cast<CXXConstructorDecl>(MDecl)) {
+ IsExplicitCtorOrConversionMethod = CtorDecl->isExplicit();
+ IsCopyCtor = CtorDecl->isCopyConstructor();
+ IsMoveCtor = CtorDecl->isMoveConstructor();
+ } else if (const auto *ConversionDecl = dyn_cast<CXXConversionDecl>(MDecl))
+ IsExplicitCtorOrConversionMethod = ConversionDecl->isExplicit();
+}
+
+bool lldb_rpc_gen::Method::operator<(const lldb_rpc_gen::Method &rhs) const {
+ return this < &rhs;
+}
+
+std::string
+lldb_rpc_gen::Method::CreateParamListAsString(GenerationKind Generation,
+ bool IncludeDefaultValue) const {
+ assert((!IncludeDefaultValue || Generation == eLibrary) &&
+ "Default values should only be emitted on the library side!");
+
+ std::vector<std::string> ParamList;
+
+ if (Generation == eLibrary && RequiresConnectionParameter())
+ ParamList.push_back("const rpc::Connection &connection");
+
+ for (const auto &Param : Params) {
+ std::string ParamString;
+ llvm::raw_string_ostream ParamStringStream(ParamString);
+
+ if (Generation == eLibrary)
+ ParamStringStream << lldb_rpc_gen::ReplaceLLDBNamespaceWithRPCNamespace(
+ Param.Type.getAsString(Policy));
+ else
+ ParamStringStream << Param.Type.getAsString(Policy);
+
+ ParamStringStream << " " << Param.Name;
+ if (IncludeDefaultValue && Generation == eLibrary &&
+ !Param.DefaultValueText.empty())
+ ParamStringStream << " = "
+ << lldb_rpc_gen::ReplaceLLDBNamespaceWithRPCNamespace(
+ Param.DefaultValueText);
+
+ ParamList.push_back(ParamString);
+ }
+
+ return llvm::join(ParamList, ", ");
+}
+
+bool lldb_rpc_gen::Method::RequiresConnectionParameter() const {
+ if (llvm::is_contained(MethodsThatUnconditionallyDoNotNeedConnection,
+ MangledName)) {
+ return false;
+ }
+ if (!IsCtor && IsInstance)
+ return false;
+ if (IsCopyCtor || IsMoveCtor)
+ return false;
+ for (const auto &Param : Params)
+ // We can re-use the connection from our parameter if possible.
+ // Const-qualified parameters are input parameters and already
+ // have a valid connection to provide to the current method.
+ if (TypeIsSBClass(Param.Type) &&
+ GetUnderlyingType(Param.Type).isConstQualified())
+ return false;
+
+ return true;
+}
+
+std::string lldb_rpc_gen::GetDefaultArgumentsForConstructor(
+ std::string ClassName, const lldb_rpc_gen::Method &method) {
+
+ std::string ParamString;
+
+ const llvm::SmallVector<llvm::StringRef> &ParamTypes =
+ ClassName_to_ParameterTypes[ClassName];
+ std::vector<std::string> Params;
+
+ Params.push_back("connection_sp");
+ for (auto &ParamType : ParamTypes) {
+ if (ParamType == "const char *")
+ Params.push_back("nullptr");
+ else if (ParamType == "bool")
+ Params.push_back("false")...
[truncated]
|
There was a problem hiding this comment.
Given that I wrote some of this code, it looks mostly good to me 😄 Note that this has a dependency on the emitters so this probably shouldn't land quite yet.
I commented on some of the FIXMEs because I think now is the time to really address them. It would also be great to get a fresh pair of eyes on this...
| // (thread_t) that is not `void *`, so special casing it is more effort than | ||
| // it's worth. | ||
| "_ZN4lldb8SBHostOS10ThreadJoinEP17_opaque_pthread_tPPvPNS_7SBErrorE", | ||
| "_ZN4lldb8SBHostOS12ThreadCancelEP17_opaque_pthread_tPNS_7SBErrorE", |
There was a problem hiding this comment.
I wonder if we could take advantage of the deprecated annotation on these methods and remove this... 🤔
There was a problem hiding this comment.
We should be able to, it would change MethodIsDisallowed to just check for some underlying DeprecatedAttr instead of reading from this list.
There was a problem hiding this comment.
Trying that idea out, it seems that just checking the attributes on a given method to see if it has a deprecated attribute isn't working? I'm wondering if it's because these methods are marked as deprecated using a preprocessor macro that wraps around the [[deprecated]] attribute itself... 🤔
There was a problem hiding this comment.
Adding the [[deprecated]] attribute does allow the approach of just checking the attributes with Clang to work as intended so we'll probably have to do some preprocessor reading to make sure that we're capturing that LLDB_DEPRECATED macro.
There was a problem hiding this comment.
@bulbazord Actually, I think this preprocessor macro has its link to the [[deprecated]] attribute removed in SBDefines if we're generating the SWIG bindings:
#if defined(SWIG) || _cplusplus < 201402L
#undef LLDB_DEPRECATED
#undef LLDB_DEPRECATED_FIXME
#define LLDB_DEPRECATED(MSG)
#define LLDB_DEPRECATED_FIXME(MSG, FIX)
#endif
and we're always generating the SWIG bindings when building RPC because of testing right? The definition for LLDB_DEPRECATED that ties it to the compiler's deprecated attribute is coming from lldb-defines.h and is overridden in SBDefines.h to just be blank (IIUC?). I think this means that when lldb-rpc-gen is reading any function that has LLDB_DEPRECATED, there's no actual [[deprecated]] attribute for the tool to pick up on.
I don't currently know why we don't add the deprecated attribute when generating SWIG bindings. If that's something that cannot change, is there a better way to work around this? If not then keeping the list and adding a note; as well as adding a check for the deprecated attribute anyways in MethodIsDisallowed would be the way to go here.
There was a problem hiding this comment.
lldb-rpc-gen should not satisfy defined(SWIG) so you should have access to the [[deprecated]] annotation.
| "SBProgress", | ||
| }; | ||
|
|
||
| static constexpr llvm::StringRef MethodsWithPointerPlusLen[] = { |
There was a problem hiding this comment.
I'd love if we could remove this list...
There was a problem hiding this comment.
Ismail had an idea here to use a typedef over methods that used pointers and lens so that the tool could pick it up and I liked that idea. Otherwise, if we have a function that has a pointer and a length then we have no way of knowing whether or not they're related to each other.
There was a problem hiding this comment.
So each method would get its own typedef?
There was a problem hiding this comment.
I misspoke here, it wouldn't be a typedef but a new SB class that holds a struct that would be used in place of pointer + lens. This class could then be picked up by the tool (i.e. if one of the function params uses this class) to know if it's a pointer + len method without using this list.
There was a problem hiding this comment.
I'm still not sure what this would look like. Do you have some pseudocode or a sketch of the idea? I'm hesitant to add an entire new class just for RPC's sake, but it's hard to evaluate without more details.
There was a problem hiding this comment.
So you'd have a class called something like SBBuffer and it would have a struct:
struct Buffer {
Buffer(void *buf, size_t size) {}
};
that covers a pointer + len. In a function that uses it (e.g. size_t ReadRawData(lldb::SBError &error, lldb::offset_t offset, void *buf, size_t size) becomes size_t ReadRawData(lldb::SBError &error, lldb::offset_t offset, SBBuffer buf).
The tool could see that the method has an SBBuffer parameter, so that could satisfy the "this method is using a pointer followed by a length". The older version of the original method would be marked as deprecated.
Obviously the main task here is having a cleaner way of knowing this detail about methods without maintaining an ever growing list. Another path we could try is using Clang's annotate attribute to note this information on the API side. The tool should also be able to pick up on this, but it also means adding an attribute to the main APIs that isn't very meaningful if you're not using RPC.
There was a problem hiding this comment.
I see. I think something like that could work, but it'd be a larger change that would require buy-in from the larger community and an audit of the SBAPI in general.
| // FIXME: Change the implementation of these methods so that they can be given a | ||
| // connection parameter. | ||
| static constexpr llvm::StringRef | ||
| MethodsThatUnconditionallyDoNotNeedConnection[] = { |
There was a problem hiding this comment.
It'd also be great if we could fix these up? These exceptions only exist for our internal clients.
There was a problem hiding this comment.
Meaning that we'd only keep this downstream?
| // lldb-rpc-gen emits classes as LocalObjectRefs by default. | ||
| // | ||
| // FIXME: Does it matter which one it emits by default? | ||
| static constexpr llvm::StringRef ClassesThatInheritFromObjectRef[] = { |
There was a problem hiding this comment.
I think something we should do while upstreaming is figure out what the differences between ObjectRef and LocalObjectRef really are and if they actually have any advantages for us.
There was a problem hiding this comment.
From how I understand that they work, all LocalObjectRefs could possibly wrapped up into ObjectRefs to avoid having 2 classes here.
| return HasCallbackParameter && HasBatonParameter; | ||
| } | ||
|
|
||
| // FIXME: Find a better way to do this. Here is why it is written this way: |
There was a problem hiding this comment.
We should look into this FIXME now that we're upstreaming this. Maybe somebody more familiar with clang tooling can help us.
There was a problem hiding this comment.
So I have a couple thoughts on this FIXME. Just to clarify the main thought from your original comment, since we're picking up all our info from liblldb, qualified types for methods and their return types/params have the lldb:: namespace and all references to this need to change to using lldb_rpc::. Currently, we change these namespaces as needed using this replace namespace function that we've implemented, but we'd rather replace those namespaces upon creating a Method object instead of when we reference information from Method.
I like the idea of using Clang's ChangeNamespace tool instead of doing our own string replacement here, but looking at some example code for it it looks like it operates at the file level. (i.e. it takes a pattern of files to do the namespace changing). If we were to try using this, that gives me the initial idea of allowing lldb-rpc-gen to write everything with the original lldb:: namespace and then using ChangeNamespace once everything has been said and done to replace the namespaces after lldb-rpc-gen has emitted everything.
I think we could also keep this ReplaceLLDBNamespaceWithRPCNamespace method, but possibly just use it to replace the strings that Method uses to hold on to the qualified name. This should reduce some of the places it's being used, but doesn't address return types or params since we hold on to those as QualTypes which is going to have the original namespaces.
It would be nice if the namespaces were already changed when coming into lldb-rpc-gen, which I think that ChangeNamespace could do, but it would apply that change to the original SB API files? That would possibly mess up the info we need in other unsavoury ways.
There was a problem hiding this comment.
I think operating in 2 passes could be a good idea here. It was a bit of whack-a-mole getting the ReplaceLLDBNamespaceWithRPCNamespace stuff all working as intended, so it'd greatly simplify the code and probably be easier to reason about.
The key would be to only apply it to the library. The server links against LLDB directly.
There was a problem hiding this comment.
I think operating in 2 passes could be a good idea here.
If we're ok with having a dependency on clang-tools-extra, then we could use the clang-change-namespace tool itself after the files have been generated. Otherwise, if we want to reimplement the code from ChangeNamespace in lldb-rpc-gen, that looks like it should be feasible? Again, I'm just looking at the code from the change namespace stuff but I think it's backed by Refactoring in Clang tooling. We already have that dependency, I think the change namespace code is a specific application of RefactoringTool.
This could be worth pursuing. It's not just that this cuts down on our string replacement function, but this might also be useful for cutting out the Python scripts that we're using to modify the namespaces for the lldb headers.
The key would be to only apply it to the library.
Correct. As it is right now, lldb-rpc-gen generates all server, client and byproduct code within the same file, I wonder if this lends credence to subtooling this? :)
chelcassanova
force-pushed
the
upstream-lldb-rpc/add-lldb-rpc-gen-tool
branch
from
9c709e6 to
59dcdc0
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
chelcassanova
force-pushed
the
upstream-lldb-rpc/add-lldb-rpc-gen-tool
branch
2 times, most recently
from
ad18563 to
85e264d
Compare
chelcassanova
force-pushed
the
upstream-lldb-rpc/add-lldb-rpc-gen-tool
branch
from
85e264d to
57d1877
Compare
There was a problem hiding this comment.
I don't have any objections to the way this works, just some code style.
I don't know enough about the FIXMEs to make you fix them or not, if you can that's great, otherwise consider accepting that they will never be fixed (as is the fate of the average FIXME) and convert them into informative comments instead.
| // really needed though. | ||
| "SBCommand", // There's nothing too difficult about this one, but many of | ||
| // its methods take a SBCommandPluginInterface pointer so | ||
| // there's no reason to support this. |
There was a problem hiding this comment.
Needs extending to explain why SBCommandPluginInterface means there's no reason to support it. Like:
SBCommandPluginInterface is no use in an lldb-rpc context because X Y and Z
|
|
||
| bool lldb_rpc_gen::Method::operator<(const lldb_rpc_gen::Method &rhs) const { | ||
| return this < &rhs; | ||
| } |
There was a problem hiding this comment.
What's the ordering here? Appears to be memory address.
There was a problem hiding this comment.
It should be on memory address, it's used so that we can have Methods in ordered containers (mainly sets).
There was a problem hiding this comment.
Add a comment to say that. I thought there might be some grander logic to it, it's fine that there isn't but document the intent.
chelcassanova
force-pushed
the
upstream-lldb-rpc/add-lldb-rpc-gen-tool
branch
from
57d1877 to
bb382b5
Compare
|
I updated this patch to remove everything that isn't related to how the tool works with the server emitters. This should simplify this patch and make it so that when later emitters are put up (such as the client and bindings emitters), the tool can be updated to accommodate for those. |
chelcassanova
force-pushed
the
upstream-lldb-rpc/add-lldb-rpc-gen-tool
branch
from
bb382b5 to
a431ee3
Compare
This commit upstreams the `lldb-rpc-gen` tool, a ClangTool that generates the LLDB RPC client and server interfaces. https://discourse.llvm.org/t/rfc-upstreaming-lldb-rpc/85804
chelcassanova
force-pushed
the
upstream-lldb-rpc/add-lldb-rpc-gen-tool
branch
from
a431ee3 to
9d6d38f
Compare
There was a problem hiding this comment.
I resolved all the comments that are done, I have a few left open that need addressing.
Then decide with @bulbazord what you do or don't do with the finer details. Everything else is looking good.
|
|
||
| bool lldb_rpc_gen::Method::operator<(const lldb_rpc_gen::Method &rhs) const { | ||
| return this < &rhs; | ||
| } |
There was a problem hiding this comment.
Add a comment to say that. I thought there might be some grander logic to it, it's fine that there isn't but document the intent.
| while (Pos != std::string::npos) { | ||
| constexpr size_t SizeOfLLDBNamespace = 6; | ||
| Name.replace(Pos, SizeOfLLDBNamespace, "lldb_rpc::"); | ||
| Pos = Name.find("lldb::"); |
There was a problem hiding this comment.
const char* lldb_namespace = "lldb::";
auto Pos = Name.find(lldb_namespace);
Then reuse the string later.
This one's pretty hard to mistake but in general, repeating a string is asking for typos.
| if (Pos != std::string::npos) { | ||
| constexpr size_t SizeOfLLDBNamespace = 6; | ||
| Name = Name.substr(Pos + SizeOfLLDBNamespace); | ||
| } |
There was a problem hiding this comment.
The replace with rpc function uses a while loop but this one only strips the first one, is that intentional?
(clearly it's not doing any harm, but should they act the same way?)
This commit upstreams the
lldb-rpc-gentool, a ClangTool that generates the LLDB RPC client and server interfaces.https://discourse.llvm.org/t/rfc-upstreaming-lldb-rpc/85804