Skip to content

Move functions to ELF sections with mangled names. #1160

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

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

CohenArthur
Copy link
Contributor

This PR adds a basic section name mangling library to encode the type information of variables and functions as a section name within the resulting binary. When codegen'ing POUs, we can now set the section name of the function using the provided inkwell API. I'll add the same functionality for variables in a later PR.

The section-mangler library should stay quite simple but it will also include decoding of a mangled string, to enable reconstructing an instance of SectionMangler from a section name. Let me know if you'd like the API to look differently or if using a simple builder-like pattern like this one is okay.

Last commit updates all of the snapshots, hence making that PR quite big.

@CohenArthur CohenArthur force-pushed the move-functions-to-sections branch from 54e04a2 to e1d10df Compare March 18, 2024 13:49
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 86.08696% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 95.62%. Comparing base (6a5304e) to head (53b44c4).

Files Patch % Lines
compiler/section_mangler/src/lib.rs 77.61% 15 Missing ⚠️
src/codegen/generators/section_names.rs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1160      +/-   ##
==========================================
- Coverage   95.64%   95.62%   -0.03%     
==========================================
  Files         148      150       +2     
  Lines       42531    42646     +115     
==========================================
+ Hits        40680    40779      +99     
- Misses       1851     1867      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghaith
Copy link
Collaborator

ghaith commented Mar 27, 2024

Hello Arthur,
Thank you for the commit. I went over the changes and it looks good.
I just had one thought here: we should make the mangling optional, this is something we could add later, but ideally it would be a command line flag similar to the -g to debug. Doing this could also allow us to reduce the amount of changes in the tests but I don't think this is required. After discussing it with @riederm it might actually make sense for us to include the mangling in our tests to maybe notice if anything there changes.
@riederm any thoughts here?

@CohenArthur
Copy link
Contributor Author

Hello Arthur, Thank you for the commit. I went over the changes and it looks good. I just had one thought here: we should make the mangling optional, this is something we could add later, but ideally it would be a command line flag similar to the -g to debug.

That makes a lot of sense!

Doing this could also allow us to reduce the amount of changes in the tests but I don't think this is required. After discussing it with @riederm it might actually make sense for us to include the mangling in our tests to maybe notice if anything there changes. @riederm any thoughts here?

I think the name mangling algorithm will evolve over time, and it will create lots of changes in the testsuite each time - furthermore, its goal is only to be consumed by the dynamic loader, so I think the format changing is not super worrisome.

I think adding a flag and keeping mangling off by default is the way to go. It might be good to create a new mangling testsuite where code is compiled using this flag to see changes to the mangling, but I don't think this is really necessary. In our local development branch we have also started adding the reverse operation, in order to decode mangled names into instances of the SectionMangler enum, and the parser contains unit testing - so changes will be visible there

Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine. Since there is lots of things open I guess we're going to discuss things in more detail.
Things that cought my attention were that we can probably discuss during future work:

  1. you're basically re-building the type-information in a separate enum that more or less (so far) reflects the DataTypeInformation enum - I think this is fine for now if that feels natural for you - for the moment it looks like it does not add any new information and will become a 1-to-1 relationship. So maybe you will decide at one point that you can mangle the type names directly from the DataTypeInformation.
  2. I dont really mind the section names in the test. If it gets annoying that you keep changing the tests I'm also fine with introducing a feature-flag
  3. Have you thought about using a hash instead of the whole signature? - but this is off-topic now ...

so overall I think this can be merged

@CohenArthur CohenArthur force-pushed the move-functions-to-sections branch from 2dc7e84 to 6a307b6 Compare April 2, 2024 15:31
@CohenArthur
Copy link
Contributor Author

I think that having a feature flag would make it easier to iterate on mangling schemes as well as test the future parser - I'll add it and push

@CohenArthur
Copy link
Contributor Author

Nevermind, accessing the CLI flags this far down the line is non-trivial, so if we do decide to go through with this I think it should be done in a separate PR. On a separate note, the "Style" check fails due to files that were not modified in this PR

@riederm
Copy link
Collaborator

riederm commented Apr 2, 2024

On a separate note, the "Style" check fails due to files that were not modified in this PR
@volsa bumped the rust version, you gonna need a rebase on #1187

@CohenArthur CohenArthur force-pushed the move-functions-to-sections branch from 6a307b6 to b77e9b6 Compare April 3, 2024 14:26
@CohenArthur
Copy link
Contributor Author

should now be rebased properly :)

@CohenArthur CohenArthur force-pushed the move-functions-to-sections branch from b77e9b6 to 53b44c4 Compare April 4, 2024 12:27
The section-mangler crate provides a simple builder for representing
StructuredText functions and variables and mangling their name.
This commit relocates generated LLVM functions to sections using the
appropriate mangled name.
This enables sharing the type mangling with the variable generator
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.

3 participants