[clang] Incorrect IR involving the use of bcopy

[vojkan99]

This patch addresses the issue regarding the call of bcopy function in a conditional expression.
It is analogous to the already accepted patch which deals with the same problem, just regarding the bzero function [0].

Here is the testcase which illustrates the issue:

void bcopy(const void *, void *, unsigned long);
void foo(void);

void test_bcopy() {
  char dst[20];
  char src[20];
  int _sz = 20, len = 20;
  return (_sz
          ? ((_sz >= len)
             ? bcopy(src, dst, len)
             : foo())
          : bcopy(src, dst, len));
}

When processing it with clang, following issue occurs:

Instruction does not dominate all uses!
%arraydecay2 = getelementptr inbounds [20 x i8], ptr %dst, i64 0, i64 0, !dbg !38
%cond = phi ptr [ %arraydecay2, %cond.end ], [ %arraydecay5, %cond.false3 ], !dbg !33
fatal error: error in backend: Broken module found, compilation aborted!

This happens because an incorrect phi node is created. It is created because bcopy function call is lowered to the call of llvm.memmove intrinsic and function memmove returns void *. Since llvm.memmove is called in two places in the same return statement, clang creates a phi node in the final basic block for the return value and that phi node is incorrect. However, bcopy function should return void in the first place, so this phi node is unnecessary. This is what this patch addresses. An appropriate test is also added and no existing tests fail when applying this patch.

Also, this crash only happens when LLVM is configured with -DLLVM_ENABLE_ASSERTIONS=On option.

[0] https://reviews.llvm.org/D39746

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (vojkan99)

Changes

This patch addresses the issue regarding the call of bcopy function in a conditional expression.
It is analogous to the already accepted patch which deals with the same problem, just regarding the bzero function [0].

Here is the testcase which illustrates the issue:

void bcopy(const void *, void *, unsigned long);
void foo(void);

void test_bcopy() {
  char dst[20];
  char src[20];
  int _sz = 20, len = 20;
  return (_sz
          ? ((_sz >= len)
             ? bcopy(src, dst, len)
             : foo())
          : bcopy(src, dst, len));
}

When processing it with clang, following issue occurs:

Instruction does not dominate all uses!
%arraydecay2 = getelementptr inbounds [20 x i8], ptr %dst, i64 0, i64 0, !dbg !38
%cond = phi ptr [ %arraydecay2, %cond.end ], [ %arraydecay5, %cond.false3 ], !dbg !33
fatal error: error in backend: Broken module found, compilation aborted!

This happens because an incorrect phi node is created. It is created because bcopy function call is lowered to the call of llvm.memmove intrinsic and function memmove returns void *. Since llvm.memmove is called in two places in the same return statement, clang creates a phi node in the final basic block for the return value and that phi node is incorrect. However, bcopy function should return void in the first place, so this phi node is unnecessary. This is what this patch addresses. An appropriate test is also added and no existing tests fail when applying this patch.

Also, this crash only happens when LLVM is configured with -DLLVM_ENABLE_ASSERTIONS=On option.

[0] https://reviews.llvm.org/D39746

---

Full diff: https://github.com/llvm/llvm-project/pull/79298.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGBuiltin.cpp (+1-1)
• (modified) clang/test/CodeGen/builtins.c (+15)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 7ef764b8e1ac80e..bfe2e45d545322f 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -4011,7 +4011,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
     EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(1)->getType(),
                         E->getArg(1)->getExprLoc(), FD, 0);
     Builder.CreateMemMove(Dest, Src, SizeVal, false);
-    return RValue::get(Dest.getPointer());
+    return RValue::get(nullptr);
   }
 
   case Builtin::BImemcpy:
diff --git a/clang/test/CodeGen/builtins.c b/clang/test/CodeGen/builtins.c
index ce1182b724dcc21..ed03233b6f1a967 100644
--- a/clang/test/CodeGen/builtins.c
+++ b/clang/test/CodeGen/builtins.c
@@ -202,6 +202,21 @@ void test_conditional_bzero(void) {
   // CHECK-NOT: phi
 }
 
+// CHECK-LABEL: define{{.*}} void @test_conditional_bcopy
+void test_conditional_bcopy(void) {
+  char dst[20];
+  char src[20];
+  int _sz = 20, len = 20;
+  return (_sz
+          ? ((_sz >= len)
+              ? __builtin_bcopy(src, dst, len)
+              : foo())
+          : __builtin_bcopy(src, dst, len));
+  // CHECK: call void @llvm.memmove
+  // CHECK: call void @llvm.memmove
+  // CHECK-NOT: phi
+}
+
 // CHECK-LABEL: define{{.*}} void @test_float_builtins
 void test_float_builtins(__fp16 *H, float F, double D, long double LD) {
   volatile int res;

[efriedma-quic]

LGTM