Page MenuHomePhabricator

Warning on redeclaring with a conflicting asm label
Needs RevisionPublic

Authored by weimingz on Jan 13 2016, 6:05 PM.

Details

Reviewers
nicholas
rsmith
Summary

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

Diff Detail

Event Timeline

weimingz updated this revision to Diff 44821.Jan 13 2016, 6:05 PM
weimingz retitled this revision from to Warning on redeclaring with a conflicting asm label.
weimingz updated this object.
weimingz added a reviewer: nicholas.
weimingz added a subscriber: cfe-commits.
nicholas edited edge metadata.Jan 14 2016, 3:34 AM
nicholas added a subscriber: 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">;

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 updated this revision to Diff 44931.Jan 14 2016, 3:03 PM
weimingz edited edge metadata.

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

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

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

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!

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

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 requested changes to this revision.May 9 2017, 3:05 PM
rsmith added a subscriber: rsmith.
../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.

This revision now requires changes to proceed.May 9 2017, 3:05 PM
rsmith added a comment.May 9 2017, 3:06 PM

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