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

crashlog: add method names from bindings #625

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Prevter
Copy link
Contributor

@Prevter Prevter commented Mar 14, 2024

Adds a basic system to read a "GeometryDash.bro" file (currently has to be placed manually into Geometry Dash/geode directory) and get function names from it to include them in the stacktrace.
This change makes crashlog look like this:
image

Unknown methods are also easier to find, because it prints the address where the method starts, instead of only the instruction address.

Some comments/questions:

  • What would be the best way to implement auto download of the bindings file (at which stage to do it, and where to actually save it)?
  • Maybe provide a toggleable option inside mod configuration to redownload bindings file automatically on every launch/crash?

Waiting for your feedback :)

Adds a basic system to read a "GeometryDash.bro" file (currently has to be placed manually into `Geometry Dash/geode` directory) and get function names from it to include them in the stacktrace.
@poweredbypie
Copy link
Member

This is a pretty cool addition, very impressive!
Although, I'm not sure if this is a good approach to implementing it: This requires us to ship the Broma binding with the loader, along with being kind of hacky about parsing it (we already have the broma parser library included in the source tree!)
I think the appropriate way to go about implementing this would be for the codegen library to emit some sort of address-to-symbol table so you can just lookup an address directly instead. This would mean no extra code units shipped (it's just included in the loader DLL) and automatically handles platform issues (just use the parsed broma to emit only the correct bindings for the respective platform).

@Prevter
Copy link
Contributor Author

Prevter commented Mar 14, 2024

Thanks for your feedback, I will try to implement your proposals.

I'm not sure if this is a good approach to implementing it

I agree, because I'm not familiar with internal structure of the project. That's why I wanted to hear some thoughts.

we already have the broma parser library included in the source tree

Didn't know about that, will check it. 😆

I think the appropriate way to go about implementing this would be for the codegen library to emit some sort of address-to-symbol table so you can just lookup an address directly instead

Yeah, this actually sounds like a more clean solution. The only downside is probably that bindings file would only be updated alongside loader updates, but I guess that it doesn't need to always have most up-to-date bindings to begin with. Also I don't think it would apply to anything other than Windows because of how bindings work on other platforms (maybe it might work on macOS, but I can't be sure).

@altalk23
Copy link
Member

codegen generates a CodegenData.txt file, i think this can be embbeded instead

this change adds a simple regex-based codegen into CMakeLists.txt, which creates a new header file containing an unordered_map with all function addresses mapped to their respective names
@Prevter
Copy link
Contributor Author

Prevter commented Apr 17, 2024

Figured out how to embed CodegenData.txt into the binary (codegen for codegen :0), so now it works without having to store the broma file locally.
CMake codegen function haven't increased configuration time at all for me, but crashlog.cpp may build for a few seconds longer now (i think this change is worth it in the long run).

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

Successfully merging this pull request may close these issues.

None yet

3 participants