Skip to content

Conversation

@kazarmy
Copy link
Member

@kazarmy kazarmy commented Nov 29, 2021

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

Like the Linux gcc builds, the Linux Clang builds should also be done under --werror.

Test plan

All builds are green.

Closing issues

...

@kazarmy kazarmy marked this pull request as ready for review November 29, 2021 12:35
@@ -1,4 +1,4 @@
project('capstone', 'c', meson_version: '>=0.55.0')
project('capstone', 'c', meson_version: '>=0.55.0', default_options: ['werror=false'])
Copy link
Member

Choose a reason for hiding this comment

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

was this due to some specific clang warning for capstone? I think we already compiled with werror with gcc and capstone-bundled builds fine there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

s_cpu_type
(https://github.com/rizinorg/rizin/runs/4351274434?check_suite_focus=true#step:12:697)

Apparently that s_cpu_type variable is only used in a CS_ASSERT() call and therefore in a release build, the CS_ASSERT()s disappear and that variable is left hanging.

I would ofc prefer fixing that upstream but it can take time for such fixes to merge and any new Clang warnings that pop up in Rizin code should be caught immediately (such as the warning in #2042).

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently that s_cpu_type variable is only used in a CS_ASSERT() call and therefore in a release build, the CS_ASSERT()s disappear and that variable is left hanging.

Wait the above is not correct:

if (M680X_CPU_TYPE_ENDING != ARR_SIZE(s_cpu_type)) {
    CS_ASSERT(M680X_CPU_TYPE_ENDING == ARR_SIZE(s_cpu_type));

    return CS_ERR_MODE;
}

Now the warning looks like a false positive so perhaps I should just disable the warning for Clang.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm now I think that Clang is justified in issuing that warning because the contents of s_cpu_type is not used.

@kazarmy kazarmy marked this pull request as draft November 29, 2021 13:29
Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

It's alright. Thanks for explaining.
ANyway, it's not our code so it is not a huge deal if there are warnings there.

@kazarmy kazarmy marked this pull request as ready for review November 29, 2021 14:22
@kazarmy kazarmy merged commit ffc0b9c into dev Nov 29, 2021
@kazarmy kazarmy deleted the asan-clang-werror branch November 29, 2021 14:37
@XVilka
Copy link
Member

XVilka commented Nov 29, 2021

@ret2libc @kazarmy I sent a PR long time ago in capstone and it was merged in "next": capstone-engine/capstone@c93fa3a

@kazarmy
Copy link
Member Author

kazarmy commented Dec 1, 2021

Ok capstone-engine/capstone#1801 is the cherry-pick pr for "v4".

@kabeor
Copy link

kabeor commented Dec 2, 2021

Hello guys, I have merged capstone-engine/capstone#1801. And btw, we are planning to pre-release capstone v5 base on next branch soon, so v4 will be deprecated. Welcome to PR or issues to next branch, I'll try my best to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants