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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

In https://reviews.llvm.org/D26438#591399, @manmanren wrote:

> > There is this bit of language in the TBAA implementation:
> > 
> >   // The second field is the access type node, it
> >   // must be a scalar type node.
> > 
> > 
> > but in the TBAA metadata generated by clang it seems like we have access types like `!{!"int", !N, i64 0}` which look like struct nodes.
>
> Can you give an example where clang generates a struct path tag node where the 2nd field is not a scalar type node?


That seems to be the case everywhere.  E.g.

  struct S {
       int i;
       float f;
  };
  
  float f(struct S* s) {
       return s->f;
  }

compiles to

  ; ModuleID = 'x.c'
  source_filename = "x.c"
  target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "x86_64-apple-macosx10.12.0"
  
  %struct.S = type { i32, float }
  
  ; Function Attrs: norecurse nounwind readonly ssp uwtable
  define float @f(%struct.S* nocapture readonly %s) local_unnamed_addr #0 {
  entry:
    %f = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 1
    %0 = load float, float* %f, align 4, !tbaa !2
    ret float %0
  }
  
  attributes #0 = { ... }
  
  !llvm.module.flags = !{!0}
  !llvm.ident = !{!1}
  
  !0 = !{i32 1, !"PIC Level", i32 2}
  !1 = !{!"clang version 4.0.0 (http://llvm.org/git/clang.git b74377b0a34a33cb3c7b565f0e543cd3e56b3f6b) (llvm/trunk 286280)"}
  !2 = !{!3, !7, i64 4}
  !3 = !{!"S", !4, i64 0, !7, i64 4}
  !4 = !{!"int", !5, i64 0}
  !5 = !{!"omnipotent char", !6, i64 0}
  !6 = !{!"Simple C/C++ TBAA"}
  !7 = !{!"float", !5, i64 0}

The tag for the load instruction is `!2` whose access type is `!7` == `!{!"float", !5, i64 0}`.

> I think we only check the immutable bit on the tag node (not the type nodes).

You're right -- that's what the implementation does; but that constraint is not mentioned in the langref or the file comment in the TBAA implementation (so just by reading the docs and comments it would seem like it is okay to mark type nodes as immutable, and even if that is a no-op today, it may help in the future).  But I will happily change the spec to conform to the implementation here since it makes life simpler. :)


https://reviews.llvm.org/D26438