Re: [PATCH v2 25/29] Doxygen documentation added

[Alex Bennée]

Nicolas Eder <nicolas.eder@lauterbach.com> writes:

> From: neder <nicolas.eder@lauterbach.com>
>
> ---
>  include/exec/mcdstub.h       |   7 -
>  include/mcdstub/syscalls.h   |   2 +-
>  mcdstub/mcd_shared_defines.h |   1 +
>  mcdstub/mcdstub.c            |  61 ++--
>  mcdstub/mcdstub.h            | 579 ++++++++++++++++++++++++++++++++++-
>  mcdstub/mcdstub_common.h     |  18 ++
>  target/arm/mcdstub.c         |   6 -
>  target/arm/mcdstub.h         | 105 +++++++
>  8 files changed, 709 insertions(+), 70 deletions(-)
>
> diff --git a/include/exec/mcdstub.h b/include/exec/mcdstub.h
> index 84f7c811cb..9b7c31a951 100644
> --- a/include/exec/mcdstub.h
> +++ b/include/exec/mcdstub.h
> @@ -3,13 +3,6 @@
>  
>  #define DEFAULT_MCDSTUB_PORT "1235"
>  
> -/* breakpoint defines */
> -#define MCD_BREAKPOINT_SW        0
> -#define MCD_BREAKPOINT_HW        1
> -#define MCD_WATCHPOINT_WRITE     2
> -#define MCD_WATCHPOINT_READ      3
> -#define MCD_WATCHPOINT_ACCESS    4
> -
>  /**
>   * mcd_tcp_server_start: start the tcp server to connect via mcd
>   * @device: connection spec for mcd
> diff --git a/include/mcdstub/syscalls.h b/include/mcdstub/syscalls.h
> index 5547d6d29e..52a26be3a9 100644
> --- a/include/mcdstub/syscalls.h
> +++ b/include/mcdstub/syscalls.h
> @@ -3,4 +3,4 @@
>  
>  typedef void (*gdb_syscall_complete_cb)(CPUState *cpu, uint64_t ret, int 
> err);
>  
> -#endif /* _SYSCALLS_H_ */
> +#endif /* _MCD_SYSCALLS_H_ */
> diff --git a/mcdstub/mcd_shared_defines.h b/mcdstub/mcd_shared_defines.h
> index 5fbd732252..110d36d975 100644
> --- a/mcdstub/mcd_shared_defines.h
> +++ b/mcdstub/mcd_shared_defines.h
> @@ -38,6 +38,7 @@
>  /* tcp query arguments */
>  #define QUERY_FIRST "f"
>  #define QUERY_CONSEQUTIVE "c"
> +#define QUERY_END_INDEX "!"
>  
>  #define QUERY_ARG_SYSTEM "system"
>  #define QUERY_ARG_CORES "cores"
> diff --git a/mcdstub/mcdstub.c b/mcdstub/mcdstub.c
> index 7613ed2c4a..a468a7d7b8 100644
> --- a/mcdstub/mcdstub.c
> +++ b/mcdstub/mcdstub.c
> @@ -218,12 +218,6 @@ int find_cpu_clusters(Object *child, void *opaque)
>          s->processes = g_renew(MCDProcess, s->processes, ++s->process_num);
>  
>          process = &s->processes[s->process_num - 1];
> -
> -        /*
> -         * GDB process IDs -1 and 0 are reserved. To avoid subtle errors at
> -         * runtime, we enforce here that the machine does not use a cluster 
> ID
> -         * that would lead to PID 0.
> -         */
>          assert(cluster->cluster_id != UINT32_MAX);
>          process->pid = cluster->cluster_id + 1;
>          process->attached = false;
> @@ -925,7 +919,7 @@ void mcd_vm_state_change(void *opaque, bool running, 
> RunState state)
>  
>  int mcd_put_packet(const char *buf)
>  {
> -    return mcd_put_packet_binary(buf, strlen(buf), false);
> +    return mcd_put_packet_binary(buf, strlen(buf));
>  }
>  
>  void mcd_put_strbuf(void)
> @@ -933,7 +927,7 @@ void mcd_put_strbuf(void)
>      mcd_put_packet(mcdserver_state.str_buf->str);
>  }
>  
> -int mcd_put_packet_binary(const char *buf, int len, bool dump)
> +int mcd_put_packet_binary(const char *buf, int len)
>  {
>      for (;;) {
>          g_byte_array_set_size(mcdserver_state.last_packet, 0);
> @@ -999,12 +993,12 @@ MCDProcess *mcd_get_process(uint32_t pid)
>      return NULL;
>  }
>  
> -CPUState *mcd_get_cpu(uint32_t i_cpu_index)
> +CPUState *mcd_get_cpu(uint32_t cpu_index)
>  {
>      CPUState *cpu = first_cpu;
>  
>      while (cpu) {
> -        if (cpu->cpu_index == i_cpu_index) {
> +        if (cpu->cpu_index == cpu_index) {
>              return cpu;
>          }
>          cpu = mcd_next_attached_cpu(cpu);
> @@ -1344,15 +1338,13 @@ void handle_open_core(GArray *params, void *user_ctx)
>  
>  void handle_query_reset_f(GArray *params, void *user_ctx)
>  {
> -    /* TODO: vull reset over the qemu monitor */
> -
>      /* 1. check length */
>      int nb_resets = mcdserver_state.resets->len;
>      if (nb_resets == 1) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
> -        g_string_printf(mcdserver_state.str_buf, "1!");
> +        g_string_printf(mcdserver_state.str_buf, "1%s", QUERY_END_INDEX);
>      }
>      /* 2. send data */
>      mcd_reset_st reset = g_array_index(mcdserver_state.resets, mcd_reset_st, 
> 0);
> @@ -1370,7 +1362,7 @@ void handle_query_reset_c(GArray *params, void 
> *user_ctx)
>      int nb_groups = mcdserver_state.resets->len;
>      if (query_index + 1 == nb_groups) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
>          g_string_printf(mcdserver_state.str_buf, "%u!", query_index + 1);
>      }
> @@ -1487,9 +1479,9 @@ void handle_query_mem_spaces_f(GArray *params, void 
> *user_ctx)
>      int nb_groups = memspaces->len;
>      if (nb_groups == 1) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
> -        g_string_printf(mcdserver_state.str_buf, "1!");
> +        g_string_printf(mcdserver_state.str_buf, "1%s", QUERY_END_INDEX);
>      }
>  
>      /* 3. send data */
> @@ -1522,7 +1514,7 @@ void handle_query_mem_spaces_c(GArray *params, void 
> *user_ctx)
>      int nb_groups = memspaces->len;
>      if (query_index + 1 == nb_groups) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
>          g_string_printf(mcdserver_state.str_buf, "%u!", query_index + 1);
>      }
> @@ -1555,9 +1547,9 @@ void handle_query_reg_groups_f(GArray *params, void 
> *user_ctx)
>      int nb_groups = reggroups->len;
>      if (nb_groups == 1) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
> -        g_string_printf(mcdserver_state.str_buf, "1!");
> +        g_string_printf(mcdserver_state.str_buf, "1%s", QUERY_END_INDEX);
>      }
>      /* 3. send data */
>      mcd_reg_group_st group = g_array_index(reggroups, mcd_reg_group_st, 0);
> @@ -1580,7 +1572,7 @@ void handle_query_reg_groups_c(GArray *params, void 
> *user_ctx)
>      int nb_groups = reggroups->len;
>      if (query_index + 1 == nb_groups) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
>          g_string_printf(mcdserver_state.str_buf, "%u!", query_index + 1);
>      }
> @@ -1604,9 +1596,9 @@ void handle_query_regs_f(GArray *params, void *user_ctx)
>      int nb_regs = registers->len;
>      if (nb_regs == 1) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
> -        g_string_printf(mcdserver_state.str_buf, "1!");
> +        g_string_printf(mcdserver_state.str_buf, "1%s", QUERY_END_INDEX);
>      }
>      /* 3. send data */
>      mcd_reg_st my_register = g_array_index(registers, mcd_reg_st, 0);
> @@ -1637,7 +1629,7 @@ void handle_query_regs_c(GArray *params, void *user_ctx)
>      int nb_regs = registers->len;
>      if (query_index + 1 == nb_regs) {
>          /* indicates this is the last packet */
> -        g_string_printf(mcdserver_state.str_buf, "0!");
> +        g_string_printf(mcdserver_state.str_buf, "0%s", QUERY_END_INDEX);
>      } else {
>          g_string_printf(mcdserver_state.str_buf, "%u!", query_index + 1);
>      }
> @@ -1672,19 +1664,8 @@ void handle_query_state(GArray *params, void *user_ctx)
>       * get state info
>       */
>      mcd_cpu_state_st state_info = mcdserver_state.cpu_state;
> -    mcd_core_event_et event = MCD_CORE_EVENT_NONE;
> -    if (state_info.memory_changed) {
> -        event = event | MCD_CORE_EVENT_MEMORY_CHANGE;
> -        state_info.memory_changed = false;
> -    }
> -    if (state_info.registers_changed) {
> -        event = event | MCD_CORE_EVENT_REGISTER_CHANGE;
> -        state_info.registers_changed = false;
> -    }
> -    if (state_info.target_was_stopped) {
> -        event = event | MCD_CORE_EVENT_STOPPED;
> -        state_info.target_was_stopped = false;
> -    }
> +    /* TODO: add event information */
> +    uint32_t event = 0;
>      /* send data */
>      g_string_printf(mcdserver_state.str_buf,
>          "%s=%s.%s=%u.%s=%u.%s=%u.%s=%lu.%s=%s.%s=%s.",
> @@ -1863,7 +1844,7 @@ void handle_write_memory(GArray *params, void *user_ctx)
>          mcd_put_packet(TCP_EXECUTION_ERROR);
>          return;
>      }
> -    /* read memory */
> +    /* write memory */
>      mcd_hextomem(mcdserver_state.mem_buf, mcdserver_state.str_buf->str, len);
>      if (mcd_write_memory(cpu, mem_address,
>          mcdserver_state.mem_buf->data, len) != 0) {
> @@ -1879,7 +1860,7 @@ int mcd_breakpoint_insert(CPUState *cpu, int type, 
> vaddr addr)
>      int bp_type = 0;
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      if (cc->gdb_stop_before_watchpoint) {
> -        //bp_type |= BP_STOP_BEFORE_ACCESS;
> +        /* bp_type |= BP_STOP_BEFORE_ACCESS; */
>      }
>      int return_value = 0;
>      switch (type) {
> @@ -1909,7 +1890,7 @@ int mcd_breakpoint_remove(CPUState *cpu, int type, 
> vaddr addr)
>      int bp_type = 0;
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      if (cc->gdb_stop_before_watchpoint) {
> -        //bp_type |= BP_STOP_BEFORE_ACCESS;
> +        /* bp_type |= BP_STOP_BEFORE_ACCESS; */
>      }
>      int return_value = 0;
>      switch (type) {
> diff --git a/mcdstub/mcdstub.h b/mcdstub/mcdstub.h
> index d3f15da180..5412b59423 100644
> --- a/mcdstub/mcdstub.h
> +++ b/mcdstub/mcdstub.h
> @@ -12,20 +12,6 @@
>  #define MCD_TRIG_OPT_DATA_IS_CONDITION 0x00000008
>  #define MCD_TRIG_ACTION_DBG_DEBUG 0x00000001
>  
> -typedef uint32_t mcd_core_event_et;
> -/* TODO: replace mcd defines with custom layer */
> -enum {
> -    MCD_CORE_EVENT_NONE            = 0x00000000,
> -    MCD_CORE_EVENT_MEMORY_CHANGE   = 0x00000001,
> -    MCD_CORE_EVENT_REGISTER_CHANGE = 0x00000002,
> -    MCD_CORE_EVENT_TRACE_CHANGE    = 0x00000004,
> -    MCD_CORE_EVENT_TRIGGER_CHANGE  = 0x00000008,
> -    MCD_CORE_EVENT_STOPPED         = 0x00000010,
> -    MCD_CORE_EVENT_CHL_PENDING     = 0x00000020,
> -    MCD_CORE_EVENT_CUSTOM_LO       = 0x00010000,
> -    MCD_CORE_EVENT_CUSTOM_HI       = 0x40000000,
> -};
> -

None of these changes have to do with adding documentation.

>  /* schema defines */
>  #define ARG_SCHEMA_QRYHANDLE 'q'
>  #define ARG_SCHEMA_STRING 's'
> @@ -187,88 +173,649 @@ static inline int tohex(int v)
>  void mcd_sigterm_handler(int signal);
>  #endif
>  
> +/**
> + * \defgroup mcdstub Main mcdstub functions
> + * All architecture independent mcdstub functions.
> + */
> +
> +/**
> + * \addtogroup mcdstub
> + * @{
> + */
> +
> +/**
> + * \brief Initializes the mcdserver_state struct.
> + *
> + * This function allocates memory for the mcdserver_state struct and sets
> + * all of its members to their inital values. This includes setting the
> + * cpu_state to halted and initializing the query functions with \ref
> + * init_query_cmds_table.
> + */

We already have a documentation standard for functions and it is kdoc:

  https://www.kernel.org/doc/html/v5.0/doc-guide/kernel-doc.html

which is integrated into our documentation system. Please use that.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro