Remove stale assert.

[void]

Summary

This is triggered during serialization. The test is for modules, but
will occur for any serialization effort using asm goto.

[jyknight]

clang/test/Modules/Inputs/asm-goto/a.h

5–7

An empty asm string will suffice for the test.

clang/test/Modules/asm-goto.cpp

1 ↗

(On Diff #293916)

New test seems to be failing on the autobuilders (yay pre-submit autobuilders!!) for 2 reasons:

1. C++ function-name mangling on Windows is different.
2. "$0 = callbr" != "%0 = callbr"

[void]

Fix test case.

clang/test/Modules/asm-goto.cpp

1 ↗

(On Diff #293916)

Don't know why I did that. Fixed. :)

[jyknight]

clang/test/Modules/asm-goto.cpp

1 ↗

(On Diff #293916)

Still have problem 1, that @"?foo@@YAHXZ"() != @_Z3foov

Probably simplest to just do
extern "C" {
int foo() {...}
}

to avoid name mangling.

[nickdesaulniers]

I'm super confused between the commit message and initial hunk, that seem to make sense and probably should go in clang-11 if it's not too late, and the additional tests for modules which the commit message does not address. Were these meant to be separate commits, because otherwise it looks like you uploaded unrelated stuff. Are C++20 modules broken with asm goto, or are you just adding additional test coverage for that?

[void]

Fix test.

[void]

In D88195#2293165, @nickdesaulniers wrote:

I'm super confused between the commit message and initial hunk, that seem to make sense and probably should go in clang-11 if it's not too late, and the additional tests for modules which the commit message does not address. Were these meant to be separate commits, because otherwise it looks like you uploaded unrelated stuff. Are C++20 modules broken with asm goto, or are you just adding additional test coverage for that?

The assert only triggers for modules, so yeah modules are broken with "asm goto", but only if asserts are enabled. The official release doesn't have asserts, so I don't know if this counts as a blocker. But it's not a change in semantics, only to remove an assert...

[nickdesaulniers]

In D88195#2293533, @void wrote:

In D88195#2293165, @nickdesaulniers wrote:

I'm super confused between the commit message and initial hunk, that seem to make sense and probably should go in clang-11 if it's not too late, and the additional tests for modules which the commit message does not address. Were these meant to be separate commits, because otherwise it looks like you uploaded unrelated stuff. Are C++20 modules broken with asm goto, or are you just adding additional test coverage for that?

The assert only triggers for modules, so yeah modules are broken with "asm goto", but only if asserts are enabled.

The assert was removed from AST/Stmt.cpp; it doesn't look module related. Wouldn't it be triggered by ANY GCCAsmStmt? (I have patches that use ASM goto w/ outputs on the kernel, let me try an assertions enabled build). It's not obvious to me why the method modified would only trigger for modules.

The official release doesn't have asserts, so I don't know if this counts as a blocker. But it's not a change in semantics, only to remove an assert...

That's fair.

[nickdesaulniers]

In D88195#2293559, @nickdesaulniers wrote:

In D88195#2293533, @void wrote:

In D88195#2293165, @nickdesaulniers wrote:

I'm super confused between the commit message and initial hunk, that seem to make sense and probably should go in clang-11 if it's not too late, and the additional tests for modules which the commit message does not address. Were these meant to be separate commits, because otherwise it looks like you uploaded unrelated stuff. Are C++20 modules broken with asm goto, or are you just adding additional test coverage for that?

The assert only triggers for modules, so yeah modules are broken with "asm goto", but only if asserts are enabled.

The assert was removed from AST/Stmt.cpp; it doesn't look module related. Wouldn't it be triggered by ANY GCCAsmStmt? (I have patches that use ASM goto w/ outputs on the kernel, let me try an assertions enabled build). It's not obvious to me why the method modified would only trigger for modules.

Indeed, I don't trip the assertion in kernel builds using outputs. Is GCCAsmStmt::setOutputsAndInputsAndClobbers only called for modules? If so, why?

[void]

In D88195#2293559, @nickdesaulniers wrote:

In D88195#2293533, @void wrote:

In D88195#2293165, @nickdesaulniers wrote:

I'm super confused between the commit message and initial hunk, that seem to make sense and probably should go in clang-11 if it's not too late, and the additional tests for modules which the commit message does not address. Were these meant to be separate commits, because otherwise it looks like you uploaded unrelated stuff. Are C++20 modules broken with asm goto, or are you just adding additional test coverage for that?

The assert only triggers for modules, so yeah modules are broken with "asm goto", but only if asserts are enabled.

The assert was removed from AST/Stmt.cpp; it doesn't look module related. Wouldn't it be triggered by ANY GCCAsmStmt? (I have patches that use ASM goto w/ outputs on the kernel, let me try an assertions enabled build). It's not obvious to me why the method modified would only trigger for modules.

Yes, but that particular function is only called during serialization reading. It would trigger for any serialization, this just happens to be for modules. I'll edit the commit message to be clearer.

[void]

Clarify commit message.

[nickdesaulniers]

In D88195#2293589, @void wrote:

Clarify commit message.

Phabricator unfortunately won't amend the review description when the commit message is updated; you'll need to manually edit the description via phab's UI for other reviewers to observe the update. :(

[jyknight]

In D88195#2293597, @nickdesaulniers wrote:

In D88195#2293589, @void wrote:

Clarify commit message.

Phabricator unfortunately won't amend the review description when the commit message is updated; you'll need to manually edit the description via phab's UI for other reviewers to observe the update. :(

You can see the description separately, under the "Commits" tab, but yea.
Also, if you use arc diff --verbatim to upload the new review, it'll update the review message at the same time.

--verbatim
    When creating a revision, try to use the working copy commit
    message verbatim, without prompting to edit it. When updating a
    revision, update some fields from the local commit message.

[nickdesaulniers]

In D88195#2293637, @jyknight wrote:

In D88195#2293597, @nickdesaulniers wrote:

In D88195#2293589, @void wrote:

Clarify commit message.

Phabricator unfortunately won't amend the review description when the commit message is updated; you'll need to manually edit the description via phab's UI for other reviewers to observe the update. :(

You can see the description separately, under the "Commits" tab, but yea.
Also, if you use arc diff --verbatim to upload the new review, it'll update the review message at the same time.

--verbatim
    When creating a revision, try to use the working copy commit
    message verbatim, without prompting to edit it. When updating a
    revision, update some fields from the local commit message.

TIL, thanks for that power up. Thanks for the fix, Bill!

clang/lib/AST/Stmt.cpp

void GCCAsmStmt::setOutputsAndInputsAndClobbers(const ASTContext &C,
                                                 unsigned NumInputs,
                                                 unsigned NumLabels,
                                                 StringLiteral **Clobbers,
                                                 unsigned NumClobbers) {
   this->NumOutputs = NumOutputs;
   this->NumInputs = NumInputs;
   this->NumClobbers = NumClobbers;
   this->NumLabels = NumLabels;
-  assert(!(NumOutputs && NumLabels) && "asm goto cannot have outputs");
 
   unsigned NumExprs = NumOutputs + NumInputs + NumLabels;
 
   C.Deallocate(this->Names);
   this->Names = new (C) IdentifierInfo*[NumExprs];
   std::copy(Names, Names + NumExprs, this->Names);
 
   C.Deallocate(this->Exprs);

clang/test/Modules/Inputs/asm-goto/a.h

+int foo(void) {
+  int x;
+
+  asm goto(""
+           : "=r"(x)
+           :
+           :
+           : indirect);
+  x = 42;
+
+indirect:
+  return x;
+}

clang/test/Modules/Inputs/asm-goto/module.modulemap

+module a { header "a.h" export * }

clang/test/Modules/asm-goto.c

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x c -I%S/Inputs/asm-goto -emit-module %S/Inputs/asm-goto/module.modulemap -fmodule-name=a -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x c -I%S/Inputs/asm-goto -emit-llvm -o - %s -fmodule-file=%t/a.pcm | FileCheck %s
+#include "a.h"
+
+// CHECK-LABEL: define i32 @foo(
+// CHECK: callbr {{.*}} "=r,X,{{.*}}"(i8* blockaddress(@foo, %indirect))
+// CHECK-NEXT: to label %asm.fallthrough [label %indirect]
+
+int bar(void) {
+  return foo();
+}