[PATCH] D16139: [MIPS] initFeatureMap() to handle empty string argument

[Eric Christopher via cfe-commits]

Hi Daniel,

Sorry for the delay, but I've been both away and catching up:

On Wed, Mar 9, 2016 at 4:00 AM Daniel Sanders <Daniel.Sanders at imgtec.com>
wrote:

> > > From: Eric Christopher [echristo at gmail.com]
> > > Sent: 09 March 2016 06:50
> > > To: reviews+D16139+public+275805419034ae04 at reviews.llvm.org; Bhushan
> Attarde; Vasileios Kalintiris; Daniel Sanders
> > > Cc: Sagar Thakur; Nitesh Jain; Mohit Bhakkad; Jaydeep Patil;
> cfe-commits at lists.llvm.org
> > > Subject: Re: [PATCH] D16139: [MIPS] initFeatureMap() to handle empty
> string argument
> > >
> > > On Sat, Mar 5, 2016 at 6:16 AM Daniel Sanders <
> daniel.sanders at imgtec.com<mailto:daniel.sanders at imgtec.com>> wrote:
> > > dsanders added a comment.
> > >
> > > In http://reviews.llvm.org/D16139#368217, @echristo wrote:
> > >
> > > > This seems wrong. You should fix setCPU instead or set a default CPU.
> > >
> > > We already set a default CPU in the constructor (e.g.
> Mips32TargetInfoBase::Mips32TargetInfoBase() provides "mips32r2").
> > > It's the CPU argument to initFeatureMap() that's the root problem. In
> several targets, this argument has the same name as
> > > a member variable and is not subject to anything the constructor or
> setCPU() does to that member variable.
> >
> > To be clear, no, this is not the problem.
>
> I can agree that there are additional problems (and that fixing them also
> fixes this problem) but I disagree that it's not a part of
> the problem. At the moment, I think we're both looking at different
> aspects of it and saying "this is the whole problem" and I
> think we've each missed the piece the other is looking at.
>
> Suppose TargetOptions::CPU is the empty string and
> TargetInfo::CreateTargetInfo() is called. The call to AllocateTarget() will
> leave
> MipsTargetInfoBase::CPU set to the default mips32r2 or mips64r2 (depending
> on the subclass). The call to MipsTargetInfoBase::setCPU()
> will not happen because the CPU is the empty string. Then when
> MipsTargetInfoBase::initFeatureMap() is called we have the following
> state:
> * MipsTargetInfoBase::CPU is mips32r2 or mips64r2
> * The CPU argument of initFeatureMap() is the empty string.
> The CPU name came from a single place but only one path resolved the empty
> string to a CPU name. I think this is wrong and that
> both paths should resolve to the default CPU, or preferably, there should
> only be one CPU variable.
>
> Let's consider something other than MIPS for a moment. I'll pick SystemZ
> because it's the only other target that initializes its CPU
> to a non-empty value in the constructor. In SystemZ, we have the following
> state for the above example:
> * SystemZTargetInfo::CPU is z10
> * The CPU argument of initFeatureMap() is the empty string.
> Now, SystemZTargetInfo::initFeatureMap() doesn't have any checks for CPU
> == "z10" but if it did there would be a difference in
> behaviour between the default 'z10' and an explicit 'z10' since CPU ==
> "z10" would be false in the default 'z10' case (because CPU
> would be the empty string).
>
> Going back to MIPS, MipsTargetInfoBase::initFeatureMap() does encounter a
> difference between a default 'mips32r2' and an explicit
> 'mips32r2' because of the 'Features[CPU] = true' line. The clang driver
> currently makes sure we're always explicit but lldb doesn't have this.
>
> Fixing the above inconsistency would resolve the problem by itself, but I
> do agree that we're also handling the CPU name incorrectly
> in MipsTargetInfoBase::initFeatureMap(). I agree that the 'Features[CPU] =
> true' is bad and fixing that should also resolve the problem by
> itself. However, it would leave this weird inconsistency between the
> default 'mips32r2' and the explicit 'mips32r2'.
>
> I'm also wondering if the 'Features[CPU] = true' line might be redundant
> since the backend Processor<> and ProcessorModel<>
> definitions should have the same effect. I'll have to look into that when
> I get chance.
>
>
It should. Anything else should be a bug.


> > > I suspect the right thing to do is to drop the CPU argument and use
> the member variable instead but there may be differences in value/usage
> that make this difficult. For now, this patch serves as a stop-gap measure
> that resolves the empty string to a real CPU name.
> >
> > This is also not the problem. There are a few problems here:
> >
> > z) This code is terrible, I did my best to clean it up recently, but
> it's a lot of code and a bit painful.
> > a) There should be a testcase, everything can be done by the driver here
> as the code is pretty specialized for that use case.
>
> The test case is intended to be the lldb testsuite, without it lldb emits
> countless warnings about the '+' feature. I'm not aware of a
> way to trigger the problem from the clang driver since it always passes an
> explicit CPU name. As a result, I'm don't know of a way to
> test on the clang side.
>
>
See below.


> > b) CPUs are not subtarget features (or they shouldn't be), they're CPUs
> that contain features. They may be generic names for ISAs as well, but
> probably best to keep them separate.
>
> I agree, we have two separate concepts that happen to use the same
> strings. We should probably map them explicitly.
>

Yes, and that's what you should do instead of this patch.


>
> > c) You should set the features based on the CPUs given to the function.
> The typical way the cpu comes in, is via -target-cpu which comes via:
> >
> >   case llvm::Triple::mips:
> >   case llvm::Triple::mipsel:
> >   case llvm::Triple::mips64:
> >   case llvm::Triple::mips64el: {
> >     StringRef CPUName;
> >     StringRef ABIName;
> >     mips::getMipsCPUAndABI(Args, T, CPUName, ABIName);
> >     return CPUName;
> >   }
> >
> > for mips.
> >
> > Now if your triple is returning an empty string here you might have
> gotten to where you are (I tried mips64r2-linux-gnu as the -target option).
> Which is what typically happens down this path.
>
> This usage is from the clang driver. On this path, getMipsCPUAndABI
> ensures that the CPU is never empty.
>

I gave you a testcase that can prove otherwise in my earlier email.

-eric


>
> > As I said, I agree the code along this path is terrible, but I don't
> think this change is correct - and it should have had a testcase anyhow :)
> >
> > -eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160323/b476e796/attachment.html>