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

[Manman Ren]

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.

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