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

[David Blaikie]

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.

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.

>
> >> 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?

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

> Adding a large BC file to test that this error message emits without
another assert getting in the way is not very useful.

I'm not suggesting adding a large BC file.

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