[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

178

name this "target_arch"

198–200

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();

206–210

Use "target_machine"

[bhushan]

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

[zturner]

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

206–210

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 ↗

(On Diff #47621)

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

206–212

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

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

ClangExpressionParser::ClangExpressionParser (ExecutionContextScope *exe_scope,
     m_compiler.reset(new CompilerInstance());
 
     // 2. Install the target.
 
     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();
+
     // TODO: figure out what to really do when we don't have a valid target.
     // 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. This needs to be fixed in a better way.
-    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;
     }
     else
     {
         m_compiler->getTargetOpts().Triple = llvm::sys::getDefaultTargetTriple();
     }
 
-    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");
     }
 
+    if (target_machine == llvm::Triple::mips ||
+        target_machine == llvm::Triple::mipsel ||
+        target_machine == llvm::Triple::mips64 ||
+        target_machine == llvm::Triple::mips64el)
+    {
+        std::string cpu;
+        switch (target_arch.GetCore())
+        {
+        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:
+            cpu = "generic"; break;
+        }
+        m_compiler->getTargetOpts().CPU = cpu;
+    }
+
     // 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";
     }