Skip to content

Conversation

@borneoa
Copy link

@borneoa borneoa commented May 7, 2025

The pull request #858 introduces four typedef for some struct in the generated 'debug_defines.h'
Using typedef is not welcome by every project that should include these generated files.
For example, OpenOCD coding style requires not adding new typedef.

This series:

  • replaces the typedefs in 'debug_defines.h' with simple 'struct struct_name';
  • still defines the old typedef, so existing code will still compile without changes;
  • modify the example code in 'debug_reg_printer.c' to use the new data types;
  • drop one unused parameter in 'debug_reg_printer.c' that triggers a compile warning.

I still have doubts if I should keep the backward compatibility. Please let me know!

borneoa added 3 commits May 7, 2025 16:16
The pull request riscv#858 introduces four typedef in debug_defines.h
that are not welcome by the coding style of project aimed at using
these generated files, e.g. by OpenOCD coding style.

Since all the typedef are for C struct, simply define the struct
with their name and, for temporarily backward compatibility, keep
defining the typedef but mark then as deprecated in a comment.
Drop the suffix '_t' used by the new types.
Only for 'riscv_debug_reg_field_list_t', that had same name for
struct and typedef, add a #define to handle both use cases.

Signed-off-by: Antonio Borneo <[email protected]>
Drop the use of types defined with typedef and use the names of
the struct.

Signed-off-by: Antonio Borneo <[email protected]>
The local function riscv_debug_reg_field_to_s() has a parameter
not used and this causes the compiler to issue a warning.

Drop the unused parameter.

Signed-off-by: Antonio Borneo <[email protected]>
Copy link
Contributor

@en-sc en-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borneoa, thank you for the change!

One more question: should this stileguide be documented somewhere?

"\tstruct riscv_debug_reg_field_list (*get_next)(struct riscv_debug_reg_ctx context);\n" +
"};\n" +
"/* deprecated, prefer 'struct riscv_debug_reg_field_list' to 'struct riscv_debug_reg_field_list_t' */\n" +
"#define riscv_debug_reg_field_list_t riscv_debug_reg_field_list\n" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's okay to break this interface from version to version since it's not the part of the spec itself.
@rtwfroody, @pdonahue-ventana, could you please share your opinion?

@en-sc
Copy link
Contributor

en-sc commented Sep 29, 2025

@pdonahue-ventana, this change does not affect the specification, so perhaps it can be merged? What is the appropriate process for such changes?

@pdonahue-ventana
Copy link
Collaborator

@ved-rivos: What's the process for handling this PR which affects only autogenerated .h files but not the spec itself?

Does anyone besides OpenOCD rely on these files?

I wonder if this script should be moved out to another project which then refers to this one (as a submodule or via some other mechanism). Then the script can change at any time without the need to make changes here.

@ved-rivos
Copy link
Contributor

@pdonahue-ventana if the .h file and/or the script needs an update to match the ratified spec then we should update to fix that issue.

@pdonahue-ventana
Copy link
Collaborator

The .h file changes have more to do with C syntax than with the actual debug spec content. This comment basically summarizes it: prefer 'struct riscv_debug_reg_info' to 'riscv_debug_reg_info_t' (i.e. use this new type rather than the old type, though both contain the same fields)

The change does, for example, have typedef struct riscv_debug_reg_field_info riscv_debug_reg_field_info_t so it's presumably backward-compatible with code that uses the old name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants