[PATCH] D14838: [ThinLTO] Metadata linking for imported functions

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Nov-30, at 07:05, Teresa Johnson <tejohnson at google.com> wrote:
> 
> tejohnson added a comment.
> 
> Ping.
> Thanks, Teresa
> 
> 
> http://reviews.llvm.org/D14838

Thanks for working on this, and sorry for the delay.

I was looking at the patch bottom-up and came across a major issue, so
I've stopped reviewing at `TrackingMDRef`.  Once you've got that solved
I can look again at the updated patch.  However, it's quite big, so if
you can possibly split out the uninteresting/separable bits that would
be helpful.

> Index: include/llvm/IR/TrackingMDRef.h
> ===================================================================
> --- include/llvm/IR/TrackingMDRef.h
> +++ include/llvm/IR/TrackingMDRef.h
> @@ -14,6 +14,7 @@
>  #ifndef LLVM_IR_TRACKINGMDREF_H
>  #define LLVM_IR_TRACKINGMDREF_H
>  
> +#include "llvm/ADT/DenseMapInfo.h"
>  #include "llvm/IR/MetadataTracking.h"
>  #include "llvm/Support/Casting.h"
>  
> @@ -81,17 +82,24 @@
>    bool operator!=(const TrackingMDRef &X) const { return MD != X.MD; }
>  
>  private:
> +  /// Enabler for DenseMapInfo of TrackingMDRefs, ensure the MD is non-null
> +  /// and not a special DenseMapInfo key.

I don't think `TrackingMDRef` is appropriate as a `DenseMap<>` key.  Its
value can change to another arbitrary value.  If that happens, the
associated `DenseMapEntry` will have the wrong hash, and the table will
be invalid.

As I mentioned above, I was reading the patch from the bottom, so I'm
not quite sure where you're actually using this... but I'd be happy to
discuss other possible solutions.

> +  bool isValid() {
> +    return MD && MD != DenseMapInfo<Metadata *>::getEmptyKey() &&
> +           MD != DenseMapInfo<Metadata *>::getTombstoneKey();
> +  }
> +
>    void track() {
> -    if (MD)
> +    if (isValid())
>        MetadataTracking::track(MD);
>    }
>    void untrack() {
> -    if (MD)
> +    if (isValid())
>        MetadataTracking::untrack(MD);
>    }
>    void retrack(TrackingMDRef &X) {
>      assert(MD == X.MD && "Expected values to match");
> -    if (X.MD) {
> +    if (X.isValid()) {
>        MetadataTracking::retrack(X.MD, MD);
>        X.MD = nullptr;
>      }
> @@ -165,6 +173,22 @@
>    }
>  };
>  
> +/// Provide DenseMapInfo for TrackingMDRefs.
> +template <> struct DenseMapInfo<TrackingMDRef> {
> +  static inline TrackingMDRef getEmptyKey() {
> +    return TrackingMDRef(DenseMapInfo<Metadata *>::getEmptyKey());
> +  }
> +  static inline TrackingMDRef getTombstoneKey() {
> +    return TrackingMDRef(DenseMapInfo<Metadata *>::getTombstoneKey());
> +  }
> +  static unsigned getHashValue(const TrackingMDRef Val) {
> +    return DenseMapInfo<Metadata *>::getHashValue(Val.get());
> +  }
> +  static bool isEqual(const TrackingMDRef LHS, const TrackingMDRef RHS) {
> +    return LHS == RHS;
> +  }
> +};
> +
>  } // end namespace llvm
>  
>  #endif
> Index: include/llvm/IR/DebugInfoMetadata.h
> ===================================================================
> --- include/llvm/IR/DebugInfoMetadata.h
> +++ include/llvm/IR/DebugInfoMetadata.h
> @@ -1118,8 +1118,11 @@
>    }
>  
>    TempDILocation cloneImpl() const {

This seems possible to split out and test with a C++ unit test.

> -    return getTemporary(getContext(), getLine(), getColumn(), getScope(),
> -                        getInlinedAt());
> +    // Get the raw scope/inlinedAt since it is possible to invoke this on
> +    // a DILocation containing temporary metadata. Specifically, this happens
> +    // when we are linking metadata as a postpass after function importing.

I think this comment belongs in the commit message, as once the code is
committed we'll grow more uses of it.  The C++ unit test should be
enough to prevent tampering.

> +    return getTemporary(getContext(), getLine(), getColumn(), getRawScope(),
> +                        getRawInlinedAt());
>    }
>  
>    // Disallow replacing operands.
>