[llvm] r186119 - In response to dblaikie's comment on r186035, replacing the

[David Blaikie]

On Mon, Jul 15, 2013 at 5:15 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Jul 15, 2013, at 3:31 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Fri, Jul 12, 2013 at 3:37 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>
>>> On Jul 12, 2013, at 2:55 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>> On Fri, Jul 12, 2013 at 2:49 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>> doesn't seem to - I attempted to revert your fix (r185966 - had some
>>>>>
>>>>> You’ll want to revert 186014, not r185966.
>>>>
>>>> Reverting r186014 doesn't cause dbg_value_direct to fail for me
>>>> (either on ToT or after reverting 186119). Am I missing something?
>>>
>>> It works for me if I reset --hard all the way back to r186013.
>>
>> Right, that helped. I've simplified this as far as I can in r186356 to
>> give you an idea of what I meant (for future reference).
>>
>>> For some reason, just reverting 186014 is not sufficient.
>>
>> That's slightly concerning - is it possible that some future change
>> removed the need for your original fix? It'd be nice if you could
>> explain that & ensure we don't have redundant/unnecessary code lying
>> around - the debug info code is hard enough to maintain as it is
>> without random excess bits left in.
>
> Given the nature of the fix (nullptr deref), at best it could have covered the problem.

I'm not sure that's necessarily true - other changes may've altered
the invariants (but, yes, there's a fair chance other changes just
changed the spilling properties - but that's equally a problem, that
means the test case is no longer testing the bug)

> It would probably be best to update the test so that we only run the InlineSpiller pass from opt.

If you could - that'd be great.

> But then again given how trivial 186014 is, I’m not entirely sure that it is actually worth spending the time on.

It's rather important that tests continue to actually test bugs -
especially for random bits of code like this. It's not unlikely that
someone might come around years from now, look at that code, guess
that it became dead, remove it & find that no tests fail - only to
have reintroduced the same bug.

I'd really like to make sure we have good test coverage/justification
for the changes we make - because I'd like to be able to improve the
quality of the implementation of debug info with the surety that if I
break things the regression tests will tell me. If we don't have
confidence in our test coverage we'll be paralyzed to make anything
but simple/isolated bug fixes - & then the code rots & becomes even
harder to maintain than it already is.

> thanks for looking into it!

Sure thing

> adrian
>