r210584 - Fix crash with x86_64-pc-win32-macho target. <rdar://problem/17235840>

[Bob Wilson]

On Jun 11, 2014, at 9:57 AM, Chandler Carruth <chandlerc at google.com> wrote:

> 
> On Wed, Jun 11, 2014 at 5:41 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>> On Jun 11, 2014, at 9:11 AM, Chandler Carruth <chandlerc at google.com> wrote:
>> 
>> 
>> On Wed, Jun 11, 2014 at 5:04 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>> There’s a big difference between “I don’t know how to write good tests for this” and “we need to rip it out because no one is maintaining it”.
>> 
>> Yes, there is. And I think I called it out in my email? If we can't find anyone on the lists who knows how to write a good test (not just you, but *anyone*) then no one is maintaining it.
> 
> Sorry, but if you already called it out, then I missed it. Can you explain? It still seems to me like a huge difference.
> 
> "If we can't find anyone on the lists who knows how to write a good test (not just you, but *anyone*) then no one is maintaining it.”

OK, we just have different understanding of “maintaining”.

I agree we do not currently have anyone “maintaining” this platform at the level you are looking for. But, there are people relying on it and it doesn’t make any sense to threaten to rip it out, just because we don’t currently have good tests. We can still respond to bug reports and fix regressions as they are discovered (adding tests in the process, of course).

>  
> 
>>  
>> We will definitely maintain it, and we (Apple) will have to deal with the pain caused by the lack of good tests.
>> 
>> This is a shared project, and we *all* have to deal with the pain caused by the lack of good tests. That's why it's so important to have them, especially for targets which not all developers have access to.
> 
> Can you please explain why the lack of tests for this is going to cause pain for anyone outside Apple? If we’re really the only ones using it, then only our stuff is going to break, and clearly the burden for fixing it will be entirely on us.
> 
>>  
>> It basically means that we are stuck waiting to find problems when people use it, rather than catching them earlier when we run the test suite. We can add tests for those issues as they arise.
>> 
>> I have seen you and others routinely ask people adding support for platforms include sufficient testing that developers not working on those platforms can at least make reasonable changes to the shared infrastructure with confidence that these platforms aren't being grossly broken. I don't understand why this platform is different. I mean, it may be circumstantially different because no one noticed how bad the testing is until now, but I don't understand why there is any response to that other than "yea, we need to get the maintainers of this platform to add some tests pronto so we can keep supporting it reasonably". =/
> 
> Good tests should have been added when the support for this platform was added. It was a mistake that we let it go in without those tests.
> 
>> 
>> The last time I tried to change header search in Clang I had a truly horrific time of it precisely because of the utter lack of testing on this front. I've worked reasonably hard to ensure that the platforms I maintain have good tests now for both header and library search. I think its reasonable to ask someone to do the same for this platform. If you're supporting this platform (because you're fixing bugs in it) it seems reasonable to ask you to either do this or find someone who can do this.
> 
> If there are no tests, then I don’t see why you or anyone else should need to be overly concerned about breaking it.
> 
> Because it isn't just "oh, there was a bug because case <X> wasn't handled". When doing refactoring or other maintenance that would impact the *design* of the system, it becomes impossible to know what to do without good testing. You know that there needs to be *something* to handle this win32-macho platform because there is some existing code to support it. But you don't really know what behavior of that code is incidental versus intentional. You don't know what design alternatives need to be eliminated because of the constraints imposed by that platform.
> 
> This isn't a new or hypothetical problem. When I worked to refactor how header search worked years ago I was unable to really finish the job because there was clearly a pile of code in Clang (some of the code you're now touching) that handled platforms for which we didn't really have any useful testing story. So I *knew* I would break those platforms if I touched the code, but I also couldn't possibly fix them given the amount of testing available. The result was that I did a half-ass refactoring job and in my commit logs "encouraged" the other maintainers to follow suit for the platforms, adding the necessary testing. And nothing has happened, which is exactly what I expected to happen. If we don't have tests so that the person motivated to do this kind of refactoring and restructuring can do it across platforms, then either the refactoring doesn't happen, or it happens but only for a subset of the platforms and we accrete more and more cruft in Clang.
> 
> Still, this isn't a new problem. Your patch doesn't make anything worse. However, I do think its reasonable to ask you or others who are actually maintaining this platform to actually find some time to add proper tests, and potentially do the kind of long-overdue refactoring that is needed here. I'm only picking on you personally because you are the first person to actually *try* to maintain this platform. Until now, for all I could tell, this platform was just dead and no one cared any more. If you can't find anyone with sufficient expertise to actually do the needed work on the platform, then I fear that eventually it *will* get nuked because no one is maintaining it. =/ I don't want to see that happen precisely because it will break people. But the open source project is kind of in a crappy position at this point with lots of code no one understands supporting a platform that no one knows how to maintain without sufficient tests to even check if the thing works. I don't know how that is a remotely viable long-term strategy.

There are a lot of things in LLVM for which we do not have good testing. It is totally reasonable of you to request that someone add better tests, but I don’t know anything more about win32-macho than you do, and I don’t know of anyone who is still involved with LLVM who would be in a better position to tackle that.

But, since I agree with you that more tests would be valuable, I’ve filed an internal request for someone to do that:
<rdar://problem/17271215> add more tests for win32-macho (EFI) platform
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140611/0ca6534a/attachment.html>