Fix: -Wformat in gdbserver (gdb_main, command, gdb_hostio)

[ALTracer]

Detailed description

• This is not a functional feature; rather, an attempt to reduce unexpected behaviour, especially in BMF.
• The problem is many printf-like calls in BMD core code are not checked by compiler for their correctness.
• This pull request solves this problem by marking the printf-like functions with format attributes. Where applicable, this PR also tries to fix appearing warnings.

I deal here only with gdb_voutf() and gdb_putpacket_f(), as their direct usage (with dubious specificators, specifically) is less numerous. Let someone else deal with all the tc_printf().

I am build-testing the changes for native on Linux amd64, and for swlink which is armv7-m. Runtime testing pending -- there is no code coverage tooling, so I have to reason about which aspects need testing, like GDB remote monitor commands for rtt options and semihosting heapinfo.

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

Closing issues

Partially fixes #1675. See that for a more detailed description.

[dragonmux]

There are a couple of items picked up in review, but with them taken care of this is looking excellent and we'll be happy to merge it. Once it is, we'll take a look at the tc_printf() side of this as that'd be a nice thing to get ironed out

[koendv]

The purpose of "mon heapinfo" is setting values for newlib/libc/sys/arm/crt0.S to pick up.
I think few use this. Maybe we can simply drop the command, and see if anyone shouts?

If changing to %x saves a few bytes, fine. If wishing to save more bytes, perhaps:

        } else
-               gdb_outf("heapinfo heap_base heap_limit stack_base stack_limit\n");
+               gdb_outf("?\n");
        return true;
 }

[dragonmux]

Ahh, that's good to know why it exists - perhaps that could be added as a comment in the code documenting its purpose and function + what it interfaces with? It's not currently obvious or noted anywhere in the source.

[ALTracer]

perhaps that could be added as a comment in the code documenting its purpose

@dragonmux @koendv As enabling attribute format for gdb_voutf triggers diagnostics on cmd_heapinfo, maybe it's better to create a new separate smaller PR for heapinfo changes? And make it a prerequisite for this PR (which is not urgent), so that builds keep passing.
That will need to contain "command: Convert format-string in heapinfo" change and optionally usage message changes, other code-annotating comments related to semihosting/heapinfo.

I think few use this. Maybe we can simply drop the command, and see if anyone shouts?

No need to drop existing tested proved working functionality.

It can also decouple me from the authors who touched that code, if Koen is okay with dedicating some time to fixing printf-correctness in heapinfo (and probably RTT).
To observe -Werror build failure logs, cherry-pick 1st/4th commit and build with or without ENABLE_RTT=1.

[koendv]

heapinfo can be used to quickly test a system with different heap and stack values, to see how much heap and stack is needed.
The mechanism is like this:

• user sets up values for heap and stack using "mon heapinfo"
• when the target boots, crt0.S does a heapinfo semihosting call to get these values for heap and stack. See newlib/libc/sys/arm/crt0.S comment: /* Issue Angel SWI to read stack info. */
• if the system crashes you need to increase heap or stack

The code in arm/crt0.S is conditionally compiled #ifdef ARM_RDI_MONITOR.

I think the "heapinfo" command is a good candidate for adding to the 2.0 build system. How about "#ifdef ENABLE_HEAPINFO", with default of 0?

[koendv]

It can also decouple me from the authors who touched that code, if Koen is okay with dedicating some time to fixing printf-correctness in heapinfo (and probably RTT).
To observe -Werror build failure logs, cherry-pick 1st/4th commit and build with or without ENABLE_RTT=1.

Have taken a look, will make PR.

[dragonmux]

I think the "heapinfo" command is a good candidate for adding to the 2.0 build system. How about "#ifdef ENABLE_HEAPINFO", with default of 0?

That'd be a KConfig style option and we're not yet sure what the macro format for those is going to look like exactly - the ENABLE_ macros we introduced are placeholders to make things work in the current build system.

We're not certain turning this command off in some builds is going to be a good idea over just.. documenting what it exists for in the source. Perhaps though - needs some careful thought and consultation with Esden before we can give a firm direction.

[koendv]

ok. done, 60 bytes saved in heapinfo, 66 bytes saved in rtt.

[ALTracer]

Have taken a look, will make PR.

Thank you very much. Rebased this PR branch on main (by cherry-picking), omitted / rebased away the two rtt/heapinfo draft commits as dealt separately in dedicated PRs.

[dragonmux]

Looks like all the big review items have been solved. We spotted one mistake in this round, a simple x vs X issue in gdb_main.c, but with that fixed we think this is ready for merge.

[ALTracer]

Even though I briefly tested these two PRs combined, I strongly recommend merging #1686 first, so that PRIx32 impact can be tested/measured on an actually working in-BMP implementation.

[dragonmux]

LGTM, we'll merge this after #1686 as suggested as we agree on that order of operations point for testing.

[ALTracer]

Rebased away the FIXME comments I left in headers; and rephrased the latest commit message to stay under 70 characters limit of most Git UIs. I don't follow the 50/72 rule (yet), but especially GitHub web interface likes to mangle long subjects.
Cheers to the next developer doing the tc_printf() refactor.

[dragonmux]

Excellent, we'll get this merged then. Thank you for all your hard work and the contribution!