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.

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

Dwarf sourceline info #3449

Closed
wants to merge 9 commits into from
Closed

Conversation

Sidhub723
Copy link
Contributor

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

Added a handler in ds_print_dwarf() function in librz/core/disasm.c, if the variable asm.dwarf.lines is set to true. asm.dwarf.lines is set to false by default.

...

Test plan

...

Closing issues
This PR is with regards to issue 653

...

@ret2libc
Copy link
Member

ret2libc commented Apr 5, 2023

Can you show here on GH how the output changes before and after your PR? What is the difference between asm.dwarf and asm.dwarf.lines?

@wargio
Copy link
Member

wargio commented Apr 5, 2023

add a test, please

@Sidhub723
Copy link
Contributor Author

When only asm.dwarf is set to true and asm.dwarf.lines is set to false, below would be the disassembly output:
image
When asm.dwarf is still true, and line debug info display is enabled by setting asm.dwarf.lines to true, the disassembly changes to:
image
One thing i still remain unsure about it how to make the Line Number -xx be displayed in a color which matches the console theme.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

PLease add a test

@@ -349,7 +350,8 @@ static void ds_print_sysregs(RzDisasmState *ds);
static void ds_print_fcn_name(RzDisasmState *ds);
static void ds_print_as_string(RzDisasmState *ds);
static bool ds_print_core_vmode(RzDisasmState *ds, int pos);
static void ds_print_dwarf(RzDisasmState *ds);
// static void ds_print_dwarf(RzDisasmState *ds);
Copy link
Member

Choose a reason for hiding this comment

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

Remove


RzBinFile *binfile = core->bin->cur;
if (!binfile || !binfile->o) {
// rz_cons_printf("No file loaded.\n");
Copy link
Member

Choose a reason for hiding this comment

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

Remove

}
RzBinSourceLineInfo *li = binfile->o->lines;
if (!li) {
// rz_cons_printf("No Source line information available");
Copy link
Member

Choose a reason for hiding this comment

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

Same

// return true;
}
if (ds->dwarfShowLines && ds->show_dwarf && SourceLineInfoExists && binFileExists) {

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary empty line

Copy link
Member

Choose a reason for hiding this comment

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

You can extract that part into a separate static function for clarity

break;
}
temps = &li->samples[i];

Copy link
Member

Choose a reason for hiding this comment

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

Same

@XVilka
Copy link
Member

XVilka commented Apr 9, 2023

The overall format should be filename:line, e.g. bla.c:315, without Line number..., etc

@Sidhub723
Copy link
Contributor Author

Will work on making these changes ASAP, thanks for the feedback!

@wargio
Copy link
Member

wargio commented Apr 10, 2023

also it should be like a comment so ; bla.c:315

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

Please add a test, for both disabled and enabled states.

@XVilka XVilka requested a review from imbillow May 5, 2023 04:12
@XVilka
Copy link
Member

XVilka commented May 5, 2023

@imbillow feel free to take over as part of your GSoC

@imbillow
Copy link
Contributor

imbillow commented May 5, 2023

@imbillow feel free to take over as part of your GSoC

What do you mean by 'take over'? Should I review the PR, or close it, open a new one and continue working from there?

@Sidhub723
Copy link
Contributor Author

Sorry that I couldnt finish it quick. Goodluck to @imbillow !

@wargio
Copy link
Member

wargio commented May 7, 2023

@imbillow feel free to take over as part of your GSoC

What do you mean by 'take over'? Should I review the PR, or close it, open a new one and continue working from there?

take over

@imbillow imbillow mentioned this pull request May 14, 2023
5 tasks
@imbillow
Copy link
Contributor

Follow-up work in the new PR #3511

@imbillow imbillow closed this May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants