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

Could not parse event_id. Found ASCII control character in <text> #207

Closed
jyn514 opened this issue May 24, 2023 · 9 comments · Fixed by rust-lang/rust#111929 or #208
Closed

Could not parse event_id. Found ASCII control character in <text> #207

jyn514 opened this issue May 24, 2023 · 9 comments · Fixed by rust-lang/rust#111929 or #208
Assignees

Comments

@jyn514
Copy link
Member

jyn514 commented May 24, 2023

I am not sure whether this is a rustc bug or a measureme bug so I'm reporting it here under the theory that it's more likely to be seen by the right people :)

I ran

x clean rustc_middle && MAGIC_EXTRA_RUSTFLAGS='-Zself-profile -Zself-profile-events=default,query-keys' x check --stage 0 rustc_middle

on rust-lang/rust@ba6f5e3. That generated a .mm_profdata file that the measureme tools fail to parse:

; crox rustc_middle-2790234.mm_profdata
Could not parse `event_id`. Found ASCII control character in <text> at 112 in "erase_regions_ty^^impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>)
-> &'b mut DiagnosticBuilder<'a, ()>"

It looks like FnOnce impls are getting an extra newline generated at the end. That seems like a bug we should fix in rustc, but not such a bad bug that it should prevent measureme from generating a report - could we translated it to an ASCII space instead, maybe?

@jyn514
Copy link
Member Author

jyn514 commented May 24, 2023

The relevant code looks like https://github.com/rust-lang/rust/blob/3df55382d46842dcaeff48ea83e0107183550dd6/compiler/rustc_middle/src/ty/print/pretty.rs#L999-L1003, which ... seems fine?

cc @compiler-errors, I see you were the last one to touch that pretty-printing code.

@compiler-errors
Copy link
Member

If this is the consequence of the pretty printing infrastructure, then I'd expect we'd see in in more places than just measureme.

@compiler-errors
Copy link
Member

So it wasn't rustc_middle::ty::pretty but it was rustc_ast_pretty's fault, since the impl Trait in the examples given by jyn were APITs.

@michaelwoerister
Copy link
Member

I don't remember the details of the encoding but I do remember that it assigns special meaning to certain ASCII control characters in order to be space efficient. I doubt, however, that newlines are affected by that.

I can look into relaxing the checks here, but unless we consider it high priority, it will probably be a while before I get to that.

@bjorn3
Copy link
Member

bjorn3 commented May 25, 2023

I doubt, however, that newlines are affected by that.

Yes they are. I did actually run into this while first wiring up cranelift's profiling system to -Zself-profile in cg_clif. Initially I just replaced all \n with | to workaround this, but also changed cranelift's profiling system to allow a proper integration later.

@michaelwoerister
Copy link
Member

I doubt, however, that newlines are affected by that.

What I mean is: I doubt that newlines really have a reserved meaning in the encoding -- so it's probably fine to relax the requirements here 🙂

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 25, 2023
…=wesleywiser

Don't print newlines in APITs

This is kind of a hack, but it gets the job done because the only "special" formatting that (afaict) `rustc_ast_pretty` does is break with newlines sometimes.

Fixes rust-lang/measureme#207
@bjorn3
Copy link
Member

bjorn3 commented May 26, 2023

Should this issue be kept open for when someone does need a newline in an argument?

@andjo403
Copy link
Contributor

@michaelwoerister made a suggestion last night but forgot to post the PR. posted it now.
if you have something better in mind I can close it

@bjorn3
Copy link
Member

bjorn3 commented May 26, 2023

It got closed again by a subtree sync...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants