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

[Bill Wendling]

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. 

>> 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. It asserted correctly because someone used the wrong API. It was a trivial change. Adding a large BC file to test that this error message emits without another assert getting in the way is not very useful. 

-bw