Fix GCC warnings

[elagil]

Detailed description

I build with GCC's -Werror, so I needed to fix a few warnings.

• Assert that constants are #defined, before checking their value
• Fixes for function declarations not being prototypes (missing void argument)
• Added a missing NO_LIBOPENCM3 check, when aiming to disable compilation of the library

Your checklist for this pull request

• I've read the Code of Conduct
• I've read the guidelines for contributing to this repository
• It builds for hardware native (make PROBE_HOST=native)
• It builds as BMDA (make PROBE_HOST=hosted)
• I've tested it to the best of my ability
• My commit messages provide a useful short description of what the commits do

[dragonmux]

We're unsure which GCC you're building with and what extra options you might be specifying, but if you could please let us know that extra information it would help us review this proposed change set. That's because even with GCC 13 on our Arch systems, we do not get any warnings (turned into errors) so this raises some interesting questions.

Aside from that, please could you rework your commit messages to comply with the contribution guidelines.

We'll endeavour to do a proper review as soon as we can once we have the requested extra information.

[elagil]

I changed the commit messages to start with misc:.

You are right, I should have specified the build settings. I am not using the BMP build system directly, but my own Makefile with some of the strictest possible GCC settings, namely:
-Wall -Wextra -Wundef -Wstrict-prototypes -Werror

I am on arm-none-eabi-gcc (Arm GNU Toolchain 13.2.rel1 (Build arm-13.7)) 13.2.1 20231009

Indeed, it is not -Werror causing this, but -Wundef and -Wstrict-prototypes. Most of the existing codebase passes and adheres to these standards.

Especially -Wundef is a good idea, because undefined variables default to 0, which might not be intended.

[dragonmux]

-Wundef only impacts preprocessor macros and them defaulting to 0 is both intended and correct in this code base.

Having it as a -Wno-error='d warning is fine, but we're not sure, at least in the current build system, it makes sense to 'fix' with code-side work over trying to get -Wconversion -Wsign-conversion -Warith-conversion enabled which would allow us to crack down on undefined behaviour invocation and logic bugs.

We'll have a think about that particular warning and the resulting code changes and get back to you though as it might be some of the changes actually migrate to the build system (setting -DENABLE_DEBUG=0 on the compiler command line, for example).

We would recommend adding to your build options -Wpedantic in conjunction with -std=c11 (note, not gnu11, that does something different and is not completely compatible with the codebase).

This all links up with issue #1590 which gives an overview of the progress that's being made toward a much stricter set of warnings (and because of -Werror, errors) being enabled to cover the multitude of crimes and bugs the compiler is able to pick up on and tell us about.

[elagil]

Since you mention ENABLE_DEBUG, its use is rather inconsistent.

In some places, it is checked for with #if defined(ENABLE_DEBUG), while in others just with #if ENABLE_DEBUG, or even #if ENABLE_DEBUG == 1. If -DENABLE_DEBUG shall enable debugging, it makes sense to check for #if defined(ENABLE_DEBUG), and not a particular value.

[dragonmux]

Aye, the result of many hands over many years all doing things slightly differently. In principal, #if ENABLE_DEBUG means the same thing as #if ENABLE_DEBUG == 1, and it's this style of check for the macro that is preferred in all new code. It's on the list of things to sort out to standardise to this.

[elagil]

As I understood it:

• #if ENABLE_DEBUG == 1 is the preferred way of checking for debug mode
• -Wundef is not a desired option
• -Wstrict-prototypes is not of interest? You mostly use strict prototypes, there are only very few instances of not using them. Having that consistent might make sense.

I can adjust this PR accordingly, and refactor the use of ENABLE_DEBUG across the codebase.

[dragonmux]

As I understood it:

• #if ENABLE_DEBUG == 1 is the preferred way of checking for debug mode

Correct

• -Wundef is not a desired option

We need to think on it, it might be that actually it makes a lot of sense to stop the current ENABLE_DEBUG mess from reoccurring - but it's late here, so we want to do that thinking after we've had sleep.

• -Wstrict-prototypes is not of interest? You mostly use strict prototypes, there are only very few instances of not using them. Having that consistent might make sense.

It is of interest, and that's actually why we didn't say anything about it - there's nothing contentious there.

I can adjust this PR accordingly, and refactor the use of ENABLE_DEBUG across the codebase.

If you could particularly deal with the ENABLE_DEBUG consistency, that would be a much appreciated contribution in addition to the strict prototypes bit. We'll ping a message on this PR in the morning once we've had a deep think on -Wundef and had a chat with Esden to see what direction he wants to take there.

[elagil]

Great, looking forward to it!

[dragonmux]

Sorry it's taken a bit more than one day to get back to you on this. Having thought on the subject of -Wundef, what we're going to suggest is that: macro usage triggering the warning should be fixed, but by fixing the build system to be more consistent rather than fixing the code to check for the macros being defined. Our thinking is this: If the macros in question are always defined to either 0 or 1, it makes the checks they're involved in not only simpler but more robust for contributors to get right. It allows the project to be perfectly consistent.

Our other ask is that the -Wundef related changes go in a separate commit to the strict prototypes fixes - this way if something is wrong or has to be revisited we can more effectively git blame and git revert those specific changes. This isn't to say that it'll be required, but more that if it is, it will be better to have such a split.

[elagil]

No worries. I will make the changes in the next few days, so they can be reviewed.

[dragonmux]

Checking back in with this as it's been a couple of weeks - it'd be nice to get this merged as we've hit some of the warnings this fixes over on the Meson feature PR.

[elagil]

Ok, I have not gotten around to it yet. I will prioritize it.

[elagil]

I think it's done. Let me know what you think.

[dragonmux]

This is looking considerably better. While there are several review items, they're mostly targeting the same one issue (excessive use of parens where they can be dropped and avoided) that should be quite an easy (fast) fix. Once these have been addressed, we'll be happy to merge.

[elagil]

I made these adjustments: https://github.com/blackmagic-debug/blackmagic/pull/1679/files/9b6a5794e75b9baf40e6ce87759f7a1f004f8baf..054499a3d5288a4558b6bf312c8ed6671e830d27

[dragonmux]

LGTM, merging once the CI builds complete. Thank you for the contribution! It's greatly appreciated 😄

[elagil]

Great, thanks for merging! 🥳