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

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

I'm concerned that fixing up GlobalOpt::deleteIfDead isn't enough
to avoid affecting CodeGen.  There are other places in the
pipeline that will check Value::use_empty and/or have logic like
(or use directly) Function::isDefTriviallyDead.  I think it's
important that the call to GV.removeDeadConstantUsers() has the
same effect on use-lists whether or not there's any debug info.
Moreover, GlobalOpt::deleteIfDead should have the same effect on
the entire module (including use-lists) regardless of debug info.

(But in case you can convince me, more comments below.)

> On 2016-Mar-15, at 13:10, Adrian Prantl <aprantl at apple.com> wrote:
> 
> aprantl added a comment.
> 
> I added a check to catch the case where the GV is only kept alive by a constant that is only kept alive by metadata.

Can you add a testcase that confirms that a GlobalVariable will be
deleted even if it has a metadata use?  Please included nested
constant expressions to test recursive descent.

Please test Function too, since it has different code paths.

> 
> http://reviews.llvm.org/D18109
> 

> Index: lib/Transforms/IPO/GlobalOpt.cpp
> ===================================================================
> --- lib/Transforms/IPO/GlobalOpt.cpp
> +++ lib/Transforms/IPO/GlobalOpt.cpp
> @@ -1683,8 +1683,19 @@
>    return true;
>  }
>  
> +/// Return true if GV is only used by constants which are only used by metadata.
> +static bool onlyUsedByMetadata(GlobalValue &GV) {

I think "hasNonMetadataUsers" might be clearer, if you need this
function at all.

> +  for (auto *U : GV.users())
> +    if (auto *C = dyn_cast<Constant>(U)) {
> +      if (!(C->use_empty() && C->isUsedByMetadata()))

For nested constant expressions, will this do the right thing?

(I don't any recursive descent...)

> +        return false;
> +    } else
> +      return false;
> +  return true;
> +}
> +
>  bool GlobalOpt::deleteIfDead(GlobalValue &GV) {
> -  GV.removeDeadConstantUsers();
> +  GV.removeDeadConstantUsers(/* skip metadata uses */false);

Can `removeDeadConstantUsers()` return whether GV only has metadata-
based users left?  Then you might avoid the new helper method, and
the compile-time cost it implies.

Also, "skip metadata users" isn't very grep-able; IMO it's better to
name this after the function parameter (`/* SkipMetadataUses = */`).

>  
>    if (!GV.isDiscardableIfUnused())
>      return false;
> @@ -1697,7 +1708,7 @@
>    if (auto *F = dyn_cast<Function>(&GV))
>      Dead = F->isDefTriviallyDead();

I think the change to `removeDeadConstantUsers()` will affect the
return of `F->isDefTriviallyDead()`.  That makes the logic pretty
complicated, since you need to skip `blockaddress()` expressions
as well as metadata expressions.

>    else
> -    Dead = GV.use_empty();
> +    Dead = GV.use_empty() || onlyUsedByMetadata(GV);
>    if (!Dead)
>      return false;
>