Differential D121970
[Verify] check that BlockAddresses don't refer to functions with certain linkages which wont be emitted Authored by nickdesaulniers on Mar 17 2022, 3:37 PM.
Details
In D120781, there's discussion that various linkages can cause the For a function with available_externally linkage, it obviously doesn't Check the users of each blockaddress; permit the above linkage if the
Diff Detail
Unit Tests
Event TimelineComment Actions whoops, forgot to add my test!
Comment Actions I'm not sure I understand the issue you're trying to prevent here. 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. 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. 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. But the way we currently emit blockaddresses referring to weak functions on ELF targets will lead to link errors, I think. 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. Comment Actions 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,@object # @glo .data .globl glo .p2align 3 glo: .quad .Ltmp0 .size glo, 8 .section ".note.GNU-stack","",@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.
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.
It's not about what instructions an available_externally function contains; more so about who is referring to available_externally functions.
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.
Runtime linkage failures, right? I'm thinking we might want to test that case, since @dexonsmith originally asked about weak functions.
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. Comment Actions Maybe we can add to https://llvm.org/docs/LangRef.html#addresses-of-basic-blocks statements like:
Or some language about what happens when the referred to function has certain linkages, and the use occurs outside of that function. Comment Actions I think I'd consider that a bug in codegen, not a fundamental IR issue. If llc decides not to emit a function, it should clean up any related blockaddresses, the same way opt would. I mean, the way you're looking at it sort of works, but it seems like you end up adding special cases to a bunch of transformation passes. You mention ipsccp, but there's also the inliner, and globalopt, and also probably other stuff I'm not thinking of.
I think llc should explicitly handle this as part of codegen. I'm not suggesting we should depend on opt passes.
It's an object format issue, which I think we should be able to solve without making IR invariants more complicated.
Static linker, not dynamic linker. The linker will get unhappy if you use a non-weak symbol to refer into a comdat. Once it links successfully, the code exists in the shared library no matter what the symbol resolver does, so there isn't an issue.
We can state this explicitly in LangRef. Not sure if it makes sense to turn this into a verifier check; the problem isn't just block addresses, it's any symbol with internal linkage. I think we'd end up introducing weird checks into a bunch of transforms to enforce it. Overall, I think it's possible to reach a consistent state with adding this invariant, but I think it would be simpler to reach a consistent state without it. I think all the problems you've brought up can be resolved in other ways. Comment Actions This is the piece I was missing (which led to me suggesting there should be a verifier check). Looks like this *is* documented in LangRef, which I think makes it clear:
Due to this, I now understand why blockaddress is lowered to constant non-null value if the function is not selected; seems like it's safe enough. Also:
I agree; based on LangRef, that has UB at the IR level. Having Clang diagnose it probably makes sense. | ||||||||||||||||||||||||
So for ConstantExpr, I think we'll want a new SmallPtrSet<ConstantExpr*> member for Verifier, akin to ConstantExprVisited because theoretically, two different BlockAddresses could appear in the same ConstantExpr...oh, but we will need to revist the ConstantExpr each time for each BlockAddress, so nvm. Guess I should delete my comment above.