command: Convert format-string in heapinfo

[koendv]

Detailed description

This PR

• replaces the "%p" format with "%x", keeping existing functionality, but saving a few bytes flash.
• documents how to use "mon heapinfo"
• see Fix: -Wformat in gdbserver (gdb_main, command, gdb_hostio) #1676

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

[dragonmux]

There are a couple of relatively small (easily solved) issues we've spotted in review, but otherwise this looks good as a change. We'll be glad to get this merged once the review items have been addressed as amendments to the existing commit (not new commits).

[dragonmux]

Please drop the extra new line so InteliSense and other tools see the comment as documentation for the function.

[koendv]

OK.
Every command has a one-line help message stored in cmd->help.
Maybe some bytes could be saved if a command, instead of printing a help message and exiting, could signal back: print the one-line help message for my command and exit.

[dragonmux]

That's a usage message, but due to space constraints doesn't detail what the command is, it's purpose for existing, etc.. think you meant to reply to the above review item on the "?\n" change though.

Assuming we're right on that, it's something we've considered before but doing that well is going to be complicated - and not just because it changes so many commands across the code base, but because sometimes the message to be printed needs to be contextual. It's a good idea though to explore.

[koendv]

I was thinking of something like:

diff --git a/src/command.c b/src/command.c
index afae27be..d6fbc7ea 100644
--- a/src/command.c
+++ b/src/command.c
@@ -154,8 +154,14 @@ int command_process(target_s *const t, char *const cmd_buffer)
                /* Accept a partial match as GDB does.
                 * So 'mon ver' will match 'monitor version'
                 */
-               if ((argc == 0) || !strncmp(argv[0], cmd->cmd, strlen(argv[0])))
-                       return !cmd->handler(t, argc, argv);
+               if ((argc == 0) || !strncmp(argv[0], cmd->cmd, strlen(argv[0]))) {
+                       bool retval = cmd->handler(t, argc, argv);
+                       if (retval == 2) {
+                               gdb_out(cmd->help);
+                               return 0;
+                       }
+                       return !retval;
+               }
        }
 
        if (!t)

so a command could do a return 2 to ask for the help message to be printed.

[dragonmux]

That could work for a simple version of the implementation - though the parens around the argc check need dropping and you'd need to use something other than bool for retval as bool is not allowed to have an inspectable numerical value other than 0 and not-0 w/ not-0 being coerced to 1 when converted back to integer.

[ALTracer]

I like the idea, but this will require changing the bool (*cmd_handler_fn)(target_s *target, int argc, const char **argv) function-signature and consequently all the commands just so they can return 2. I'm more inclined towards using identical message strings. See PR1522 for an example of gcc/ld.bfd folding many identical constant string literals.

[ALTracer]

Suggested change

	gdb_outf("heapinfo heap_base heap_limit stack_base stack_limit\n");
	gdb_outf("%s\n", "Set semihosting heapinfo: HEAP_BASE HEAP_LIMIT STACK_BASE STACK_LIMIT");

Such that we reuse the Usage/help message but still can stick a newline after it. Maybe even "Usage: %s\n".

[koendv]

Such that we reuse the Usage/help message but still can stick a newline after it.

That's a good one. Maybe even pass the help string to the command?

--- a/src/command.c
+++ b/src/command.c
@@ -155,7 +155,7 @@ int command_process(target_s *const t, char *const cmd_buffer)
                 * So 'mon ver' will match 'monitor version'
                 */
                if ((argc == 0) || !strncmp(argv[0], cmd->cmd, strlen(argv[0])))
-                       return !cmd->handler(t, argc, argv);
+                       return !cmd->handler(t, argc, argv, cmd->help);
        }
 
        if (!t)

[dragonmux]

The current patch as it stands looks good to us - addressing all the review points. We're going to merge it as it stands to clear the way for #1676.

Further improvements to the command system to allow either the commands to take their help strings as a parameter, or for them to return if they want their usage displayed can be worked out in a separate PR that addresses the problem across all commands. Until a particular direction is agreed, may we suggest making an issue ticket for that so we can put all the conversation for it in one spot?