[PATCH] D18109: Prevent GlobalOpts from dropping ASANitized global variables

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

Ugh, this is ugly.  We should really start using a !dbg attachment on
global variables, much like we changed functions.  Then ASan can update
the attachment (if necessary), and we don't rely on RAUW to get things
right.  (I'm not sure that needs to happen before we fix this bug.)

Comments inline.

> On 2016-Mar-11, at 16:18, Adrian Prantl <aprantl at apple.com> wrote:
> 
> aprantl created this revision.
> aprantl added reviewers: dblaikie, samsonov, dexonsmith, echristo.
> aprantl added a subscriber: llvm-commits.
> aprantl set the repository for this revision to rL LLVM.
> 
> ASAN transforms global variables into an array with extra storage at the end and RAUWs the global value with a GEP into the array. GlobalOpts will think that the GEP constexpr is dead because its only use is a Metadata node and eliminate it, thus dropping the storage from the debug info.
> This patch fixes this by adding a flag to Constant::removeDeadConstantUsers() to optionally ignore metadata users.
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D18109
> 
> Files:
>  include/llvm/IR/Constant.h
>  lib/IR/Constants.cpp
>  lib/Transforms/IPO/GlobalOpt.cpp
>  test/Transforms/GlobalOpt/debug-info-global-vars.ll
> 
> <D18109.50480.patch>

> Index: lib/Transforms/IPO/GlobalOpt.cpp
> ===================================================================
> --- lib/Transforms/IPO/GlobalOpt.cpp
> +++ lib/Transforms/IPO/GlobalOpt.cpp
> @@ -1684,7 +1684,7 @@
>  }
>  
>  bool GlobalOpt::deleteIfDead(GlobalValue &GV) {
> -  GV.removeDeadConstantUsers();
> +  GV.removeDeadConstantUsers(/* skip metadata uses */false);

This seems fishy.  How sure are you that this can never affect the
emitted code?

>  
>    if (!GV.isDiscardableIfUnused())
>      return false;
> Index: lib/IR/Constants.cpp
> ===================================================================
> --- lib/IR/Constants.cpp
> +++ lib/IR/Constants.cpp
> @@ -466,16 +466,19 @@
>  /// removeDeadUsersOfConstant - If the specified constantexpr is dead, remove
>  /// it.  This involves recursively eliminating any dead users of the
>  /// constantexpr.
> -static bool removeDeadUsersOfConstant(const Constant *C) {
> +static bool removeDeadUsersOfConstant(const Constant *C, bool SkipMetadataUses) {

I think `IgnoreMetadata` is clearer.

>    if (isa<GlobalValue>(C)) return false; // Cannot remove this
>  
>    while (!C->use_empty()) {
>      const Constant *User = dyn_cast<Constant>(C->user_back());
>      if (!User) return false; // Non-constant usage;
> -    if (!removeDeadUsersOfConstant(User))
> +    if (!removeDeadUsersOfConstant(User, SkipMetadataUses))
>        return false; // Constant wasn't dead
>    }
>  
> +  if (!SkipMetadataUses && C->isUsedByMetadata())
> +    return false;
> +
>    const_cast<Constant*>(C)->destroyConstant();
>    return true;
>  }
> @@ -485,7 +488,7 @@
>  /// off of this constant, remove them.  This method is useful for clients
>  /// that want to check to see if a global is unused, but don't want to deal
>  /// with potentially dead constants hanging off of the globals.
> -void Constant::removeDeadConstantUsers() const {
> +void Constant::removeDeadConstantUsers(bool SkipMetadataUses) const {

(I prefer IgnoreMetadata.)

>    Value::const_user_iterator I = user_begin(), E = user_end();
>    Value::const_user_iterator LastNonDeadUser = E;
>    while (I != E) {
> @@ -496,7 +499,7 @@
>        continue;
>      }
>  
> -    if (!removeDeadUsersOfConstant(User)) {
> +    if (!removeDeadUsersOfConstant(User, SkipMetadataUses)) {
>        // If the constant wasn't dead, remember that this was the last live use
>        // and move on to the next constant.
>        LastNonDeadUser = I;
> Index: include/llvm/IR/Constant.h
> ===================================================================
> --- include/llvm/IR/Constant.h
> +++ include/llvm/IR/Constant.h
> @@ -148,7 +148,7 @@
>    /// them. This method is useful for clients that want to check to see if a
>    /// global is unused, but don't want to deal with potentially dead constants
>    /// hanging off of the globals.
> -  void removeDeadConstantUsers() const;
> +  void removeDeadConstantUsers(bool SkipMetadataUses = true) const;

(I prefer IgnoreMetadata.)

>  
>    Constant *stripPointerCasts() {
>      return cast<Constant>(Value::stripPointerCasts());
>