[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG246398ece711: [clang][Parse] properly parse asm-qualifiers, 
asm inline (authored by nickdesaulniers).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1529,13 +1529,13 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 249997.
nickdesaulniers marked 4 inline comments as done.
nickdesaulniers added a comment.

- restore order between Diag and SkipUntil


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1528,13 +1528,13 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:937
+case AQ_goto: return "goto";
+case AQ_unspecified:;
+  }

nickdesaulniers wrote:
> aaron.ballman wrote:
> > This looks wrong to me -- it flows through to an unreachable despite being 
> > reachable. I think this should `return "unspecified";`.
> This method is only called for printing; it seems weird to return 
> "unspecified" when it's kind of an invariant that that should never happen.  
> Maybe an assert here would be better?
> 
> I've updated the implementation slightly, but leaving this comment thread 
> open for more feedback.
An assert seems wrong to me -- the enumeration value is valid and the user has 
asked to print it. There's not much to assert on there. It's the caller of the 
method that should know whether it's violating an invariant to have that value 
in the first place. Also, people print from debuggers with relative frequency, 
so I'd personally avoid an assert in this case.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:684
+SkipUntil(tok::r_paren, StopAtSemi);
+Diag(Tok.getLocation(), diag::err_asm_qualifier_ignored);
+return true;

I think this will point to the right paren because of the preceding `SkipUntil` 
when it should point at the original token location.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 249760.
nickdesaulniers added a comment.

- drop `auto` as type is no longer implicit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1528,13 +1528,13 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = ConsumeToken();
 
-  if (Tok.is(tok::kw_volatile)) {
-   

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 249754.
nickdesaulniers marked 8 inline comments as done.
nickdesaulniers added a comment.

- address latest review comments, more refactoring


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1528,13 +1528,13 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 249757.
nickdesaulniers added a comment.

- remove isUnspecified(), was partially committed from refactoring I changed my 
mind on


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1528,13 +1528,13 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:937
+case AQ_goto: return "goto";
+case AQ_unspecified:;
+  }

aaron.ballman wrote:
> This looks wrong to me -- it flows through to an unreachable despite being 
> reachable. I think this should `return "unspecified";`.
This method is only called for printing; it seems weird to return "unspecified" 
when it's kind of an invariant that that should never happen.  Maybe an assert 
here would be better?

I've updated the implementation slightly, but leaving this comment thread open 
for more feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:17
+  "expected 'volatile', 'inline', 'goto', or '('">, CatInlineAsm;
+def err_global_asm_qualifier_ignored : Error<
+  "meaningless '%0' on asm outside function">, CatInlineAsm;

If we know of any compilers that support asm statements with qualifiers other 
than what Clang supports, it may make sense for these diagnostics to be 
warnings that are treated as an error by default so that users can disable the 
warning when compiling with Clang. I don't know of any such compilers off the 
top of my head, but perhaps you know of some.



Comment at: clang/include/clang/Parse/Parser.h:3215
+GNUAsmQualifiers() : Qualifiers(0) {}
+static const char *getSpecifierName(const AQ AQ);
+static AQ getQualifierForTokenKind(const tok::TokenKind K);

nickdesaulniers wrote:
> Now that we're not using `DeclSpec`, maybe we could drop this in preference 
> of `operator<<`.
I prefer the named function to a streaming operator given how little this will 
be used -- it's easier to call the named function from within a debugger than 
to use a streaming operator.

Please don't use a type name as a parameter identifier. Also, drop the 
top-level const.



Comment at: clang/include/clang/Parse/Parser.h:3206
+  class GNUAsmQualifiers {
+unsigned Qualifiers : 3; // Bitwise OR of AQ.
+  public:

There's really no benefit to using a bit-field here, so I'd just store an 
`unsigned`.



Comment at: clang/include/clang/Parse/Parser.h:3214
+};
+GNUAsmQualifiers() : Qualifiers(0) {}
+static const char *getSpecifierName(const AQ AQ);

I think you should use an in-class initializer for `Qualifiers` and then can 
drop the constructor entirely.



Comment at: clang/include/clang/Parse/Parser.h:3216
+static const char *getSpecifierName(const AQ AQ);
+bool setAsmQualifier(const AQ AQ);
+inline bool isVolatile() const { return Qualifiers & AQ_volatile; };

Please don't use a type name as a parameter identifier. Also, drop the 
top-level `const`.



Comment at: clang/include/clang/Parse/Parser.h:3224
+  GNUAsmQualifiers::AQ getGNUAsmQualifier(const Token ) const;
+  bool ParseGNUAsmQualifierListOpt(GNUAsmQualifiers *AQ);
 };

This should be taking a reference rather than a pointer. It should also be 
named `parseGNUAsmQualifierListOpt()` with a lowercase `p`.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:358
+bool Parser::isGNUAsmQualifier(const Token ) const {
+  return !!getGNUAsmQualifier(TokAfterAsm);
 }

This is too clever for my tastes -- I'd prefer an explicit test against 
`AQ_unspecified`.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:932
+
+const char *Parser::GNUAsmQualifiers::getSpecifierName(const AQ AQ) {
+  switch (AQ) {

type vs param name and top-level `const`.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:937
+case AQ_goto: return "goto";
+case AQ_unspecified:;
+  }

This looks wrong to me -- it flows through to an unreachable despite being 
reachable. I think this should `return "unspecified";`.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:951
+}
+bool Parser::GNUAsmQualifiers::setAsmQualifier(const AQ AQ) {
+  bool IsDuplicate = Qualifiers & AQ;

type vs param name and top-level `const`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 249412.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

- small refactorings


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1528,13 +1528,13 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = ConsumeToken();
 
-  if 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers marked 3 inline comments as done.
nickdesaulniers added a comment.

Sorry, got a little trigger happy with the refactoring.  Having a weekend to 
think about this more definitely helps.  It's nice to avoid instantiating a 
whole instance of `SemaDecl` and instead use a single `unsigned` bitfield!  I 
have a few more refactorings I'd like to do, but I'm working remote and my 
workstation is suddenly unavailable over SSH. Hopefully I can reboot it 
tomorrow.




Comment at: clang/include/clang/Parse/Parser.h:3214
+};
+GNUAsmQualifiers() : Qualifiers(0) {}
+static const char *getSpecifierName(const AQ AQ);

Might be nice to have an implicit constructor that takes a `GNUAsmQualifier` 
operand that defaults to `AQ_unspecified`.



Comment at: clang/include/clang/Parse/Parser.h:3215
+GNUAsmQualifiers() : Qualifiers(0) {}
+static const char *getSpecifierName(const AQ AQ);
+static AQ getQualifierForTokenKind(const tok::TokenKind K);

Now that we're not using `DeclSpec`, maybe we could drop this in preference of 
`operator<<`.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:358-359
+bool Parser::isGNUAsmQualifier(const Token ) const {
+  return TokAfterAsm.is(tok::kw_volatile) || TokAfterAsm.is(tok::kw_inline) ||
+ TokAfterAsm.is(tok::kw_goto);
 }

Can be implemented in terms of 
`Parser::GNUAsmQualifiers::getQualifierForTokenKind`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 249254.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

- DRY up getting the Qualifier, and checking Tok.is


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1528,13 +1528,15 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 249246.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

- rebase, divorce from DeclSpec


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Parse/Parser.cpp
===
--- clang/lib/Parse/Parser.cpp
+++ clang/lib/Parse/Parser.cpp
@@ -1528,13 +1528,22 @@
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = ConsumeToken();
 
- 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers added a comment.

In D75563#1911375 , @aaron.ballman 
wrote:

> Thank you for working on this, this LGTM! If you wanted a follow-up patch 
> beyond adding semantic support for the `inline` keyword, I think it might 
> make sense to investigate divorcing the qualifier parsing from the `DeclSpec` 
> interface. These are ASM statements, not declarations, so the fact that we're 
> using a DeclSpec to smuggle the qualifiers around is a bit unexpected. 
> However, I don't think that work needs to hold up this patch.


I agree and I think that recommendation is in good taste. I'd rather do things 
the right way now and hopefully never need to revisit.  Also, there's no rush 
on this.  I'll declare a new class for the qualifiers, since we just need to 
know which qualifiers were used (what is essentially 3 booleans) and strings 
for each.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.



Comment at: clang/lib/Parse/Parser.cpp:1545
+Diag(Tok, diag::err_global_asm_qualifier_ignored)
+<< GNUAsmQualifiers::getSpecifierName(AQ)
+<< FixItHint::CreateRemoval(RemovalRange);

This pattern is repeated twice, let me see if I can DRY that up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Thank you for working on this, this LGTM! If you wanted a follow-up patch 
beyond adding semantic support for the `inline` keyword, I think it might make 
sense to investigate divorcing the qualifier parsing from the `DeclSpec` 
interface. These are ASM statements, not declarations, so the fact that we're 
using a DeclSpec to smuggle the qualifiers around is a bit unexpected. However, 
I don't think that work needs to hold up this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248827.
nickdesaulniers added a comment.

- add release note about change in behavior of -Wduplicate-decl-specifier


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248825.
nickdesaulniers added a comment.

- use better error name
- reorder new errors
- git-clang-format HEAD~


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:15
 
-def warn_asm_qualifier_ignored : Warning<
-  "ignored %0 qualifier on asm">, CatInlineAsm, InGroup;
-def warn_file_asm_volatile : Warning<
-  "meaningless 'volatile' on asm outside function">, CatInlineAsm,
-  InGroup;
+def err_file_asm_volatile : Error<
+  "meaningless '%0' on asm outside function">, CatInlineAsm;

oh, I meant to rename this; and I think flipping the order of this and 
following def is a little clearer for code review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248816.
nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.
Herald added subscribers: jfb, aheejin.

- completely drop use of Parse::ParseTypeQualifierListOpt
- move paren parsing into helper
- fix up test cases for global asm statements
- delete duplicate test cases covered by the added test file


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Parse/Parser.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/CodeGen/inline-asm-mixed-style.c
  clang/test/Parser/asm-qualifiers.c
  clang/test/Parser/asm.c
  clang/test/Sema/asm.c

Index: clang/test/Sema/asm.c
===
--- clang/test/Sema/asm.c
+++ clang/test/Sema/asm.c
@@ -91,9 +91,6 @@
   return a;
 }
 
-// 
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
Index: clang/test/Parser/asm.c
===
--- clang/test/Parser/asm.c
+++ clang/test/Parser/asm.c
@@ -12,12 +12,6 @@
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// 
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}
Index: clang/test/CodeGen/inline-asm-mixed-style.c
===
--- clang/test/CodeGen/inline-asm-mixed-style.c
+++ clang/test/CodeGen/inline-asm-mixed-style.c
@@ -14,11 +14,6 @@
   // CHECK: movl%ebx, %eax
   // CHECK: movl%ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl%ebx, %eax
-  // CHECK: movl%ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl%ecx, %edx
 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked 4 inline comments as done.
nickdesaulniers added inline comments.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:746-755
   ParseTypeQualifierListOpt(DS, AR_VendorAttributesParsed);
 
   // GNU asms accept, but warn, about type-qualifiers other than volatile.
   if (DS.getTypeQualifiers() & DeclSpec::TQ_const)
 Diag(Loc, diag::warn_asm_qualifier_ignored) << "const";
   if (DS.getTypeQualifiers() & DeclSpec::TQ_restrict)
 Diag(Loc, diag::warn_asm_qualifier_ignored) << "restrict";

nickdesaulniers wrote:
> I think in a follow up commit, I'll just delete this (the call to 
> `ParseTypeQualifierListOpt` and subsequent `warn_asm_qualifier_ignored` 
> emission); making it a parse error to put type qualifiers after the keyword 
> `asm`, which would match the behavior of GCC, and not waste time looking for 
> and parsing type qualifiers.  It maybe made sense when 
> `ParseTypeQualifierListOpt` was being abused to parse `volatile`, but this 
> patch fixes that.
Chatting with @aaron.ballman , I think it makes sense to tackle this in this 
CL, which will simplify the requested changes to parsing (above).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:704
+  AQ = DeclSpec::AQ_goto;
+else {
+  if (EndLoc.isValid())

nickdesaulniers wrote:
> aaron.ballman wrote:
> > I would expect a diagnostic if the unknown token is anything other than a 
> > left paren.
> That currently is is handled by the caller, see `if (Tok.isNot(tok::l_paren)) 
> {` in `Parser::ParseAsmStatement` (line 762 of 
> clang/lib/Parse/ParseStmtAsm.cpp).
My suggestion is to move it out of the caller and into the helper. This 
function is expected to parse the asm qualifiers, so it stands to reason if it 
gets something other than an asm qualifier, it should diagnose. (We probably 
won't need to re-use this parsing logic in other contexts, but that's why it's 
better to have the diagnostic here rather than assume the caller will do it.)



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:725
 /// [GNU] gnu-asm-statement:
-/// 'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+/// 'asm' asm-qualifier[opt] '(' asm-argument ')' ';'
 ///

nickdesaulniers wrote:
> I guess this should be `asm-qualifier-list[opt]`?
Yup, good catch!



Comment at: clang/test/Parser/asm-qualifiers.c:12-14
+  asm noodle(""); // expected-error {{expected '(' after 'asm'}}
+  asm goto noodle("" foo); // expected-error {{expected '(' after 'asm'}}
+  asm volatile noodle inline(""); // expected-error {{expected '(' after 
'asm'}}

I think these diagnostics should be improved as none of them suggest the 
underlying problem. As they read now, it sounds like you can write 
`asm(volatile noodle inline(""))` to appease the diagnostic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked 2 inline comments as done and an inline comment as not 
done.
nickdesaulniers added inline comments.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:725
 /// [GNU] gnu-asm-statement:
-/// 'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+/// 'asm' asm-qualifier[opt] '(' asm-argument ')' ';'
 ///

I guess this should be `asm-qualifier-list[opt]`?



Comment at: clang/test/Parser/asm-qualifiers.c:54
+asm ("");
+asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside 
function}}
+asm inline (""); // expected-error {{expected '(' after 'asm'}}

I'll probably also drop parsing for this here, too, in a follow up; GCC treats 
this case as a parse error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248603.
nickdesaulniers marked 9 inline comments as done.
nickdesaulniers added a comment.

- address review comments, add tests for global asm statements.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/Parser/asm-qualifiers.c

Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected '(' after 'asm'}}
+  asm goto noodle("" foo); // expected-error {{expected '(' after 'asm'}}
+  asm volatile noodle inline(""); // expected-error {{expected '(' after 'asm'}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{expected '(' after 'asm'}}
+// expected-error@-1 {{expected ';' after top-level asm block}}
+// expected-error@-2 {{expected identifier or '('}}
+// expected-error@-3 {{expected ')'}}
+// expected-note@-4 {{to match this '('}}
+asm goto (""noodle); // expected-error {{expected '(' after 'asm'}}
+// expected-error@-1 {{expected ';' after top-level asm block}}
+// expected-error@-2 {{expected identifier or '('}}
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown typespec!");
 }
 
+const char *DeclSpec::getSpecifierName(AQ A) {
+  switch (A) {
+case DeclSpec::AQ_unspecified: return "unspecified";
+case DeclSpec::AQ_volatile: return "volatile";
+case DeclSpec::AQ_inline: return "inline";
+case DeclSpec::AQ_goto: return "goto";
+  }
+  llvm_unreachable("Unknown GNUAsmQualifier!");
+}
+
 bool DeclSpec::SetStorageClassSpec(Sema , SCS SC, SourceLocation Loc,
const char *,
unsigned ,
@@ -938,6 +948,12 @@
   llvm_unreachable("Unknown type qualifier!");
 }
 
+bool DeclSpec::setGNUAsmQualifier(AQ A, SourceLocation Loc) {
+  bool IsInvalid = GNUAsmQualifiers & A;
+  GNUAsmQualifiers |= A;
+  return IsInvalid;
+}
+
 bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *,
  unsigned ) {
   // 'inline inline' is ok.  However, since this is likely not what the user
Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,13 +684,45 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///   asm-qualifier:
+/// volatile
+/// inline
+/// goto
+///
+///   asm-qualifier-list:
+/// asm-qualifier
+/// asm-qualifier-list asm-qualifier
+void Parser::ParseGNUAsmQualifierListOpt(DeclSpec ) {
+  SourceLocation EndLoc;
+  while (1) {
+SourceLocation Loc = Tok.getLocation();
+DeclSpec::AQ AQ = DeclSpec::AQ_unspecified;
+
+if (Tok.getKind() == tok::kw_volatile)
+

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:704
+  AQ = DeclSpec::AQ_goto;
+else {
+  if (EndLoc.isValid())

aaron.ballman wrote:
> I would expect a diagnostic if the unknown token is anything other than a 
> left paren.
That currently is is handled by the caller, see `if (Tok.isNot(tok::l_paren)) 
{` in `Parser::ParseAsmStatement` (line 762 of 
clang/lib/Parse/ParseStmtAsm.cpp).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:32
   "'asm goto' cannot have output constraints">;
+def err_asm_duplicate_qual : Error<"duplicate asm qualifier %0">;
 }

`duplicate asm qualifier '%0'` (with the quotes around the `%0`).



Comment at: clang/include/clang/Sema/DeclSpec.h:324
 
+  // GNU Asm Qualifiers
+  enum AQ {

Asm Qualifiers -> asm qualifiers



Comment at: clang/include/clang/Sema/DeclSpec.h:378
+  // GNU asm specifier
+  unsigned GNUAsmQualifiers : 4;
+

I think we only need three bits to store this. Also, the comment should be 
clear that this is a bitwise OR of AQ, similar to the comment for 
`TypeQualifiers`.



Comment at: clang/include/clang/Sema/DeclSpec.h:582
+  /// getGNUAsmQualifiers - Return a set of AQs.
+  unsigned GetGNUAsmQual() const { return GNUAsmQualifiers; }
+

The comment and the code don't match. I prefer the spelling in the comment.



Comment at: clang/include/clang/Sema/DeclSpec.h:736
 
+  bool SetGNUAsmQual(AQ A, SourceLocation Loc);
+

Given that we're already inconsistent in this area regarding naming, I'd prefer 
to follow the LLVM naming convention and use `setGNUAsmQualifier()`



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:688
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///   asm-qualifiers:
+/// volatile

The comment doesn't match the code -- I think the comment should be:
```
/// asm-qualifier:
///   volatile
///   inline
///   goto
///
/// asm-qualifier-list:
///   asm-qualifier
///   asm-qualifier-list asm-qualifier
```



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:704
+  AQ = DeclSpec::AQ_goto;
+else {
+  if (EndLoc.isValid())

I would expect a diagnostic if the unknown token is anything other than a left 
paren.



Comment at: clang/test/Parser/asm-qualifiers.c:3
+
+void qualifiers(void) {
+  asm("");

There should also be tests where the qualifiers are incorrect. e.g.,
```
asm noodle("");
asm goto noodle("");
asm volatile noodle inline("");
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.



Comment at: clang/lib/Parse/ParseStmtAsm.cpp:746-755
   ParseTypeQualifierListOpt(DS, AR_VendorAttributesParsed);
 
   // GNU asms accept, but warn, about type-qualifiers other than volatile.
   if (DS.getTypeQualifiers() & DeclSpec::TQ_const)
 Diag(Loc, diag::warn_asm_qualifier_ignored) << "const";
   if (DS.getTypeQualifiers() & DeclSpec::TQ_restrict)
 Diag(Loc, diag::warn_asm_qualifier_ignored) << "restrict";

I think in a follow up commit, I'll just delete this (the call to 
`ParseTypeQualifierListOpt` and subsequent `warn_asm_qualifier_ignored` 
emission); making it a parse error to put type qualifiers after the keyword 
`asm`, which would match the behavior of GCC, and not waste time looking for 
and parsing type qualifiers.  It maybe made sense when 
`ParseTypeQualifierListOpt` was being abused to parse `volatile`, but this 
patch fixes that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248226.
nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.

- git-clang-format HEAD~


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/Parser/asm-qualifiers.c

Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier volatile}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier volatile}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier inline}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier inline}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+foo:;
+}
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown typespec!");
 }
 
+const char *DeclSpec::getSpecifierName(AQ A) {
+  switch (A) {
+case DeclSpec::AQ_unspecified: return "unspecified";
+case DeclSpec::AQ_volatile: return "volatile";
+case DeclSpec::AQ_inline: return "inline";
+case DeclSpec::AQ_goto: return "goto";
+  }
+  llvm_unreachable("Unknown GNUAsmQualifier!");
+}
+
 bool DeclSpec::SetStorageClassSpec(Sema , SCS SC, SourceLocation Loc,
const char *,
unsigned ,
@@ -938,6 +948,12 @@
   llvm_unreachable("Unknown type qualifier!");
 }
 
+bool DeclSpec::SetGNUAsmQual(AQ A, SourceLocation Loc) {
+  bool IsInvalid = GNUAsmQualifiers & A;
+  GNUAsmQualifiers |= A;
+  return IsInvalid;
+}
+
 bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *,
  unsigned ) {
   // 'inline inline' is ok.  However, since this is likely not what the user
Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,13 +684,41 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///   asm-qualifiers:
+/// volatile
+/// inline
+/// goto
+void Parser::ParseGNUAsmQualifierListOpt(DeclSpec ) {
+  SourceLocation EndLoc;
+  while (1) {
+SourceLocation Loc = Tok.getLocation();
+DeclSpec::AQ AQ = DeclSpec::AQ_unspecified;
+
+if (Tok.getKind() == tok::kw_volatile)
+  AQ = DeclSpec::AQ_volatile;
+else if (Tok.getKind() == tok::kw_inline)
+  AQ = DeclSpec::AQ_inline;
+else if (Tok.getKind() == tok::kw_goto)
+  AQ = DeclSpec::AQ_goto;
+else {
+  if (EndLoc.isValid())
+DS.SetRangeEnd(EndLoc);
+  return;
+}
+if (DS.SetGNUAsmQual(AQ, Loc))
+  Diag(Loc, diag::err_asm_duplicate_qual) << DeclSpec::getSpecifierName(AQ);
+EndLoc = ConsumeToken();
+  }
+}
+
 /// ParseAsmStatement - Parse a GNU extended asm statement.
 ///   asm-statement:
 /// gnu-asm-statement
 /// ms-asm-statement
 ///
 /// [GNU] gnu-asm-statement:
-/// 'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+/// 'asm' asm-qualifier[opt] '(' asm-argument ')' ';'
 ///
 /// [GNU] asm-argument:
 /// asm-string-literal
@@ -713,6 +741,7 @@
   }
 
   DeclSpec DS(AttrFactory);
+  ParseGNUAsmQualifierListOpt(DS);
   SourceLocation Loc = Tok.getLocation();
   ParseTypeQualifierListOpt(DS, AR_VendorAttributesParsed);

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked 4 inline comments as done.
nickdesaulniers added inline comments.



Comment at: clang/test/Parser/asm-qualifiers.c:20
+
+void combinations(void) {
+  asm volatile inline("");

nathanchance wrote:
> nickdesaulniers wrote:
> > nathanchance wrote:
> > > I'm probably being dense but what is intended to be tested differently 
> > > between `combinations` and `permutations`? I assume the order of the 
> > > qualifiers? Wouldn't it just be better to merge `combinations` into 
> > > `permutations` or was there some deeper reasoning for the 
> > > compartmentalization?
> > `combinations` tests a combination of different `asm-qualifiers` together. 
> > `permutations` are just permutations of the combinations that have not been 
> > tested above. I may not even have my nomenclature correct.  Shall I combine 
> > them?
> I assume that you want permutations since you want to make sure that the 
> ordering does not matter, right? If you just care about combinations then
> 
> ```
>   asm inline goto volatile("" foo);
>   asm inline volatile goto("" foo);
> 
>   asm goto inline volatile("" foo);
>   asm goto volatile inline("" foo);
> 
>   asm volatile goto inline("" foo); // note, this one should probably be 
> added in permutations
>   asm volatile inline goto("" foo);
> ``` 
> 
> could just be distilled down to one of those since they are the same 
> combination of qualifiers (combinations do not care about order). I would say 
> that moving `combinations` into `permutations` would be wise since 
> `permutations` tests the same thing that `combinations` does and more.
Great suggestion, done. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248225.
nickdesaulniers added a comment.

- combine combinations into permutations test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/Parser/asm-qualifiers.c

Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" foo);
+  asm goto inline("");
+  asm goto volatile inline("" foo);
+  asm goto volatile("");
+  asm inline goto volatile("" foo);
+  asm inline goto("" foo);
+  asm inline volatile goto("" foo);
+  asm inline volatile("");
+  asm volatile goto("" foo);
+  asm volatile inline goto("" foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier volatile}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier volatile}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier inline}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier inline}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+foo:;
+}
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown typespec!");
 }
 
+const char *DeclSpec::getSpecifierName(AQ A) {
+  switch (A) {
+case DeclSpec::AQ_unspecified: return "unspecified";
+case DeclSpec::AQ_volatile: return "volatile";
+case DeclSpec::AQ_inline: return "inline";
+case DeclSpec::AQ_goto: return "goto";
+  }
+  llvm_unreachable("Unknown GNUAsmQualifier!");
+}
+
 bool DeclSpec::SetStorageClassSpec(Sema , SCS SC, SourceLocation Loc,
const char *,
unsigned ,
@@ -938,6 +948,12 @@
   llvm_unreachable("Unknown type qualifier!");
 }
 
+bool DeclSpec::SetGNUAsmQual(AQ A, SourceLocation Loc) {
+  bool IsInvalid = GNUAsmQualifiers & A;
+  GNUAsmQualifiers |= A;
+  return IsInvalid;
+}
+
 bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *,
  unsigned ) {
   // 'inline inline' is ok.  However, since this is likely not what the user
Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,13 +684,42 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///   asm-qualifiers:
+/// volatile
+/// inline
+/// goto
+void Parser::ParseGNUAsmQualifierListOpt(DeclSpec ) {
+  SourceLocation EndLoc;
+  while (1) {
+SourceLocation Loc = Tok.getLocation();
+DeclSpec::AQ AQ = DeclSpec::AQ_unspecified;
+
+if (Tok.getKind() == tok::kw_volatile)
+  AQ = DeclSpec::AQ_volatile;
+else if (Tok.getKind() == tok::kw_inline)
+  AQ = DeclSpec::AQ_inline;
+else if (Tok.getKind() == tok::kw_goto)
+  AQ = DeclSpec::AQ_goto;
+else {
+  if (EndLoc.isValid())
+DS.SetRangeEnd(EndLoc);
+  return;
+}
+if (DS.SetGNUAsmQual(AQ, Loc))
+  Diag(Loc, diag::err_asm_duplicate_qual)
+  << DeclSpec::getSpecifierName(AQ);
+EndLoc = ConsumeToken();
+  }
+}
+
 /// ParseAsmStatement - Parse a GNU extended asm statement.
 ///   asm-statement:
 /// gnu-asm-statement
 /// ms-asm-statement
 ///
 /// [GNU] gnu-asm-statement:
-/// 'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+/// 'asm' asm-qualifier[opt] '(' asm-argument ')' ';'
 ///
 /// [GNU] asm-argument:
 /// asm-string-literal
@@ -713,6 +742,7 @@
   }
 
   DeclSpec DS(AttrFactory);
+  ParseGNUAsmQualifierListOpt(DS);
   SourceLocation Loc = Tok.getLocation();
   ParseTypeQualifierListOpt(DS, AR_VendorAttributesParsed);
 
@@ -726,14 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added inline comments.



Comment at: clang/test/Parser/asm-qualifiers.c:20
+
+void combinations(void) {
+  asm volatile inline("");

nickdesaulniers wrote:
> nathanchance wrote:
> > I'm probably being dense but what is intended to be tested differently 
> > between `combinations` and `permutations`? I assume the order of the 
> > qualifiers? Wouldn't it just be better to merge `combinations` into 
> > `permutations` or was there some deeper reasoning for the 
> > compartmentalization?
> `combinations` tests a combination of different `asm-qualifiers` together. 
> `permutations` are just permutations of the combinations that have not been 
> tested above. I may not even have my nomenclature correct.  Shall I combine 
> them?
I assume that you want permutations since you want to make sure that the 
ordering does not matter, right? If you just care about combinations then

```
  asm inline goto volatile("" foo);
  asm inline volatile goto("" foo);

  asm goto inline volatile("" foo);
  asm goto volatile inline("" foo);

  asm volatile goto inline("" foo); // note, this one should probably be 
added in permutations
  asm volatile inline goto("" foo);
``` 

could just be distilled down to one of those since they are the same 
combination of qualifiers (combinations do not care about order). I would say 
that moving `combinations` into `permutations` would be wise since 
`permutations` tests the same thing that `combinations` does and more.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/test/Parser/asm-qualifiers.c:20
+
+void combinations(void) {
+  asm volatile inline("");

nathanchance wrote:
> I'm probably being dense but what is intended to be tested differently 
> between `combinations` and `permutations`? I assume the order of the 
> qualifiers? Wouldn't it just be better to merge `combinations` into 
> `permutations` or was there some deeper reasoning for the 
> compartmentalization?
`combinations` tests a combination of different `asm-qualifiers` together. 
`permutations` are just permutations of the combinations that have not been 
tested above. I may not even have my nomenclature correct.  Shall I combine 
them?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added inline comments.



Comment at: clang/test/Parser/asm-qualifiers.c:20
+
+void combinations(void) {
+  asm volatile inline("");

I'm probably being dense but what is intended to be tested differently between 
`combinations` and `permutations`? I assume the order of the qualifiers? 
Wouldn't it just be better to merge `combinations` into `permutations` or was 
there some deeper reasoning for the compartmentalization?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248027.
nickdesaulniers added a comment.

- make sure to initialize GNUAsmQualifiers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/Parser/asm-qualifiers.c

Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void combinations(void) {
+  asm volatile inline("");
+  asm volatile goto("" foo);
+  asm inline goto("" foo);
+  asm volatile inline goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm inline volatile("");
+  asm goto volatile("");
+  asm goto inline("");
+  asm inline goto volatile("" foo);
+  asm inline volatile goto("" foo);
+  asm goto inline volatile("" foo);
+  asm goto volatile inline("" foo);
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier volatile}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier volatile}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier inline}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier inline}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+foo:;
+}
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown typespec!");
 }
 
+const char *DeclSpec::getSpecifierName(AQ A) {
+  switch (A) {
+case DeclSpec::AQ_unspecified: return "unspecified";
+case DeclSpec::AQ_volatile: return "volatile";
+case DeclSpec::AQ_inline: return "inline";
+case DeclSpec::AQ_goto: return "goto";
+  }
+  llvm_unreachable("Unknown GNUAsmQualifier!");
+}
+
 bool DeclSpec::SetStorageClassSpec(Sema , SCS SC, SourceLocation Loc,
const char *,
unsigned ,
@@ -938,6 +948,12 @@
   llvm_unreachable("Unknown type qualifier!");
 }
 
+bool DeclSpec::SetGNUAsmQual(AQ A, SourceLocation Loc) {
+  bool IsInvalid = GNUAsmQualifiers & A;
+  GNUAsmQualifiers |= A;
+  return IsInvalid;
+}
+
 bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *,
  unsigned ) {
   // 'inline inline' is ok.  However, since this is likely not what the user
Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,13 +684,42 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///   asm-qualifiers:
+/// volatile
+/// inline
+/// goto
+void Parser::ParseGNUAsmQualifierListOpt(DeclSpec ) {
+  SourceLocation EndLoc;
+  while (1) {
+SourceLocation Loc = Tok.getLocation();
+DeclSpec::AQ AQ = DeclSpec::AQ_unspecified;
+
+if (Tok.getKind() == tok::kw_volatile)
+  AQ = DeclSpec::AQ_volatile;
+else if (Tok.getKind() == tok::kw_inline)
+  AQ = DeclSpec::AQ_inline;
+else if (Tok.getKind() == tok::kw_goto)
+  AQ = DeclSpec::AQ_goto;
+else {
+  if (EndLoc.isValid())
+DS.SetRangeEnd(EndLoc);
+  return;
+}
+if (DS.SetGNUAsmQual(AQ, Loc))
+  Diag(Loc, diag::err_asm_duplicate_qual)
+  << DeclSpec::getSpecifierName(AQ);
+EndLoc = ConsumeToken();
+  }
+}
+
 /// ParseAsmStatement - Parse a GNU extended asm statement.
 ///   asm-statement:
 /// gnu-asm-statement
 /// ms-asm-statement
 ///
 /// [GNU] gnu-asm-statement:
-/// 'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+/// 'asm' asm-qualifier[opt] '(' asm-argument ')' ';'
 ///
 /// [GNU] asm-argument:
 /// asm-string-literal
@@ -713,6 +742,7 @@
   }
 
   DeclSpec DS(AttrFactory);
+  ParseGNUAsmQualifierListOpt(DS);
   SourceLocation Loc = Tok.getLocation();
   ParseTypeQualifierListOpt(DS, 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248020.
nickdesaulniers added a comment.

- seems `arc diff` wiped out my previous changes...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/Parser/asm-qualifiers.c

Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void combinations(void) {
+  asm volatile inline("");
+  asm volatile goto("" foo);
+  asm inline goto("" foo);
+  asm volatile inline goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm inline volatile("");
+  asm goto volatile("");
+  asm goto inline("");
+  asm inline goto volatile("" foo);
+  asm inline volatile goto("" foo);
+  asm goto inline volatile("" foo);
+  asm goto volatile inline("" foo);
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier volatile}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier volatile}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier inline}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier inline}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+foo:;
+}
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown typespec!");
 }
 
+const char *DeclSpec::getSpecifierName(AQ A) {
+  switch (A) {
+case DeclSpec::AQ_unspecified: return "unspecified";
+case DeclSpec::AQ_volatile: return "volatile";
+case DeclSpec::AQ_inline: return "inline";
+case DeclSpec::AQ_goto: return "goto";
+  }
+  llvm_unreachable("Unknown GNUAsmQualifier!");
+}
+
 bool DeclSpec::SetStorageClassSpec(Sema , SCS SC, SourceLocation Loc,
const char *,
unsigned ,
@@ -938,6 +948,12 @@
   llvm_unreachable("Unknown type qualifier!");
 }
 
+bool DeclSpec::SetGNUAsmQual(AQ A, SourceLocation Loc) {
+  bool IsInvalid = GNUAsmQualifiers & A;
+  GNUAsmQualifiers |= A;
+  return IsInvalid;
+}
+
 bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *,
  unsigned ) {
   // 'inline inline' is ok.  However, since this is likely not what the user
Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,13 +684,42 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///   asm-qualifiers:
+/// volatile
+/// inline
+/// goto
+void Parser::ParseGNUAsmQualifierListOpt(DeclSpec ) {
+  SourceLocation EndLoc;
+  while (1) {
+SourceLocation Loc = Tok.getLocation();
+DeclSpec::AQ AQ = DeclSpec::AQ_unspecified;
+
+if (Tok.getKind() == tok::kw_volatile)
+  AQ = DeclSpec::AQ_volatile;
+else if (Tok.getKind() == tok::kw_inline)
+  AQ = DeclSpec::AQ_inline;
+else if (Tok.getKind() == tok::kw_goto)
+  AQ = DeclSpec::AQ_goto;
+else {
+  if (EndLoc.isValid())
+DS.SetRangeEnd(EndLoc);
+  return;
+}
+if (DS.SetGNUAsmQual(AQ, Loc))
+  Diag(Loc, diag::err_asm_duplicate_qual)
+  << DeclSpec::getSpecifierName(AQ);
+EndLoc = ConsumeToken();
+  }
+}
+
 /// ParseAsmStatement - Parse a GNU extended asm statement.
 ///   asm-statement:
 /// gnu-asm-statement
 /// ms-asm-statement
 ///
 /// [GNU] gnu-asm-statement:
-/// 'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+/// 'asm' asm-qualifier[opt] '(' asm-argument ')' ';'
 ///
 /// [GNU] asm-argument:
 /// asm-string-literal
@@ -713,6 +742,7 @@
   }
 
   DeclSpec DS(AttrFactory);
+  ParseGNUAsmQualifierListOpt(DS);
   SourceLocation Loc = Tok.getLocation();
   ParseTypeQualifierListOpt(DS, 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 248019.
nickdesaulniers added a comment.

- remove impossible case in ParseGNUAsmQualifierListOpt
- simplify SetGNUAsmQual
- fix typo in comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563

Files:
  clang/lib/Parse/ParseStmtAsm.cpp


Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,7 +684,7 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
-/// ParseGNUAsmQualifierList - Parse a GNU extended asm qualifier list.
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
 ///   asm-qualifiers:
 /// volatile
 /// inline


Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,7 +684,7 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
-/// ParseGNUAsmQualifierList - Parse a GNU extended asm qualifier list.
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
 ///   asm-qualifiers:
 /// volatile
 /// inline
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
nickdesaulniers added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
nickdesaulniers added a comment.

I still plan to add CodeGen support for `asm inline` before the next release, 
which should be as simple as emitting an additional `inlinehint`, but I'd like 
to save that for a follow up patch on top of this.


The parsing of GNU C extended asm statements was a little brittle and
had a few issues:

- It was using Parse::ParseTypeQualifierListOpt to parse the `volatile` 
qualifier.  That parser is really meant for TypeQualifiers; an asm statement 
doesn't really have a type qualifier. This is still maybe nice to have, but not 
necessary. We now can check for the `volatile` token by properly expanding the 
grammer, rather than abusing Parse::ParseTypeQualifierListOpt.
- The parsing of `goto` was position dependent, so `asm goto volatile` wouldn't 
parse. The qualifiers should be position independent to one another. Now they 
are.
- We would warn on duplicate `volatile`, but the parse error for duplicate 
`goto` was a generic parse error and wasn't clear.
- We need to add support for the recent GNU C extension `asm inline`. Adding 
support to the parser with the above issues highlighted the need for this 
refactoring.

Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75563

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/Parse/ParseStmtAsm.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/test/Parser/asm-qualifiers.c

Index: clang/test/Parser/asm-qualifiers.c
===
--- /dev/null
+++ clang/test/Parser/asm-qualifiers.c
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" foo);
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" foo);
+foo:;
+}
+
+void combinations(void) {
+  asm volatile inline("");
+  asm volatile goto("" foo);
+  asm inline goto("" foo);
+  asm volatile inline goto("" foo);
+foo:;
+}
+
+void permutations(void) {
+  asm inline volatile("");
+  asm goto volatile("");
+  asm goto inline("");
+  asm inline goto volatile("" foo);
+  asm inline volatile goto("" foo);
+  asm goto inline volatile("" foo);
+  asm goto volatile inline("" foo);
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile(""); // expected-error {{duplicate asm qualifier volatile}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier volatile}}
+  asm inline inline(""); // expected-error {{duplicate asm qualifier inline}}
+  __asm__ __inline__ __inline__(""); // expected-error {{duplicate asm qualifier inline}}
+  asm goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+  __asm__ goto goto("" foo); // expected-error {{duplicate asm qualifier goto}}
+foo:;
+}
Index: clang/lib/Sema/DeclSpec.cpp
===
--- clang/lib/Sema/DeclSpec.cpp
+++ clang/lib/Sema/DeclSpec.cpp
@@ -589,6 +589,16 @@
   llvm_unreachable("Unknown typespec!");
 }
 
+const char *DeclSpec::getSpecifierName(AQ A) {
+  switch (A) {
+case DeclSpec::AQ_unspecified: return "unspecified";
+case DeclSpec::AQ_volatile: return "volatile";
+case DeclSpec::AQ_inline: return "inline";
+case DeclSpec::AQ_goto: return "goto";
+  }
+  llvm_unreachable("Unknown GNUAsmQualifier!");
+}
+
 bool DeclSpec::SetStorageClassSpec(Sema , SCS SC, SourceLocation Loc,
const char *,
unsigned ,
@@ -938,6 +948,16 @@
   llvm_unreachable("Unknown type qualifier!");
 }
 
+bool DeclSpec::SetGNUAsmQual(AQ A, SourceLocation Loc) {
+  if (!(A == AQ_unspecified || A == AQ_volatile || A == AQ_inline ||
+A == AQ_goto) ||
+  GNUAsmQualifiers & A)
+return true;
+
+  GNUAsmQualifiers |= A;
+  return false;
+}
+
 bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *,
  unsigned ) {
   // 'inline inline' is ok.  However, since this is likely not what the user
Index: clang/lib/Parse/ParseStmtAsm.cpp
===
--- clang/lib/Parse/ParseStmtAsm.cpp
+++ clang/lib/Parse/ParseStmtAsm.cpp
@@ -684,13 +684,43 @@
 ClobberRefs, Exprs, EndLoc);
 }
 
+/// ParseGNUAsmQualifierList - Parse a GNU extended asm qualifier list.
+///   asm-qualifiers:
+/// 

[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

I still plan to add CodeGen support for `asm inline` before the next release, 
which should be as simple as emitting an additional `inlinehint`, but I'd like 
to save that for a follow up patch on top of this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563



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