[llvm] r187106 - Debug Info: improve the verifier to check field types.

[David Blaikie]

On Wed, Jul 31, 2013 at 3:36 PM, Manman Ren <manman.ren at gmail.com> wrote:
> On 7/25/13, 2:40 PM, David Blaikie wrote:
>>
>> On Thu, Jul 25, 2013 at 2:25 PM, Manman Ren <manman.ren at gmail.com> wrote:
>>>
>>> On 7/25/13, 2:01 PM, David Blaikie wrote:
>>>>
>>>> On Thu, Jul 25, 2013 at 1:57 PM, Manman Ren <manman.ren at gmail.com>
>>>> wrote:
>>>>>
>>>>> On 7/25/13, 1:46 PM, David Blaikie wrote:
>>>>>>
>>>>>> On Thu, Jul 25, 2013 at 1:40 PM, Eric Christopher <echristo at gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> Oh I see what you mean now. We should just disallow the usage of
>>>>>>> strings in the MDNode fields.
>>>>>>
>>>>>> I think we use i32 0 for null in these fields too, if I recall
>>>>>> correctly - if I'm right, we probably should disallow that too. If
>>>>>> it's an MDNode field, it should be an MDNode, most likely (I'm open to
>>>>>> being shown a reason for this not to be true - but we should talk
>>>>>> about/justify that rather than just accepting it as "that's the way
>>>>>> things are").
>>>>>
>>>>> Yes, MDNodes should be MDNodes, but currently we use null, i32 0, maybe
>>>>> empty string "".
>>>>> We can improve the verifier incrementally.
>>>>> And each time we improve the verifier, we will update the testing
>>>>> cases.
>>>>
>>>> Yep, please include a FIXME that the implementation of that function
>>>> should be replaced by simply "getFieldAs<MDNode>(num)" or whatever
>>>> simple implementation would work.
>>>
>>> getFieldAs used a dyn_cast so it is not going to catch any error.
>>
>> Sure, but for every place you currently have:
>>
>> if (!fieldIsMDNode(DbgNode, 9))
>>    return false;
>>
>> you'd replace it with:
>>
>> if (!getFieldAs<MDNode>(9))
>>    return false;
>
> We currently use "i32 0" and an empty string to represent null, before that
> is cleaned up,
> we can't use getFieldAs since it will return null for "i32 0" and an empty
> string, which should be allowed.

This statement doesn't make sense to me (I mean I know what I think
you're trying to say, but it doesn't sound like what you're saying, so
I'm confused/concerned that I've misunderstood something)

Current state:
i32 0 and empty string are allowed for null MDNode fields in the debug
info metadata
getFieldAs<MDNode> returns null for those values, and many others...
OK, so we can't actually use it to verify because some of these fields
should be acceptably null.

So it seems at some point getNodeField (the function underlying
getFieldAs) should use "cast_or_null" instnead of "dyn_cast_or_null"
once we've cleaned up all the node fields to be the right type, but
we'd still need a separate verifying function because getNodeField
would just assert-fail inside the verifier which isn't what we want.

That still doesn't seem to correlate to what you just said, but anyway...

- David

>
> Manman
>
>>
>> I would imagine
>>
>>> I added a FIXME in r187157.
>>>
>>> Thanks,
>>> Manman
>>>
>>>> (& if you want, you could include the i32 int case, rather than
>>>> allowing any i32 value)
>>>>
>>>>> Thanks,
>>>>> Manman
>>>
>>>
>