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 option to not demangle symbols #607

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChrisDenton
Copy link
Member

I've been playing with various backtrace related things in std, one of which is skipping demangling. And I thought this might be kinda useful more generally? Even if you want symbolization, you may want to bring your own demangler without paying the cost of the built-in one. Maybe this is a too niche scenario though 🤷.

So this is a new API for consideration. It simply adds a new RawSymbolName type. I considered just returning &[u8] but then thought that having some of the convinces of SymbolName is actually useful. And this is Rust so we can never have too many types.

@ChrisDenton
Copy link
Member Author

Oh wow, CI is fully working again! So even if this PR isn't merged it has served a purpose, lol.

@workingjubilee
Copy link
Member

So, I'm receptive to the core idea. Haven't evaluated the impl details closely yet. But I'm thinking and... it would be an obvious breaking change but I gotta ask:

Is it actually our business to demangle symbols to begin with? 🤔

@ChrisDenton
Copy link
Member Author

Well I think not tbh. We also have a weird hack to optionally add C++ demangling too, which seems silly to me. I mean, even if this crate does want to include demangling support, it could just be a separate function rather than baked in.

But yeah, backwards compatibility.

@bjorn3
Copy link
Member

bjorn3 commented Apr 1, 2024

Is it actually our business to demangle symbols to begin with? 🤔

For the Display impls, IMO yes as Display is meant for human consumption and mangled symbol names are hard to read (the v0 mangled symbols even more so than the legacy mangled symbols. the latter I can read with a little bit of effort. the former I pretty much can't.). For programmatically getting the symbol name, IMO no it should give the symbol name that is actually stored in the object file.

@ChrisDenton
Copy link
Member Author

This feels somewhat similar to why Path has a display method rather than implementing the Display trait directly.

@ChrisDenton
Copy link
Member Author

Well different also in that there's lots of different ways to demangle symbols (depending on the language, etc).

@Mark-Simulacrum
Copy link
Member

Isn't it a moot point? Even if we don't do it here, it would be a pretty bad user experience regression for rust backtraces to not demangle by default in std. I'm not sure mitigating that with extra tooling is a viable solution.

Splitting that out from this library just means that theres extra cost paid - you need to run a demangling pass over the full backtrace after formatting or otherwise potentially incur extra allocations/time to do the demangling I suspect.

@ChrisDenton
Copy link
Member Author

Well yes, the default to std is unlikely to change. At least not in this way. I would however like to explore giving people more options (via build-std).

And eventually it would be nice if it were possible to compile rust std applications without also having to compile a debugger into every single binary. But that's another issue.

@ChrisDenton
Copy link
Member Author

Oh, hm... this probably also needs some support in BacktraceFrameFmt otherwise there would be no way to skip symbolization when printing via the helpers.

@workingjubilee
Copy link
Member

Splitting that out from this library just means that theres extra cost paid - you need to run a demangling pass over the full backtrace after formatting or otherwise potentially incur extra allocations/time to do the demangling I suspect.

Ooh, this is a good point: you want to generate the symbol string once, because generating and regenerating it is going to be nasty. Ideally you'd have a formatter you can just point at a stdout instead of allocating at all.

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

Successfully merging this pull request may close these issues.

4 participants