[llvm-commits] [PATCH][RFC] PR10367: don't allow GlobalAlias's aliasee to be an arbitrary Constant

[Duncan Sands]

Hi Jay, thanks for working on this.

> To summarise the PR, Chris said that a GlobalAlias's aliasee should
> always be a GlobalValue (we shouldn't allow arbitrary Constants), and
> the types of the alias and the aliasee shouldn't be required to match.

If we go this way then we eliminate the possibility of having an alias into
the middle of a global variable.  This has been requested a few times on the
mailing list.  For example, recently someone wanted to put canaries around
global variables.  To put such a buffer around a global variable G, the idea
is to create a new global G2 of the form "{ buff_type, typeof(G), buff_type }"
and then replace G with an alias of the middle field (of type typeof(G)) in
G2.  This seems like a useful thing to be able to do.  So I reckon first we
need to decide if want to move towards allowing this, or away from allowing
it (your patch moves away).

Also, since you are obliged to allow bitcasts and GEPs as an operand, allowing
the type of the alias to be different to the type of the operand doesn't win
you much.  Your changes do make things neater - but is it worth it?

Otherwise, some quick comments on your patch:

> --- llvm/trunk/lib/VMCore/Verifier.cpp  (revision 136357)
> +++ llvm/trunk/lib/VMCore/Verifier.cpp  (working copy)
> @@ -455,20 +455,8 @@
>            "Alias should have external or external weak linkage!", &GA);
>    Assert1(GA.getAliasee(),
>            "Aliasee cannot be NULL!", &GA);
> -  Assert1(GA.getType() == GA.getAliasee()->getType(),
> -          "Alias and aliasee types should match!", &GA);
>    Assert1(!GA.hasUnnamedAddr(), "Alias cannot have unnamed_addr!", &GA);
>
> -  if (!isa<GlobalValue>(GA.getAliasee())) {
> -    const ConstantExpr *CE = dyn_cast<ConstantExpr>(GA.getAliasee());
> -    Assert1(CE &&
> -            (CE->getOpcode() == Instruction::BitCast ||
> -             CE->getOpcode() == Instruction::GetElementPtr) &&
> -            isa<GlobalValue>(CE->getOperand(0)),
> -            "Aliasee should be either GlobalValue or bitcast of GlobalValue",
> -            &GA);
> -  }
> -
>    const GlobalValue* Aliasee = GA.resolveAliasedGlobal(/*stopOnWeak*/ false);
>    Assert1(Aliasee,
>            "Aliasing chain should end with function or global variable", &GA);

Don't you still need to check that the operand is either a global, or a bitcast
or GEP of a global?

> -const GlobalValue *GlobalAlias::getAliasedGlobal() const {
> -  const Constant *C = getAliasee();
> -  if (C == 0) return 0;
> -
> -  if (const GlobalValue *GV = dyn_cast<GlobalValue>(C))
> -    return GV;
> +  // Strip off any bitcast that might have been introduced by someone calling
> +  // ReplaceAllUsesWith() on our operand.
> +  if (ConstantExpr *CE = dyn_cast<ConstantExpr>(C)) {
> +    assert((CE->getOpcode() == Instruction::BitCast ||
> +            CE->getOpcode() == Instruction::GetElementPtr) &&
> +           "Unsupported aliasee");
> +    C = CE->getOperand(0);
> +  }
>
> -  const ConstantExpr *CE = cast<ConstantExpr>(C);
> -  assert((CE->getOpcode() == Instruction::BitCast ||
> -          CE->getOpcode() == Instruction::GetElementPtr) &&
> -         "Unsupported aliasee");
> -
> -  return dyn_cast<GlobalValue>(CE->getOperand(0));
> +  return cast<GlobalValue>(C);
>  }

This looks like a job for stripPointerCasts.

Ciao, Duncan.