Hello Jeff, all, On 19 Sep 2014, at 05:14, Jeff Law wrote:
> On 09/18/14 16:20, Iain Sandoe wrote: >> >> 1. There has been a change made "to make the upper path like the lower path" >> (as you said on IRC). >> - apparently (from our conversation) you don't expect this to be a general >> optimisation improvement. >> - but it doesn't seem to be either "obvious" or "a cleanup" to me >> - if you are asserting that it is a cleanup, then some explanation is in >> order. > Seems reasonable. > >> >> 2. Apparently you don't think it is necessary to have any testcase to >> demonstrate that the new code is working? >> - perhaps you are asserting that the code is "correct by inspection"? > A testcase would be nice to add and I'd strongly prefer to have one in the > suite -- even if it's Darwin specific. I have not succeeded in getting this code-path to trigger on Linux** - on irc, IIUC, Kai was saying that he didn't expect it would trigger on either Windows or Linux? * if it's really intended to be mach-o specific then: a) it should have an "if (TARGET_MACHO && ...)" so that it's DCE'd for other targets. b) I'd like a clear explanation of what it's supposed to do so that we can examine why it doesn't do that.. c) ..and, until we fix it it, it should be disabled or left out. * If it's not darwin-specific then surely a change in code-gen must be accompanied by some test that demonstrates it DTRT on at least one target? -- ** I put a gcc_abort() in the path and ran "bootstrap" and "make check" on x86-64-Linux without seeing any aborts from this (so currently i have no non-darwin data). > My understanding is the problem is Darwin's linker (or dynamic loader?) can't > handle the code we end up generating. While there may be other systems where > one could show the problem due to limitations of the linker/loader on thos > systems, probably the most common will be Darwin. So we probably need a > Darwin specific test. This is not a ld64 or dyld issue. The fail is a compiler ICE (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61387) Kai's speculation is that there's some unspecified fault with Darwin's address legalisation, although I have pointed out that for x86_64, the darwin and linux ABIs are the same, so there's a lot of shared code (the code is triggered by a different symbol visibility - which might, in itself, be a bug but that's separate from the matter at hand). --- IMO, we need to put Darwin to one side, and have a clear explanation of what the change is supposed to achieve on a NON-Darwin system. > However, I don't think Kai has a Darwin box to do the necessary testing. If > you, or someone, could build a test and verify it fails without Kai's patch, > then passes with Kai's patch, that'd be quite helpful. If there's a genuine Darwin problem, then we will try to fix it, of course; and Darwin folks will be happy to test prospective patches. - but I remain concerned that this is just papering over an underlying issue with the change, that is being thrown up by Darwin (by luck that it happens to trigger the code path). >> 3. You don't seem to think it necessary to amend the comments in the code to >> reflect the new functionality? > Kai, can you update the comments, please? thanks for reviewing, Iain > > Jeff
