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

[Nick Lewycky]

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

I don't see why this is harder than any of the other tests in 
unittest/IR/VerifierTest.cpp?

Nick

>>> 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>