Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the BUG_REPORT_URL configurable by tools #110989

Merged
merged 2 commits into from
May 6, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 29, 2023

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc @rust-lang/clippy

Fixes #109486.

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2023

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Manishearth
Copy link
Member

Manishearth commented Apr 29, 2023

Doing it in the subtree is fine for clippy!

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

I see the current hook also includes clippy's version, which can be different from rustc's version - I'll see if I can add a custom callback to the hook so clippy can keep that info.

format!("Clippy version: {version_info}").into(),

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@jyn514
Copy link
Member Author

jyn514 commented Apr 29, 2023

cc @rust-lang/rustdoc as well but the changes to rustdoc are very minor

Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, rustdoc changes look fine.

compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved
src/tools/miri/src/bin/miri.rs Outdated Show resolved Hide resolved
compiler/rustc_driver_impl/src/lib.rs Outdated Show resolved Hide resolved
src/tools/miri/src/bin/miri.rs Outdated Show resolved Hide resolved
@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2023
This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

- Switch clippy to the new hook

  This also adds a `extra_info` callback so clippy can include its own version number, which differs
  from rustc's.

- Call `install_ice_hook` in rustfmt
@jyn514 jyn514 force-pushed the bug-report-url branch from d54a9ee to 2469afe Compare May 2, 2023 02:44
@rustbot
Copy link
Collaborator

rustbot commented May 2, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2023
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me. r=me once tool maintainers are satisfied.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clippy changes look awesome!

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on the Miri changes

@bors
Copy link
Contributor

bors commented May 3, 2023

📌 Commit 2469afe has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2023
Manishearth added a commit to Manishearth/rust that referenced this pull request May 4, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc `@rust-lang/clippy`

Fixes rust-lang#109486.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 4, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ``@rust-lang/clippy``

Fixes rust-lang#109486.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 6, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ```@rust-lang/clippy```

Fixes rust-lang#109486.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#110577 (Use fulfillment to check `Drop` impl compatibility)
 - rust-lang#110610 (Add Terminator conversion from MIR to SMIR, part #1)
 - rust-lang#110985 (Fix spans in LLVM-generated inline asm errors)
 - rust-lang#110989 (Make the BUG_REPORT_URL configurable by tools )
 - rust-lang#111167 (debuginfo: split method declaration and definition)
 - rust-lang#111230 (add hint for =< as <=)
 - rust-lang#111279 (More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8ec84dd into rust-lang:master May 6, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 6, 2023
@uweigand
Copy link
Contributor

The new tests/rustdoc-ui/ice-bug-report-url.rs test fails for me on s390x-unknown-linux-gnu due to the following difference in stderr output:

---- [ui] tests/rustdoc-ui/ice-bug-report-url.rs stdout ----
diff of stderr:

5          |          ^ expected one of `->`, `where`, or `{`
6
7       thread panicked at 'aborting due to `-Z treat-err-as-bug`'
-       stack backtrace:
-       error: the compiler unexpectedly panicked. this is a bug.
+       stack backtrace:                             error: the compiler unexpectedly panicked. this is a bug.
10
11      note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-rustdoc&template=ice.md
12


The actual stderr differed from the expected stderr.

Not sure why this happens, could this be some issue with the normalize-stderr-test rules?

The complete (unnormalized) output is:

error: expected one of `->`, `where`, or `{`, found `<eof>`
  --> /home/uweigand/rust/tests/rustdoc-ui/ice-bug-report-url.rs:13:10
   |
LL | fn wrong()
   |          ^ expected one of `->`, `where`, or `{`

thread 'rustc' panicked at 'aborting due to `-Z treat-err-as-bug=1`', compiler/rustc_errors/src/lib.rs:1711:30
stack backtrace:
   0:      0x3ffec4992a2 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h413b3b0f9de3326b
   1:      0x3ffec56739e - core::fmt::write::h84c7cedca7777db7
   2:      0x3ffec4ade78 - std::panicking::default_hook::h89c76c4bb766adfe
   3:      0x3ffed577204 - rustc_driver_impl[b094fb3383cea06f]::install_ice_hook::{closure#0}
   4:      0x3ffec4af758 - std::panicking::rust_panic_with_hook::h3b0cf6703ee96eed
   5:      0x3ffec499854 - std::panicking::begin_panic_handler::{{closure}}::h36b73881ffb76e47
   6:      0x3ffec4995d0 - std::sys_common::backtrace::__rust_end_short_backtrace::h9f7578507492eb4f
   7:      0x3ffec4af2ae - rust_begin_unwind
   8:      0x3ffec47058c - core::panicking::panic_fmt::h9c206364a104a065
   9:      0x3fff4944962 - <rustc_errors[23e277f92d39e05]::HandlerInner>::emit_diagnostic::{closure#2}
  10:      0x3fff4943c28 - <rustc_errors[23e277f92d39e05]::HandlerInner>::emit_diagnostic
  11:      0x3fff49b3be6 - <rustc_span[fa50c18a7876c42a]::ErrorGuaranteed as rustc_errors[23e277f92d39e05]::diagnostic_builder::EmissionGuarantee>::diagnostic_builder_emit_producing_guarantee
  12:      0x3ffed6acba2 - <rustc_interface[6001cdef06e51c04]::queries::Queries>::pre_configure
  13:      0x3ffed6ad5f8 - <rustc_interface[6001cdef06e51c04]::queries::Queries>::global_ctxt
  14:      0x2aa0048f836 - <rustc_interface[6001cdef06e51c04]::interface::Compiler>::enter::<rustdoc[9110c6921b1a8d2a]::main_args::{closure#1}::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>
  15:      0x2aa001f4318 - rustc_span[fa50c18a7876c42a]::set_source_map::<core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>, rustc_interface[6001cdef06e51c04]::interface::run_compiler<core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>, rustdoc[9110c6921b1a8d2a]::main_args::{closure#1}>::{closure#0}::{closure#0}>
  16:      0x2aa004915d8 - <scoped_tls[de53d4204225384f]::ScopedKey<rustc_span[fa50c18a7876c42a]::SessionGlobals>>::set::<rustc_interface[6001cdef06e51c04]::interface::run_compiler<core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>, rustdoc[9110c6921b1a8d2a]::main_args::{closure#1}>::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>
  17:      0x2aa000f1cf8 - std[4f5ed56b8f53302d]::sys_common::backtrace::__rust_begin_short_backtrace::<rustc_interface[6001cdef06e51c04]::util::run_in_thread_pool_with_globals<rustc_interface[6001cdef06e51c04]::interface::run_compiler<core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>, rustdoc[9110c6921b1a8d2a]::main_args::{closure#1}>::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>
  18:      0x2aa000f35ae - std[4f5ed56b8f53302d]::panicking::try::do_call::<core[1e85ed23d5535b8b]::panic::unwind_safe::AssertUnwindSafe<<std[4f5ed56b8f53302d]::thread::Builder>::spawn_unchecked_<rustc_interface[6001cdef06e51c04]::util::run_in_thread_pool_with_globals<rustc_interface[6001cdef06e51c04]::interface::run_compiler<core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>, rustdoc[9110c6921b1a8d2a]::main_args::{closure#1}>::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>::{closure#1}::{closure#0}>, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>
  19:      0x2aa0014c090 - __rust_try.llvm.11562562457469458502
  20:      0x2aa003315aa - <<std[4f5ed56b8f53302d]::thread::Builder>::spawn_unchecked_<rustc_interface[6001cdef06e51c04]::util::run_in_thread_pool_with_globals<rustc_interface[6001cdef06e51c04]::interface::run_compiler<core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>, rustdoc[9110c6921b1a8d2a]::main_args::{closure#1}>::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[1e85ed23d5535b8b]::result::Result<(), rustc_span[fa50c18a7876c42a]::ErrorGuaranteed>>::{closure#1} as core[1e85ed23d5535b8b]::ops::function::FnOnce<()>>::call_once::{shim:vtable#0}
  21:      0x3ffec4fe0b0 - std::sys::unix::thread::Thread::new::thread_start::ha67c2f2eda1f7e3b
  22:      0x3ffec09f5c8 - start_thread
                               at /usr/src/debug/glibc-2.36-9.1.ibm.fc37.s390x/nptl/pthread_create.c:442:8
  23:      0x3ffec11b51e - <unknown>
                               at /usr/src/debug/glibc-2.36-9.1.ibm.fc37.s390x/misc/../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:66
  24:                0x0 - <unknown>

error: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-rustdoc&template=ice.md

note: rustc 1.71.0-dev running on s390x-unknown-linux-gnu

note: compiler flags: -Z threads=1 -C codegen-units=1 -Z ui-testing -Z simulate-remapped-rust-src-base=/rustc/FAKE_PREFIX -Z translate-remapped-path-to-local-path=no -Z deduplicate-diagnostics=no -C strip=debuginfo -C debuginfo=0 -Z treat-err-as-bug

query stack during panic:
end of query stack

@jyn514 jyn514 deleted the bug-report-url branch May 10, 2023 15:58
@jyn514
Copy link
Member Author

jyn514 commented May 10, 2023

@uweigand that test should not be platform-specific. I don't know why it would fail only on s390x.

@jyn514
Copy link
Member Author

jyn514 commented May 10, 2023

By chance, does s390x have \r\n line endings?

@jyn514
Copy link
Member Author

jyn514 commented May 10, 2023

This is the relevant normalization: // normalize-stderr-test "\s*\d{1,}: .*\n" -> ""

it should remove the line completely, but somehow it's being replaced with a space instead.

@uweigand
Copy link
Contributor

By chance, does s390x have \r\n line endings?

No, it's just \n like on other Linux platforms.

it should remove the line completely, but somehow it's being replaced with a space instead.

It looks like it is removing the line ending of the stack backtrace: line for some reason, merging the next line into it?

-       stack backtrace:
-       error: the compiler unexpectedly panicked. this is a bug.
+       stack backtrace:                             error: the compiler unexpectedly panicked. this is a bug.

Not sure how that could happen ...

@uweigand
Copy link
Contributor

Ah, I think I see what the problem is. The \s in a Rust regex matches any whitespace, including newline. Therefore, when looking at these two lines:

// normalize-stderr-test "\s*\d{1,}: .*\n" -> ""
// normalize-stderr-test "\s at .*\n" -> ""

the first one will remove (multiple) newlines before the line (because of the \s*) and one newline after the line (because of the \n). However, the second one has just a single \s, which due to its position relative to the at can never match a newline, just a space.

You can see that when removing the second of those normalize-stderr-test, the output is:

thread panicked at 'aborting due to `-Z treat-err-as-bug`'
stack backtrace:                               at /usr/src/debug/glibc-2.36-9.1.ibm.fc37.s390x/nptl/pthread_create.c:442:8                               at /usr/src/debug/glibc-2.36-9.1.ibm.fc37.s390x/misc/../sysdeps/unix/sysv/linux/s390/s390-64/clone.S:66
error: the compiler unexpectedly panicked. this is a bug.

So if we add the second normalize-stderr-test back, it will merge remove everything from two spaces before the first at up to and including the newline at the end, which results in the merged output we see.

Note that this only happens if the last line in the backtrace is one of lines with an address - if it is one of the lines with an at, the result will be different. This would explain why the effect is platform-specific.

(We see that last line with address 0 on s390x - that's probably a bug in our backtrace implementation that should be fixed, but really those regexes should handle that case too ...)

I think the whitespace in the backtrace output is actually always just plain spaces, so maybe we can simply use:

// normalize-stderr-test " +\d{1,}: .*\n" -> ""
// normalize-stderr-test " + at .*\n" -> ""

This works for me (we also need to add one blank line to the stderr output that isn't removed any more now).

@jyn514
Copy link
Member Author

jyn514 commented May 10, 2023

sounds great! @uweigand could you make a PR with that change, please? :)

@uweigand
Copy link
Contributor

sounds great! @uweigand could you make a PR with that change, please? :)

#111439

flip1995 pushed a commit to flip1995/rust that referenced this pull request May 20, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ````@rust-lang/clippy````

Fixes rust-lang#109486.
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 20, 2023
Make the BUG_REPORT_URL configurable by tools

This greatly simplifies how hard it is to set a custom bug report url; previously tools had to copy
the entire hook implementation.

I haven't changed clippy in case they want to make the change upstream instead of the subtree, but
I'm happy to do so here if the maintainers want - cc ````@rust-lang/clippy````

Fixes rust-lang#109486.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-clippy Area: Clippy A-miri Area: The miri tool A-rustfmt Area: Rustfmt S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the BUG_REPORT_URL configurable for tools