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.

Sign up for GitHub

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

Jump to bottom

Feature/filtering la #4786

Merged
merged 4 commits into from
Dec 26, 2024
Merged

Feature/filtering la #4786

merged 4 commits into from
Dec 26, 2024

Conversation

well-mannered-goat
Copy link
Contributor

@well-mannered-goat well-mannered-goat commented Dec 21, 2024 •
edited
Loading

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

I updated the rz_core_asm_plugins_print function to handle the input of features argument and also removed the arch argument from rz_core_asm_plugin_print while also adding argument to handle the features

  • the strings feat and feat2 in rz_core_asm_plugin_print were being made and then I concatenate them into feat, my filter is passed as flags argument in the function and if any character of flags is not present in feat the function returns RZ_CMD_STATUS_OK rather than printing the output for that RzAsmPlugin
  • updated the rz_plugins_asm_print_handler to handle the rz_core_plugins_asm_print accordingly with the number of arguments
  • also updated the call of function rz_core_asm_plugins_print at other places by passing NULL as flags
  • updated plugins_asm_print_args to have an optional argument name 'features'
  • updated the cmd_plugins.yaml to handle an argument for La command

Test plan

  • Normal working of La
    image
  • Output for only assemblers
    image
  • Output for only Analysis with RzIL
    image

Closing issues

closes #4756

wargio
wargio requested changes Dec 22, 2024
View reviewed changes
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

Add a test under test/db/cmd/

librz/core/casm.c Outdated
}
feat2 = has_esil(core, ap->name);
strcat(feat, feat2);

for (int i = 0;flags && flags[i] != '\0'; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

I like how you solved it, but this is not clang formatted. Run sys/clang-format.py (requires clang format) and submit the changes.

@well-mannered-goat
Copy link
Contributor Author

So I have been working on the testing of this command and found out that Laj command expected wrong output with only a and d features in output as initially only those were printed in JSON output.
I updated it to have others too.

Also since I have applied filtering on Laq and Laj commands too, should I do extensive testing for each filtering and their combinations with La command output types?

@wargio
Copy link
Member

wargio commented Dec 24, 2024

Just add a test for each j and q variant.

@wargio
Copy link
Member

wargio commented Dec 24, 2024

Ps: check the options of sys/clang-format.py to pass the correct version of clang format

@well-mannered-goat
Copy link
Contributor Author

I cant pass the version in the script it seems

I have installed 16.0.0 on my machine

image

image

is this the correct version needed?

@wargio
Copy link
Member

wargio commented Dec 24, 2024

No, i meant the binary location of clang-format16
-C should allow this

@well-mannered-goat
Copy link
Contributor Author

I passed the path to binary location of clang-format in -c argument and there was no change in any file
Also i did not push the output_file in test directory, do I need to push it too?

@wargio
Copy link
Member

wargio commented Dec 25, 2024 •
edited
Loading

this is how i run it ./sys/clang-format.py -C /opt/clang-format-static/clang-format-16, also is upper case C

@well-mannered-goat
Copy link
Contributor Author

So this is my binary location
image

this is what i run ./sys/clang-format.py -C /opt/clang-16.0.0/bin/clang-format

wargio
wargio requested changes Dec 26, 2024
View reviewed changes
Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

Ok, just manually revert the changes of

  • librz/arch/isa/rx/rx_opcode_detail.c
  • librz/bin/format/elf/glibc_elf.h
  • librz/hash/algorithms/ssdeep/fnv_hash.h
  • librz/include/rz_magic.h

Then squash and we can merge.

@well-mannered-goat
chore:updated rz_core_asm_plugin_print to filter with flags
6312291 
chore:updated rz_core_asm_plugins_print to have flag arguments

add optional flags support to rz_core_asm_plugins_print

add feature argument

add features argument

rz-test: add `-y` option to accept all (rizinorg#4772)

* Add rz-test '-y' option to accept all
* Add '-y' into the help text

add clang format

correct Laj output to have features other than ad

add tests for filtering La

add tests for filtering Laq

add tests for filtering Laj

clang format

rz-test: add `-y` option to accept all (rizinorg#4772)

* Add rz-test '-y' option to accept all
* Add '-y' into the help text

dbtj: Add space between flags in `desc` property (rizinorg#4790)

subprojects: update rz-libdemangle (rizinorg#4791)

Remove trailing space in dbg_trace test (rizinorg#4792)

merge the filtering tests

add clang format

Remove trailing space in dbg_trace test (rizinorg#4792)

rzil: add VM event memory (read|write) index (rizinorg#4789)

This is because the memory of the RzIL VM has an index, but the index is ignored when an event is generated.

revert changes
@well-mannered-goat
Copy link
Contributor Author

squashed the commits
sorry didnt know had to keep less number of commits, would keep in mind about it.

@wargio
Copy link
Member

wargio commented Dec 26, 2024

squashed the commits sorry didnt know had to keep less number of commits, would keep in mind about it.

Not really, we normally always squash, but is just to have a clean history here (for readability)

@github-actions github-actions bot removed the RZIL label Dec 26, 2024
@well-mannered-goat
Copy link
Contributor Author

Seems like RzIL support for xtensa is added

would update the tests

@XVilka XVilka merged commit d771043 into rizinorg:dev Dec 26, 2024
44 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wargio wargio wargio approved these changes

@XVilka XVilka XVilka approved these changes

@kazarmy kazarmy Awaiting requested review from kazarmy kazarmy is a code owner

@thestr4ng3r thestr4ng3r Awaiting requested review from thestr4ng3r thestr4ng3r is a code owner

@ret2libc ret2libc Awaiting requested review from ret2libc ret2libc is a code owner

Assignees
No one assigned
Labels
API rz-asm rz-test RzCore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement filter for La output to filter over features
3 participants
@well-mannered-goat @wargio @XVilka