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

[David Blaikie]

On Thu, Sep 6, 2012 at 10:17 AM, David Tweed <david.tweed at arm.com> wrote:
> 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

Incomplete sentence?

I'm curious why you chose the armv7-none-eabi triple on some of the
cases? (not that variety is necessarily bad, but lack of consistency
just makes me wonder whether it's deliberate (& if so, what the reason
is) or incidental)

Also on the first (CodeGenCXX/copy-constructor-elim-2.cpp) and last
(test/CodeGenCXX/throw-expression-cleanup.cpp) tests you added RUN
lines rather than modifying the existing RUN lines. Won't that mean
you'll still get failures from the original RUN lines in those tests?

[general note: configuring git with '[diff] noprefix = true' will
produce diffs without the a/ and b/ prefixes in the paths - this makes
them easier to apply for some of the LLVM developers that work on
Windows & don't have trivial access to a path-length-trimming diff
application tool (& saves those developers that do have access to such
tools from having to think about/check whether a patch has such
prefixes or doesn't)]

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