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:[email protected]] Sent: 07 September 2012 08:56 To: 'David Blaikie' Cc: John McCall; [email protected]; 陳韋任 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 <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
