Workaround for bug 16070 ?

Hi there,

I'm still affected by

http://llvm.org/bugs/show_bug.cgi?id=16070

Given that it appears that a change in clang causes hitting the assertion in
binutils (what should the actual behavior be?), can we get a workaround in
clang for it?

Thanks,

Steve.

Hi Steve,

Given that it appears that a change in clang causes hitting the assertion in
binutils (what should the actual behavior be?), can we get a workaround in
clang for it?

I think it's unlikely anyone will approve a change in Clang
specifically to avoid a binutils bug on that scale, so there'll
probably be no workaround in that sense. If someone (hint hint!)
submits a patch to improve clang that happens to sidestep this
binutils issue as a side effect, everything's fine.

I can't reproduce it with my (admittedly possibly dodgy) gnueabihf
toolchain either, but from reading the description I think a case
could be made for making "arm-linux-gnueabihf" default to a different
CPU than "arm-linux-gnueabi"; one that *does* support hardware float,
for example.

We'd probably want to consider what an arm-linux-gnueabihf-gcc
defaults to in making that change. But I'd almost be tempted to make
the change even if GCC didn't. Having arm-linux-gnueabihf default to a
non-HF CPU is insane.

If you're interested, I *think* the place to start looking is Clang's
lib/Driver/Tools.cpp. Specifically the getARMTargetCPU function.

Cheers.

Tim.

Tim Northover <t.p.northover@...> writes:

We'd probably want to consider what an arm-linux-gnueabihf-gcc
defaults to in making that change. But I'd almost be tempted to make
the change even if GCC didn't. Having arm-linux-gnueabihf default to a
non-HF CPU is insane.

If you're interested, I *think* the place to start looking is Clang's
lib/Driver/Tools.cpp. Specifically the getARMTargetCPU function.

Hi,

I tried looking into this and didn't get very far.

In getARMTargetCPU,

MArch = Triple.getArchName();

results in MArch == "armv4t"

That then causes arm7tdmi to be returned instead of cortex-a8.

How does it calculate that armv4t is the archName?

Thanks,

Steve.

Hi Stephen,

Ah, it appears there are two getARMTargetCPU functions; the one that
makes the first decision is in ToolChain.cpp. This then gets used to
determine the suffix added to the triple.

Sorry about that.

Tim.

Tim Northover wrote:

Hi Stephen,

Ah, it appears there are two getARMTargetCPU functions; the one that
makes the first decision is in ToolChain.cpp. This then gets used to
determine the suffix added to the triple.

Sorry about that.

Tim.

Ah, ok.

This patch solves the problem for me, but I have no idea if it is correct:

diff --git a/lib/Driver/ToolChain.cpp b/lib/Driver/ToolChain.cpp
index 8676f7f..51e06d8 100644
--- a/lib/Driver/ToolChain.cpp
+++ b/lib/Driver/ToolChain.cpp
@@ -201,7 +201,7 @@ static const char *getARMTargetCPU(const ArgList &Args,
     .Case("xscale", "xscale")
     // If all else failed, return the most base CPU with thumb interworking
     // supported by LLVM.
- .Default("arm7tdmi");
+ .Default("cortex-a8");
}

Is this better?

diff --git a/lib/Driver/ToolChain.cpp b/lib/Driver/ToolChain.cpp
index 8676f7f..6dad2f1 100644
--- a/lib/Driver/ToolChain.cpp
+++ b/lib/Driver/ToolChain.cpp
@@ -174,7 +174,7 @@ static const char *getARMTargetCPU(const ArgList &Args,
     MArch = Triple.getArchName();
   }

- return llvm::StringSwitch<const char *>(MArch)
+ const char *result = llvm::StringSwitch<const char *>(MArch)
     .Cases("armv2", "armv2a","arm2")
     .Case("armv3", "arm6")
     .Case("armv3m", "arm7m")
@@ -199,9 +199,14 @@ static const char *getARMTargetCPU(const ArgList &Args,
     .Case("ep9312", "ep9312")
     .Case("iwmmxt", "iwmmxt")
     .Case("xscale", "xscale")
- // If all else failed, return the most base CPU with thumb interworking
- // supported by LLVM.
- .Default("arm7tdmi");
+ .Default(0);

Stephen Kelly wrote:

Ah, ok.

This patch solves the problem for me, but I have no idea if it is correct:

Tim, any further input here?

http://thread.gmane.org/gmane.comp.compilers.clang.devel/30170/focus=30395

Thanks,

Steve.

Stephen Kelly <steveire@...> writes:

Tim, any further input here?

http://thread.gmane.org/gmane.comp.compilers.clang.devel/30170/focus=30395

It has been two weeks. Tim any further input?

Anyone else willing to help move this forward please?

Thanks,

Steve.

It has been two weeks. Tim any further input?

I started a reply earlier, but somehow deleted it, sorry. I think it's
a tricky situation because Cortex-A8 isn't the minimal CPU that could
reasonably have VFP. GCC, for example, defaults to some ARM9 with an
FPU added (which has some claim to being the minimal possible
combination that would make sense for gnueabihf).

We don't have to follow them, of course. If no-one is actually using
configurations like that we could choose arm1176jzf-s (Raspberry Pi
has this, so it's reasonably common at the moment). In my opinion we
can forget about arm9 and earlier for this, so unless anyone pipes up
to disagree I'd be happy to commit the arm1176jzf-s one.

As for HasHardwareFloat, I think I'd just go for
"Triple.getEnvironment() == llvm::Triple::GNUEABIHF", I don't think
any of the others need hardware float.

It's probably best to post patches as an attachment to cfe-commits
rather than cfe-dev too (more people read that list for reviews and so
on). Tests are also a good idea. See
http://llvm.org/docs/DeveloperPolicy.html#making-a-patch

Tim.

Tim Northover wrote:

It has been two weeks. Tim any further input?

I started a reply earlier, but somehow deleted it, sorry. I think it's
a tricky situation because Cortex-A8 isn't the minimal CPU that could
reasonably have VFP. GCC, for example, defaults to some ARM9 with an
FPU added (which has some claim to being the minimal possible
combination that would make sense for gnueabihf).

We don't have to follow them, of course. If no-one is actually using
configurations like that we could choose arm1176jzf-s (Raspberry Pi
has this, so it's reasonably common at the moment). In my opinion we
can forget about arm9 and earlier for this, so unless anyone pipes up
to disagree I'd be happy to commit the arm1176jzf-s one.

As for HasHardwareFloat, I think I'd just go for
"Triple.getEnvironment() == llvm::Triple::GNUEABIHF", I don't think
any of the others need hardware float.

Ok, thanks for the guidance. I have implemented that in my patch.

It's probably best to post patches as an attachment to cfe-commits
rather than cfe-dev too (more people read that list for reviews and so
on).

I'm afraid that does not match my experience.

But as I have your attention, I guess you'll commit it eventually, when the
tests are in place. I can post it to the other mailing list too, but as far
as I see, people active in (at least post-commit) reviews there are also on
this mailing list.

Tests are also a good idea. See
http://llvm.org/docs/DeveloperPolicy.html#making-a-patch

I have no idea how to unit test this change. Any guidance on that?

Thanks,

Steve.

Stephen Kelly wrote:

I have no idea how to unit test this change. Any guidance on that?

Looks like I forgot to attach the patch.

Here it is.

Thanks,

Steve.

0001-Use-a-better-default-arch-for-a-cpu-that-supports-ha.patch (1.34 KB)

Stephen Kelly wrote:

Stephen Kelly wrote:

I have no idea how to unit test this change. Any guidance on that?

Looks like I forgot to attach the patch.

I figured out how to add a unit test for this. Updated patch attached.

Thanks,

Steve.

0001-Use-a-better-default-arch-for-a-cpu-that-supports-ha.patch (1.34 KB)

Hi Stephen,

I figured out how to add a unit test for this. Updated patch attached.

Did you forget to add the test file before diffing? I still only see
the source change.

Cheers.

Tim.

Stephen Kelly wrote:

Stephen Kelly wrote:

Stephen Kelly wrote:

I have no idea how to unit test this change. Any guidance on that?

Looks like I forgot to attach the patch.

I figured out how to add a unit test for this. Updated patch attached.

But I forgot to add the test to that patch :/. Updated again.

Thanks,

Steve.

0001-Use-a-better-default-arch-for-a-cpu-that-supports-ha.patch (1.86 KB)

Hi Steve,

But I forgot to add the test to that patch :/. Updated again.

Thanks for keeping on working at this, and your patience; I don't
think I've been as helpful as I could have been.

I've committed this as r188796 and r188797 (I forgot the test too, first time).

Cheers.

Tim.

Tim Northover wrote:

Hi Steve,

But I forgot to add the test to that patch :/. Updated again.

Thanks for keeping on working at this, and your patience; I don't
think I've been as helpful as I could have been.

I've committed this as r188796 and r188797 (I forgot the test too, first
time).

Great, thanks for the commits!

- Steve.