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

[David Tweed]

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?

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