Skip to content

[lldb][ClangExpressionParser][NFC] Factor out TargetOpts logic out of ClangExpressionParser #101681

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

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

Michael137
Copy link
Member

Same motivation as #101669. We plan to eventually use the Clang driver to initialize the CompilerInstance.

This should make refactorings of this code more straightforward.

Changes:

  • Introduced SetupTargetOpts
  • Called them from ClangExpressionParser::ClangExpressionParser
  • Made GetClangTargetABI a file-local function since it's not using any of the state in ClangExpressionParser, and isn't used anywhere outside the source file

@llvmbot
Copy link
Member

llvmbot commented Aug 2, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

Same motivation as #101669. We plan to eventually use the Clang driver to initialize the CompilerInstance.

This should make refactorings of this code more straightforward.

Changes:

  • Introduced SetupTargetOpts
  • Called them from ClangExpressionParser::ClangExpressionParser
  • Made GetClangTargetABI a file-local function since it's not using any of the state in ClangExpressionParser, and isn't used anywhere outside the source file

Full diff: https://github.com/llvm/llvm-project/pull/101681.diff

2 Files Affected:

  • (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+75-67)
  • (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h (-9)
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 303e88feea20b..2d34f657793c3 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -351,6 +351,80 @@ static void SetupDefaultClangDiagnostics(CompilerInstance &compiler) {
   }
 }
 
+/// Returns a string representing current ABI.
+///
+/// \param[in] target_arch
+///     The target architecture.
+///
+/// \return
+///     A string representing target ABI for the current architecture.
+static std::string GetClangTargetABI(const ArchSpec &target_arch) {
+  std::string abi;
+
+  if (target_arch.IsMIPS()) {
+    switch (target_arch.GetFlags() & ArchSpec::eMIPSABI_mask) {
+    case ArchSpec::eMIPSABI_N64:
+      abi = "n64";
+      break;
+    case ArchSpec::eMIPSABI_N32:
+      abi = "n32";
+      break;
+    case ArchSpec::eMIPSABI_O32:
+      abi = "o32";
+      break;
+    default:
+      break;
+    }
+  }
+  return abi;
+}
+
+static void SetupTargetOpts(CompilerInstance &compiler,
+                            lldb_private::Target const &target) {
+  Log *log = GetLog(LLDBLog::Expressions);
+  ArchSpec target_arch = target.GetArchitecture();
+
+  const auto target_machine = target_arch.GetMachine();
+  if (target_arch.IsValid()) {
+    std::string triple = target_arch.GetTriple().str();
+    compiler.getTargetOpts().Triple = triple;
+    LLDB_LOGF(log, "Using %s as the target triple",
+              compiler.getTargetOpts().Triple.c_str());
+  } else {
+    // If we get here we don't have a valid target and just have to guess.
+    // Sometimes this will be ok to just use the host target triple (when we
+    // evaluate say "2+3", but other expressions like breakpoint conditions and
+    // other things that _are_ target specific really shouldn't just be using
+    // the host triple. In such a case the language runtime should expose an
+    // overridden options set (3), below.
+    compiler.getTargetOpts().Triple = llvm::sys::getDefaultTargetTriple();
+    LLDB_LOGF(log, "Using default target triple of %s",
+              compiler.getTargetOpts().Triple.c_str());
+  }
+  // Now add some special fixes for known architectures: Any arm32 iOS
+  // environment, but not on arm64
+  if (compiler.getTargetOpts().Triple.find("arm64") == std::string::npos &&
+      compiler.getTargetOpts().Triple.find("arm") != std::string::npos &&
+      compiler.getTargetOpts().Triple.find("ios") != std::string::npos) {
+    compiler.getTargetOpts().ABI = "apcs-gnu";
+  }
+  // Supported subsets of x86
+  if (target_machine == llvm::Triple::x86 ||
+      target_machine == llvm::Triple::x86_64) {
+    compiler.getTargetOpts().FeaturesAsWritten.push_back("+sse");
+    compiler.getTargetOpts().FeaturesAsWritten.push_back("+sse2");
+  }
+
+  // Set the target CPU to generate code for. This will be empty for any CPU
+  // that doesn't really need to make a special
+  // CPU string.
+  compiler.getTargetOpts().CPU = target_arch.GetClangTargetCPU();
+
+  // Set the target ABI
+  if (std::string abi = GetClangTargetABI(target_arch); !abi.empty())
+    compiler.getTargetOpts().ABI = std::move(abi);
+}
+
 //===----------------------------------------------------------------------===//
 // Implementation of ClangExpressionParser
 //===----------------------------------------------------------------------===//
@@ -395,12 +469,6 @@ ClangExpressionParser::ClangExpressionParser(
   // Defaults to lldb::eLanguageTypeUnknown.
   lldb::LanguageType frame_lang = expr.Language().AsLanguageType();
 
-  std::string abi;
-  ArchSpec target_arch;
-  target_arch = target_sp->GetArchitecture();
-
-  const auto target_machine = target_arch.GetMachine();
-
   // If the expression is being evaluated in the context of an existing stack
   // frame, we introspect to see if the language runtime is available.
 
@@ -419,45 +487,7 @@ ClangExpressionParser::ClangExpressionParser(
 
   // 2. Configure the compiler with a set of default options that are
   // appropriate for most situations.
-  if (target_arch.IsValid()) {
-    std::string triple = target_arch.GetTriple().str();
-    m_compiler->getTargetOpts().Triple = triple;
-    LLDB_LOGF(log, "Using %s as the target triple",
-              m_compiler->getTargetOpts().Triple.c_str());
-  } else {
-    // If we get here we don't have a valid target and just have to guess.
-    // Sometimes this will be ok to just use the host target triple (when we
-    // evaluate say "2+3", but other expressions like breakpoint conditions and
-    // other things that _are_ target specific really shouldn't just be using
-    // the host triple. In such a case the language runtime should expose an
-    // overridden options set (3), below.
-    m_compiler->getTargetOpts().Triple = llvm::sys::getDefaultTargetTriple();
-    LLDB_LOGF(log, "Using default target triple of %s",
-              m_compiler->getTargetOpts().Triple.c_str());
-  }
-  // Now add some special fixes for known architectures: Any arm32 iOS
-  // environment, but not on arm64
-  if (m_compiler->getTargetOpts().Triple.find("arm64") == std::string::npos &&
-      m_compiler->getTargetOpts().Triple.find("arm") != std::string::npos &&
-      m_compiler->getTargetOpts().Triple.find("ios") != std::string::npos) {
-    m_compiler->getTargetOpts().ABI = "apcs-gnu";
-  }
-  // Supported subsets of x86
-  if (target_machine == llvm::Triple::x86 ||
-      target_machine == llvm::Triple::x86_64) {
-    m_compiler->getTargetOpts().FeaturesAsWritten.push_back("+sse");
-    m_compiler->getTargetOpts().FeaturesAsWritten.push_back("+sse2");
-  }
-
-  // Set the target CPU to generate code for. This will be empty for any CPU
-  // that doesn't really need to make a special
-  // CPU string.
-  m_compiler->getTargetOpts().CPU = target_arch.GetClangTargetCPU();
-
-  // Set the target ABI
-  abi = GetClangTargetABI(target_arch);
-  if (!abi.empty())
-    m_compiler->getTargetOpts().ABI = abi;
+  SetupTargetOpts(*m_compiler, *target_sp);
 
   // 3. Create and install the target on the compiler.
   m_compiler->createDiagnostics();
@@ -1179,28 +1209,6 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
   return num_errors;
 }
 
-std::string
-ClangExpressionParser::GetClangTargetABI(const ArchSpec &target_arch) {
-  std::string abi;
-
-  if (target_arch.IsMIPS()) {
-    switch (target_arch.GetFlags() & ArchSpec::eMIPSABI_mask) {
-    case ArchSpec::eMIPSABI_N64:
-      abi = "n64";
-      break;
-    case ArchSpec::eMIPSABI_N32:
-      abi = "n32";
-      break;
-    case ArchSpec::eMIPSABI_O32:
-      abi = "o32";
-      break;
-    default:
-      break;
-    }
-  }
-  return abi;
-}
-
 /// Applies the given Fix-It hint to the given commit.
 static void ApplyFixIt(const FixItHint &fixit, clang::edit::Commit &commit) {
   // This is cobbed from clang::Rewrite::FixItRewriter.
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
index 0852e928a9d42..93e0b007dbcc8 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
@@ -119,15 +119,6 @@ class ClangExpressionParser : public ExpressionParser {
       bool &can_interpret,
       lldb_private::ExecutionPolicy execution_policy) override;
 
-  /// Returns a string representing current ABI.
-  ///
-  /// \param[in] target_arch
-  ///     The target architecture.
-  ///
-  /// \return
-  ///     A string representing target ABI for the current architecture.
-  std::string GetClangTargetABI(const ArchSpec &target_arch);
-
 private:
   /// Parses the expression.
   ///

@Michael137 Michael137 merged commit d6cbcf9 into llvm:main Aug 2, 2024
8 checks passed
adrian-prantl pushed a commit to adrian-prantl/llvm-project that referenced this pull request Oct 11, 2024
… ClangExpressionParser (llvm#101681)

Same motivation as llvm#101669. We
plan to eventually use the Clang driver to initialize the
`CompilerInstance`.

This should make refactorings of this code more straightforward.

**Changes**:
* Introduced `SetupTargetOpts`
* Called them from `ClangExpressionParser::ClangExpressionParser`
* Made `GetClangTargetABI` a file-local function since it's not using
any of the state in `ClangExpressionParser`, and isn't used anywhere
outside the source file

(cherry picked from commit d6cbcf9)
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.

2 participants