This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix crash upon stray coloncolon token in C2x mode
ClosedPublic

Authored by SuibianP on Sep 2 2022, 9:07 PM.

Details

Summary

The parser assumes that the lexer never emits coloncolon token for C code, but this assumption no longer holds in C2x attribute namespaces. As a result, stray coloncolon tokens out of attributes cause assertion failures and hangs in release build, which this patch tries to handle.

Crash input minimal example: T n::v

Diff Detail

Event Timeline

SuibianP created this revision.Sep 2 2022, 9:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 9:07 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
SuibianP requested review of this revision.Sep 2 2022, 9:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 9:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
SuibianP edited the summary of this revision. (Show Details)Sep 2 2022, 9:11 PM
SuibianP added reviewers: aaron.ballman, rsmith.
inclyc added a subscriber: inclyc.Sep 2 2022, 9:37 PM
inclyc added inline comments.Sep 2 2022, 10:43 PM
clang/lib/Parse/ParseDecl.cpp
5340

Maybe we can make a new error diagnostic definition and fire that here?

clang/test/Parser/c2x-attributes.c
146

Could we improve this diagnostic message?

expected ';' after top level declarator}
tbaeder added inline comments.
clang/include/clang/Parse/Parser.h
863
aaron.ballman accepted this revision.Sep 6 2022, 6:29 AM

Good catch that this was crashing! LGTM aside from the nit pointed out by @tbaeder. Can you also add a release note for the fix? Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

clang/lib/Parse/ParseDecl.cpp
5340

I don't think we should -- this function is used to query whether something is a declaration specifier or not; it'd be surprising to issue a diagnostic from that kind of interface.

clang/test/Parser/c2x-attributes.c
146

That might be possible, but it should happen as a separate patch. That said, I'm not certain how much improvement there is to be had there, especially in C mode. It really does look like the user is trying to declare a variable named n and got the wrong kind of colon.

This revision is now accepted and ready to land.Sep 6 2022, 6:29 AM
SuibianP updated this revision to Diff 468488.Oct 18 2022, 5:00 AM

Remove superfluous parentheses

SuibianP updated this revision to Diff 468509.Oct 18 2022, 6:01 AM

Add release note entry

SuibianP marked an inline comment as done.Oct 18 2022, 6:07 AM

@aaron.ballman Thanks for the guidance! I have rectified the parentheses and appended to the release notes. Please feel free to point out any additional issues with the Differential.

If all is good, I would like to have the patch attributed to Jialun Hu <jialun_hu@apple.com>.

This revision was landed with ongoing or failed builds.Oct 18 2022, 6:58 AM
This revision was automatically updated to reflect the committed changes.

@aaron.ballman Thanks for the guidance! I have rectified the parentheses and appended to the release notes. Please feel free to point out any additional issues with the Differential.

If all is good, I would like to have the patch attributed to Jialun Hu <jialun_hu@apple.com>.

Thanks @SuibianP ! I've committed this for you.