Skip to content

Refactor rizin graph in rz_util#4788

Closed
Heersin wants to merge 15 commits intorizinorg:devfrom
Heersin:rz_graph_refactor
Closed

Refactor rizin graph in rz_util#4788
Heersin wants to merge 15 commits intorizinorg:devfrom
Heersin:rz_graph_refactor

Conversation

@Heersin
Copy link
Copy Markdown
Member

@Heersin Heersin commented Dec 22, 2024

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

working on implement functions
Add more graph representation for rizin, see #4231

  1. support identifier for nodes
  2. matrix based graph

Test plan

  1. unit test
  2. replace old graph api and test

Closing issues

...

XVilka
XVilka previously requested changes Dec 22, 2024
Copy link
Copy Markdown
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Looks already way way better then before! Thanks a lot.

Some questions you might have already considered.

  1. Should the graph's API mostly work on Node pointers or the node ids? Having both would be nice. If the node content doesn't matter to the user. But working on pointers is fine for me as well for now.
  2. Runtime complexity for getting neighbors. Please let's not clone the nodes every time the neighbor list is requested. This doesn't scale well. Instead a read only RzIterator over the lists/matrix is a better fit I think.
  3. Could the elements in the List based graph implementation contain edges? It would maybe reduce internal complexity getting the neighbors. Especially for directed graphs.

@Heersin
Copy link
Copy Markdown
Member Author

Heersin commented Jan 9, 2025

Looks already way way better then before! Thanks a lot.

Some questions you might have already considered.

1. Should the graph's API mostly work on Node pointers or the node ids? Having both would be nice. If the node content doesn't matter to the user. But working on pointers is fine for me as well for now.

2. Runtime complexity for getting neighbors. Please let's not clone the nodes every time the neighbor list is requested. This doesn't scale well. Instead a read only `RzIterator` over the lists/matrix is a better fit I think.

3. Could the elements in the List based graph implementation contain edges? It would maybe reduce internal complexity getting the neighbors. Especially for directed graphs.
  1. yes, this is also something I have been contemplating. don't know if it would be confused to provide both way to access. so I defined them mostly work on Node pointer, and they could turn id into pointer through hash_find
  2. exactly, it did bring crazy workload for runtime. I'm looking for a better way to return neighbors, do u know which file provide a best practice sample for returning RzIterator?
  3. no they don't contain explicit edges, initially I implemented it as edge and nodes, but later remove edges to abstract the RzGraph and implement PoC. You r right It's good to have edge in impl for list-based graph, and to reduce the complexity, thanks for point out that

@Rot127
Copy link
Copy Markdown
Member

Rot127 commented Jan 9, 2025

do u know which file provide a best practice sample for returning RzIterator

You can checkout ht_inc.c. The functions Ht_(as_iter_keys), Ht_(as_iter_mut) and the corresponding (iter_next etc.) return an iterator over a hash map. Your usecase is even simpler I think.

no they don't contain explicit edges, initially I implemented it as edge and nodes, but later remove edges to abstract the RzGraph and implement PoC. You r right It's good to have edge in impl for list-based graph, and to reduce the complexity, thanks for point out that

You make a good point there. Considering code complexity (meaning: many lines of code) vs. low runtime complexity.

If you don't want to add an edge list for now (because too much work/ too many things to consider), it is fine.
We can still extend it.

cc @wargio

@wargio
Copy link
Copy Markdown
Member

wargio commented Jan 9, 2025

agreed with @Rot127

I think it is ok to have first an implementation that works, then we can improve the execution time.

@notxvilka
Copy link
Copy Markdown
Contributor

@Heersin have you had any updates since?

@notxvilka
Copy link
Copy Markdown
Contributor

@Heersin ping

@notxvilka notxvilka added the waiting-for-author Used to mark PRs where more work is needed label Jan 18, 2026
@Heersin
Copy link
Copy Markdown
Member Author

Heersin commented Feb 12, 2026

@Heersin ping

Waiting to implement DFS. A question is I reserved edge_data in RzGraphEdgeNew, not sure if we need such a data for analysis inside rizin @XVilka @Rot127

@Rot127
Copy link
Copy Markdown
Member

Rot127 commented Feb 12, 2026

@Heersin

A question is I reserved edge_data in RzGraphEdgeNew, not sure if we need such a data for analysis inside rizin

Please, yes!
I am currently missing exactly this feature:

My use case is a control flow graph. Each node is a basic block (a sequence of instructions with the last instruction being a branch (jump or call)).

Now, the edges of the CFG are the jumps. Obvious.
But there is no edge between the call and its next instruction (the instruction where the callee would return to). But it would be nice to have some different kind of edge between call and return point (can eases analysis).

Then I would have edges of type I for normal branches, and edges of type II for "follows call".

@notxvilka
Copy link
Copy Markdown
Contributor

i agree, it makes sense to support generic graphs with whatever data provided.

@Heersin Heersin dismissed XVilka’s stale review February 14, 2026 17:11

refactor the whole file

Copy link
Copy Markdown
Contributor

@notxvilka notxvilka left a comment

Choose a reason for hiding this comment

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

It probably makes sense to split graph into two files: implementation and algorithms.

return NULL;
}

RZ_API void rz_graph_free_new(RZ_NULLABLE RZ_OWN RzGraphNew *g) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
RZ_API void rz_graph_free_new(RZ_NULLABLE RZ_OWN RzGraphNew *g) {
RZ_API void rz_graph_free(RZ_NULLABLE RZ_OWN RzGraphNew *g) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just so we don't forget about it :)

@Heersin
Copy link
Copy Markdown
Member Author

Heersin commented Feb 16, 2026

It probably makes sense to split graph into two files: implementation and algorithms.

DONE
should we replace the old API in another PR? or just replace them in the same one ? @notxvilka @Rot127

@Heersin Heersin marked this pull request as ready for review February 16, 2026 06:26
@Heersin Heersin self-assigned this Feb 16, 2026
@Rot127
Copy link
Copy Markdown
Member

Rot127 commented Feb 16, 2026

@Heersin Please replace it here, so the test coverage is higher.

@Rot127 Rot127 removed the waiting-for-author Used to mark PRs where more work is needed label Feb 16, 2026
@Heersin Heersin force-pushed the rz_graph_refactor branch from 594b106 to b29c7b1 Compare March 20, 2026 17:06
@Heersin Heersin requested a review from b1llow as a code owner March 20, 2026 17:06
@Heersin
Copy link
Copy Markdown
Member Author

Heersin commented Mar 20, 2026

Still Some DB fixing WIP

Copy link
Copy Markdown
Contributor

@notxvilka notxvilka left a comment

Choose a reason for hiding this comment

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

A few small nitpicks

* \param visitor callbacks
*/
static void dfs_edge_policy(RzGraph *g, RzGraphNode *from, RzGraphNode *to, ut8 *edge_color, RzGraphVisitor *visitor) {
// assert g, from, to, edge_color, visitor NON NULL
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You could just use rz_return_if_fail()

dfs_edge_policy(g, from, to, color, visitor);
}

// CORLOR changes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A typo in COLOR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Heersin you forgot also this one

@Heersin Heersin force-pushed the rz_graph_refactor branch 2 times, most recently from 5362d9b to aefe77a Compare March 29, 2026 13:02
Heersin added 13 commits April 2, 2026 20:10
Add comments

Basic support for list and matrix based graph refactor

Add rz_graph_*_new node and edge API

Modify calling for rz graph edge data

Add dfs and visitor mode

Add new get nth neighbours

Add unit test and wrapper for get edges

Bug fixed and unit test

Solve TODO about better semantic of get nodes

Doc RZ_API and other functions, split impl to new files

[cannot build] remove old graph impl and rename new graph API
Update Wrapper for graph identifier

Ordered Nodes in graph drawable

Fix icfg and several agraph refactor issues

Use kahn to get topo sort in assign_layers

Introduce khan for assign_layer and find DAG cycle and backedges in algorithm
@Heersin Heersin force-pushed the rz_graph_refactor branch from 17793c9 to f89f79d Compare April 2, 2026 12:10
@notxvilka notxvilka requested a review from Rot127 April 3, 2026 16:38
Copy link
Copy Markdown
Contributor

@notxvilka notxvilka left a comment

Choose a reason for hiding this comment

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

Missing type comment at <rizin/librz/util/graph_impl.c:1547:59> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1564:46> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1573:46> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1583:46> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1593:46> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1603:77> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1613:61> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1622:60> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1631:65> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1640:64> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1649:54> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1658:53> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_impl.c:1667:87> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:54:38> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:89:42> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:120:37> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:184:31> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:239:35> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:254:43> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:270:45> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:286:53> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:296:43> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:348:77> (token is not a comment)
Missing type comment at <rizin/librz/util/graph_algorithm.c:462:87> (token is not a comment)
...

@notxvilka
Copy link
Copy Markdown
Contributor

Also compilation on Windows should be fixed:

aph_new.c.obj "/c" ../test/unit/test_graph_new.c
../test/unit/test_graph_new.c(40): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(45): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(49): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(53): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(56): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(59): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(70): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(71): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(72): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(105): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(106): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(107): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(133): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(134): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(135): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(136): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(181): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(182): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(183): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(227): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(228): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(229): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(261): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(262): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(263): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(281): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(282): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(283): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(284): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(312): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(313): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(314): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(315): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(316): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(349): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(350): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(351): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(373): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(374): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(375): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(448): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(453): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(457): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(461): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(464): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(467): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(478): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(479): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(480): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(513): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(514): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(515): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(541): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(542): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(543): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(544): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(589): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(590): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(591): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(632): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(633): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(634): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(666): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(667): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(668): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(686): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(687): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(688): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(689): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(716): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(717): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(718): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(719): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(720): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(753): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(754): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(755): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(777): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(778): error C2036: 'void *': unknown size
../test/unit/test_graph_new.c(779): error C2036: 'void *': unknown size

@Rot127
Copy link
Copy Markdown
Member

Rot127 commented Apr 4, 2026

@Heersin I continue working on it here: #6152
Can you push to the repo branches as well? Otherwise I can cherry-pick your changes you do here into the dist- branch

@Heersin
Copy link
Copy Markdown
Member Author

Heersin commented Apr 6, 2026

@Heersin I continue working on it here: #6152 Can you push to the repo branches as well? Otherwise I can cherry-pick your changes you do here into the dist- branch

ok

@notxvilka
Copy link
Copy Markdown
Contributor

Superseded by #6152

@notxvilka notxvilka closed this Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants