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? 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 <[email protected]> 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. > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
