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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/IR/Verifier.cpp:3706
+  (void)InsertResult;
+  assert(InsertResult.second && "We just checked!");
+  return Result;
----------------
manmanren wrote:
> Is this necessary given that we just checked BaseNode is not in TBAABaseNodes?
No, it is redundant, which is why it is an `assert` and not an `Assert`. :)


================
Comment at: lib/IR/Verifier.cpp:3802
+static bool IsSingularStructTBAANode(const MDNode *MD,
+                                     SmallPtrSetImpl<const MDNode *> &Visited) {
+  if (MD->getNumOperands() != 3)
----------------
manmanren wrote:
> 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.
Renamed and added a test case.


https://reviews.llvm.org/D26438