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

Add More GNU v2/v3 Tests #68

Merged
merged 13 commits into from
Dec 23, 2024
Merged

Add More GNU v2/v3 Tests #68

merged 13 commits into from
Dec 23, 2024

Conversation

brightprogrammer
Copy link
Contributor

@brightprogrammer brightprogrammer commented Dec 2, 2024

Adding more tests.
Tests taken and modified from here
Source test code is licensed under BSD, modified test code is licensed under LGPL-3.0-only

Tests taken for gnu v2 are from here
Both source test code and the modified test code is licensed under LGPL v3

Tests taken for gnu v2 are from libiberty/testsuite/demangle-expected from gcc-2.95.3 source code are licensed under GPL v2

@wargio
Copy link
Member

wargio commented Dec 2, 2024

looks like we do not handle many cases (the copied code is probably old and should be updated?)

@brightprogrammer
Copy link
Contributor Author

Yes, some fixmes lying around. Will fix those...

@brightprogrammer brightprogrammer changed the title Add More GNU v3 Tests Add More GNU v2/v3 Tests Dec 4, 2024
Fix breaking tests for gnu v2 demangler
@XVilka

This comment was marked as resolved.

Some test cases are failing because __gnu_cxx is not getting printed in
the demangled name. These are marked with AFTER-REWRITE.

Some tests are failing because the test is incorrect, these are marked
with INCORRECT, which can be checked later on. These are less than 30 in
count.

Some tests are failing because current demangler code does not handle
substitution. For this we need the rewrite. Will work on those in
following commit.
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.

Looks good now, just please remove formatting changes. That code will be removed once you rewrite it anyway, no need for that, and it creates unnecessary bulk of changes in this PR.

src/cxx.c Show resolved Hide resolved
src/gnu_v2/cplus-dem.c Outdated Show resolved Hide resolved
src/gnu_v2/cplus-dem.c Outdated Show resolved Hide resolved
src/gnu_v2/cplus-dem.c Outdated Show resolved Hide 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.

Split some tests into a new category with RZ_DEMANGLE_OPT_BASE

Some tests were failing because of regex replacement performed after
demangling, to simplify the demangled output. Thec c++ tests are now
split into two parts : test_cxx.c and test_cxx_base.c

Full discussion can be viewed in the thread here :
https://im.rizin.re/rizinorg/pl/prasfjqxc3fgb8uxxoh565ku4w
@XVilka
Copy link
Member

XVilka commented Dec 23, 2024

@wargio take a look again.

Copy link
Member

@wargio wargio left a comment

Choose a reason for hiding this comment

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

i'm ok with the changes, i just don't see any real upgrade of the old code, so i guess it's ok?

src/gnu_v2/cplus-dem.c Outdated Show resolved Hide resolved
@XVilka
Copy link
Member

XVilka commented Dec 23, 2024

i'm ok with the changes, i just don't see any real upgrade of the old code, so i guess it's ok?

That's the plan. Cover as much as possible with tests before starting actual rewrite. So that we could be sure the new code works properly.

@brightprogrammer
Copy link
Contributor Author

Yes, and in total, only around 300 tests fail in c++ v3 test set. Some return NULL and some are completely wrong. Rewrite will aim to cover all these (and give a more permissive license as well)

@XVilka XVilka merged commit d3083c1 into rizinorg:main Dec 23, 2024
5 of 6 checks passed
@brightprogrammer brightprogrammer deleted the more_tests branch January 3, 2025 08:16
@brightprogrammer brightprogrammer mentioned this pull request Jan 3, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants