This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by bhushan on Feb 9 2016, 1:54 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

bhushan updated this revision to Diff 47305.Feb 9 2016, 1:54 AM
bhushan retitled this revision from to [LLDB][MIPS] Provide CPU string to compiler for appropriate code generation for MIPS.
bhushan updated this object.
bhushan added reviewers: clayborg, spyffe.
bhushan set the repository for this revision to rL LLVM.
bhushan added subscribers: lldb-commits, nitesh.jain, jaydeep and 2 others.
clayborg requested changes to this revision.Feb 9 2016, 12:50 PM
clayborg edited edge metadata.

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"

This revision now requires changes to proceed.Feb 9 2016, 12:50 PM
bhushan updated this revision to Diff 47422.Feb 9 2016, 10:26 PM
bhushan edited edge metadata.

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

zturner requested changes to this revision.Feb 9 2016, 10:39 PM
zturner added a reviewer: zturner.
zturner added a subscriber: zturner.
zturner added inline comments.
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.

This revision now requires changes to proceed.Feb 9 2016, 10:39 PM
ldrumm added a subscriber: ldrumm.Feb 10 2016, 5:02 AM

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 requested changes to this revision.Feb 10 2016, 9:52 AM
clayborg edited edge metadata.

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 updated this revision to Diff 47621.Feb 11 2016, 3:20 AM
bhushan edited edge metadata.

Addressed review comments:

  • Rebased this patch off top of tree
  • Added ArchSpec::GetClangTargetCPU() as suggested and moved large switch statement into it.
clayborg requested changes to this revision.Feb 11 2016, 10:34 AM
clayborg edited edge metadata.

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..

This revision now requires changes to proceed.Feb 11 2016, 10:34 AM
bhushan updated this revision to Diff 47765.Feb 11 2016, 8:13 PM
bhushan edited edge metadata.

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)".

clayborg accepted this revision.Feb 12 2016, 10:07 AM
clayborg edited edge metadata.

Hi Zachary,

Can you please find some time to review this?

Yes looks fine

Hi @zturner,

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

zturner accepted this revision.Mar 15 2016, 8:57 AM
zturner edited edge metadata.
This revision is now accepted and ready to land.Mar 15 2016, 8:57 AM