-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Merge asm/ and analysis/ into arch/ but keep plugin formats. #4332
Conversation
Why removing the common logic in |
because now it's built as a library, so if we split stuff is easier to handle. also bin plugins has RzBinPlugin and RzBinXtrPlugin so that common logic cannot be easily kept. |
yeah. that piece of code was made of |
I merged TMS320C64x into TMS320 because it was a dup. |
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 don't see why we can't use the approach that was used before to avoid the duplicated meson code. We were already handling RzBinXtr and RzBin plugins. Also, we can probably also just have a single rz_plugins.h.in
for all the various modules since it is the same.
# this was never built. | ||
# 'p/bin_xtr_pemixed.c', |
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.
what do you mean that it was never built? if it was here it should have been built. I don't get it.
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.
the code was there, but it was never enabled as plugin. that is why when i added it back we had "test failing" but just because now was showing up.
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.
LGTM as is. Windows builds and rz-bindgen could be fixed separately after this jumbo PR is merged.
You were not handling plugins, but just creating a list of plugins which is easier to do. |
This comment was marked as resolved.
This comment was marked as resolved.
The first one creates ad-hoc file per lib. within the header i have a The huge difference (and why this is needed) is that if i follow the old way to generate those, i would need to add the headers of all the libs into the So to avoid to have such horrendous |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Whoops, this of course needs to be fixed. it should be enough to add a |
1a3f58e
to
99cc534
Compare
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.
Better now! Thanks 👍
The tms320c64x has been merged into tms320.
This ignores warning 'yield from' directly instead of yielding each element one by one (use-yield-from)
DO NOT SQUASH
Your checklist for this pull request
Detailed description
This is the first step against rz_arch. this merges still the
librz/asm/
andlibrz/analysis/
into the same folder structure but keeps the same formats.One change that can be noticed is that now we do have a list of all deactivated plugins.
Another big change is also related how we generate the plugins headers, which before needed to be included by the developer each time you add an arch, but now is automatically generated by meson.
After merging this pr, it will be still possible to load external asm and analysis plugins as standalone libraries.
Also I have merged TMS320C64x into TMS320 because it was a duplicated architecture which was available in both tms320 and tms320c64x with code that was duplicated between the two plugins.
Old PR #4120