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 <nlewy...@google.com> wrote: > On 11 December 2015 at 12:57, Richard Smith <rich...@metafoo.co.uk> wrote: > >> On Fri, Dec 11, 2015 at 12:43 PM, Gao, Yunzhong via cfe-commits < >> cfe-commits@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:nlewy...@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_...@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-boun...@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@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