Show Caller Name in XREF

[lorsanta]

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 updated the documentation with the relevant information (if needed)

Detailed description

Ref #3416

Test plan (required)

Open a project, select a function and check for xref.
The dialogue now can also show the name of the caller.

Closing issues

Closes #3416

[wargio]

Looks good! @karliss

[karliss]

Rizin should already have a function which formats an address as "name+offset" (can't comment whether there is a suitable Cutter wrapper for it already), where name might be a function and at least in theory it could be something else like a global array name if xref is done by a pointer in some kind of table or even source file+line numbers when debug info is available.
Other benefit is that it will make things a bit clearer when a function contains multiple xrefs to the same thing.

Whatever it is it should be delegated to rizin to make the most suitable description of address, you shouldn't attempt to reimplement the things I described on cutter side.

For example here is backtrace widget doing similar thing:

And here is disassembly with asm.reloff option turned on.

Usefulness for "Xrefs from" seems very limited. For all the call instructions the target function should be already visible, possibly twice. Once in address column and second time in code column. No point adding extra noise by displaying same information third time.

[karliss]

^

[wargio]

Hmm makes sense. this is true also for xrefs that are not calls. (like data xrefs etc..), where there is "no caller"

[notxvilka]

Yes, there is a function: RzFlagItem *f = rz_flag_get_at(core->flags, addr, !strict_offset); It's used in the Rizin's fd command that returns precisely this: name + offset:

		if (f->offset != addr) {
			rz_cons_printf("%s + %d\n", name, (int)(addr - f->offset));

[lorsanta]

I didn't found a specific function which given an address formats it as "name+offset", rather it seems it's copy pasted in different parts in Rizin:
https://github.com/rizinorg/rizin/blob/58a26a749487568f088cd8b71c4d0d307675fbc9/librz/core/disasm.c#L2787-L2791
https://github.com/rizinorg/rizin/blob/58a26a749487568f088cd8b71c4d0d307675fbc9/librz/core/cmd/cmd_flag.c#L348-L351
https://github.com/rizinorg/rizin/blob/58a26a749487568f088cd8b71c4d0d307675fbc9/librz/core/cdebug.c#L921-L928

If I can't do it the way @notxvilka suggests it, I'm not sure what I could do.

[wargio]

The last link you have shared does exactly what you have described. flag + offset (just called delta there)

[lorsanta]

Sorry, I'm confused @wargio , I could do something like this:

index e674d9a6..3abd9b21 100644
--- a/src/core/Cutter.cpp
+++ b/src/core/Cutter.cpp
@@ -4021,7 +4021,24 @@ QList<XrefDescription> CutterCore::getXRefs(RVA addr, bool to, bool whole_functi
             continue;
         }
 
-        xd.from_str = RzAddressString(xd.from);
+        if(to) {
+            RzFlagItem *f = rz_flag_get_at(Core()->core()->flags, xd.from, true);
+            if(f) {
+                int delta = xd.from - f->offset;
+                if (delta > 0) {
+                    xd.from_str = rz_str_newf("%s+%x", f->name, delta);
+                } else if (delta < 0) {
+                    xd.from_str = rz_str_newf("%s-%x", f->name, delta);
+                } else {
+                    xd.from_str = rz_str_newf("%s", f->name);
+                }
+            } else {
+                xd.from_str = RzAddressString(xd.from);
+            }
+        }
+        else {
+            xd.from_str = RzAddressString(xd.from);
+        }
         xd.to_str = Core()->flagAt(xd.to); 

But wouldn't that contradict what @karliss was saying about not reimplementing things on cutter side?

[wargio]

yes, but delta is already an int so you should probably use %d instead of %x

and modify the negative to not include the sign, which will be added by rz_str_newf

+                } else if (delta < 0) {
+                    xd.from_str = rz_str_newf("%s%d", f->name, delta);

[notxvilka]

I wonder if it makes sense to make it an API function from the Rizin side. To avoid copy-pasting the same code between Rizin and cutter. Something like rz_flag_offset_describe()

But then it would be blocked on #3418

So, for now, this change should be ok, I think.

[wargio]

yes probably we can merge this and then have an api returning a string.

[wargio]

LGTM

[wargio]

@karliss give it a look

[karliss]

As a side note I highly recommend to find a suitable resource and learn the basics of git. Making pull requests from the dev branch of your fork will likely cause you problems in future.

[lorsanta]

Sorry for the mess but I think I'm finally on the right track: I replaced rz_str_newf() with QString::arg() (didn't used QString::asprintf() since the qt docs recommend not to use it), and moved CORE_LOCK(); one scope higher

[karliss]

The code is fine.

But I have doubts about merging it in current state.

It adds a bunch of code which will have to be later thrown out, and contains no code which will be useful later once rizin has appropriate API. Sure there are situations where it's useful where it's useful to merge partial implementation first, and then replace some of it withe calls to better API when available. But that assumes that the changes contain at least some useful code (some UI additions or code restructuring) which will be reusable once the better API is available.

Other case where it might be worth merging partial implementation is if it still provides significant improvement in usability. That in current state is also limited due to inconsistency with rizin and not handling a bunch of cases which rizin does.

Here are some examples:
Useless "case.xxx.yy+offset" since the current version takes any flag without considering it's type.

Uses different style of formatting fcn.00b200+81 vs fcn.00b200+0x51.

I have also seen cases where current implementation displays data_something+offset even though asm.reloff displayed fcn.xxxx+offset. And that was with asm.relof.flags also enabled.

For the temporary implementation might be better to use CutterCore::functionIn like the initial implementation did (but keeping rest of the structure from current version). While in theory this feature should display of not just function relative stuff, in the current version use of rz_flag_get_at makes the result worse more often than it improves. Need to try out with functionIn to be sure.

Also should use offset as hexadecimal(1) number like the asm.reloff mode does.

Correction:
(1) asm.reloff chooses base based on asm.decoff