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

[Bill Wendling]

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. 

> 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 *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. :-)

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. :-(

-bw
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130618/21591983/attachment.html>