[PATCH] D16171: Warning on redeclaring with a conflicting asm label

2017-05-09 Thread Richard Smith via Phabricator via cfe-commits 
rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/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.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4275-4276
   "use of %0 with tag type that does not match previous declaration">;
+def warn_different_asm_label : Warning<"conflicting asm label">,
+InGroup;
 def warn_struct_class_tag_mismatch : Warning<

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.



Comment at: lib/Sema/SemaDecl.cpp:2388
 // This redeclaration changes __asm__ label.
-Diag(New->getLocation(), diag::err_different_asm_label);
+if (New->isUsed())
+  Diag(New->getLocation(), diag::err_different_asm_label);

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.


https://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16171: Warning on redeclaring with a conflicting asm label

2017-05-09 Thread Richard Smith via Phabricator via cfe-commits 
rsmith added a comment.

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


https://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-12-18 Thread Leslie Zhai via Phabricator via cfe-commits 
xiangzhai added a comment.

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

In https://reviews.llvm.org/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!





https://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-09-12 Thread Zhao, Weiming via cfe-commits 

Sorry, I was distracted by other issues after I uploaded the patch.

I will take another look of the implementation.

Thanks,

Weiming


On 9/12/2016 1:31 PM, Sam Shepperd wrote:

phabricss added a comment.

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!


https://reviews.llvm.org/D16171





--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by 
The Linux Foundation

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-09-12 Thread Sam Shepperd via cfe-commits 
phabricss added a comment.

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!


https://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-09-12 Thread Nick Lewycky via cfe-commits 
On 11 September 2016 at 21:14, Sam Shepperd via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> phabricss added a subscriber: phabricss.
> phabricss added a comment.
>
> It is still impossible to compile glibc with clang due to this bug.  Could
> this patch be reviewed please.
>

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

As for the patch, its goal is to only demote an error to warning when the
function with conflicting asm labels is never used. That's pretty clearly a
workaround (if you ever call acoshl, then you'll get an error), but
moreover the check is implemented incorrectly. "New->isUsed()" will be
performed at the moment the new declaration is being parsed; of course it
hasn't been used yet. isUsed() will turn true only after it's used, after
we actually parse the rest of the code and build AST for the uses of the
new declaration. As I suggested in my last review comment, you would need
to defer the check until ActOnEndOfTranslationUnit in order to correctly
answer New->isUsed().

Or just not try to implement this workaround, and instead change how
codegen works so that we emit calls to functions with the same asm label
that gcc would choose.

https://reviews.llvm.org/D16171
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-09-12 Thread Sam Shepperd via cfe-commits 
phabricss added a subscriber: phabricss.
phabricss added a comment.

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


https://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Nick Lewycky via cfe-commits 

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()) {
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(Old)<<  
New->getAttr()->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;
+def warn_different_asm_label : Warning<"conflicting asm label">,
+  InGroup;
  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">;




___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Nick Lewycky via cfe-commits 
nicholas added a subscriber: nicholas.
nicholas added a comment.

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()) { 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(Old)<<  New->getAttr()->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; +def warn_different_asm_label : Warning<"conflicting 
> asm label">, +  InGroup; 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">;



http://reviews.llvm.org/D16171



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Joerg Sonnenberger via cfe-commits 
On Thu, Jan 14, 2016 at 02:05:21AM +, Weiming Zhao via cfe-commits wrote:
> r255371 errors on redeclaring with a conflicting asm label.
> This patch changes errors to warnings to prevent breaking existing codes.

I'm not sure I agree with this. We have triggered this in NetBSD in two
different situations and both are bugs. What example do you have where
the result was "as intended"?

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Nick Lewycky via cfe-commits 
On 14 January 2016 at 10:38, Weiming Zhao via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> weimingz added a comment.
>
> Hi Nick,
>
> Below is a reduced code:
> t.c:
>
>   static long double acoshl (long double __x) __asm__ ("" "acosh") ;  //
> this is from /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") ;
>

That's the same case as in this testcase:

  void foo() __asm__("one");
  void foo() __asm__("two");
  void test() { foo(); }

GCC emits a call to 'one' while Clang emits a call to 'two'. This is a real
bug. Please don't downgrade this to a warning.

As an alternative, I would accept a patch which changes how clang generates
code so that it also produces a call to 'one', with a warning. It looks
like what we need to do is drop subsequent asm label declarations on
functions that already have one.

Nick
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Weiming Zhao via cfe-commits 
weimingz updated this revision to Diff 44931.
weimingz added a comment.

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


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
@@ -10,7 +10,7 @@
 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");
@@ -21,7 +21,7 @@
 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.
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -2385,7 +2385,10 @@
 if (AsmLabelAttr *OldA = Old->getAttr()) {
   if (OldA->getLabel() != NewA->getLabel()) {
 // This redeclaration changes __asm__ label.
-Diag(New->getLocation(), diag::err_different_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()) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -4272,6 +4272,8 @@
 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;
 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">,


Index: test/Sema/asm-label.c
===
--- test/Sema/asm-label.c
+++ test/Sema/asm-label.c
@@ -10,7 +10,7 @@
 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");
@@ -21,7 +21,7 @@
 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.
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -2385,7 +2385,10 @@
 if (AsmLabelAttr *OldA = Old->getAttr()) {
   if (OldA->getLabel() != NewA->getLabel()) {
 // This redeclaration changes __asm__ label.
-Diag(New->getLocation(), diag::err_different_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()) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -4272,6 +4272,8 @@
 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;
 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">,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Zhao, Weiming via 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

On 1/14/2016 2:28 PM, Nick Lewycky wrote:
On 14 January 2016 at 10:38, Weiming Zhao via cfe-commits 
> wrote:


weimingz added a comment.

Hi Nick,

Below is a reduced code:
t.c:

  static long double acoshl (long double __x) __asm__ ("" "acosh")
;  // this is from
/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") ;


That's the same case as in this testcase:

  void foo() __asm__("one");
  void foo() __asm__("two");
  void test() { foo(); }

GCC emits a call to 'one' while Clang emits a call to 'two'. This is a 
real bug. Please don't downgrade this to a warning.


As an alternative, I would accept a patch which changes how clang 
generates code so that it also produces a call to 'one', with a 
warning. It looks like what we need to do is drop subsequent asm label 
declarations on functions that already have one.


Nick


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Nick Lewycky via cfe-commits 
On 14 January 2016 at 15:05, Zhao, Weiming  wrote:

> 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.
>

I think your second patch checks whether it's used at the time of the
redeclaration, which is too early. It may be used between there and the end
of the program. I expect it not to warn but not error on the testcase I
posted in my previous email?

To fix, you'd need to store a list of different-asm-label declarations in
Sema, check it each time something is ODR-used (see the
Sema::Mark<...>Referenced family of calls) to emit the error and remove it
from the list. Also, when emitting a PCH or a Module, you need to serialize
and deserialize this list.

Nick

Weiming
>
>
> On 1/14/2016 2:28 PM, Nick Lewycky wrote:
>
> On 14 January 2016 at 10:38, Weiming Zhao via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> weimingz added a comment.
>>
>> Hi Nick,
>>
>> Below is a reduced code:
>> t.c:
>>
>>   static long double acoshl (long double __x) __asm__ ("" "acosh") ;  //
>> this is from /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") ;
>>
>
> That's the same case as in this testcase:
>
>   void foo() __asm__("one");
>   void foo() __asm__("two");
>   void test() { foo(); }
>
> GCC emits a call to 'one' while Clang emits a call to 'two'. This is a
> real bug. Please don't downgrade this to a warning.
>
> As an alternative, I would accept a patch which changes how clang
> generates code so that it also produces a call to 'one', with a warning. It
> looks like what we need to do is drop subsequent asm label declarations on
> functions that already have one.
>
> Nick
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-14 Thread Joerg Sonnenberger via cfe-commits 
On Thu, Jan 14, 2016 at 03:05:26PM -0800, Zhao, Weiming via cfe-commits wrote:
> 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.

By the same argument, redefinitions with different types should be valid
in C. I don't like this slippery slope at all, code should really be
just fixed here.

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16171: Warning on redeclaring with a conflicting asm label

2016-01-13 Thread Weiming Zhao via cfe-commits 
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.

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()) {
   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(Old) << New->getAttr()->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;
+def warn_different_asm_label : Warning<"conflicting asm label">,
+  InGroup;
 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">;


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()) {
   if (OldA->getLabel() != NewA->getLabel()) {
 // This redeclaration changes __asm__ label.
-Diag(New->getLocation(), diag::err_different_asm_label);
+Diag(New->getLocation(),