break rust 💥 #2086

bugaevc · 2023-04-03T16:17:03Z

Since #1996 has received positive feedback, here's a tentative implementation 😄

TODO:

Better / funnier error message
- Suggestions welcome!
- ...or is the current one fine?
- It would be nice to make it a little bit more explicit that this is not really an ICE, there is no bug to report, etc.
  - rustc says: "the compiler expectedly panicked. this is a feature."
Fix/verify Changelog entries
- I've no idea how to do GNU-style changelogs! I've read this doc and skimmed through some other gccrs commits, and tried to write some entries based on that
- glibc people have accepted my patches without Changelog entries; are they really required?
Try to make the error point at the whole statement and not just the name
- Or... is it worth it?
Tests!
- make check-rust seems to pass locally, that's a start
- Need to test that break rust and break gcc still compile just fine if rust/gcc is actually in scope
- Need to test that break (rust) still causes the old error
- How do I even test for something that is supposed to ICE?
clang-format is angry with me, need to appease it

bugaevc · 2023-04-04T09:07:29Z

There are some tests now 🙂

How do I even test for something that is supposed to ICE?

There's apparently dg-ice made exactly for this case!

clang-format is angry with me, need to appease it

clang-format should be happy now; but I may have to revert some of the changes in case we decide on a longer error message text

philberty

I think its looking ok just not sure we need the the funny_error flag

gcc/rust/resolve/rust-ast-resolve-expr.cc

philberty · 2023-04-05T09:28:13Z

Also just for the record we definitely need this critical change :D

CohenArthur

The tests look good, as well as the implementation :) nice work! Once everything is approved we'll probably ask you to rebase your commits into one, but that can be done later. Thank you for working on this!

gcc/rust/resolve/rust-ast-resolve-expr.cc

CohenArthur · 2023-04-06T09:16:58Z

Since #1996 has received positive feedback, here's a tentative implementation smile

TODO:

* [ ]  Better / funnier error message
  
  * Suggestions welcome!
  * ...or is the current one fine?
  * It would be nice to make it a little bit more explicit that this is not _really_ an ICE, there is no bug to report, etc.

Submitted as a review suggestion :)

    * rustc says: "the compiler expectedly panicked. this is a feature."

* [ ]  Fix/verify Changelog entries
  
  * I've no idea how to do GNU-style changelogs! I've read [this doc](https://www.gnu.org/prep/standards/html_node/Change-Logs.html) and skimmed through some other gccrs commits, and tried to write some entries based on that
  * glibc people have accepted my patches without Changelog entries; are they really required?

Yes, we do have to write Changelog entries for GCC. You can run contrib/gcc-git-customization.sh to get new git subcommands, such as git gcc-commit-mklog which will produce a Changelog skeleton in your commit's body. You can use it like a regular git commit, so you can do things like git gcc-commit-mklog --amend to fix your existing commits.

* [ ]  Try to make the error point at the whole statement and not just the name

  * Or... is it worth it?

Complicated and not worth it imo :) this is already nice and fun haha.

* [x]  Tests!
  
  * `make check-rust` seems to pass locally, that's a start
  * Need to test that `break rust` and `break gcc` still compile just fine if `rust`/`gcc` is actually in scope
  * Need to test that `break (rust)` still causes the old error
  * How do I even test for something that is supposed to ICE?

* [x]  clang-format is angry with me, need to appease it

bugaevc · 2023-04-06T13:59:46Z

Submitted as a review suggestion :)

And implemented 🙂

Yes, we do have to write Changelog entries for GCC. You can run contrib/gcc-git-customization.sh to get new git subcommands, such as git gcc-commit-mklog which will produce a Changelog skeleton in your commit's body. You can use it like a regular git commit, so you can do things like git gcc-commit-mklog --amend to fix your existing commits.

Does that mean that my existing changelongs are not fine? Any specific things for me to fix?

CohenArthur · 2023-04-07T06:27:00Z

Does that mean that my existing changelongs are not fine? Any specific things for me to fix?

They're great! I hadn't checked them yet haha, I thought you hadn't written any and was looking for help on how to proceed. The ones you wrote are really good

philberty

LGTM i think i can live with a flag for this :D

bugaevc · 2023-04-13T12:28:11Z

I think this still needs more comments in the source code (and not just on GH):

What we are doing (reporting a funny error, but only if this unresolved identifier is rust or gcc and is immediately inside a break expression) -- this would answer why we need a flag
Why we are doing it (why this particular Easter egg at all? why it must be an ICE and not just an error?)

philberty · 2023-04-13T16:18:32Z

@bugaevc are you going to add another commit with more comments?

bugaevc · 2023-04-13T16:34:40Z

I am; but before I do that, a question: should I stash all the commits into one, or are multiple commits fine?

CohenArthur · 2023-04-14T10:12:01Z

I am; but before I do that, a question: should I stash all the commits into one, or are multiple commits fine?

having multiple commits is all good (and preferred!) as long as they all build :) I've checked and all your 4 commits build (we'll soon have more CI for that) so you're fine. Feel free to add the documentation in a new commit on top of these 4 :)

We're going to introduce AST::Kind::IDENTIFIER next, and with the default C-style enum member scoping, this would cause name clashes. Instead, convert AST::Kind into an enum class, so that its members are properly namespaced. gcc/rust/ChangeLog: * ast/rust-ast.h (Kind): Convert into a C++ enum class * expand/rust-macro-builtins.cc: Adapt to the change Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

...and return it from IdentifierExpr::get_ast_kind (). This enables other code to dynamically test whether an expression is in fact an IdentifierExpr. gcc/rust/ChangeLog: * ast/rust-ast.h: Add AST::Kind::IDENTIFIER Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

...and thread it through the constructors and the ResolveExpr::go () method. This will be used for implementing the "break rust" Easter egg. gcc/rust/ChangeLog: * resolve/rust-ast-resolve-expr.h, resolve/rust-ast-resolve-expr.cc: Add ResolveExpr::funny_error Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

When we encounter a "break rust" statement, emit a funny error message and intentionally cause an ICE. This matches the corresponding Easter egg in rustc. As a GNU extension, "break gcc" is also supported. The conditions for this to happen are: * The break expression must be literally "rust" or "gcc". For instance, "break (rust)" will not trigger the Easter egg. * The name ("rust" or "gcc") must not be in scope; if it is, no error is emitted, and the compilation proceeds as usual. In other words, this only affects how GCC diagnoses programs that would fail to compile anyway. Note that this is different from the conditions under which rustc emits its ICE. For rustc, it matters whether or not the "break" is inside a loop, and for us it matters whether or not the name resolves. The end result should be the same anyway: valid programs continue to compile, and typing in fn main() { break rust; } triggers a funny ICE. Closes Rust-GCC#1996 gcc/rust/ChangeLog: * resolve/rust-ast-resolve-expr.cc: Add "break rust" Easter egg gcc/testsuite/ChangeLog: * lib/prune.exp (prune_ices): Also prune "You have broken GCC Rust. This is a feature." * rust/compile/break-rust1.rs: New test * rust/compile/break-rust2.rs: New test * rust/compile/break-rust3.rs: New test Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

bugaevc · 2023-04-14T15:04:25Z

Wrote a comment (several of them actually). Please tell me if these are comprehensive enough (and please point out any grammar / spelling / other mistakes) 🙂. I opted to fold this into the last commit though, and slightly expanded the commit message.

Assuming this passes CI, it should be good to go from my side 🚀

CohenArthur

Everything looks good to me, thank you @bugaevc !

bugaevc · 2023-04-17T09:48:28Z

Thank you for guiding me! And please don't forget to mention this important feature in the next progress report 😃

Accordingly also adjust #2086 "break rust 💥" code, to avoid: [...]/source-gcc/gcc/rust/resolve/rust-ast-resolve-expr.cc: In member function ‘virtual void Rust::Resolver::ResolveExpr::visit(Rust::AST::IdentifierExpr&)’: [...]/source-gcc/gcc/rust/resolve/rust-ast-resolve-expr.cc:164:42: error: invalid conversion from ‘void (*)(diagnostic_context*, diagnostic_info*, diagnostic_t)’ to ‘diagnostic_finalizer_fn’ {aka ‘void (*)(diagnostic_context*, con iagnostic_info*, diagnostic_t)’} [-fpermissive] 164 | diagnostic_finalizer (global_dc) = funny_ice_finalizer; | ^~~~~~~~~~~~~~~~~~~ | | | void (*)(diagnostic_context*, diagnostic_info*, diagnostic_t)

eli-schwartz · 2024-04-07T23:46:53Z

Fascinatingly, it appears the german translators have gone on strike: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114629

bugaevc force-pushed the break-rust branch 2 times, most recently from 32c568d to 8657790 Compare April 4, 2023 09:04

philberty requested review from CohenArthur and philberty April 5, 2023 09:25

philberty added the diagnostic diagnostic static analysis label Apr 5, 2023

philberty reviewed Apr 5, 2023

View reviewed changes

gcc/rust/resolve/rust-ast-resolve-expr.cc Show resolved Hide resolved

CohenArthur approved these changes Apr 6, 2023

View reviewed changes

gcc/rust/resolve/rust-ast-resolve-expr.cc Outdated Show resolved Hide resolved

bugaevc force-pushed the break-rust branch from 8657790 to d85837d Compare April 6, 2023 13:57

bugaevc force-pushed the break-rust branch 2 times, most recently from 8e64c72 to d19c019 Compare April 6, 2023 14:11

philberty approved these changes Apr 13, 2023

View reviewed changes

philberty force-pushed the break-rust branch from d19c019 to 8ba1251 Compare April 13, 2023 16:17

bugaevc added 3 commits April 14, 2023 17:15

bugaevc force-pushed the break-rust branch 2 times, most recently from acfccf3 to 4dc555b Compare April 14, 2023 15:02

bugaevc force-pushed the break-rust branch from 4dc555b to 32c745f Compare April 14, 2023 15:04

CohenArthur approved these changes Apr 17, 2023

View reviewed changes

CohenArthur added this pull request to the merge queue Apr 17, 2023

Merged via the queue into Rust-GCC:master with commit e7ea5fb Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

break rust 💥 #2086

break rust 💥 #2086

bugaevc commented Apr 3, 2023 •

edited

Loading

bugaevc commented Apr 4, 2023

philberty left a comment

philberty commented Apr 5, 2023

CohenArthur left a comment

CohenArthur commented Apr 6, 2023

bugaevc commented Apr 6, 2023

CohenArthur commented Apr 7, 2023

philberty left a comment

bugaevc commented Apr 13, 2023

philberty commented Apr 13, 2023

bugaevc commented Apr 13, 2023

CohenArthur commented Apr 14, 2023

bugaevc commented Apr 14, 2023

CohenArthur left a comment

bugaevc commented Apr 17, 2023

eli-schwartz commented Apr 7, 2024

break rust 💥 #2086

break rust 💥 #2086

Conversation

bugaevc commented Apr 3, 2023 • edited Loading

bugaevc commented Apr 4, 2023

philberty left a comment

Choose a reason for hiding this comment

philberty commented Apr 5, 2023

CohenArthur left a comment

Choose a reason for hiding this comment

CohenArthur commented Apr 6, 2023

bugaevc commented Apr 6, 2023

CohenArthur commented Apr 7, 2023

philberty left a comment

Choose a reason for hiding this comment

bugaevc commented Apr 13, 2023

philberty commented Apr 13, 2023

bugaevc commented Apr 13, 2023

CohenArthur commented Apr 14, 2023

bugaevc commented Apr 14, 2023

CohenArthur left a comment

Choose a reason for hiding this comment

bugaevc commented Apr 17, 2023

eli-schwartz commented Apr 7, 2024

bugaevc commented Apr 3, 2023 •

edited

Loading