[LLDB] Refactored CPlusPlusLanguage::MethodName to break lldb-server dependencies #132274
Conversation
|
@llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) ChangesThis patch addresses the issue #129543. lldb/source/Core/Module.cpp uses few static helpers and the class CPlusPlusLanguage::MethodName from the CPlusPlusLanguage plug-in. The CPlusPlusLanguage plug-in depends on other plug-ins. This causes many plugins linking, including TypeSystemClang and a lot of clang code. After this patch the size of lldb-server is reduced by 9MB. Full diff: https://github.com/llvm/llvm-project/pull/132274.diff 3 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt b/lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
index ccdc4d0ae99b3..5b866ee8edc02 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
+++ b/lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
@@ -2,6 +2,7 @@ add_lldb_library(lldbPluginCPlusPlusLanguage PLUGIN
BlockPointer.cpp
Coroutines.cpp
CPlusPlusLanguage.cpp
+ CPlusPlusLanguageMethod.cpp
CPlusPlusNameParser.cpp
CxxStringTypes.cpp
GenericBitset.cpp
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
index 4b045d12ad494..2696fa87fafbf 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -36,7 +36,6 @@
#include "lldb/ValueObject/ValueObjectVariable.h"
#include "BlockPointer.h"
-#include "CPlusPlusNameParser.h"
#include "Coroutines.h"
#include "CxxStringTypes.h"
#include "Generic.h"
@@ -44,7 +43,6 @@
#include "LibCxxAtomic.h"
#include "LibCxxVariant.h"
#include "LibStdcpp.h"
-#include "MSVCUndecoratedNameParser.h"
#include "lldb/lldb-enumerations.h"
using namespace lldb;
@@ -106,74 +104,6 @@ Language *CPlusPlusLanguage::CreateInstance(lldb::LanguageType language) {
return nullptr;
}
-void CPlusPlusLanguage::MethodName::Clear() {
- m_full.Clear();
- m_basename = llvm::StringRef();
- m_context = llvm::StringRef();
- m_arguments = llvm::StringRef();
- m_qualifiers = llvm::StringRef();
- m_return_type = llvm::StringRef();
- m_parsed = false;
- m_parse_error = false;
-}
-
-static bool ReverseFindMatchingChars(const llvm::StringRef &s,
- const llvm::StringRef &left_right_chars,
- size_t &left_pos, size_t &right_pos,
- size_t pos = llvm::StringRef::npos) {
- assert(left_right_chars.size() == 2);
- left_pos = llvm::StringRef::npos;
- const char left_char = left_right_chars[0];
- const char right_char = left_right_chars[1];
- pos = s.find_last_of(left_right_chars, pos);
- if (pos == llvm::StringRef::npos || s[pos] == left_char)
- return false;
- right_pos = pos;
- uint32_t depth = 1;
- while (pos > 0 && depth > 0) {
- pos = s.find_last_of(left_right_chars, pos);
- if (pos == llvm::StringRef::npos)
- return false;
- if (s[pos] == left_char) {
- if (--depth == 0) {
- left_pos = pos;
- return left_pos < right_pos;
- }
- } else if (s[pos] == right_char) {
- ++depth;
- }
- }
- return false;
-}
-
-static bool IsTrivialBasename(const llvm::StringRef &basename) {
- // Check that the basename matches with the following regular expression
- // "^~?([A-Za-z_][A-Za-z_0-9]*)$" We are using a hand written implementation
- // because it is significantly more efficient then using the general purpose
- // regular expression library.
- size_t idx = 0;
- if (basename.starts_with('~'))
- idx = 1;
-
- if (basename.size() <= idx)
- return false; // Empty string or "~"
-
- if (!std::isalpha(basename[idx]) && basename[idx] != '_')
- return false; // First character (after removing the possible '~'') isn't in
- // [A-Za-z_]
-
- // Read all characters matching [A-Za-z_0-9]
- ++idx;
- while (idx < basename.size()) {
- if (!std::isalnum(basename[idx]) && basename[idx] != '_')
- break;
- ++idx;
- }
-
- // We processed all characters. It is a vaild basename.
- return idx == basename.size();
-}
-
/// Writes out the function name in 'full_name' to 'out_stream'
/// but replaces each argument type with the variable name
/// and the corresponding pretty-printed value
@@ -208,206 +138,12 @@ static bool PrettyPrintFunctionNameWithArgs(Stream &out_stream,
return true;
}
-bool CPlusPlusLanguage::MethodName::TrySimplifiedParse() {
- // This method tries to parse simple method definitions which are presumably
- // most comman in user programs. Definitions that can be parsed by this
- // function don't have return types and templates in the name.
- // A::B::C::fun(std::vector<T> &) const
- size_t arg_start, arg_end;
- llvm::StringRef full(m_full.GetCString());
- llvm::StringRef parens("()", 2);
- if (ReverseFindMatchingChars(full, parens, arg_start, arg_end)) {
- m_arguments = full.substr(arg_start, arg_end - arg_start + 1);
- if (arg_end + 1 < full.size())
- m_qualifiers = full.substr(arg_end + 1).ltrim();
-
- if (arg_start == 0)
- return false;
- size_t basename_end = arg_start;
- size_t context_start = 0;
- size_t context_end = full.rfind(':', basename_end);
- if (context_end == llvm::StringRef::npos)
- m_basename = full.substr(0, basename_end);
- else {
- if (context_start < context_end)
- m_context = full.substr(context_start, context_end - 1 - context_start);
- const size_t basename_begin = context_end + 1;
- m_basename = full.substr(basename_begin, basename_end - basename_begin);
- }
-
- if (IsTrivialBasename(m_basename)) {
- return true;
- } else {
- // The C++ basename doesn't match our regular expressions so this can't
- // be a valid C++ method, clear everything out and indicate an error
- m_context = llvm::StringRef();
- m_basename = llvm::StringRef();
- m_arguments = llvm::StringRef();
- m_qualifiers = llvm::StringRef();
- m_return_type = llvm::StringRef();
- return false;
- }
- }
- return false;
-}
-
-void CPlusPlusLanguage::MethodName::Parse() {
- if (!m_parsed && m_full) {
- if (TrySimplifiedParse()) {
- m_parse_error = false;
- } else {
- CPlusPlusNameParser parser(m_full.GetStringRef());
- if (auto function = parser.ParseAsFunctionDefinition()) {
- m_basename = function->name.basename;
- m_context = function->name.context;
- m_arguments = function->arguments;
- m_qualifiers = function->qualifiers;
- m_return_type = function->return_type;
- m_parse_error = false;
- } else {
- m_parse_error = true;
- }
- }
- m_parsed = true;
- }
-}
-
-llvm::StringRef CPlusPlusLanguage::MethodName::GetBasename() {
- if (!m_parsed)
- Parse();
- return m_basename;
-}
-
-llvm::StringRef CPlusPlusLanguage::MethodName::GetContext() {
- if (!m_parsed)
- Parse();
- return m_context;
-}
-
-llvm::StringRef CPlusPlusLanguage::MethodName::GetArguments() {
- if (!m_parsed)
- Parse();
- return m_arguments;
-}
-
-llvm::StringRef CPlusPlusLanguage::MethodName::GetQualifiers() {
- if (!m_parsed)
- Parse();
- return m_qualifiers;
-}
-
-llvm::StringRef CPlusPlusLanguage::MethodName::GetReturnType() {
- if (!m_parsed)
- Parse();
- return m_return_type;
-}
-
-std::string CPlusPlusLanguage::MethodName::GetScopeQualifiedName() {
- if (!m_parsed)
- Parse();
- if (m_context.empty())
- return std::string(m_basename);
-
- std::string res;
- res += m_context;
- res += "::";
- res += m_basename;
- return res;
-}
-
-llvm::StringRef
-CPlusPlusLanguage::MethodName::GetBasenameNoTemplateParameters() {
- llvm::StringRef basename = GetBasename();
- size_t arg_start, arg_end;
- llvm::StringRef parens("<>", 2);
- if (ReverseFindMatchingChars(basename, parens, arg_start, arg_end))
- return basename.substr(0, arg_start);
-
- return basename;
-}
-
-bool CPlusPlusLanguage::MethodName::ContainsPath(llvm::StringRef path) {
- if (!m_parsed)
- Parse();
-
- // If we can't parse the incoming name, then just check that it contains path.
- if (m_parse_error)
- return m_full.GetStringRef().contains(path);
-
- llvm::StringRef identifier;
- llvm::StringRef context;
- std::string path_str = path.str();
- bool success = CPlusPlusLanguage::ExtractContextAndIdentifier(
- path_str.c_str(), context, identifier);
- if (!success)
- return m_full.GetStringRef().contains(path);
-
- // Basename may include template arguments.
- // E.g.,
- // GetBaseName(): func<int>
- // identifier : func
- //
- // ...but we still want to account for identifiers with template parameter
- // lists, e.g., when users set breakpoints on template specializations.
- //
- // E.g.,
- // GetBaseName(): func<uint32_t>
- // identifier : func<int32_t*>
- //
- // Try to match the basename with or without template parameters.
- if (GetBasename() != identifier &&
- GetBasenameNoTemplateParameters() != identifier)
- return false;
-
- // Incoming path only had an identifier, so we match.
- if (context.empty())
- return true;
- // Incoming path has context but this method does not, no match.
- if (m_context.empty())
- return false;
-
- llvm::StringRef haystack = m_context;
- if (!haystack.consume_back(context))
- return false;
- if (haystack.empty() || !isalnum(haystack.back()))
- return true;
-
- return false;
-}
-
-bool CPlusPlusLanguage::IsCPPMangledName(llvm::StringRef name) {
- // FIXME!! we should really run through all the known C++ Language plugins
- // and ask each one if this is a C++ mangled name
-
- Mangled::ManglingScheme scheme = Mangled::GetManglingScheme(name);
-
- if (scheme == Mangled::eManglingSchemeNone)
- return false;
-
- return true;
-}
-
bool CPlusPlusLanguage::DemangledNameContainsPath(llvm::StringRef path,
ConstString demangled) const {
MethodName demangled_name(demangled);
return demangled_name.ContainsPath(path);
}
-bool CPlusPlusLanguage::ExtractContextAndIdentifier(
- const char *name, llvm::StringRef &context, llvm::StringRef &identifier) {
- if (MSVCUndecoratedNameParser::IsMSVCUndecoratedName(name))
- return MSVCUndecoratedNameParser::ExtractContextAndIdentifier(name, context,
- identifier);
-
- CPlusPlusNameParser parser(name);
- if (auto full_name = parser.ParseAsFullName()) {
- identifier = full_name->basename;
- context = full_name->context;
- return true;
- }
- return false;
-}
-
namespace {
class NodeAllocator {
llvm::BumpPtrAllocator Alloc;
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguageMethod.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguageMethod.cpp
new file mode 100644
index 0000000000000..cd22b784de019
--- /dev/null
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguageMethod.cpp
@@ -0,0 +1,279 @@
+//===-- CPlusPlusLanguageMethod.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 "CPlusPlusLanguage.h"
+
+#include "lldb/Core/Mangled.h"
+
+#include "CPlusPlusNameParser.h"
+#include "MSVCUndecoratedNameParser.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+void CPlusPlusLanguage::MethodName::Clear() {
+ m_full.Clear();
+ m_basename = llvm::StringRef();
+ m_context = llvm::StringRef();
+ m_arguments = llvm::StringRef();
+ m_qualifiers = llvm::StringRef();
+ m_return_type = llvm::StringRef();
+ m_parsed = false;
+ m_parse_error = false;
+}
+
+static bool ReverseFindMatchingChars(const llvm::StringRef &s,
+ const llvm::StringRef &left_right_chars,
+ size_t &left_pos, size_t &right_pos,
+ size_t pos = llvm::StringRef::npos) {
+ assert(left_right_chars.size() == 2);
+ left_pos = llvm::StringRef::npos;
+ const char left_char = left_right_chars[0];
+ const char right_char = left_right_chars[1];
+ pos = s.find_last_of(left_right_chars, pos);
+ if (pos == llvm::StringRef::npos || s[pos] == left_char)
+ return false;
+ right_pos = pos;
+ uint32_t depth = 1;
+ while (pos > 0 && depth > 0) {
+ pos = s.find_last_of(left_right_chars, pos);
+ if (pos == llvm::StringRef::npos)
+ return false;
+ if (s[pos] == left_char) {
+ if (--depth == 0) {
+ left_pos = pos;
+ return left_pos < right_pos;
+ }
+ } else if (s[pos] == right_char) {
+ ++depth;
+ }
+ }
+ return false;
+}
+
+static bool IsTrivialBasename(const llvm::StringRef &basename) {
+ // Check that the basename matches with the following regular expression
+ // "^~?([A-Za-z_][A-Za-z_0-9]*)$" We are using a hand written implementation
+ // because it is significantly more efficient then using the general purpose
+ // regular expression library.
+ size_t idx = 0;
+ if (basename.starts_with('~'))
+ idx = 1;
+
+ if (basename.size() <= idx)
+ return false; // Empty string or "~"
+
+ if (!std::isalpha(basename[idx]) && basename[idx] != '_')
+ return false; // First character (after removing the possible '~'') isn't in
+ // [A-Za-z_]
+
+ // Read all characters matching [A-Za-z_0-9]
+ ++idx;
+ while (idx < basename.size()) {
+ if (!std::isalnum(basename[idx]) && basename[idx] != '_')
+ break;
+ ++idx;
+ }
+
+ // We processed all characters. It is a vaild basename.
+ return idx == basename.size();
+}
+
+bool CPlusPlusLanguage::MethodName::TrySimplifiedParse() {
+ // This method tries to parse simple method definitions which are presumably
+ // most comman in user programs. Definitions that can be parsed by this
+ // function don't have return types and templates in the name.
+ // A::B::C::fun(std::vector<T> &) const
+ size_t arg_start, arg_end;
+ llvm::StringRef full(m_full.GetCString());
+ llvm::StringRef parens("()", 2);
+ if (ReverseFindMatchingChars(full, parens, arg_start, arg_end)) {
+ m_arguments = full.substr(arg_start, arg_end - arg_start + 1);
+ if (arg_end + 1 < full.size())
+ m_qualifiers = full.substr(arg_end + 1).ltrim();
+
+ if (arg_start == 0)
+ return false;
+ size_t basename_end = arg_start;
+ size_t context_start = 0;
+ size_t context_end = full.rfind(':', basename_end);
+ if (context_end == llvm::StringRef::npos)
+ m_basename = full.substr(0, basename_end);
+ else {
+ if (context_start < context_end)
+ m_context = full.substr(context_start, context_end - 1 - context_start);
+ const size_t basename_begin = context_end + 1;
+ m_basename = full.substr(basename_begin, basename_end - basename_begin);
+ }
+
+ if (IsTrivialBasename(m_basename)) {
+ return true;
+ } else {
+ // The C++ basename doesn't match our regular expressions so this can't
+ // be a valid C++ method, clear everything out and indicate an error
+ m_context = llvm::StringRef();
+ m_basename = llvm::StringRef();
+ m_arguments = llvm::StringRef();
+ m_qualifiers = llvm::StringRef();
+ m_return_type = llvm::StringRef();
+ return false;
+ }
+ }
+ return false;
+}
+
+void CPlusPlusLanguage::MethodName::Parse() {
+ if (!m_parsed && m_full) {
+ if (TrySimplifiedParse()) {
+ m_parse_error = false;
+ } else {
+ CPlusPlusNameParser parser(m_full.GetStringRef());
+ if (auto function = parser.ParseAsFunctionDefinition()) {
+ m_basename = function->name.basename;
+ m_context = function->name.context;
+ m_arguments = function->arguments;
+ m_qualifiers = function->qualifiers;
+ m_return_type = function->return_type;
+ m_parse_error = false;
+ } else {
+ m_parse_error = true;
+ }
+ }
+ m_parsed = true;
+ }
+}
+
+llvm::StringRef CPlusPlusLanguage::MethodName::GetBasename() {
+ if (!m_parsed)
+ Parse();
+ return m_basename;
+}
+
+llvm::StringRef CPlusPlusLanguage::MethodName::GetContext() {
+ if (!m_parsed)
+ Parse();
+ return m_context;
+}
+
+llvm::StringRef CPlusPlusLanguage::MethodName::GetArguments() {
+ if (!m_parsed)
+ Parse();
+ return m_arguments;
+}
+
+llvm::StringRef CPlusPlusLanguage::MethodName::GetQualifiers() {
+ if (!m_parsed)
+ Parse();
+ return m_qualifiers;
+}
+
+llvm::StringRef CPlusPlusLanguage::MethodName::GetReturnType() {
+ if (!m_parsed)
+ Parse();
+ return m_return_type;
+}
+
+std::string CPlusPlusLanguage::MethodName::GetScopeQualifiedName() {
+ if (!m_parsed)
+ Parse();
+ if (m_context.empty())
+ return std::string(m_basename);
+
+ std::string res;
+ res += m_context;
+ res += "::";
+ res += m_basename;
+ return res;
+}
+
+llvm::StringRef
+CPlusPlusLanguage::MethodName::GetBasenameNoTemplateParameters() {
+ llvm::StringRef basename = GetBasename();
+ size_t arg_start, arg_end;
+ llvm::StringRef parens("<>", 2);
+ if (ReverseFindMatchingChars(basename, parens, arg_start, arg_end))
+ return basename.substr(0, arg_start);
+
+ return basename;
+}
+
+bool CPlusPlusLanguage::MethodName::ContainsPath(llvm::StringRef path) {
+ if (!m_parsed)
+ Parse();
+
+ // If we can't parse the incoming name, then just check that it contains path.
+ if (m_parse_error)
+ return m_full.GetStringRef().contains(path);
+
+ llvm::StringRef identifier;
+ llvm::StringRef context;
+ std::string path_str = path.str();
+ bool success = CPlusPlusLanguage::ExtractContextAndIdentifier(
+ path_str.c_str(), context, identifier);
+ if (!success)
+ return m_full.GetStringRef().contains(path);
+
+ // Basename may include template arguments.
+ // E.g.,
+ // GetBaseName(): func<int>
+ // identifier : func
+ //
+ // ...but we still want to account for identifiers with template parameter
+ // lists, e.g., when users set breakpoints on template specializations.
+ //
+ // E.g.,
+ // GetBaseName(): func<uint32_t>
+ // identifier : func<int32_t*>
+ //
+ // Try to match the basename with or without template parameters.
+ if (GetBasename() != identifier &&
+ GetBasenameNoTemplateParameters() != identifier)
+ return false;
+
+ // Incoming path only had an identifier, so we match.
+ if (context.empty())
+ return true;
+ // Incoming path has context but this method does not, no match.
+ if (m_context.empty())
+ return false;
+
+ llvm::StringRef haystack = m_context;
+ if (!haystack.consume_back(context))
+ return false;
+ if (haystack.empty() || !isalnum(haystack.back()))
+ return true;
+
+ return false;
+}
+
+bool CPlusPlusLanguage::IsCPPMangledName(llvm::StringRef name) {
+ // FIXME!! we should really run through all the known C++ Language plugins
+ // and ask each one if this is a C++ mangled name
+
+ Mangled::ManglingScheme scheme = Mangled::GetManglingScheme(name);
+
+ if (scheme == Mangled::eManglingSchemeNone)
+ return false;
+
+ return true;
+}
+
+bool CPlusPlusLanguage::ExtractContextAndIdentifier(
+ const char *name, llvm::StringRef &context, llvm::StringRef &identifier) {
+ if (MSVCUndecoratedNameParser::IsMSVCUndecoratedName(name))
+ return MSVCUndecoratedNameParser::ExtractContextAndIdentifier(name, context,
+ identifier);
+
+ CPlusPlusNameParser parser(name);
+ if (auto full_name = parser.ParseAsFullName()) {
+ identifier = full_name->basename;
+ context = full_name->context;
+ return true;
+ }
+ return false;
+}
|
|
After this patch and #131645 the size of lldb-server is reduced from 47MB to 8MB. |
|
The patch looks fine but I'm not sure how this actually addresses the issue. I don't think Module should depend on any partiuclar language plugin directly. Can we solve this problem by moving the functionality of that function ( |
CPlusPlusLanguage::IsCPPMangledName() contains the following comment: But I think it is out of scope of this NFC patch. |
|
What kind of a build (OS, compiler, etc.) is this exactly? The fact that moving the functions to a different compile unit actually makes a difference, makes me think that your linker is treating each CU as a monolith, which is definitely not a way to make small binaries. With |
I used the latest clang from mainline on Windows x86 and Linux x86 hosts. lldb-server is built for Linux Aarch64. As I mentioned here #129543 (comment) |
|
Okay, and what are those conditions? We could come up with a reasonable way to split this file in two (though I think it'd be better to do what Jonas suggested), but I don't think we can/should rely on those conditions continuing to hold in the face of future changes, if we don't know what they are. And we can't continue splitting files just because someone's compiler/linker makes better code that way. |
I don't know. Probably code fragmentation within CU or number of references makes the linker to link the entire CU.
Note it is mainline clang/lld. Moving all code used by Module.cpp to a separated module ensures that there is no unused code in the final binary. |
|
@labath Note I have synced with mainline and something changed. Moving out only 1 function |
I still don't think we should be doing this, and this is the reason why. It's not a good solution if random perturbations in unrelated code can cause this to work or not.
All the more reason to get to the bottom of this. Either we find a bug in the compiler/linker (and make it better for everyone), or we learn what is the property that makes this work (and then we can try to find a way to make it work reliably). |
|
@labath What do you suggest for a solution? Reducing lldb-server size from 17MB to 8MB is the significant reason to do something. It work reliably if all plug-in code used from the core directly is moved to a separated module. We can even create a subclass CPlusPlusLanguage::Helpers or such. |
|
Note These functions depend on BlockPointer.cpp, BlockPointer.cpp depends on TypeSystemClang.cpp, etc.
To avoid changes that may bring the problem back, I suggest to place moved functions to a subclass. |
|
I'm similarly confused about how moving these method definitions into a separate CU makes a difference. +1 to Jonas's suggestion, properly refactoring Module to use language plugins. For reference, I attempted this a few years ago (albeit on Apple's swift fork) and had trouble getting it working. You can take a look at my attempt here: swiftlang#3240 |
Please note Module.cpp uses only few functions which have no extra dependencies. After this patch the new CU CPlusPlusLanguageMethod.cpp with only necessary functions is linked. The rest CPlusPlusLanguage.cpp is not linked at all. |
|
Looks good.
It seems one solution might be to sort language plugins. |
I don't think it's as simple as that. I don't see a reason for an function-local static to be a gc root. In fact, when I look at your example (thank you for that), I see that none of the code in BlockPointer.cpp is actually present in the final binary, despite it obviously being somehow implicated in the size increase. That said, all of this gives me an idea for a plausible explanation: AIUI, linking basically works in two stages. First, you determine which object files are going to be a part of the link. This part works at object file level, and doesn't do any reachability analysis -- if we see an undefined symbol, we pull in the object which defines it. In the second stage (if --gc-sections is used), we remove any sections/functions which are not reachable from a gc root. Function-local statics should not be a gc root, but file-level globals definitely are. So any code that is reachable from global initializer is kept, even though the file containing the initializer was only referenced through dead code. Global variables are banned by the coding standards, but I wouldn't be surprised if there were still some around that cause this. |
TypeSystemClang.cpp contains global The dependencies chain looks like Module.cpp -> CPlusPlusLanguage::ExtractContextAndIdentifier() -> CPlusPlusLanguage.cpp -> CPlusPlusLanguage::GetHardcodedSummaries() -> BlockPointerSummaryProvider -> BlockPointer.cpp -> TypeSystemClang::ID GC can remove unused CPlusPlusLanguage::GetHardcodedSummaries(), but TypeSystemClang and its dependencies will be kept. |
|
@labath So, this patch solves the specific issue by breaking the dependency chain. |
Despite being banned, global variables with non-trivial constructors are still widely used for command line options. |
Constructor of what? The Here's a test case that shows that a static member does not prevent a class from being GC-ed: I think there's something else happening here.
That's true, but the command line library is a low level library in llvm, and it doesn't have any dependencies on the rest of llvm, let alone lldb. So, while these globals may cause us to link in some code that we theoretically do not need, I doubt they're responsible for the majority of the bloat. |
Ok, it seems the dependency chain is longer: Note the function clang::CreateLLVMCodeGen() is missing in the final binary (GC-ed). But we need to somehow break the dependency chain for lldb-server.
|
|
GC roots are
Thanks to @igorkudrin I think it is out of scope. We must break the dependency chain inside lldb. |
|
Okay. The last comment makes sense to me. Thank you for getting to the bottom of this. I fully agree this should be resolved in lldb as lldb-server has no good reason to depend on this code in the first place. So as far as I am concerned (I can't speak about others), I think something like this could work under two conditions:
That said, I would much very much prefer to resolve this in a proper way, by removing the (direct) dependency on the C++ language plugin. It sounds like swiftlang#3240 is not far from working. As far as I am concerned (I again don't speak for others), the plugin ordering thing could be handled by making sure the plugins are registered in the right order (in cmake). |
slydiman
force-pushed
the
lldb-server-reduce-size2
branch
from
cc89498 to
09f889f
Compare
slydiman
changed the title
|
Disabling asserts locally solved the problem with LLVM_DUMP_METHOD. This patch works again. |
…dependencies This patch addresses the issue llvm#129543. After this patch the size of lldb-server is reduced by 9MB. Co-authored-by: @bulbazord Alex Langford
slydiman
force-pushed
the
lldb-server-reduce-size2
branch
from
09f889f to
40d69ed
Compare
There was a problem hiding this comment.
I like the version a lot, but I'd like to leave to final approval to @bulbazord and/or @JDevlieghere.
| if (ObjCLanguage::IsPossibleObjCMethodName(name.GetCString())) { | ||
| func_name_type = eFunctionNameTypeFull; | ||
| } | ||
|
|
||
| if (ObjCLanguage::IsPossibleObjCSelector(name.GetCString())) { | ||
| func_name_type |= eFunctionNameTypeSelector; | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
This comes from Alex's patch. Updated.
|
Maybe rename |
lldb/include/lldb/Core/Mangled.h
Outdated
| @@ -246,6 +246,8 @@ class Mangled { | |||
| /// for s, otherwise the enumerator for the mangling scheme detected. | |||
| static Mangled::ManglingScheme GetManglingScheme(llvm::StringRef const name); | |||
|
|
|||
| static bool IsCPPMangledName(llvm::StringRef name); | |||
There was a problem hiding this comment.
How about calling this IsMangledName and. dropping the CPP/Cxx part altogether? Confusingly the implementation isn't actually checking if the mangling scheme is Itanium, so. this will fire too for Rust and Swift. Maybe something (like Rust) is relying on that, I don't know.
| static bool IsCPPMangledName(llvm::StringRef name); | |
| static bool IsMangledName(llvm::StringRef name); |
That would also provide some parity with this oddly named helper function:
static inline bool cstring_is_mangled(llvm::StringRef s) {
return Mangled::GetManglingScheme(s) != Mangled::eManglingSchemeNone;
}
The current name makes it look like it should really belong in the C++ language plugin, where it was previously.
| lldbPluginCPlusPlusLanguage | ||
| lldbPluginObjCLanguage |
There was a problem hiding this comment.
You can now add NO_PLUGIN_DEPENDENCIES to add_lldb_library above and resolve the FIXME. 🥳
Added NO_PLUGIN_DEPENDENCIES.
slydiman
force-pushed
the
lldb-server-reduce-size2
branch
from
72702af to
e43c31e
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/7726 Here is the relevant piece of the build log for the reference |
…-server dependencies" (#134995) Reverts #132274 Broke a test on LLDB Widows on Arm: https://lab.llvm.org/buildbot/#/builders/141/builds/7726 ``` FAIL: test_dwarf (lldbsuite.test.lldbtest.TestExternCSymbols.test_dwarf) <...> self.assertTrue(self.res.Succeeded(), msg + output) AssertionError: False is not true : Command 'expression -- foo()' did not return successfully Error output: error: Couldn't look up symbols: int foo(void) Hint: The expression tried to call a function that is not present in the target, perhaps because it was optimized out by the compiler. ```
|
The above failure is legit, this is the relevant bit of the log: No idea why at the moment, I have some other issues to deal with before I can look into it. |
|
FYI, this also broke the |
|
I'm looking on it. |
… break lldb-server dependencies" (#134995) Reverts llvm/llvm-project#132274 Broke a test on LLDB Widows on Arm: https://lab.llvm.org/buildbot/#/builders/141/builds/7726 ``` FAIL: test_dwarf (lldbsuite.test.lldbtest.TestExternCSymbols.test_dwarf) <...> self.assertTrue(self.res.Succeeded(), msg + output) AssertionError: False is not true : Command 'expression -- foo()' did not return successfully Error output: error: Couldn't look up symbols: int foo(void) Hint: The expression tried to call a function that is not present in the target, perhaps because it was optimized out by the compiler. ```
…-server dependencies The original PR is llvm#132274.
…-server dependencies The original PR is llvm#132274.
…-server dependencies The original PR is llvm#132274. Co-authored-by: @bulbazord Alex Langford
…-server dependencies The original PR is llvm#132274. Co-authored-by: @bulbazord Alex Langford
…-server dependencies" (llvm#134995) Reverts llvm#132274 Broke a test on LLDB Widows on Arm: https://lab.llvm.org/buildbot/#/builders/141/builds/7726 ``` FAIL: test_dwarf (lldbsuite.test.lldbtest.TestExternCSymbols.test_dwarf) <...> self.assertTrue(self.res.Succeeded(), msg + output) AssertionError: False is not true : Command 'expression -- foo()' did not return successfully Error output: error: Couldn't look up symbols: int foo(void) Hint: The expression tried to call a function that is not present in the target, perhaps because it was optimized out by the compiler. ```
…-server dependencies (#135033) The original PR is #132274. Co-authored-by: @bulbazord Alex Langford
…dependencies (llvm#132274) This patch addresses the issue llvm#129543. After this patch the size of lldb-server is reduced by 9MB. Co-authored-by: @bulbazord Alex Langford
…-server dependencies" (llvm#134995) Reverts llvm#132274 Broke a test on LLDB Widows on Arm: https://lab.llvm.org/buildbot/#/builders/141/builds/7726 ``` FAIL: test_dwarf (lldbsuite.test.lldbtest.TestExternCSymbols.test_dwarf) <...> self.assertTrue(self.res.Succeeded(), msg + output) AssertionError: False is not true : Command 'expression -- foo()' did not return successfully Error output: error: Couldn't look up symbols: int foo(void) Hint: The expression tried to call a function that is not present in the target, perhaps because it was optimized out by the compiler. ```
…-server dependencies (llvm#135033) The original PR is llvm#132274. Co-authored-by: @bulbazord Alex Langford
This patch addresses the issue #129543.
After this patch the size of lldb-server is reduced by 9MB.
Co-authored-by: @bulbazord Alex Langford