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

[David Blaikie]

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;

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