-
Notifications
You must be signed in to change notification settings - Fork 38
Arch-independent demangling and add gnuv2_demangle
for old g++ projects
#262
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
base: main
Are you sure you want to change the base?
Conversation
I've seen old and new mangling formats in gcc built binaries for Windows and Mac so it's definitely not a old mips binary exclusive thing |
What arch is used for Mac? I assume Windows uses x86, but dunno what Mac uses. Maybe ppc? |
gnuv2_demangle
for better demangling of old g++ compiled projectsgnuv2_demangle
for better demangling of old g++ compiled projects
Yes it is ppc/intel but most likely ppc |
gnuv2_demangle
for better demangling of old g++ compiled projectsgnuv2_demangle
for old g++ projects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big improvement!
@@ -12,11 +13,12 @@ pub fn demangle_window( | |||
show: &mut bool, | |||
state: &mut DemangleViewState, | |||
appearance: &Appearance, | |||
demangler: Demangler, | |||
) { | |||
egui::Window::new("Demangle").open(show).show(ctx, |ui| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add the demangler
option to this window as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As-in, keeping each option independent to each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
objdiff-core/config-schema.json
Outdated
"name": "MSVC" | ||
}, | ||
{ | ||
"value": "gnu_modern", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just call this itanium
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I don't really know what's the best way to refer to this. I guess plain "itanium" should be fine
if name.starts_with('?') { | ||
msvc_demangler::demangle(name, msvc_demangler::DemangleFlags::llvm()).ok() | ||
} else { | ||
name = name.trim_start_matches('.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this .
trimming logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see only ppc has this trimming logic.
Do you want to always trim .
when the demangler is not msvc? Or should it be only be applied to specific demanglers (cpp_demangle, cwdemangle, etc) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just add it for itanium/gnuv2
if name.starts_with('?') { | ||
msvc_demangler::demangle(name, msvc_demangler::DemangleFlags::llvm()).ok() | ||
} else { | ||
name = name.trim_start_matches('.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just add it for itanium/gnuv2
@@ -157,6 +157,7 @@ rlwinmdec = { version = "1.1", optional = true } | |||
|
|||
# mips | |||
rabbitizer = { version = "2.0.0-alpha.4", default-features = false, features = ["all_extensions"], optional = true } | |||
gnuv2_demangle ={ version = "0.1.0", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gnuv2_demangle ={ version = "0.1.0", optional = true } | |
gnuv2_demangle = { version = "0.1.0", optional = true } |
@@ -12,11 +13,12 @@ pub fn demangle_window( | |||
show: &mut bool, | |||
state: &mut DemangleViewState, | |||
appearance: &Appearance, | |||
demangler: Demangler, | |||
) { | |||
egui::Window::new("Demangle").open(show).show(ctx, |ui| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
"name": "Itanium" | ||
}, | ||
{ | ||
"value": "gnu_v2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on gnu_legacy
? And "GNU g++ (Legacy)". I think it would make it more clear that this is the old version. Open to other ideas
I tested it locally and works fine.
I'm unsure about a few things.
cpp_demangle
be removed from mips? I don't know if there's any mips project that uses a new enough g++ compiler that uses the new mangling abi. Or maybe invertcwdemangle
andcpp_demangle
since it is more likely for cw to be used in mips than a new g++.