Page MenuHomePhabricator

Remove stale assert.
ClosedPublic

Authored by void on Wed, Sep 23, 7:10 PM.

Details

Summary

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

Diff Detail

Event Timeline

void created this revision.Wed, Sep 23, 7:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 23, 7:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
void requested review of this revision.Wed, Sep 23, 7:10 PM
jyknight added inline comments.Wed, Sep 23, 8:08 PM
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 updated this revision to Diff 293938.Wed, Sep 23, 9:50 PM

Fix test case.

clang/test/Modules/asm-goto.cpp
1 ↗(On Diff #293916)

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

jyknight added inline comments.Thu, Sep 24, 7:31 AM
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.

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 updated this revision to Diff 294141.Thu, Sep 24, 12:59 PM

Fix test.

void added a comment.Thu, Sep 24, 1:01 PM

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...

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.

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 added a comment.Thu, Sep 24, 1:28 PM

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 updated this revision to Diff 294150.Thu, Sep 24, 1:30 PM

Clarify commit message.

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 accepted this revision.Thu, Sep 24, 1:53 PM

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.
This revision is now accepted and ready to land.Thu, Sep 24, 1:53 PM
void edited the summary of this revision. (Show Details)Thu, Sep 24, 1:55 PM
nickdesaulniers accepted this revision.Thu, Sep 24, 1:58 PM

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!

This revision was automatically updated to reflect the committed changes.