[PATCH] D121970: [Verify] check that BlockAddresses don't refer to functions with certain linkages which wont be emitted

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D121970#3391502 <https://reviews.llvm.org/D121970#3391502>, @efriedma wrote:

> I'm not sure I understand the issue you're trying to prevent here.

Two concerns. The first is that I don't think it makes sense at all to have a blockaddress refer to an `available_externally` function be used outside of that function itself, ever.  Consider this IR from my added test case above:

  define available_externally void @foo() {
  entry:
    br label %hello
  hello:
    ret void
  }
  
  @glo = global i8* blockaddress(@foo, %hello)

Running `llc` on the above will produce:

  	.text
  	.file	"z.ll"
  	.type	glo, at object                     # @glo
  	.data
  	.globl	glo
  	.p2align	3
  glo:
  	.quad	.Ltmp0
  	.size	glo, 8
  
  	.section	".note.GNU-stack","", at progbits

There is no `.Ltmp0`. Clang will not assemble that (GAS will!).

The second concern is that I think IPSCCP is too aggressive when it comes to sinking BlockAddress Constants.  I don't think it should do so when that would move BlockAddresses that refer to functions with available_externally linkage (and possibly other linkage types).

Consider the following C code.

  static void *a(void *x) {
    return x;
  }
  
  __attribute__((gnu_inline))
  extern inline void* b(void) {
  b_label:
    return a(&&b_label);
  }
  
  void *c(void) {
    return b();
  }

My concern is IPSCCP sinking `&&b_label` into `a`.  If you do: `clang -O2 z.c -c -emit-llvm -S -Xclang -disable-llvm-passes -o z.ll; opt -passes=ipsccp -S z.ll` you get this for `@a`:

  define internal i8* @a(i8* noundef %x) #0 {
  entry:
    %x.addr = alloca i8*, align 8
    store i8* blockaddress(@b, %b_label), i8** %x.addr, align 8, !tbaa !3 ; oops! @b has available_externally linkage and wont be emitted
    %0 = load i8*, i8** %x.addr, align 8, !tbaa !3
    ret i8* %0
  }

I think I should add that test and prevent IPSCCP in this patch as well.

> You can have basic blocks that won't be emitted into the object file for a bunch of reasons; it might not have any predecessors, it might be inside an unused internal function, or something else.  In all those cases, we use exactly the same strategy: replace the blockaddress with "1".  I don't see any reason to special-case available_externally.

That assumes that we've run optimizations and computed reachability. I suspect that linkage type and reachability are distinct concerns here.  I guess I'm concerned that if you take "bad" IR and just feed it into llc without running opt, we get some weird results with blockaddresses.  Sure, running opt may help replace bad blockaddresses, but I don't think there's guarantees that code is generally compiled with optimizations on.

> Granted, available_externally functions that contain an indirectbr are currently pretty useless, since they almost always just get discarded.  But we could improve that situation if it mattered.

It's not about what instructions an available_externally function contains; more so about who is referring to available_externally functions.

> If you're dealing with weak symbols, there might be issues... but not because there's any fundamental semantic hole.  If a function definition gets discarded, the values of blockaddresses which refer to that definition don't matter.

The values don't matter, until we emit dead references (see `.Ltmp0` example above) or fail to import the use in bitcode reader during LTO.

> But the way we currently emit blockaddresses referring to weak functions on ELF targets will lead to link errors, I think.

Runtime linkage failures, right? I'm thinking we might want to test that case, since @dexonsmith originally asked about `weak` functions.

> We could solve that by changing the way we lower blockaddress for such functions.  For example, by using a relative offset instead of an absolute address.  I guess that only works for indirectbr, not callbr, but we're likely going to change the way callbr works anyway.
>
> https://reviews.llvm.org/D120781#3383492 has an example of a blockaddress in a linkonce_odr global variable, but I'm pretty sure that has undefined behavior at the IR level.  A blockaddress in one translation unit is never equivalent to a blockaddress in another translation unit, so it breaks the rules for linkonce_odr.  It might make sense for clang to diagnose that.

That also sounds like something good to codify. I would pursue checking that via IR verification, then knee-capping places that might produce such invalid IR, like clang.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121970/new/

https://reviews.llvm.org/D121970