Skip to content

Conversation

@devalgupta404
Copy link

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 every RZ_API function and struct this PR changes.
  • I've added tests that prove my changes are effective (required for changes to RZ_API).
  • I've updated the Rizin book with the relevant information (if needed).
  • I've used AI tools to generate fully or partially these code changes and I'm sure the changes are not copyrighted by somebody else.

Detailed description

The rop.cache config option existed but was never implemented. The cache field in RzRopSearchContext was set from config but never used in the search logic.

This PR implements the caching functionality by:

  • Using the existing ht_rop hashtable in RzAnalysis to store search results
  • Keying results by DJB2 hash of the grep argument
  • Returning cached results immediately on subsequent identical searches
  • Properly freeing the hashtable on analysis cleanup

Test plan

  1. Unit tests pass:
    meson test -C build rop rop_constraint
    1/2 rizin:unit / rop OK
    2/2 rizin:unit / rop_constraint OK

  2. Timing proof (same as issue rop.cache seems not work in risc-v binary #5544):
    [0x00000000]> e rop.cache=true
    [0x00000000]> time /R/ jalr
    0x000001fa 2ad6 sw a0, 44(sp)
    0x000001fc 6df2 bnez a2, 0x1de
    0x000001fe 0293 jalr t1
    ...
    0.056576
    [0x00000000]> time /R/ jalr
    0x000001fa 2ad6 sw a0, 44(sp)
    0x000001fc 6df2 bnez a2, 0x1de
    0x000001fe 0293 jalr t1
    ...
    0.000016

Search Time Notes
1st 0.056576s Actual search
2nd 0.000016s Cached (~3500x faster)

Closing issues

closes #5544

@codecov
Copy link

codecov bot commented Dec 29, 2025

Codecov Report

❌ Patch coverage is 27.77778% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.81%. Comparing base (bb70c1a) to head (c0491b5).
⚠️ Report is 6 commits behind head on dev.

Files with missing lines Patch % Lines
librz/core/rop.c 23.52% 9 Missing and 4 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
librz/arch/analysis.c 70.00% <100.00%> (+0.05%) ⬆️
librz/core/rop.c 57.01% <23.52%> (-0.50%) ⬇️

... and 13 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6940c5...c0491b5. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

core, "mov", false, RZ_ROP_GADGET_PRINT, RZ_ROP_DETAIL_SEARCH_NON, NULL);
RzCmdStatus status = rz_core_rop_search(core, context);
mu_assert_eq(status, RZ_CMD_STATUS_OK, "rop search should succeed");
mu_assert_notnull(core->analysis->ht_rop, "ht_rop should be initialized after cached search");
Copy link
Member

Choose a reason for hiding this comment

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

Please also check if the correct result is in the cache.

Then do another search for the same gadget.

Copy link
Author

@devalgupta404 devalgupta404 Dec 30, 2025

Choose a reason for hiding this comment

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

@Rot127 should i manually insert result into cache and then verify the second search returns the cached result correctly?

Copy link
Member

Choose a reason for hiding this comment

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

No, after the search the result is expected to be in the cache.

@Rot127 Rot127 added the Requirements not met The PR doesn't meet the minimum contribution requirements. See CONTRIBUTING.md for details. label Dec 30, 2025
@Rot127 Rot127 marked this pull request as draft December 30, 2025 15:37
@devalgupta404 devalgupta404 force-pushed the fix/rop-cache-5544 branch 2 times, most recently from c0491b5 to c068675 Compare January 5, 2026 13:44
@devalgupta404
Copy link
Author

@Rot127 did you check the updated test?

@Rot127
Copy link
Member

Rot127 commented Jan 11, 2026

Please fix the build first

@devalgupta404
Copy link
Author

@Rot127 i fixed the build issue please take a look.

@Rot127
Copy link
Member

Rot127 commented Jan 13, 2026

Tests still fail though. Please check

@devalgupta404
Copy link
Author

devalgupta404 commented Jan 15, 2026

@Rot127 i fixed the build issue and test locally whatever was failing previously

Local Test Verification

Fix: test_rop binary path resolution

Updated test/unit/test_rop.c to correctly resolve the test binary path.

Results

test_rop:

$ build/test/unit/test_rop
test_rop_cache OK
test_rz_direct_solver OK

meson test:

Ok: 146
Fail: 0

rz-test:

24068 OK 1048 BR 11 XX 25 FX

(11 XX are pre-existing failures unrelated to this PR)

So can you please rerun the workflows

@devalgupta404
Copy link
Author

@Rot127

While running CI tests locally, I found memory leaks but noticed that CI already
tolerates them (.github/workflows/ci.yml lines 106, 125, 143 - detect_leaks=0
and allow_failure: true).

I fixed some leaks anyway - should I keep or revert these changes?

Also, I moved some tests from unit to integration after learning that unit tests
shouldn't use binaries. Is this correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Requirements not met The PR doesn't meet the minimum contribution requirements. See CONTRIBUTING.md for details. rz-test RzAnalysis RzCore RZIL RzType RzUtil

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rop.cache seems not work in risc-v binary

2 participants