[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith marked an inline comment as done.
rsmith added a comment.

Looks good with a few largely-mechanical changes.



================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:30
   "GNU-style inline assembly is disabled">;
-def err_asm_goto_not_supported_yet : Error<
-  "'asm goto' constructs are not supported yet">;
+def err_asm_goto_can_not_have_output : Error<
+  "'asm goto' cannot have output constraints">;
----------------
Nit: `can_not` -> `cannot`


================
Comment at: lib/Parse/ParseStmtAsm.cpp:806
     // Parse the asm-string list for clobbers if present.
-    if (Tok.isNot(tok::r_paren)) {
+    if (!AteExtraColon && Tok.isNot(tok::r_paren) &&
+        isTokenStringLiteral()) {
----------------
Nit: the `Tok.isNot(tok::r_paren)` check here is redundant, since `isTokenStringLiteral()` covers that check.


================
Comment at: lib/Parse/ParseStmtAsm.cpp:821
   }
+  if (!isGotoAsm && Tok.isNot(tok::r_paren)) {
+    Diag(Tok, diag::err_expected) << tok::r_paren;
----------------
I think this should be:

```
  if (!isGotoAsm && (Tok.isNot(tok::r_paren) || AteExtraColon)) {
```

otherwise we'll accept a spurious extra colon split from a `::` at the end of a non-`goto` `asm`. Eg, I think in C++:

```
void f() { asm("" : : :: ); }
```

... would be incorrectly accepted, because we'll parse the final `::` as introducing the clobbers list, consume the `::` token, pass this check because the next token is now `)`, and never notice we didn't use the second half of the `::` token.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:659-671
+    if (NS->getIdentifier(i))
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+                                         Exprs[i]));
+  for (unsigned i = NumOutputs, e = NumOutputs + NumInputs; i != e; ++i)
+    if (NS->getIdentifier(i))
+      MyList.emplace_back(std::make_pair(NS->getIdentifier(i)->getName(),
+                                         Exprs[i]));
----------------
jyu2 wrote:
> rsmith wrote:
> > This is still exposing / relying on an implementation detail of `GCCAsmStmt`. Please replace `getIdentifier` with `getOuptutIdentifier` / `getInputIdentifier` / `getLabelIdentifier`, adjust the indexing to match, and remove `GCCAsmStmt::getIdentifier`. Or alternatively, iterate over `Names` here rather than walking the parts of the `GCCAsmStmt`.
> Changed.  Like this!!!
This is really neat, thanks!


================
Comment at: lib/Sema/SemaStmtAsm.cpp:672
+  // Sort NamedOperandList.
+  stable_sort(NamedOperandList.begin(), NamedOperandList.end(),
+              [](const NamedOperand &LHS, const NamedOperand &RHS) {
----------------
Nit: explicitly spell this as `std::stable_sort` instead of relying on ADL to find it from the `std::` in the type of `NamedOperand`.


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

https://reviews.llvm.org/D56571