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

Add CL and CMD into to pdb debug info #113492

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Conversation

nebulark
Copy link
Contributor

@nebulark nebulark commented Jul 8, 2023

Partial fix for #96475

The Arg0 and CommandLineArgs of the MCTargetOptions cpp class are not set within

extern "C" LLVMTargetMachineRef LLVMRustCreateTargetMachine(

This causes LLVM to not neither output any compiler path (cl) nor the arguments that were used when invoking it (cmd) in the PDB file.

This fix adds the missing information to the target machine so LLVM can use it.

@rustbot
Copy link
Collaborator

rustbot commented Jul 8, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @b-naber (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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. labels Jul 8, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nebulark
Copy link
Contributor Author

nebulark commented Jul 9, 2023

I really have trouble understanding what caused the check to fail here. The thing I most likely see that could have caused it is adding extra members to rustc_session::Session and rustc_interface::interface::Config, to be able to pass the expanded through some functions.
But I really don't understand why this occurs.

Note that I am developing on Windows.

@jyn514
Copy link
Member

jyn514 commented Jul 9, 2023

this is an interesting error. i'm going to go through it step-by-step to see if i can help you figure out how i found the root cause :)

the first thing i did was look at the rust-log-analyzer output and decide it was too long, so i looked at the original github actions workflow in case RLA got confused. that shows the following error at the end (along with ~16 or so others):

error[E0277]: the trait bound `syn::ItemImpl: quote::ToTokens` is not satisfied
Error:   --> tests/ui/auxiliary/proc_macro_attr.rs:96:23
   |
LL |     TokenStream::from(quote!(#item))
   |                       ^^^^^^^^^^^^^
   |                       |
   |                       the trait `quote::ToTokens` is not implemented for `syn::ItemImpl`
   |                       required by a bound introduced by this call
   |
   = help: the following other types implement trait `quote::ToTokens`:
             &'a T
             &'a mut T
             bool
             char
             f32
             f64
             i128
             i16
           and 23 others
   = note: this error originates in the macro `quote` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 6 previous errors

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.


FAILURES:
    tests/ui/empty_line_after_doc_comments.rs
    tests/ui/empty_line_after_outer_attribute.rs
    tests/ui/needless_arbitrary_self_type_unfixable.rs

test result: FAIL. 3 tests failed, 868 tests passed, 4 ignored, 0 filtered out
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: tests failed

Location:
    /cargo/registry/src/index.crates.io-6f17d22bba15001f/ui_test-0.11.6/src/lib.rs:308:13', src/tools/clippy/tests/compile-test.rs:99:6
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: test failed, to rerun pass `--test compile-test`
Build completed unsuccessfully in 0:02:40

there are some important clues here. first, the panic happens in src/tools/clippy/tests/compile-test.rs:99:6. that tells me that this is

  • a compiletest testsuite
  • for clippy
  • and that rustc and clippy compiled successfully, and this only broke when running clippy on a test. so your hypothesis about new struct fields breaking the code seems unlikely, since we compiled clippy without errors.

now i know how to replicate the failure locally: x test src/tools/clippy. that takes a while to run, but unfortunately does not show me the error from CI, it succeeds instead.

there are more clues here.

  • the error happens in tests/ui/auxiliary/proc_macro_attr.rs:39:26. auxiliary is what the compiletest test suite uses instead of cargo; proc_macro_attr is a dependency that will be used in the test. auxiliary crates are always supposed to build, so the problem here is that we're getting the "trait bound for syn::ItemImpl: quote::ToTokens not satisfied" error where we weren't before.
  • all three tests mention proc_macro_attr so we know there is only one thing that is breaking, all the failures have the same cause.

at this point i look at the diff in your PR and notice you didn't modify clippy at all, so i think it's likely this is due to breakage in either syn or quote. i run git log --stat on your PR which shows that you didn't update Cargo.lock either. i now suspect that clippy's fork of compile-test does a bad job of pinning dependency versions. looking at the --extern args it passes, i see "--extern" "quote=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/ui_test/x86_64-unknown-linux-gnu/debug/deps/libquote-36054bcec486f3c0.rmeta" which doesn't tell me which version it used.

https://crates.io/crates/syn shows that the latest release was 2.0.24, 8 hours ago, just after CI ran. https://crates.io/crates/quote was last released 10 days ago. the timing for syn is suspicious, which makes me think there was some bug fix published just after the workflow ran.

i think this is upstream breakage in syn that was already fixed last night after the workflow ran, so i retriggered CI in hopes that will used the updated version on crates.io.

@jyn514
Copy link
Member

jyn514 commented Jul 9, 2023

yeah @oli-obk i don't think this is stable because it only uses Cargo.toml without allowing clippy to specify a lockfile

dependencies_crate_manifest_path: Some("clippy_test_deps/Cargo.toml".into()),

@jyn514
Copy link
Member

jyn514 commented Jul 9, 2023

hmm. ui_test is using --manifest-path though, which i expect would discover that it's in a workspace and use the top-level Cargo.lock from rust-lang/rust? it doesn't really have enough logging for me to tell ....
https://github.com/oli-obk/ui_test/blob/5f391d3f734c16f26e1220737a2a923b5190fdb5/src/dependencies.rs#L51-L53

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@nebulark
Copy link
Contributor Author

@rustbot author
going to fix the memory leak issue, probably does not make sense to review before then

@rustbot rustbot 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 Jul 18, 2023
@bors

This comment was marked as resolved.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jul 20, 2023
@nebulark nebulark force-pushed the pr_96475 branch 2 times, most recently from 6a54a58 to 155759c Compare July 26, 2023 11:13
@rust-log-analyzer

This comment has been minimized.

@nebulark
Copy link
Contributor Author

Now storing the cpp args in an object instead of global. I put it into ModuleLlvm so it should outlive the targetmachine that has reference to it, and allows me to dispose of it to avoid memory leaks.
Also added a windows only test using findstr.

@nebulark nebulark marked this pull request as ready for review July 26, 2023 14:50
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 6, 2023
@bors
Copy link
Contributor

bors commented Sep 6, 2023

⌛ Testing commit 7509c34 with merge cb956d913092ea9e2c7f25f11969512d77563758...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 6, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 6, 2023
@petrochenkov petrochenkov 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 Sep 7, 2023
Set Arg0 and CommandLineArgs in MCTargetoptions so LLVM outputs correct CL and CMD in LF_DEBUGINFO instead of empty/invalid values.
@nebulark
Copy link
Contributor Author

nebulark commented Sep 7, 2023

Changed unit test to use to run only on windows-msvc instead for all windows variants.

@rustbot review

@rustbot rustbot 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 Sep 7, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 8, 2023

📌 Commit 4cdc633 has been approved by petrochenkov

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 Sep 8, 2023
@bors
Copy link
Contributor

bors commented Sep 8, 2023

⌛ Testing commit 4cdc633 with merge 9be4eac...

@bors
Copy link
Contributor

bors commented Sep 8, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 9be4eac to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 8, 2023
@bors bors merged commit 9be4eac into rust-lang:master Sep 8, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 8, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9be4eac): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [1.0%, 1.5%] 2
Regressions ❌
(secondary)
2.3% [1.4%, 3.0%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-3.9%, -1.0%] 3
All ❌✅ (primary) 1.3% [1.0%, 1.5%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.7% [2.6%, 2.8%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 629.137s -> 629.695s (0.09%)
Artifact size: 318.13 MiB -> 318.12 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.