From GCC 3.4 to 8.2, GCC would accept asm volatile(""); at file scope in C++ mode, though not in C mode. This no longer works in GCC 8.3, in either mode. It now results in test.cc:1:5: error: expected ‘(’ before ‘volatile’ asm volatile(""); ^~~~~~~~ ( test.cc:1:14: error: expected unqualified-id before string constant asm volatile(""); ^~ test.cc:1:14: error: expected ‘)’ before string constant asm volatile(""); ~^~ ) This may have been useless (as covered by -Wasm-ignored-qualifier), but making it a hard error breaks the compilation of existing projects after upgrading from GCC 8.2 to 8.3.
(Sorry, to be clear, the comment about -Wasm-ignored-qualifier is about a warning clang emits for this construct, not a GCC warning.)
This was intentionally(!) broken in a backport, https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=267534, which specifically adds a test that this results not even in a user-friendly error message, but a bad syntax error, even though it had previously been accepted.
dup of bug 89443 *** This bug has been marked as a duplicate of bug 89443 ***
Re-opening. Marking this as invalid for GCC 9 is fine. Breaking existing code without so much as a warning in a minor release is not.
According to <https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01698.html>, the GCC 8 backport was not supposed to break existing code, it was supposed to warn about code that would become invalid in GCC 9. It seems like this case was missed.
*** Bug 89589 has been marked as a duplicate of this bug. ***
Since it's now rejected with 8.3 there's no point in accepting it again (IMHO). But the diagnostic could be improved.
(In reply to Richard Biener from comment #7) > Since it's now rejected with 8.3 there's no point in accepting it again > (IMHO). The point in accepting it again would be that people can skip 8.3 because of this and upgrade from 8.2 to a future 8.4 without worrying too much about existing code being broken. The alternative is that people who have to deal with the possibility of old code are forced treat 8.2 as the last release in the 8 series, and to treat 8.3 and newer as if they were a new major version.
I actually agree it would be better to accept it for the rest of 8.x lifetime. Yes, it doesn't make any sense in the code and people will eventually fix it for 9.x.
1) It wasn't defined behaviour before, so nothing changed in that regard. Many changes changes behaviour on invalid code. Not all invalid code gets a diagnostic. 2) Incompatible clang behaviour on GNU C extensions is not GCC's fault. 3) I've heard of only one program where this change showed this problem (Qt), and it has been fixed there long ago. So silly me thought that was the end of it. And then my backport was approved, too. 4) Yeah, a more specific diagnostic might be nice (not all qualifiers are asm qualifiers). This should go to trunk first then, of course.
(In reply to Segher Boessenkool from comment #10) > 1) It wasn't defined behaviour before, so nothing changed in that regard. > Many changes changes behaviour on invalid code. Not all invalid code gets > a diagnostic. That doesn't mean it's acceptable to change how that's handled in a minor release. The whole concept of a release series doesn't make sense if you take that route. Take a look at how signed integer overflow was handled, for instance. We all know that's invalid and has always been invalid, but the change to enable -fstrict-overflow at -O2 was saved for the next major release and clearly documented in the GCC 4.2 Changes page <https://gcc.gnu.org/gcc-4.2/changes.html>. A release series should mean that it is safe for distros to upgrade within the release series. Right now, it's not. > 2) Incompatible clang behaviour on GNU C extensions is not GCC's fault. There was no incompatible clang behaviour, so this comment does not make sense. clang accepted the code with a warning where GCC accepted it without a warning, but it behaved the same in both compilers, until GCC 8.3. > 3) I've heard of only one program where this change showed this problem (Qt), > and it has been fixed there long ago. So silly me thought that was the end > of it. Long ago? It definitely breaks last years' releases, though I have not tested with the most recent versions. Even if it didn't, distros are likely to be stuck with supporting Qt 4.x for a while. When they bump to GCC 9, they know they have to deal with breakage and patch code, but when they bump to GCC 8.3, this is a nasty surprise. > And then my backport was approved, too. Your backport was approved after a highly misleading message on the mailing list. You wrote: > I'd like to backport the "asm inline" series to 8 branch and 7 branch. > The patches are identical to trunk, except I added a patch 8/8 that > makes these branches not error on code it only warned on before (that > is, C code that uses restrict or const as asm qualifier). Technically, this is 100% true. What's missing from this is that it *does* error on code it previously accepted *without* a warning. Whether it would have been approved if you had included that bit in your e-mail is something we'll never know. > 4) Yeah, a more specific diagnostic might be nice (not all qualifiers are > asm qualifiers). This should go to trunk first then, of course. Given that any changes to re-accept the code would be limited to GCC 8 only, that probably shouldn't go on trunk. But if a change on trunk to give a decent diagnostic can be trivially tweaked to allow the code on GCC 8 again, with a warning, then great! I'll be happy to help test anything that can be backported to GCC 8 to allow this again.
(In reply to Harald van Dijk from comment #11) > > That doesn't mean it's acceptable to change how that's handled in a minor > release. The whole concept of a release series doesn't make sense if you > take that route. Take a look at how signed integer overflow was handled, for > instance. We all know that's invalid and has always been invalid, but the > change to enable -fstrict-overflow at -O2 was saved for the next major > release and clearly documented in the GCC 4.2 Changes page > <https://gcc.gnu.org/gcc-4.2/changes.html>. > > A release series should mean that it is safe for distros to upgrade within > the release series. Right now, it's not. This is wrong too since some of what -fstrict-overflow did was enabled at -O1 (and -O2) before hand; just the option was added for GCC 4.2.0 and only enabled at -O2 and above.
(In reply to Andrew Pinski from comment #12) > This is wrong too since some of what -fstrict-overflow did was enabled at > -O1 (and -O2) before hand; just the option was added for GCC 4.2.0 and only > enabled at -O2 and above. Thanks, I stand corrected. The basic idea should still hold though, even if I picked a bad example. FWIW, I checked the current release of Qt 5, and just grepping the sources, I still see file scope asm volatile in there.
See https://bugreports.qt.io/browse/QTBUG-74196 That's how I found this at all: the latest current releases don't build together. And I guess that most projects that somehow are descendants of KJS will be affected, as seen here: https://bugs.gentoo.org/679330 The other thing is: given that 8.3 does not show a diagnostic message that is at least remotely helpful this looks for any end user just like a compiler bug. I hope gcc 9 will say what it doesn't like, and maybe even suggest a fixit. For 8.x I would very much welcome that it just works as before, maybe with a clear error message.
(In reply to Rolf Eike Beer from comment #14) > The other thing is: given that 8.3 does not show a diagnostic message that > is at least remotely helpful this looks for any end user just like a > compiler bug. Don't exaggerate please. It gives exactly the same error as when you would say "asm double" or similar. It is perfectly clear: the "volatile" there was not expected, it is a programming mistake, it has no meaning. How do you expect "end users" who have no idea what they are doing to compile anything ever?
(In reply to Segher Boessenkool from comment #15) Please stop with the unnecessary and unhelpful insults. This is the second time I've had to ask you that.
Hello Harald, Yes, that is what I would like you guys to do. But I'll stop working on this now, that is fine as well.
I would have expected something that is more like the error message in this case: class foo { static void bar() const; }; error: static member function ‘static void foo::bar()’ cannot have cv-qualifier
Created attachment 45903 [details] gcc9-pr89585.patch Untested patch for better diagnostics of this in GCC 9, for GCC 8 we could just not diagnose volatile and ignore it. Now, looking at documentation, I'd say that the C parser didn't match the documentation and the C++ parser did (and no longer does), because we said that volatile is optional on Basic Asm, volatile and goto are allowed on Extended Asm, that at toplevel only Basic Asm is allowed and for Basic Asm: "The optional volatile qualifier has no effect. All basic asm blocks are implicitly volatile." To me that looks like a strong reason to at least accept it again in 8.x, it wasn't any kind of unspecified behavior, it was documented to be ignored. And perhaps we could reconsider this even for GCC 9 and allow and ignore even in C at toplevel.
(In reply to Jakub Jelinek from comment #19) > Created attachment 45903 [details] > gcc9-pr89585.patch Thanks. I'll test this on GCC 8, with trivial changes to accept asm volatile, when I get back home.
Created attachment 45904 [details] gcc8-pr89585.patch The 8.x version would be like this.
I can confirm that with gcc 8.3 with the gcc8-patch Qt 5.12.1 builds fine.
Created attachment 45909 [details] gcc9-pr89585.patch Updated trunk patch based on IRC discussions with Segher.
Created attachment 45910 [details] gcc-8-pr89585-warn.patch (In reply to Jakub Jelinek from comment #21) > Created attachment 45904 [details] > gcc8-pr89585.patch > > The 8.x version would be like this. Depending on whether this will remain an error in GCC 9, would it make sense to have a warning for file scope asm volatile, like so? (Minimally tested so far. If it makes sense, I can re-do it based on the updated GCC 9 patch.)
Created attachment 45912 [details] gcc-8-pr89585-warn-v2.patch Re-done based on the GCC 9 patch, modified to issue a warning for asm volatile on GCC 8. Since Qt 5 has already been tested, I looked at Qt 4.8.7 and verified that bitwise identical code is generated for what GCC 8.3 produces when the volatile keyword is removed, and that the warning is issued when the volatile keyword is left in: ../3rdparty/javascriptcore/JavaScriptCore/jit/JITStubs.cpp:483:5: warning: asm qualifier outside of function body asm volatile ( ^~~~~~~~ ../3rdparty/javascriptcore/JavaScriptCore/jit/JITStubs.cpp:518:5: warning: asm qualifier outside of function body asm volatile ( ^~~~~~~~ ../3rdparty/javascriptcore/JavaScriptCore/jit/JITStubs.cpp:534:5: warning: asm qualifier outside of function body asm volatile ( ^~~~~~~~
Author: jakub Date: Thu Mar 7 08:18:36 2019 New Revision: 269451 URL: https://gcc.gnu.org/viewcvs?rev=269451&root=gcc&view=rev Log: PR c++/89585 * doc/extend.texi (Basic Asm): Document qualifiers are not allowed at toplevel. * parser.c (cp_parser_asm_definition): Parse asm qualifiers even at toplevel, but diagnose them. * g++.dg/asm-qual-3.C: Adjust expected diagnostics. Modified: trunk/gcc/ChangeLog trunk/gcc/cp/ChangeLog trunk/gcc/cp/parser.c trunk/gcc/doc/extend.texi trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/g++.dg/asm-qual-3.C
Author: jakub Date: Thu Mar 7 08:32:09 2019 New Revision: 269452 URL: https://gcc.gnu.org/viewcvs?rev=269452&root=gcc&view=rev Log: PR c++/89585 * parser.c (cp_parser_asm_definition): Parse asm qualifiers even at toplevel, but diagnose them. * g++.dg/asm-qual-3.C: Adjust expected diagnostics. Modified: branches/gcc-8-branch/gcc/cp/ChangeLog branches/gcc-8-branch/gcc/cp/parser.c branches/gcc-8-branch/gcc/testsuite/ChangeLog branches/gcc-8-branch/gcc/testsuite/g++.dg/asm-qual-3.C
Okay, the version without a warning was checked in. Should I have suggested the warning elsewhere?
I guess this would also need a backport into gcc-7-branch as the "asm inline" changes were backported also there, no?
(In reply to Harald van Dijk from comment #28) > Okay, the version without a warning was checked in. Should I have suggested > the warning elsewhere? For 8.x we do not want to warn, that will break people building with -Werror. For 9.x, Jason said that perhaps we should warn instead of error for volatile, so I'll submit incremental patch.
Created attachment 45914 [details] gcc9-pr89585.patch Incremental patch to warn for GCC 9 instead of error.
(In reply to Jakub Jelinek from comment #30) > For 8.x we do not want to warn, that will break people building with -Werror. > For 9.x, Jason said that perhaps we should warn instead of error for > volatile, so I'll submit incremental patch. Good point about -Werror. The suggestion for a warning was based on it remaining an error in GCC 9. If it is going to be downgraded to a warning there, there's no point in a warning in GCC 8, so I'm happy with that approach. Thank you! One comment about the patch: it looks like resetting volatile_p (which you removed) and inline_p (which is still in) will have no effect, since the one use is conditional on parser->in_function_body anyway. Am I missing a use there? If not, is that in because the code is easier to understand if it is, or is it an oversight? The reset of goto_p affects how the remainder will be parsed, so that definitely does have an effect.
Author: jakub Date: Fri Mar 8 07:45:23 2019 New Revision: 269483 URL: https://gcc.gnu.org/viewcvs?rev=269483&root=gcc&view=rev Log: PR c++/89585 * parser.c (cp_parser_asm_definition): Just warn instead of error on volatile qualifier outside of function body. * g++.dg/asm-qual-3.C: Adjust expected diagnostics for toplevel asm volatile. Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/parser.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/g++.dg/asm-qual-3.C
Author: doko Date: Sun Mar 10 07:25:13 2019 New Revision: 269546 URL: https://gcc.gnu.org/viewcvs?rev=269546&root=gcc&view=rev Log: gcc/cp/ 2019-04-10 Matthias Klose <doko@ubuntu.com> Backport from the gcc-8 branch 2019-03-07 Jakub Jelinek <jakub@redhat.com> PR c++/89585 * parser.c (cp_parser_asm_definition): Parse asm qualifiers even at toplevel, but diagnose them. gcc/testsuite/ 2019-04-10 Matthias Klose <doko@ubuntu.com> Backport from the gcc-8 branch 2019-03-07 Jakub Jelinek <jakub@redhat.com> PR c++/89585 * g++.dg/asm-qual-3.C: Adjust expected diagnostics. Modified: branches/gcc-7-branch/gcc/cp/ChangeLog branches/gcc-7-branch/gcc/cp/parser.c branches/gcc-7-branch/gcc/testsuite/ChangeLog branches/gcc-7-branch/gcc/testsuite/g++.dg/asm-qual-3.C
(In reply to Matthias Klose from comment #34) > Author: doko > Date: Sun Mar 10 07:25:13 2019 > New Revision: 269546 > > URL: https://gcc.gnu.org/viewcvs?rev=269546&root=gcc&view=rev > Log: > gcc/cp/ > > 2019-04-10 Matthias Klose <doko@ubuntu.com> > > Backport from the gcc-8 branch > 2019-03-07 Jakub Jelinek <jakub@redhat.com> > > PR c++/89585 > * parser.c (cp_parser_asm_definition): Parse asm qualifiers even > at toplevel, but diagnose them. > > gcc/testsuite/ > > 2019-04-10 Matthias Klose <doko@ubuntu.com> > > Backport from the gcc-8 branch > 2019-03-07 Jakub Jelinek <jakub@redhat.com> > > PR c++/89585 > * g++.dg/asm-qual-3.C: Adjust expected diagnostics. > > Modified: > branches/gcc-7-branch/gcc/cp/ChangeLog > branches/gcc-7-branch/gcc/cp/parser.c > branches/gcc-7-branch/gcc/testsuite/ChangeLog > branches/gcc-7-branch/gcc/testsuite/g++.dg/asm-qual-3.C Is this fixed now?
I think so.