[cfe-commits] [PATCH v2] Fixing some clang tests where ARM C++ ABI differs

[David Tweed]

Hi,

I've reworked the patch taking into account the comments. Could someone
review it with a view towards committing please: 

There are several clang C++ tests which are failing when run on ARM purely
due to a combination of (1) being run without an explicit triple and (2) the
ARM C++ ABI specifying constructors/destructors return the "this" pointer
(whereas itanium based C++ ABIs return void). This patch converts these
tests so that each one is run with a specific triple forcing a well-defined
constructor/destructor signature. Most of the tests use the x86-64 triple,
but a couple use the (It looks like this constructor ABI issue also arises
in a couple of other tests, but these tests have additional issues on ARM
and should be dealt with in separate patches.)


On Sep 5, 2012, at 1:11 AM, David Tweed wrote:
> On Sep 4, 2012, at 10:20 AM, David Blaikie wrote:
>> On Tue, Sep 4, 2012 at 8:19 AM, David Tweed <david.tweed at arm.com> wrote:
>>> There are several clang C++ tests which are failing when run on ARM
> purely
>>> due to a combination of (1) being run without an explicit triple and (2)
> the
>>> ARM C++ ABI specifying constructors/destructors return the "this"
pointer
>>> (whereas itanium based C++ ABIs return void). This patch converts these
>>> tests into 3 runs: once to test elements which don't depend on
> constructor
>>> ABI, once with an explicit x86-64 abi and once with an explicit ARM abi.
>>> These tests no longer fail on ARM and have been verified to still work
on
>>> x86. (It looks like this constructor ABI issue also arises in a couple
of
>>> other tests, but these tests have additional issues on ARM and should be
>>> dealt with in separate patches.)
>> 
>> Thanks a bunch for looking at this sort of thing, the public buildbot
>> state will hopefully be the better for it (we've got several bots that
>> have been red for quite a while & don't seem to get enough respect to
>> be maintained green & I suspect these sort of issues might be part of
>> the problem).
>> 
>> One question (to you & anyone else looking at this, maybe John McCall):
>> 
>> Do we need to test this particular ABI feature in all these test
>> cases? Or could we test that we do the right thing on ARM and X86 in
>> one test and then just test for one triple in the other cases (or use
>> a regex to hand-wave over the return type)? That way keeping our test
>> matrix down a little bit (number of separate process executions is
>> pretty much the cost metric for the tests - the fewer processes, the
>> faster we can all iterate).
> 
> Thanks: we certainly share the view that getting the ARM buildbots having
a
> baseline green state would be beneficial for everyone. Could I ask, at a
> convenient juncture, if you could look over the patch to give it a review
> towards being committed please?
> 
> | We have explicit tests for the ARM behavior;  I don't see any reason to
> | duplicate them on arbitrary other tests, and it's likely that they
contain
> | subtle target-dependencies anyway.  Explicit triples are always best.
> 
> It's an awkward question. On the one hand, clearly these tests are testing
> behaviour that ought to be target independent, so we ought to be able to
> have a set of tests that pick a particular ABI and just test that. On the
> other hand, the reasons for these tests is to catch non-obvious breakage
> early so it would be beneficial
> to be testing them on various native platforms. (I'm looking at some of
the
> other failures and while I can't be definitive at this stage it looks like
> there may be issues that show up on ARM in addition to pure ABI
> differences.)
> 
> In the end it probably comes down to deciding a trade-off: does the
> additional lines of checking obscure what's going on more or less than it
> increases the likelihood of catching platform specific (particularly ARM)
> issues? I don't have a good feel for that.

Relatively little about IR-generation varies from platform to platform,
but a lot of details can vary enough to make tests difficult to port.  This
is one of them ― I really don't want every test where we're testing a
constructor or destructor to have to worry about the slightly different
signature on ARM.  But there are much greater potential problems.
For example, there have been efforts to support platforms with other
than 8 bits in a char, but it'd be ridiculous to try to abstract tests
around
that ― and yet clearly we should pass tests when hosted on such a
machine.

Like I said, we should just add a triple to every IR-gen test.

John.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch3.diff
Type: application/octet-stream
Size: 4449 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120906/0cf3a1ff/attachment.obj>