PR5172: Fix for a bug in pragma redefine_extname implementation.

[andreybokhanko]

Summary

PR5172: Fix for a bug in pragma redefine_extname implementation: it doesn't work correctly when a structure is declared before pragma and then a function with the same name declared after pragma.

[andreybokhanko]

Aaron, thank you for the review. I did as you suggested; patch updated.

[andreybokhanko]

Patch updated again (the only difference is that a pair of curly braces went away).

David [Chisnall], Aaron wrote (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150601/130212.html) that:

"Changes look good... However, since I don't know about the semantics of this particular pragma, I'll let someone else give the final LGTM."

You are the original author of this pragma support; could you, please, LGTM my fix?

Andrey

[theraven]

I'm also not completely sure of the semantics - I just made it work well enough for the Solaris headers that libc++ needed to work. The changes look fine to me though.

Have they been tested with the various things in compiler-rt that use this pragma? We currently use it as a hack because clang doesn't allow you to implement functions that have the same name as builtins (even when those builtins are ones that need implementing by a function of the same name on some archs).

[andreybokhanko]

In D10187#183409, @theraven wrote:

I'm also not completely sure of the semantics - I just made it work well enough for the Solaris headers that libc++ needed to work. The changes look fine to me though.

The semantic is described here: http://docs.oracle.com/cd/E19205-01/819-5265/bjacu/index.html

IMHO, clang implements it right; the only thing I did is fixed a corner case.

Have they been tested with the various things in compiler-rt that use this pragma? We currently use it as a hack because clang doesn't allow you to implement functions that have the same name as builtins (even when those builtins are ones that need implementing by a function of the same name on some archs).

I did "make check-all" + ran a few internal Intel tests.

What is the best way to test compiler-rt with my changes?

Andrey

[andreybokhanko]

David, I checked out compiler-rt and ran "make check-all". No new fails (several AddressSanitizer tests fail, but they do so both with and without my changes).

Is this the testing you wanted?

Andrey

[theraven]

Should be fine then, though I'm not sure if the code for the C11 atomic helpers is compiled by default (it needs to go in a shared lib to work).

[andreybokhanko]

In D10187#183593, @theraven wrote:

Should be fine then, though I'm not sure if the code for the C11 atomic helpers is compiled by default (it needs to go in a shared lib to work).

OK, thanks!

Is it OK to commit this or should I wait for explicit "LGTM" from someone?

Andrey

[rsmith]

I think the right way to handle this would be to filter out declarations that are neither FunctionDecls nor VarDecls after performing the lookup. (According to the specification, we should also filter out declarations with internal linkage.) We should also filter out internal-linkage declarations and non-TU-scope declarations when we perform the deferred handling of ExtnameUndeclaredIdentifiers in ActOnVariableDeclarator and ActOnFunctionDeclarator.

That said, I'm not sure why your testcase was failing -- LookupSingleName in LookupOrdinaryName mode in C should not find tag names, so we should have deferred adding the attribute already. Is a typedef missing from the testcase, perhaps?

[andreybokhanko]

Patch updated in response to Richard Smith's comments.

[andreybokhanko]

In D10187#184399, @rsmith wrote:

I think the right way to handle this would be to filter out declarations that are neither FunctionDecls nor VarDecls after performing the lookup. (According to the specification, we should also filter out declarations with internal linkage.)

OK -- I did as you suggested. Please review updated patch.

We should also filter out internal-linkage declarations and non-TU-scope declarations when we perform the deferred handling of ExtnameUndeclaredIdentifiers in ActOnVariableDeclarator and ActOnFunctionDeclarator.

This probably belongs to a separate patch (for starters, I don't have a test demonstrating that compiler is currently acting incorrectly).

That said, I'm not sure why your testcase was failing -- LookupSingleName in LookupOrdinaryName mode in C should not find tag names, so we should have deferred adding the attribute already. Is a typedef missing from the testcase, perhaps?

You mean modifying it like this:

typedef struct {

int f;

} statvfs64;

?

Then compiler issue an error: "redefine_extname.c:10:5: error: redefinition of 'statvfs64' as different kind of symbol". No errors are issued in the original test.

[andreybokhanko]

Patch updated according to Richard Smith's comments (from http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150608/130447.html).

[rsmith]

Ah, the new test makes perfect sense. Thanks!

test/CodeGen/redefine_extname.cpp

1 ↗

(On Diff #27378)

This test should be in test/CodeGenCXX now.

[andreybokhanko]

Richard, thank you for the review!

I moved redefine_extname.cpp to test/CodeGenCXX/.

I don't have commit access; Alexander [Musman], could you, please, commit this patch?

lib/Sema/SemaDecl.cpp

void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation Loc,
   VisibleModules.setVisible(Mod, Loc);
 }
 
 void Sema::ActOnPragmaRedefineExtname(IdentifierInfo* Name,
                                       IdentifierInfo* AliasName,
                                       SourceLocation PragmaLoc,
                                       SourceLocation NameLoc,
                                       SourceLocation AliasNameLoc) {
-  Decl *PrevDecl = LookupSingleName(TUScope, Name, NameLoc,
+  NamedDecl *PrevDecl = LookupSingleName(TUScope, Name, NameLoc,
                                          LookupOrdinaryName);
-  AsmLabelAttr *Attr = ::new (Context) AsmLabelAttr(AliasNameLoc, Context,
-                                                    AliasName->getName(), 0);
+  AsmLabelAttr *Attr =
+      AsmLabelAttr::CreateImplicit(Context, AliasName->getName(), AliasNameLoc);
 
-  if (PrevDecl) 
+  // If a declaration that:
+  // 1) declares a function or a variable
+  // 2) has external linkage
+  // already exists, add a label attribute to it.
+  if (PrevDecl &&
+      (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl)) &&
+      PrevDecl->hasExternalFormalLinkage())
     PrevDecl->addAttr(Attr);
-  else 
-    (void)ExtnameUndeclaredIdentifiers.insert(
-      std::pair<IdentifierInfo*,AsmLabelAttr*>(Name, Attr));
+  // Otherwise, add a label atttibute to ExtnameUndeclaredIdentifiers.
+  else
+    (void)ExtnameUndeclaredIdentifiers.insert(std::make_pair(Name, Attr));
 }
 
 void Sema::ActOnPragmaWeakID(IdentifierInfo* Name,
                              SourceLocation PragmaLoc,
                              SourceLocation NameLoc) {
   Decl *PrevDecl = LookupSingleName(TUScope, Name, NameLoc, LookupOrdinaryName);
 
   if (PrevDecl) {

test/CodeGenCXX/redefine_extname.cpp

+// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -w -emit-llvm %s -o - | FileCheck %s
+
+extern "C" {
+  struct statvfs64 {
+    int f;
+  };
+#pragma redefine_extname statvfs64 statvfs
+  int statvfs64(struct statvfs64 *);
+}
+
+void foo() {
+  struct statvfs64 st;
+  statvfs64(&st);
+// Check that even if there is a structure with redefined name before the
+// pragma, subsequent function name redefined properly. PR5172, Comment 11.
+// CHECK:  call i32 @statvfs(%struct.statvfs64* %st)
+}
+