[llvm] r184239 - We want a string representation of the attribute, not the kind as a string.

[David Blaikie]

On Tue, Jun 18, 2013 at 9:52 PM, Bill Wendling <isanbard at gmail.com> wrote:
> On Jun 18, 2013, at 7:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Jun 18, 2013 6:30 PM, "Bill Wendling" <isanbard at gmail.com> wrote:
>>
>> On Jun 18, 2013, at 5:33 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> > On Tue, Jun 18, 2013 at 5:28 PM, Bill Wendling <isanbard at gmail.com>
>> > wrote:
>> >> On Jun 18, 2013, at 5:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >>
>> >>> On Tue, Jun 18, 2013 at 4:46 PM, Bill Wendling <isanbard at gmail.com>
>> >>> wrote:
>> >>>> On Jun 18, 2013, at 3:43 PM, David Blaikie <dblaikie at gmail.com>
>> >>>> wrote:
>> >>>>
>> >>>>> On Tue, Jun 18, 2013 at 2:52 PM, Bill Wendling <isanbard at gmail.com>
>> >>>>> wrote:
>> >>>>>> On Jun 18, 2013, at 2:39 PM, Bill Wendling <isanbard at gmail.com>
>> >>>>>> wrote:
>> >>>>>>
>> >>>>>>> On Jun 18, 2013, at 2:35 PM, David Blaikie <dblaikie at gmail.com>
>> >>>>>>> wrote:
>> >>>>>>>
>> >>>>>>>> On Tue, Jun 18, 2013 at 2:27 PM, Bill Wendling
>> >>>>>>>> <isanbard at gmail.com> wrote:
>> >>>>>>>>> Author: void
>> >>>>>>>>> Date: Tue Jun 18 16:27:00 2013
>> >>>>>>>>> New Revision: 184239
>> >>>>>>>>>
>> >>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=184239&view=rev
>> >>>>>>>>> Log:
>> >>>>>>>>> We want a string representation of the attribute, not the kind
>> >>>>>>>>> as a string.
>> >>>>>>>>
>> >>>>>>>> Test case? (do we have test cases for the verifier?)
>> >>>>>>> I'll cobble one together.
>> >>>>>> I don't think I can. This requires that the .bc file be malformed.
>> >>>>>> So the `llvm-as' will reject it before it gets to the code I just fixed...
>> >>>>>
>> >>>>> Can we remove this as dead code then? (replace it with an assert)
>> >>>>
>> >>>> No. The check is valid for checking bitcode rather than text.
>> >>>
>> >>> Be nice to have tests for it, then - even if they're checked in binary
>> >>> files. (I'd be OK with them being derived from known good files with a
>> >>> comment (in the corresponding 'foo.test' text file that actually has
>> >>> the RUN line in it) explaining which bit was twiddled)
>> >>>
>> >>> Do we not have any tests for binary-only issues the verifier can
>> >>> diagnose?
>> >> No. And the file they sent is rather large...and because of the nature
>> >> of the bug, I can't reduce it.
>> >
>> > Not sure I follow as to why it would be hard to produce an artificial
>> > case that would reach the code based on an understanding of the code
>> > (since you know what the bug is) rather than based on a manual
>> > reduction from the reported sample (when you don't know where the bug
>> > is I can understand the need to reduce the original test case in an
>> > effort to help isolate the piece of code to fix the bug in)
>> >
>> Because I would have to write a program that could produce unverified llvm
>> IR. That's exactly what this fix is for.
>
> Not necessarily - I'm just suggesting it could be done manually Wight a hex
> editor or similar. Though I've not looked AAT the particularly complexity of
> this case or the bitcode format to understand exactly how complex that would
> be.
>
>
> It would be pretty difficult unfortunately.

I'm not able to agree or disagree with this. I wouldn't've thought it
was that difficult - write it with one attribute, hand-hack it to
another. But I don't know anything about the BC format, so that's only
a stab in the dark.

> If it did require writing a program to genertw such an input, that might
> even be justified - are the set of possible verifier failures only reachable
> by bitcode input very small? I wouldn't think so. Seems like. Substantial
> amount of test coverage were missing at the moment.
>
> We almost certainly have a lot of holes in the verifier coverage. And a tool
> that generated invalid IR might be good. But it's more like a project than a
> simple test case. :-)

I'm not really sure how much use a tool would be here - given the
arbitrarily invalid input we'd want to produce.

A BC fuzzer might be fun, but probably not worth it.

>
>> >> I *really* think it's okay that this go in without a test. :-)
>> >
>> > I *really* dislike untested code (let alone substantial chunks of code
>> > without even a test strategy - in part because every person who goes
>> > to fix a bug here will have the same incremental reason not to bother
>> > adding a test case because there aren't any others/no obvious way to
>> > do it... so we end up with a growing body of untested code in an area
>> > that it's sort of important that we have good coverage because I
>> > expect users who need the verifier would really like it to work
>> > correctly).
>>
>> The fix I made was to fix how a message was output by the verifier.
>
> I see that.
>
>> It asserted correctly because someone used the wrong API.
>
> Unrelated to this discussion, though I'm curious - what do you mean by the
> 'wrong API'? Was there some incorrect use of llvms APIs that lead to invalid
> bitcode? Could we improve the APIs to diagnose the problem sooner?
>
> Probably the best way to avoid this particular type of misuse would be to
> improve the documentation of the API.
>
>> It was a trivial change.
>
> I'm not suggesting the change was non trivial. I'm suggesting that it
> indicates a lack of test coverage which is greater concern than this one bug
> & presents an opportunity, now that its clear/demonstrated that were missing
> a bunch of test coverage, to begin to address that shortcoming.
>
>
> I understand. And I agree. I'm just saying that in this particular case it's
> not particularly useful. Or rather the cost of generating the invalid file
> overshadows the bug that was fixed. As mentioned above, a tool that can
> generate these types of bit code files that exercise all of the verifier
> would be a good thing. I just see that as larger project. A PR about it
> would be nice. :-)

Sure enough, if you could file a PR, CC me. If I can find the time to
try this & find a reasonably (my time) efficient approach, great.

> I don't want this to become a big thing. If you're adamant that a test case
> is needed, I'll try to create one somehow. It just won't be checked in
> quickly. :-(

It might not be your priority immediately - just technical debt that
should be paid sooner or later. The sooner the better (to make it easy
to keep test coverage good as we add new code/make changes -
retroactively adding test coverage is pretty much a non-starter),
though.