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

[David Blaikie]

On Mon, Sep 10, 2012 at 11:41 AM, David Tweed <david.tweed at arm.com> wrote:
> ping on patch review status: the original submission did have a couple of
> issues pointed out in review, primarily because it wasn't quite the patch I
> intended to send. With the explanation of the alternating choice of triples
> and the correct patch sent, is this suitable for committing in order to fix
> those test failures on ARM?

I had half a mind to wait for John to OK the alternating choices of
triples, but honestly that minor detail seems fine for post-commit
feedback if he feels strongly about it.

LGTM, please commit - or I can do that for you if you don't have commit rights.

- David

>
> -----Original Message-----
> From: David Tweed [mailto:david.tweed at arm.com]
> Sent: 07 September 2012 08:56
> To: 'David Blaikie'
> Cc: John McCall; cfe-commits at cs.uiuc.edu; 陳韋任
> Subject: RE: [cfe-commits] [PATCH v2] Fixing some clang tests where ARM C++
> ABI differs
>
> Hi, thanks for looking at this. I'm basically moved to a new work
> environment and I'm still ironing out some of the systems issues. Firstly,
> I'm working on the bureaucracy getting a better mail agent installed; please
> bear with me.
>
> 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?
>
> Yep, patch description should say
>
> "Most of the tests use the x86-64 triple, but a couple use the
> armv7-none-eabi."
>
> | 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)
>
> Bear with me as I argue for motherhood and apple pie... In an ideal world we
> want fully working code which causes the reghression tests not to encounter
> any problems, rather than just to do something to get the regression tests
> to pass. So it would be helpful in stopping regressions on the ARM port to
> be testing some stuff on there, just as x86 gets very extensive test
> coverage. (Though I'm open to the argument that given the manpower levels
> only having test writers need to be aware of one ABI might be the best
> pragmatic solution.) One of the reasons I'm looking at this is a desire to
> have ARM being tested effectively in the tree.
>
> Regarding the choice, I just went through the files in the original diff in
> order of appearance alternating between x86-64 and ARM to get roughly 50 per
> cent coverage each.
>
> | 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?
>
> Screw up on my part: I got the tests running on ARM, copied the diff to x86
> to fix anything I'd broken there but then attached the wrong diff file.
> Attached is a the patch fixing that oversight, and running git-diff without
> a prefix. (I'm also working on simplifying the multi-platform workflow so
> hopefully I won't make this mistake again.)
>
> Regards,
> David
>
>
>