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

Improve DWARF5 support and refactor #3565

Closed
wants to merge 187 commits into from
Closed

Improve DWARF5 support and refactor #3565

wants to merge 187 commits into from

Conversation

imbillow
Copy link
Contributor

@imbillow imbillow commented Jun 8, 2023

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

Test plan

Add tests for dwarf5

Closing issues

closes #3548
closes #3535
closes #1004
closes #3541
partially addresses #3581
...

See also https://gcc.gnu.org/wiki/DebugFission

librz/core/cdwarf.c Outdated Show resolved Hide resolved
@imbillow

This comment was marked as resolved.

@imbillow imbillow force-pushed the dwarf5 branch 2 times, most recently from 5ab5d52 to 8fb26b8 Compare June 9, 2023 16:40
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 also extract column value when it's available

librz/analysis/dwarf_process.c Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
librz/bin/dwarf.c Outdated Show resolved Hide resolved
@XVilka

This comment was marked as resolved.

@imbillow

This comment was marked as resolved.

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 check also the columns information extraction where available, likely it will be easier to do in the same PR since it's a big refactoring anyway.

librz/analysis/dwarf_process.c Outdated Show resolved Hide resolved
librz/analysis/dwarf_process.c Outdated Show resolved Hide resolved
librz/bin/dwarf/abbrev.inc Outdated Show resolved Hide resolved
librz/bin/dwarf/abbrev.inc Outdated Show resolved Hide resolved
librz/bin/dwarf/aranges.inc Outdated Show resolved Hide resolved
librz/bin/dwarf/unit.inc Outdated Show resolved Hide resolved
librz/bin/dwarf/unit.inc Outdated Show resolved Hide resolved
librz/bin/dwarf/unit.inc Outdated Show resolved Hide resolved
librz/bin/dwarf/unit.inc Outdated Show resolved Hide resolved
librz/util/buf.c Outdated Show resolved Hide resolved
@XVilka
Copy link
Member

XVilka commented Jun 15, 2023

This also could be helpful if you haven't seen it yet: https://gcc.gnu.org/wiki/DebugFission

librz/include/rz_core.h Outdated Show resolved Hide resolved
@imbillow imbillow force-pushed the dwarf5 branch 2 times, most recently from 9ebc275 to c649ba0 Compare June 15, 2023 11:10
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.

One more thing, once you finished the refactoring, I recommend running afl++ fuzzer with DWARF v2-v5 seeds from our testsuite and your own files, to catch obvious bugs at the early stage. Likely most obvious crashes will happen in less than an hour of fuzzing.

https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md

@XVilka
Copy link
Member

XVilka commented Jun 16, 2023

@XVilka
Copy link
Member

XVilka commented Jun 16, 2023

These tests also could be helpful: https://github.com/davea42/libdwarf-regressiontests

@XVilka

This comment was marked as resolved.

librz/bin/dwarf/value.inc Outdated Show resolved Hide resolved
@XVilka
Copy link
Member

XVilka commented Jun 19, 2023

For the record, no need to think about them in this PR, but could be nice to handle them somehow in the future:

For now please just check the doc and think if they can be accomodated in the current architecture at some point in future.

librz/bin/dwarf/op.inc Outdated Show resolved Hide resolved
test/db/rzil/ppc64 Outdated Show resolved Hide resolved
@XVilka

This comment was marked as resolved.

@XVilka

This comment was marked as resolved.

test/integration/test_dwarf_info.c Outdated Show resolved Hide resolved
test/integration/test_dwarf_info.c Outdated Show resolved Hide resolved
librz/analysis/serialize_analysis.c Show resolved Hide resolved
test/integration/test_dwarf.c Outdated Show resolved Hide resolved
@XVilka XVilka requested a review from wargio July 31, 2023 00:45
@XVilka
Copy link
Member

XVilka commented Jul 31, 2023

@imbillow please also rebase on top of the latest dev, especially important since there is upgraded capstone.

@XVilka
Copy link
Member

XVilka commented Jul 31, 2023

Please update tests output too:

[XX] db/cmd/dwarf Invalid kind for DW_AT_name
RZ_NOPLUGINS=1 /private/var/folders/bc/yvvw2g2s4jlg9r8x7tf61xv800009d/T/woodpecker-local-1287038090/rizinorg/rizin/prefix/bin/rizin -escr.utf8=0 -escr.color=0 -escr.interactive=0 -eflirt.sigdb.load.system=false -eflirt.sigdb.load.home=false -N -Qc 'aaa
id
' bins/elf/dwarf3_c.elf.patched0
-- stdout
--- expected
+++ actual
@@ -29,7 +29,7 @@
     DW_AT_high_pc                  DW_FORM_addr
     DW_AT_frame_base               DW_FORM_block1
     DW_AT_GNU_all_tail_call_sites  DW_FORM_flag
-    DW_AT_siblings                 DW_FORM_ref4
+    DW_AT_sibling                  DW_FORM_ref4
     5      DW_TAG_variable           [no children] (0x4d)
     DW_AT_name                     DW_FORM_string
     DW_AT_decl_file                DW_FORM_data1
@@ -91,7 +91,7 @@
      DW_AT_high_pc             : 0x1156
      DW_AT_frame_base          : 1 byte block: 0x9c
      DW_AT_GNU_all_tail_call_sites : 1
-     DW_AT_siblings            : <0x7b>
+     DW_AT_sibling             : <0x7b>
 <0x6d>: Abbrev Number: 5    (DW_TAG_variable)
      DW_AT_name                : i
      DW_AT_decl_file           : 1

@XVilka
Copy link
Member

XVilka commented Jul 31, 2023

You can run bindgen linter locally to check for all errors at once:

Run python3 rz-bindgen/src/lint.py \
Mismatched function annotation for rz_bin_dwarf_expression_dump at <rizin/librz/include/rz_bin_dwarf.h:1[7](https://github.com/rizinorg/rizin/actions/runs/5709818923/job/15469144904?pr=3565#step:6:8)27:1> : RZ_NONNULL (was RZ_NULLABLE at <rizin/librz/bin/dwarf/op.c:1501:1>

@XVilka XVilka requested a review from ret2libc July 31, 2023 04:04
@XVilka
Copy link
Member

XVilka commented Jul 31, 2023

@imbillow could you please open a new PR once you rebase? This was reviewed so much that it's hard to get track of the things?

@XVilka
Copy link
Member

XVilka commented Jul 31, 2023

@imbillow please update Doxygen for this function in librz/analysis/dwarf_process.c too:

/**
 * \brief Use parsed DWARF function info from Sdb in the function analysis
 *  XXX right now we only save parsed name and variables, we can't use signature now
 *  XXX refactor to be more readable
 * \param analysis
 * \param dwarf_sdb
 */
RZ_API void
rz_analysis_dwarf_integrate_functions(RzAnalysis *analysis, RzFlag *flags) {
	rz_return_if_fail(analysis && analysis->debug_info);
	ht_up_foreach(analysis->debug_info->function_by_addr, dwarf_integrate_function, analysis);
}

There is no SDB anymore

@imbillow
Copy link
Contributor Author

@imbillow please also rebase on top of the latest dev, especially important since there is upgraded capstone.

I should have rebased the latest dev branch, is there something wrong?

@XVilka
Copy link
Member

XVilka commented Jul 31, 2023

@imbillow please also rebase on top of the latest dev, especially important since there is upgraded capstone.

I should have rebased the latest dev branch, is there something wrong?

My bad, just cached reference state. After removing dwarf5 local branch and fetching all refs everything is fine.

@imbillow
Copy link
Contributor Author

@imbillow could you please open a new PR once you rebase? This was reviewed so much that it's hard to get track of the things?

OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants