runners: promote --no-load argument into common code and add support in stlink_gdbrunner

[mathieuchopstm]

Promote the existing --no-load argument from OpenOCD/Intel Cyclone V runners into common runner code. This argument inhibits loading of image on target before a debug session, allowing west debug to effectively be used as a west attach-after-reset.

Also add support of this argument to the stlink_gdbserver runner.

[marc-hb]

The formatting fixes belong to a separate commit

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#multiple-commits-on-a-single-pr

This flag functions exactly like its identically-named counterpart in the OpenOCD runner: when present, the "load" command is not added to the GDB client's command line.

... duplication which points at the lack of a common function or class. You probably not interested in changing and testing the OpenOCD runner, but you can still create that common function / class and start using it only in stlink_gdbserver.py. Then it's ready for both existing and future runners to use.

EDIT: as a pure coincidence, here is a current example of moving generic code to generic places, away from product-specific code:

• #98443

[mathieuchopstm]

Moved to common runner code as suggested by @marc-hb and @pdgendt (inspired by #98443).

Help message uses a generic wording (instead of "don't send GDB load command") as other runners may perform load-image-on-target before starting their GDB server - --no-load would apply for these too, if they support a way to inhibit the mechanism.

[pdgendt]

Nice, thanks!

[mathieuchopstm]

Rebased to resolve conflict

[mathieuchopstm]

@marc-hb PTAL

[marc-hb]

I'm confused by this new debug_load capability: what happens when a future runner does not to have the capability? Does that runner -ex load or not? Does it depend on the specific runner? Inconsistently...

• Developers could assume "loading" is a capability. No capability = no load command issued.
• But they could assume instead that skipping the load is the capability. So, loading when no skip capability (similar to right now before this PR)

Generally speaking, having "two levels" of booleans adds complexity. So you could just drop the capability and force all runners to implement the option. It requires only one line of code in the runner. If some future runners can't support one of the two choices (cause they don't use gdb?), they could just fail when someone tries to unsupported choice.

Except... there isn't really a way to force runners to look at args.load, is there?

This is surprisingly more complicated that I thought.

[mathieuchopstm]

I'm confused by this new debug_load capability: what happens when a future runner does not to have the capability? Does that runner -ex load or not? Does it depend on the specific runner? Inconsistently...

I do agree the capability name is not great. Maybe something like noload / noload_debug / debug_without_load would be more understandable?

• Developers could assume "loading" is a capability. No capability = no load command issued.

Per "specification", west debug is expected to flash before starting the session:

$ west debug --help
usage: west debug [-h] [-d DIR] [-r RUNNER] [--domain DOMAIN] [--skip-rebuild | --rebuild | --no-rebuild] [-H] [--board-dir DIR] [--gdb GDB] [--openocd OPENOCD]
                  [--openocd-search DIR]

Connect to the board, flash the program, and start a debugging session. Use "west attach" instead to attach a debugger without reflashing.

Emphasis on flash the program, hence the comment in my code:

# by default, 'west debug' is expected to flash before starting the session

(to clarify again, the reason why west flash --no-load would be useful is that west attach merely starts a debugger, connects to and halts the target, whereas west debug also resets the target, breaks on entrypoint, ...)

Generally speaking, having "two levels" of booleans adds complexity. So you could just drop the capability and force all runners to implement the option. It requires only one line of code in the runner. If some future runners can't support one of the two choices (cause they don't use gdb?), they could just fail when someone tries to unsupported choice.

What I had in mind was instead tools which would (forcefully!) download an image before starting the GDB server, or tools which don't use GDB at all. Looking at tree for runners which support debug, most do currently merely append -ex load to GDB commandline, but there are some exceptions:

• Synopsys Metaware Debugger(mdb): seems to be its own thing entirely (i.e., not GDB)
  

  zephyr/scripts/west_commands/runners/mdb.py

  Lines 75 to 82 in 8c264d3

  	if command == 'flash':
  	if is_flash_cmd_need_exit_immediately(mdb_runner):
  	mdb_run = ['-run', '-cmd=-nowaitq run', '-cmd=quit', '-cl']
  	else:
  	mdb_run = ['-run', '-cl']
  	elif command == 'debug':
  	# use mdb gui to debug
  	mdb_run = ['-OKN']

• native: executes native binary - load/no-load wouldn't apply
• probe-rs: uses an opaque gdb argument for which I can't find documentation 🤷‍♂️
  

  zephyr/scripts/west_commands/runners/probe_rs.py

  Lines 167 to 187 in 8c264d3

  	self.check_call([self.probe_rs, 'download']
  	+ self.args + download_args)
  	if self.reset:
  	self.check_call([self.probe_rs, 'reset']
  	+ self.args)
  	def do_debug_debugserver(self, command, **kwargs):
  	'''Start GDB server or debug session using probe-rs gdb command.'''
  	debug_args = ['--gdb-connection-string', f"{self.gdb_host}:{self.gdb_port}"]
  	if command == 'debug':
  	if self.gdb_cmd is None:
  	raise ValueError('Cannot debug; gdb is missing')
  	debug_args += [self.elf_name]
  	debug_args += ['--gdb', self.gdb_cmd]
  	else:
  	# debugserver mode
  	self.logger.info(f'probe-rs GDB server running on port {self.gdb_port}')
  	self.check_call([self.probe_rs, 'gdb']
  	+ self.args + debug_args)

• spi_burn: doesn't seem to download image when west debug'ing?
• trace32: seems to be its own thing?
  

  zephyr/scripts/west_commands/runners/trace32.py

  Lines 131 to 142 in 8c264d3

  	def debug(self, **kwargs) -> None:
  	cmd = self.get_launch_command('debug')
  	self.logger.info(f'Launching TRACE32: {" ".join(cmd)}')
  	self.check_call(cmd)
  	def get_launch_command(self, command_name: str,
  	cfg: Path | None = None) -> list[str]:
  	cmd = [
  	str(self.t32_exec),
  	'-c', str(cfg if cfg else self.t32_cfg),
  	'-s', str(self.startup_dir / f'{command_name}.cmm')
  	]

Except... there isn't really a way to force runners to look at args.load, is there?

Yes, hence why it should be a capability if we follow what's already in tree.

[marc-hb]

Per "specification", west debug is expected to flash before starting the session

Thanks. I generally hate negative options like noload because they always lead to double (or triple) negations but I think this is a valid exception. I mean I think the capability should be debug_noload (instead of debug_load) because as you very clearly demonstrated, anything missing the capability does the load or equivalent. This new capability really gives the ability NOT to load, that's what it does!

Also, debug_noload will not cause double negations because the capability can be the only negative thing while the flag does not need any change, it can stay the same, "neutral" --load/--no-load and the default can stay True. The main thing does not need to change at all.

So this would become:

   debug_noload: whether the runner supports the --[no-]load option,
   which makes `west debug` skip the gdb `load` command or its equivalent.
   .
   .
   .
   parser.add_argument('--load', action=argparse.BooleanOptionalAction,
        help=("load image on target before 'west debug' session"
       if caps.debug_noload else argparse.SUPPRESS),
               default=True)

Very small change.

Makes sense?

[marc-hb]

Or, you could call it debug_skip_load if you hate negative capabilities even more than I do.

parser.add_argument('--load', action=argparse.BooleanOptionalAction,
        help=("load image on target before 'west debug' session"
       if caps.debug_skip_load else argparse.SUPPRESS),
            default=True)

[mathieuchopstm]

Renamed capability from debug_load to skip_load, dropping the debug_ prefix which adds little value. Also reworded the capability docstring text a little bit.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud