Warning on redeclaring with a conflicting asm label

[weimingz]

Summary

r255371 errors on redeclaring with a conflicting asm label.
This patch changes errors to warnings to prevent breaking existing codes.

[nicholas]

Weiming Zhao wrote:

weimingz created this revision.
weimingz added a reviewer: nicholas.
weimingz added a subscriber: cfe-commits.

r255371 errors on redeclaring with a conflicting asm label.
This patch changes errors to warnings to prevent breaking existing codes.

Can you include a testcase where the warning is issued but clang and gcc
emit the same .o file?

http://reviews.llvm.org/D16171

Files:

include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaDecl.cpp
test/Sema/asm-label.c

Index: test/Sema/asm-label.c

• test/Sema/asm-label.c

+++ test/Sema/asm-label.c
@@ -7,21 +7,21 @@

void f() {
  g();
}

-void g() asm("gold"); expected-error{{cannot apply asm label to function after its first use}}
+void g() asm("gold"); expected-warning{{cannot apply asm label to function after its first use}}

void h() __asm__("hose");  // expected-note{{previous declaration is here}}

-void h() asm("hair"); expected-error{{conflicting asm label}}
+void h() asm("hair"); expected-warning{{conflicting asm label}}

int x;
int x __asm__("xenon");
int y;

int test() { return y; }

-int y asm("yacht"); expected-error{{cannot apply asm label to variable after its first use}}
+int y asm("yacht"); expected-warning{{cannot apply asm label to variable after its first use}}

int z __asm__("zebra");  // expected-note{{previous declaration is here}}

-int z asm("zooms"); expected-error{{conflicting asm label}}
+int z asm("zooms"); expected-warning{{conflicting asm label}}

// No diagnostics on the following.

Index: lib/Sema/SemaDecl.cpp

• lib/Sema/SemaDecl.cpp

+++ lib/Sema/SemaDecl.cpp
@@ -2385,13 +2385,13 @@

if (AsmLabelAttr *OldA = Old->getAttr<AsmLabelAttr>()) {
  if (OldA->getLabel() != NewA->getLabel()) {
    // This redeclaration changes __asm__ label.

• Diag(New->getLocation(), diag::err_different_asm_label);

+ Diag(New->getLocation(), diag::warn_different_asm_label);

    Diag(OldA->getLocation(), diag::note_previous_declaration);
  }
} else if (Old->isUsed()) {
  // This redeclaration adds an __asm__ label to a declaration that has
  // already been ODR-used.

• Diag(New->getLocation(), diag::err_late_asm_label_name)

+ Diag(New->getLocation(), diag::warn_late_asm_label_name)

      <<  isa<FunctionDecl>(Old)<<  New->getAttr<AsmLabelAttr>()->getRange();
  }
}

Index: include/clang/Basic/DiagnosticSemaKinds.td

• include/clang/Basic/DiagnosticSemaKinds.td

+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -4266,9 +4266,11 @@

def err_conflicting_types : Error<"conflicting types for %0">;
def err_different_pass_object_size_params : Error<
  "conflicting pass_object_size attributes on parameters">;

-def err_late_asm_label_name : Error<

• "cannot apply asm label to %select{variable|function}0 after its first use">;

-def err_different_asm_label : Error<"conflicting asm label">;
+def warn_late_asm_label_name : Warning<
+ "cannot apply asm label to %select{variable|function}0 after its first use">,
+ InGroup<MismatchedTags>;
+def warn_different_asm_label : Warning<"conflicting asm label">,
+ InGroup<MismatchedTags>;

def err_nested_redefinition : Error<"nested redefinition of %0">;
def err_use_with_wrong_tag : Error<
  "use of %0 with tag type that does not match previous declaration">;

[weimingz]

Hi Nick,

Below is a reduced code:
t.c:

static long double acoshl (long double __x) __asm__ ("" "acosh") ;  // this is from <gcc4.9>/arm-linux-gnueabi/libc/usr/include/bits/mathcalls.h
extern long double acoshl (long double) __asm__ ("" "__acoshl_finite") ; // this is from existing code

GCC gives warning like:
/tmp/t.c:2:1: warning: asm declaration ignored due to conflict with previous rename [-Wpragmas]
extern long double acoshl (long double) asm ("" "__acoshl_finite") ;

[weimingz]

if the new decl is not used, we can just give warnings

[cfe-commits]

I agree what you said about different code generated with clang and GCC
generates. In this case, we should throw an error (err_late_asm_label).

But in this example, there is no use of the function. They are just
redundant declarations and there is no actual code generated.
So I suggest we just give warnings for this case. Otherwise, it will
break existing code like some SPEC benchmarks.

Please review my 2nd patch.

Weiming

• msg-21753-560.txt159 BDownload

[phabricss]

It is still impossible to compile glibc with clang due to this bug. Could this patch be reviewed please.

[phabricss]

On 09/12/2016 01:26 PM, Nick Lewycky wrote:

Firstly, I thought glibc had applied a patch to fix this bug? As in, the error is correct and glibc fixed their bug?

I can confirm that the bug still exists in glibc 2.24 and HEAD from glibc git. Also it appears that the issue on the llvm mailing list was just dropped without any resolution:

r255371 - Error on redeclaring with a conflicting asm label

../include/sys/stat.h:16:15: error: cannot apply asm label to function after its first use
hidden_proto (__fxstat)
~~~~~~~~~~~~~~^~~~~~~~~
./../include/libc-symbols.h:420:19: note: expanded from macro 'hidden_proto'
  __hidden_proto (name, , __GI_##name, ##attrs)
  ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./../include/libc-symbols.h:424:33: note: expanded from macro '__hidden_proto'
  extern thread __typeof (name) name __asm__ (__hidden_asmname (#internal)) \

As far as your review of the patch by weimingz, that is beyond my skill level so I will let him reply. Thanks!

[xiangzhai]

LLVMBUG-31017 https://llvm.org/bugs/show_bug.cgi?id=31017#c4

In D16171#540261, @phabricss wrote:

On 09/12/2016 01:26 PM, Nick Lewycky wrote:

Firstly, I thought glibc had applied a patch to fix this bug? As in, the error is correct and glibc fixed their bug?

I can confirm that the bug still exists in glibc 2.24 and HEAD from glibc git. Also it appears that the issue on the llvm mailing list was just dropped without any resolution:

r255371 - Error on redeclaring with a conflicting asm label

../include/sys/stat.h:16:15: error: cannot apply asm label to function after its first use
hidden_proto (__fxstat)
~~~~~~~~~~~~~~^~~~~~~~~
./../include/libc-symbols.h:420:19: note: expanded from macro 'hidden_proto'
  __hidden_proto (name, , __GI_##name, ##attrs)
  ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./../include/libc-symbols.h:424:33: note: expanded from macro '__hidden_proto'
  extern thread __typeof (name) name __asm__ (__hidden_asmname (#internal)) \

As far as your review of the patch by weimingz, that is beyond my skill level so I will let him reply. Thanks!

[rsmith]

In D16171#540261, @phabricss wrote:

../include/sys/stat.h:16:15: error: cannot apply asm label to function after its first use
hidden_proto (__fxstat)
~~~~~~~~~~~~~~^~~~~~~~~
./../include/libc-symbols.h:420:19: note: expanded from macro 'hidden_proto'
  __hidden_proto (name, , __GI_##name, ##attrs)
  ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./../include/libc-symbols.h:424:33: note: expanded from macro '__hidden_proto'
  extern thread __typeof (name) name __asm__ (__hidden_asmname (#internal)) \

This patch does nothing about that error. This patch is dealing with a case where two distinct asm labels are provided for the same declaration, which would simply be a bug in the code. Given that we are not seeing that error any more, and that nicholas suggests that the reason is that the glibc maintainers applied a patch to fix it, this change seems unnecessary and deleterious.

As for the above change, we could theoretically accept such shenanigans, but it would require a much more invasive approach than the one in this patch -- in particular, it would require rewriting any code we've already emitted using the old name, changing it to use the new name instead. And even then, that is fundamentally not possible for some use cases of Clang (where we might have already, say, created machine code and started running it, possibly on a different machine, before we reach this asm label).

I would strongly suggest pushing back hard on the glibc maintainers and suggesting they don't rely on this working.

include/clang/Basic/DiagnosticSemaKinds.td

4275–4276

Move this earlier so that it's adjacent to the error form.

Please also use a different diagnostic group. This makes no sense in -Wmismatched-tags. Please also make this DefaultError; allowing it to be turned off might be OK, but this code pattern is still horribly broken, and we should not accept it by default.

lib/Sema/SemaDecl.cpp

2388

This looks wrong. How could New already be used here, since it's new? Should this say Old instead?

Please also make sure we have test coverage for this. None of the tests below use the declaration before adding the conflicting label.

[rsmith]

Er, please ignore the inline review comments; those predated the realisation that this doesn't actually fix the glibc build problem.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_different_pass_object_size_params : Error<
   "conflicting pass_object_size attributes on parameters">;
 def err_late_asm_label_name : Error<
   "cannot apply asm label to %select{variable|function}0 after its first use">;
 def err_different_asm_label : Error<"conflicting asm label">;
 def err_nested_redefinition : Error<"nested redefinition of %0">;
 def err_use_with_wrong_tag : Error<
   "use of %0 with tag type that does not match previous declaration">;
+def warn_different_asm_label : Warning<"conflicting asm label">,
+    InGroup<MismatchedTags>;
 def warn_struct_class_tag_mismatch : Warning<
     "%select{struct|interface|class}0%select{| template}1 %2 was previously "
     "declared as a %select{struct|interface|class}3%select{| template}1">,
     InGroup<MismatchedTags>, DefaultIgnore;
 def warn_struct_class_previous_tag_mismatch : Warning<
     "%2 defined as %select{a struct|an interface|a class}0%select{| template}1 "
     "here but previously declared as "
     "%select{a struct|an interface|a class}3%select{| template}1">,

lib/Sema/SemaDecl.cpp

void Sema::mergeDeclAttributes(NamedDecl *New, Decl *Old,
 
   // Attributes declared post-definition are currently ignored.
   checkNewAttributesAfterDef(*this, New, Old);
 
   if (AsmLabelAttr *NewA = New->getAttr<AsmLabelAttr>()) {
     if (AsmLabelAttr *OldA = Old->getAttr<AsmLabelAttr>()) {
       if (OldA->getLabel() != NewA->getLabel()) {
         // This redeclaration changes __asm__ label.
+        if (New->isUsed())
           Diag(New->getLocation(), diag::err_different_asm_label);
+        else
+          Diag(New->getLocation(), diag::warn_different_asm_label);
         Diag(OldA->getLocation(), diag::note_previous_declaration);
       }
     } else if (Old->isUsed()) {
       // This redeclaration adds an __asm__ label to a declaration that has
       // already been ODR-used.
       Diag(New->getLocation(), diag::err_late_asm_label_name)
         << isa<FunctionDecl>(Old) << New->getAttr<AsmLabelAttr>()->getRange();
     }

test/Sema/asm-label.c

 // RUN: %clang_cc1 -verify %s
 
 void f();
 void f() __asm__("fish");
 void g();
 
 void f() {
   g();
 }
 void g() __asm__("gold");  // expected-error{{cannot apply asm label to function after its first use}}
 
 void h() __asm__("hose");  // expected-note{{previous declaration is here}}
-void h() __asm__("hair");  // expected-error{{conflicting asm label}}
+void h() __asm__("hair");  // expected-warning{{conflicting asm label}}
 
 int x;
 int x __asm__("xenon");
 int y;
 
 int test() { return y; }
 
 int y __asm__("yacht");  // expected-error{{cannot apply asm label to variable after its first use}}
 
 int z __asm__("zebra");  // expected-note{{previous declaration is here}}
-int z __asm__("zooms");  // expected-error{{conflicting asm label}}
+int z __asm__("zooms");  // expected-warning{{conflicting asm label}}
 
 
 // No diagnostics on the following.
 void __real_readlink() __asm("readlink");
 void readlink() __asm("__protected_readlink");
 void readlink() { __real_readlink(); }