[LLDB][MIPS] Provide CPU string to compiler for appropriate code generation for MIPS

[bhushan]

Summary

Currently, LLDB (ClangExpressionParser) does not pass clang::TargetOptions::CPU to clang which is supposed to contain the name of the target CPU to generate code for.
So, compiler generates the code for its default CPU (mips32r2 and mips64r2 for MIPS32 and MIPS64 respectively).

This causes problems in evaluating expressions in some cases.
For example, if we are debugging MIPS revision 6 (R6) application, as we do not pass CPU information to the compiler so compiler chooses default target and generates code for mips32r2/mips64r2.
The code generated for expression then fails to run on R6 because instruction set differs for R2 and R6 (few instructions in R2 are not available in R6).
The causes expression to fail.

This patch sets clang::TargetOptions::CPU with appropriate string so that compiler can generate correct code for that target.

[clayborg]

We should use local variable and avoid calling accessors many times. See inlined comments. I know the code was like this before, but we should fix these things as we go.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

182

name this "target_arch"

223–224

Many people are playing with the Target arhitecture's machine, please put this in a local variable and use it to avoid calling the access many times.

const auto target_machine = target_arch.GetMachine();

225–229

Use "target_machine"

[bhushan]

Addresses review comments.
Used local variables instead of calling accessors each time.

[zturner]

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

225–229

I don't like this big switch statement. This logic should be somewhere else, for example on the ArchSpec class. And it should handle not just mips, but everything.

[ldrumm]

Hi

This patch is on an out of date ClangExpressionParser.cpp. The feature added in http://reviews.llvm.org/rL259644 allows a more generic way of implementing what I believe you're trying to achieve with this patch. It's probably worth a look.

At the very least you need to base this CL off the top of tree.

[clayborg]

As Zach said, you could add a function to ArchSpec like:

std::string
ArchSpec::GetClangTargetCPU();

and move your code from the large switch statement into there. It can return an empty string for any architecture that don't add their own case for the switch statement.

[bhushan]

Addressed review comments:

• Rebased this patch off top of tree
• Added ArchSpec::GetClangTargetCPU() as suggested and moved large switch statement into it.

[clayborg]

Don't assert in ArchSpec::GetClangTargetCPU () and always call this function. See inlined comments.

source/Core/ArchSpec.cpp

558

I would remove the assert and let people opt into supplying a valid target CPU for their architecture if they ever need to. We shouldn't crash.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

240–246

If we remove the assert, can we just always assign this (remove the "if (is_mips)")? Is the CPU string empty before this call all the time? If so "ArchSpec::GetClangTargetCPU ()" can just return an empty string for any CPU that doesn't really need to make a special CPU string..

[bhushan]

Removed assert statement so now "ArchSpec::GetClangTargetCPU ()" returns an empty string (instead of crashing) for any CPU that doesn't really need to make a special CPU string.
Removed the "if (is_mips)".

[bhushan]

@zturner,

Ok to commit?

[bhushan]

Hi Zachary,

Can you please find some time to review this?

[zturner]

Yes looks fine

[bhushan]

Hi @zturner,

Can you please "accept" this revision so that I can "close" this one?

[bhushan]

Closed by commit http://reviews.llvm.org/rL261206

include/lldb/Core/ArchSpec.h

public:
     ///
     /// @return A static string correcponding to the current
     ///         architecture.
     //------------------------------------------------------------------
     const char *
     GetArchitectureName () const;
 
     //------------------------------------------------------------------
+    /// Returns a string representing current architecture as a target CPU
+    /// for tools like compiler, disassembler etc.
+    ///
+    /// @return A string representing target CPU for the current
+    ///         architecture.
+    //------------------------------------------------------------------
+    std::string
+    GetClangTargetCPU ();
+
+    //------------------------------------------------------------------
     /// Clears the object state.
     ///
     /// Clears the object state back to a default invalid state.
     //------------------------------------------------------------------
     void
     Clear ();
 
     //------------------------------------------------------------------

source/Core/ArchSpec.cpp

 ArchSpec::GetArchitectureName () const
 {
     const CoreDefinition *core_def = FindCoreDefinition (m_core);
     if (core_def)
         return core_def->name;
     return "unknown";
 }
 
+std::string
+ArchSpec::GetClangTargetCPU ()
+{
+    std::string cpu;
+    const llvm::Triple::ArchType machine = GetMachine();
+
+    if (machine == llvm::Triple::mips ||
+        machine == llvm::Triple::mipsel ||
+        machine == llvm::Triple::mips64 ||
+        machine == llvm::Triple::mips64el)
+    {
+        switch (m_core)
+        {
+        case ArchSpec::eCore_mips32:
+        case ArchSpec::eCore_mips32el:
+            cpu = "mips32"; break;
+        case ArchSpec::eCore_mips32r2:
+        case ArchSpec::eCore_mips32r2el:
+            cpu = "mips32r2"; break;
+        case ArchSpec::eCore_mips32r3:
+        case ArchSpec::eCore_mips32r3el:
+            cpu = "mips32r3"; break;
+        case ArchSpec::eCore_mips32r5:
+        case ArchSpec::eCore_mips32r5el:
+            cpu = "mips32r5"; break;
+        case ArchSpec::eCore_mips32r6:
+        case ArchSpec::eCore_mips32r6el:
+            cpu = "mips32r6"; break;
+        case ArchSpec::eCore_mips64:
+        case ArchSpec::eCore_mips64el:
+            cpu = "mips64"; break;
+        case ArchSpec::eCore_mips64r2:
+        case ArchSpec::eCore_mips64r2el:
+            cpu = "mips64r2"; break;
+        case ArchSpec::eCore_mips64r3:
+        case ArchSpec::eCore_mips64r3el:
+            cpu = "mips64r3"; break;
+        case ArchSpec::eCore_mips64r5:
+        case ArchSpec::eCore_mips64r5el:
+            cpu = "mips64r5"; break;
+        case ArchSpec::eCore_mips64r6:
+        case ArchSpec::eCore_mips64r6el:
+            cpu = "mips64r6"; break;
+        default:
+            break;
+        }
+    }
+    return cpu;
+}
+
 uint32_t
 ArchSpec::GetMachOCPUType () const
 {
     const CoreDefinition *core_def = FindCoreDefinition (m_core);
     if (core_def)
     {
         const ArchDefinitionEntry *arch_def = FindArchDefinitionEntry (&g_macho_arch_def, core_def->core);
         if (arch_def)

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

ClangExpressionParser::ClangExpressionParser (ExecutionContextScope *exe_scope,
     m_compiler.reset(new CompilerInstance());
     lldb::LanguageType frame_lang = expr.Language(); // defaults to lldb::eLanguageTypeUnknown
     bool overridden_target_opts = false;
     lldb_private::LanguageRuntime *lang_rt = nullptr;
     lldb::TargetSP target_sp;
     if (exe_scope)
         target_sp = exe_scope->CalculateTarget();
 
+    ArchSpec target_arch;
+    if (target_sp)
+        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.
     auto frame = exe_scope->CalculateStackFrame();
 
     // Make sure the user hasn't provided a preferred execution language
     // with `expression --language X -- ...`
     if (frame && frame_lang == lldb::eLanguageTypeUnknown)
         frame_lang = frame->GetLanguage();
 
     if (frame_lang != lldb::eLanguageTypeUnknown)
     {
         lang_rt = exe_scope->CalculateProcess()->GetLanguageRuntime(frame_lang);
         if (log)
             log->Printf("Frame has language of type %s", Language::GetNameForLanguageType(frame_lang));
     }
 
     // 2. Configure the compiler with a set of default options that are appropriate
     // for most situations.
-    if (target_sp && target_sp->GetArchitecture().IsValid())
+    if (target_sp && target_arch.IsValid())
     {
-        std::string triple = target_sp->GetArchitecture().GetTriple().str();
+        std::string triple = target_arch.GetTriple().str();
         m_compiler->getTargetOpts().Triple = triple;
         if (log)
             log->Printf("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();
         if (log)
             log->Printf("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_sp->GetArchitecture().GetMachine() == llvm::Triple::x86 ||
-        target_sp->GetArchitecture().GetMachine() == llvm::Triple::x86_64)
+    if (target_machine == llvm::Triple::x86 ||
+        target_machine == llvm::Triple::x86_64)
     {
         m_compiler->getTargetOpts().Features.push_back("+sse");
         m_compiler->getTargetOpts().Features.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();
+
     // 3. Now allow the runtime to provide custom configuration options for the target.
     // In this case, a specialized language runtime is available and we can query it for extra options.
     // For 99% of use cases, this will not be needed and should be provided when basic platform detection is not enough.
     if (lang_rt)
         overridden_target_opts = lang_rt->GetOverrideExprOptions(m_compiler->getTargetOpts());
 
     if (overridden_target_opts)
         if (log)
         {
             log->Debug("Using overridden target options for the expression evaluation");