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

[Manman Ren]

On 7/31/13, 3:45 PM, David Blaikie wrote:
> 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.
True. And that is what I said above.
>
> 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,
True, in fact I tried to replace "dyn_cast_or_null" with cast_or_null, 
and got a lot of errors.
> but
> we'd still need a separate verifying function because getNodeField
> would just assert-fail inside the verifier which isn't what we want.
We have a FIXME in fieldIsMDNode to use "!Fld || isa<MDNode>(Fld)".

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