PATCH: error on code that redeclares with an __asm__ label after the first ODR use

[Richard Smith via cfe-commits]

LGTM

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

Comment is underindented by one space.

On Fri, Dec 11, 2015 at 1:17 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 11 December 2015 at 12:57, Richard Smith <richard at metafoo.co.uk> wrote:
>
>> On Fri, Dec 11, 2015 at 12:43 PM, Gao, Yunzhong via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> gcc 4.8.2 accepts the following case silently without error or warning:
>>>
>>>   void f();
>>>
>>>   void g() __asm__(“real_g”);
>>>
>>>   void f() { g(); }  // gcc emits a call to real_g() here
>>>
>>>   void real_g() __asm__(“gold”);
>>>
>>> void real_g() { } // gcc generates a body for gold() here
>>>
>>>
>>>
>>> gcc 4.8.2 generates a warning for the following test case:
>>>
>>> void g() __asm__(“func1”);
>>>
>>> void g() __asm__(“func2”); // gcc generates a warning and ignores this
>>> asm label
>>>
>>> void g() { }
>>>
>>>
>>>
>>> Comparing to gcc behavior, I think it is better to emit error in both
>>> cases.
>>>
>>
>> Why? I don't see anything wrong with the first example. We have a
>> function whose source-level name is "g()" and whose asm name is "real_g",
>> and we have a function whose source-level name is "real_g()" and whose asm
>> name is "gold". What's wrong with that?
>>
>
> Hah, I completely misread the example and fixed something else. What I
> added an error for is:
>
>   void f() __asm__("something");
>   void f() __asm__("somethingelse");
>
> With my patch, we don't issue an error on the case Yunzhong actually
> supplied. Joerg has pointed out over IRC that this is a really useful
> construct and that NetBSD's stack smashing protection uses it. We shouldn't
> break that. I've added a testcase to make sure it continues working. Please
> review!
>
> Nick
>
> Looks good to me. Thanks!
>>>
>>> - Gao
>>>
>>>
>>>
>>>
>>>
>>> *From:* Nick Lewycky [mailto:nlewycky at google.com]
>>> *Sent:* Friday, December 11, 2015 12:44 AM
>>> *To:* Gao, Yunzhong
>>> *Cc:* Clang Commits
>>> *Subject:* Re: PATCH: error on code that redeclares with an __asm__
>>> label after the first ODR use
>>>
>>>
>>>
>>> On 10 December 2015 at 17:42, Gao, Yunzhong <
>>> yunzhong_gao at playstation.sony.com> wrote:
>>>
>>> Out of curiosity, is the following test case possible too?
>>>
>>>
>>>
>>> void f();
>>>
>>> void g() __asm__(“real_g”); // rename g into real_g.
>>>
>>>
>>>
>>> void f() {
>>>
>>>   g(); // this would actually be calling real_g()
>>>
>>> }
>>>
>>> void real_g() __asm__("gold");  // re-declaring real_g() into gold <--
>>> should this be an error too?
>>>
>>>
>>>
>>> I can't see any reason why not. Both clang and gcc will emit "real_g"
>>> here instead of "gold", but changing the asm label in a program is highly
>>> dubious. I added an error for this too.
>>>
>>>
>>>
>>> Thanks!
>>>
>>>
>>>
>>> *From:* cfe-commits [mailto:cfe-commits-bounces at lists.llvm.org] *On
>>> Behalf Of *Nick Lewycky via cfe-commits
>>> *Sent:* Thursday, December 10, 2015 3:15 PM
>>> *To:* Clang Commits
>>> *Subject:* PATCH: error on code that redeclares with an __asm__ label
>>> after the first ODR use
>>>
>>>
>>>
>>> PR22830 shows a case where some code was adding an __asm__ label after
>>> the first use of a function. GCC and Clang diverged on this code, GCC
>>> emitting the name in the last __asm__ label for all uses while clang would
>>> switch in the middle of the TU as the redeclaration was parsed. The
>>> attached patch detects this case and issues an error on such a
>>> redeclaration. If this breaks real code, we can turn it down to a warning.
>>>
>>>
>>>
>>> Please review!
>>>
>>>
>>>
>>> Nick
>>>
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151211/90e4a8ed/attachment.html>