Page MenuHomePhabricator

Compiler crashing when compiling coroutine intrinsics without the fcoroutines-ts option.
AcceptedPublic

Authored by zahiraam on May 20 2021, 1:37 PM.

Details

Summary

Instead of generating an error the compiler is crashing. This patch is to fix this issue.

https://bugs.llvm.org/show_bug.cgi?id=50406

Diff Detail

Event Timeline

zahiraam created this revision.May 20 2021, 1:37 PM
zahiraam requested review of this revision.May 20 2021, 1:37 PM
zahiraam added a reviewer: aaron.ballman.
rnk added inline comments.May 20 2021, 4:40 PM
clang/lib/Basic/Builtins.cpp
70

The existing convention is to mark the builtins with a flag that enables them, see this MS_LANG example. Would that be a reasonable alternative? Maybe one day we will have builtins that don't start with __builtin_coro, so this would be nicer.

zahiraam updated this revision to Diff 347447.Mon, May 24, 10:29 AM
aaron.ballman added inline comments.Mon, May 24, 10:46 AM
clang/include/clang/Basic/Builtins.h
40

Can you add a comment as to what the builtin requires?

clang/lib/Basic/Builtins.cpp
64

I think this should be using & rather than ==, as with the other cases (don't forget to surround the bitwise AND expression with parens to silence warnings).

zahiraam updated this revision to Diff 347481.Mon, May 24, 1:05 PM
aaron.ballman added inline comments.Mon, May 24, 3:29 PM
clang/include/clang/Basic/Builtins.h
40

Sorry, I didn't catch this earlier, but shouldn't this be 0x200? I suspect this will fix the CI failures.

zahiraam updated this revision to Diff 347657.Tue, May 25, 5:54 AM
zahiraam marked 3 inline comments as done.
aaron.ballman accepted this revision.Tue, May 25, 7:20 AM

LGTM, but please wait for a few days in case @rnk or others have a concern. I don't think the CI failures are related to your changes here, btw.

This revision is now accepted and ready to land.Tue, May 25, 7:20 AM

No real feedback, however I note that the bug link you have is one that was resolved in 2009, so I suspect it is completely unrelated.

erichkeane added inline comments.Tue, May 25, 7:47 AM
clang/test/SemaCXX/coroutine-builtins.cpp
8

Another nit/idea to make this a little nicer: If you use the 'bookmark' mechanism for the VerifyDiagnosticConsumer these will be much more reliable.

Basically you put

// #SomeBookmarkName

at the end of the line, and then do:

// expected-error@#SomeBookMarkName {{...
zahiraam edited the summary of this revision. (Show Details)Tue, May 25, 7:49 AM

No real feedback, however I note that the bug link you have is one that was resolved in 2009, so I suspect it is completely unrelated.

Thanks Erich. Yes wrong link. Fixed.

zahiraam updated this revision to Diff 347678.Tue, May 25, 8:08 AM
zahiraam marked an inline comment as done.