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

[next] SH: Use bitwise OR with mask for sign extension #2389

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

lhsazevedo
Copy link
Contributor

Cherry pick of #2371 to next branch

@github-actions github-actions bot added the SH Arch label Jun 13, 2024
@lhsazevedo lhsazevedo changed the title [next] Use bitwise OR with mask for sign extension [next] SH: Use bitwise OR with mask for sign extension Jun 13, 2024
Copy link
Contributor

@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.

Though SH disassembler generates clang-tidy warnings:

/home/runner/work/capstone/capstone/arch/SH/SHDisassembler.c:102:9: warning: va_arg() is called on an uninitialized va_list [clang-analyzer-valist.Uninitialized]
                grp = va_arg(g, sh_insn_group);
                      ^
/usr/lib/llvm-14/lib/clang/14.0.0/include/stdarg.h:19:29: note: expanded from macro 'va_arg'
#define va_arg(ap, type)    __builtin_va_arg(ap, type)
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/capstone/capstone/arch/SH/SHDisassembler.c:100:9: note: Assuming 'n' is > 0
        while (n > 0) {
               ^~~~~
/home/runner/work/capstone/capstone/arch/SH/SHDisassembler.c:100:2: note: Loop condition is true.  Entering loop body
        while (n > 0) {
        ^
/home/runner/work/capstone/capstone/arch/SH/SHDisassembler.c:102:9: note: va_arg() is called on an uninitialized va_list
                grp = va_arg(g, sh_insn_group);
                      ^
/usr/lib/llvm-14/lib/clang/14.0.0/include/stdarg.h:19:29: note: expanded from macro 'va_arg'
#define va_arg(ap, type)    __builtin_va_arg(ap, type)
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/capstone/capstone/arch/SH/SHDisassembler.c:937:4: warning: Value stored to 'insn' is never read [clang-analyzer-deadcode.DeadStores]
                        insn = SH_INS_JMP;
                        ^      ~~~~~~~~~~
/home/runner/work/capstone/capstone/arch/SH/SHDisassembler.c:937:4: note: Value stored to 'insn' is never read
                        insn = SH_INS_JMP;
                        ^      ~~~~~~~~~~

cc @Rot127

@Rot127
Copy link
Collaborator

Rot127 commented Jun 15, 2024

@lhsazevedo Please test it with clang-tidy-18. The CI Ubuntu only has version 14 or something. Which reports false positives for the va_args.
If everything is fine with clang-tidy-18 it can be ignored.

@lhsazevedo
Copy link
Contributor Author

@Rot127 clang-tidy 18 also warns about uninitialized va_args. Perhaps it is a bug in clang-tidy?

/home/lhsazevedo/code/tweaking/capstone/arch/AArch64/AArch64Mapping.c:2162:14: warning: va_arg() is called on an uninitialized va_list [clang-analyzer-valist.Uninitialized]
 2162 |                 int Tile = va_arg(args, int);
/home/lhsazevedo/code/tweaking/capstone/arch/AArch64/AArch64Mapping.c:2211:18: warning: va_arg() is called on an uninitialized va_list [clang-analyzer-valist.Uninitialized]
 2211 |                 int8_t First = va_arg(args, int);
/home/lhsazevedo/code/tweaking/capstone/arch/SH/SHDisassembler.c:102:9: warning: va_arg() is called on an uninitialized va_list [clang-analyzer-valist.Uninitialized]
  102 |                 grp = va_arg(g, sh_insn_group);

There are more warnings for SHDisassembler, but they all seem to be unrelated to this change

@Rot127
Copy link
Collaborator

Rot127 commented Jun 18, 2024

Sorry, those AArch64 ones can be ignored. I fixed them in the AArch64 update. Just check the SH one please.

@lhsazevedo
Copy link
Contributor Author

@Rot127, I haven't had time to check the SH disassembler yet as the warnings are from a different part of the code. I'll get on it as soon as I can. Thanks for your patience!

@Rot127
Copy link
Collaborator

Rot127 commented Aug 16, 2024

Little reminder :)

Sign extend using bitwise OR with mask, instead of unary minus.
Fixes error when building for UWP with Security Development Lifecycle (SDL).
See https://learn.microsoft.com/en-us/cpp/build/reference/sdl-enable-additional-security-checks?view=msvc-170
Remove unused store caught by clang-tidy.
Ignore an unitialized va_list false positive emitted by clang-tidy
@lhsazevedo
Copy link
Contributor Author

lhsazevedo commented Aug 19, 2024

@Rot127 Done! Sorry for the delay. There were just two warnings, and one turned out to be a false positive.


P.S.: I noticed SH isn't listed in the auto-sync progress tracker. Any thoughts on how challenging its refactor might be?

@lhsazevedo lhsazevedo requested a review from XVilka August 19, 2024 21:24
@Rot127
Copy link
Collaborator

Rot127 commented Aug 20, 2024

P.S.: I noticed SH isn't listed in the auto-sync progress tracker. Any thoughts on how challenging its refactor might be?

SH is not supported by LLVM. And Auto-Sync works exclusively on LLVM.
I could not find a fork which implements it. If you aware of one, please let me know and I take a look.

arch/SH/SHDisassembler.c Outdated Show resolved Hide resolved
@lhsazevedo lhsazevedo requested a review from Rot127 August 20, 2024 14:24
Copy link
Collaborator

@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.

@Rot127
Copy link
Collaborator

Rot127 commented Aug 31, 2024

@lhsazevedo Ok, so @kabeor and me decided to suppress the warnings. Sorry for the unclear requests. Could you please reset to the previous commit and force push.

@lhsazevedo
Copy link
Contributor Author

Of course, sure

@Rot127
Copy link
Collaborator

Rot127 commented Sep 1, 2024

Mh, ok. So I think with the update clang-format-18 we get now some new stuff:

 /home/runner/work/capstone/capstone/arch/SH/SHDisassembler.c:1960:9: note: The value '29' provided to the cast expression is not in the valid range of values for 'sh_dsp_insn_type'
 1960 |                                                                 (sh_dsp_insn_type) SH_INS_DSP_PWAD,

I am sorry this PR takes so much longer although it is so simple. We try to improve code quality overall. If you could fix this issues there (multiple occurrences of casting an instr. enum to a type enum? No idea why, probably typos no one saw) I would really appreciate it.

@Rot127
Copy link
Collaborator

Rot127 commented Sep 6, 2024

@lhsazevedo Ignore my comment above. I'll fix the issues in #2469
@kabeor This is ready to be merged.

@lhsazevedo
Copy link
Contributor Author

Ok!

Copy link
Member

@kabeor kabeor left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions!

@kabeor kabeor merged commit 67d9756 into capstone-engine:next Sep 18, 2024
19 checks passed
@lhsazevedo lhsazevedo deleted the fix/sh-sign-extension-next branch September 18, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SH Arch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants