[cfe-commits] [patch] Alternative fix for pr9614

[Eli Friedman]

2011/10/26 Rafael Ávila de Espíndola <rafael.espindola at gmail.com>:
> This patch implements something on the line of Eli's comment:
>
>>Oh, also, if you want to get something into 3.0 quickly, just hacking
>>clang so it doesn't emit the definition at all in situations like
>>PR9614 would be more obviously acceptable.
>
> I now think that this is the correct fix given how bad the code snippet is.
> What the patch does is try to detect cases where the external functions
> "cannot" be equivalent to available_externally one we are about to emit.

+  // PR9614. Avoid cases where the source code is lying to us. An available
+  // externally function should have an equivalent function somewhere else,
+  // but a function that calls itself is clearly not equivalent to the real
+  // implementation.
+  if (isTriviallyRecursiveViaAsm(F))
+    return false;
+  if (F->hasAttr<AlwaysInlineAttr>())
+    return true;
+  if (CodeGenOpts.OptimizationLevel == 0)
+    return false;
+  return true;

Please keep the CodeGenOpts.OptimizationLevel test first, and put the
isTriviallyRecursiveViaAsm test at the end.

You might want to more explicitly note that this is working around a
trick used in the glibc headers.

+++ b/test/CodeGen/pr9614.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+extern int foo_alias (void) __asm ("foo");
+inline int foo (void) {
+  return foo_alias ();
+}
+int f(void) {
+  return foo();
+}
+
+// CHECK: define i32 @f()
+// CHECK: %call = call i32 @foo()
+// CHECK: ret i32 %call

This test doesn't actually seem to check that we don't emit an
available_externally definition for foo.

Besides that, it seems okay to me.

-Eli