[PATCH] D92657: Fix interaction between clang and some inline builtins from glibc under _FORTIFY_SOURCE

[Reid Kleckner via Phabricator via llvm-commits]

rnk added subscribers: mcgrathr, efriedma, bkramer.
rnk added a comment.

I apologize for taking an unreasonably long time to respond, this feature doesn't fill me with a sense of joy and fulfillment, but I do want to help get things unblocked.

Let me try getting more input and visibility from some folks: @bkramer @efriedma @mcgrathr

My basic input is that the frontend should try to untangle the complexity of the glibc fortify implementation so that the middle-end can remain blissfully unaware of it.



================
Comment at: clang/test/CodeGen/memmove-always-inline-definition-used.c:17-19
+// CHECK-LABEL: define available_externally i8* @memmove
+// CHECK: call void @llvm.memcpy
+// CHECK: call i8* @memmove({{.*}}) #[[ATTR:[0-9]+]]
----------------
rnk wrote:
> Even if this happens to work in practice, memmove is still mutually recursive, and some other IPO transform could break this. The code pattern seems fragile.
> 
> If we are serious about implementing glibc fortify, which, to be clear, is a new direction for us, we should think about what IR pattern we really want to see out of clang.
> 
> I think this IR better represents the intention of the original program, and is better for optimizations and other IR analysis consumers:
> ```
> define void @usercode() {
> ...
>   call void @__fortify_memmove(...)
> ...
> }
> define i8* @__fortify_memmove(...) {
> ...
>   call i8* @llvm.memcpy(...)
> ...
>   call i8* @memmove(...)
> ...
> }
> declare i8* @memmove(...)
> ```
> 
> If we're going to have to live with this glibc fortify code pattern, maybe clang should try to spot it and rewrite it into more helpful and analyzable LLVM IR.
> 
> ---
> 
> If people agree that this is a reasonable direction, let's think about how to implement it.
> 
> One way to get there would be to implement asm labels by waiting until after IRGen to do some value replacement. So, clang would emit code for the inline definition of `@memmove`, and then later, it would rename `@memmove_alias` to `@memmove`. At that point, it would invent a new name for the original `@memmove`.  The least effort thing to do would be to rename it to `@memmove.1` using the normal value renaming. We would never see this symbol in the ELF symbol table, because these are usually marked always inline.
I think my concern about having an infinitely recursive function in the IR is still here. I still think the right way to handle this is to rename the fortify wrappers to something else, memmove.1 or something. You'd also have to change the linkage to internal from available_externally.

I think the main case where that doesn't work is when you take the address of a fortify wrapper. In this case, you will end up with the address of the wrapper, and the compiler is forced to emit a standalone copy of the implementation.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:624
+      if (AllUsesAreBuiltin) {
+        F->deleteBody();
+        F->setAttributes({});
----------------
Changing the inliner doesn't seem like the right way to address this.


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

https://reviews.llvm.org/D92657