[PATCH] D26438: [Verifier] Add verification for TBAA metadata

[Manman Ren via Phabricator via llvm-commits]

manmanren added a comment.

Thanks for the work!

Manman



================
Comment at: lib/IR/Verifier.cpp:3706
+  (void)InsertResult;
+  assert(InsertResult.second && "We just checked!");
+  return Result;
----------------
Is this necessary given that we just checked BaseNode is not in TBAABaseNodes?


================
Comment at: lib/IR/Verifier.cpp:3802
+static bool IsSingularStructTBAANode(const MDNode *MD,
+                                     SmallPtrSetImpl<const MDNode *> &Visited) {
+  if (MD->getNumOperands() != 3)
----------------
sanjoy wrote:
> manmanren wrote:
> > This seems somewhat heavy-weighted. I guess you are trying to differentiate a scalar type node from a struct type node with a single field, given that both can have 3 operands with the 2nd operand being a type and the 3rd being an offset. I don't think you can really differentiate the two cases. I feel like checking the offset being zero for the access type node of a tag should be enough.
> > 
> > I will not call them *singular" struct type node, since they are scalar type node.
> > 
> > createTBAAScalarTypeNode will generate such a scalar type node with 3 operands, createTBAANode (which is not used in clang any more) can generate a scalar type node with 2 operands.
> Hi Manman,
> 
> I'd like to keep the recursive nature of this check and not just check
> the offset only on the MDNode for the access tag.
> 
> IOW, I'd like to avoid invalid cases where the access tag has a full
> blown struct type descriptor at offset 0, since in such (invalid)
> cases `getMostGenericTBAA` will produce bad merged metadata that would
> be hard to debug:
> 
> ```
> ;; Invalid metadata that I want the verifier to reject
> 
> !root = !{!"C++ TBAA"}
> !int = !{!"int", !root}
> !float = !{!"float", !root}
> !s_a = !{"s_a", !int, 5, !float, 20}
> !s_b = !{"s_b", !int, 6, !float, 20}
> 
> !scalar_a = !{!"scalar-a", !s_a, 0}
> !scalar_b = !{!"scalar-b", !s_b, 0}
> 
> !tag_a = !{!some_struct, !scalar_a, 20}
> !tag_b = !{!some_struct, !scalar_b, 20}
> 
> !tag_c = !{!float, !float, 0}
> 
> !generic_tag = !{!int, !int, 0}
> ```
> 
> Both `!tag_a` and `!tag_b` should may-alias with `!tag_c`, but
> `getMostGenericTBAA` will merge `!tag_a` and `!tag_b` into
> `!generic_tag` which does not alias with `!tag_c`.
> 
> However, I'm happy to call replace "singular" with "scalar".
> 
Sounds good. More checking means fewer bugs.
Please rename singular to scalar and add a testing case for the above example if it is not in the patch.


https://reviews.llvm.org/D26438