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

[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<MismatchedTags>;
 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