RTEMS | Draft: Feature/stack reporter config (!86)

[Joel Sherrill (@joel)]

Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86 was reviewed by Joel Sherrill

--
  
Joel Sherrill commented on a discussion on cpukit/libmisc/stackchk/check.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108702

 >      );
 > -    Stack_check_report_blown_task( heir, pattern_ok );
 > +    _Stack_checker_reporter_initialize( heir, pattern_ok );

It just occurred to me what might help. Using "initialize" in the name of the type or function name is bad and confusing. All names I suggest likely could still be better but it is a step in the right direction.

- `_Stack_checker_reporter_initialize`is a bad name for the reporter function or type.
  - `_Stack_checker_Reporter_handler` is better name for the type.
  - `_Stack_checker_Reporter` is a better name for the variable configured.
- Eventually providing the current blown task handler and a quieter alternative would be useful. Both will be prototyped in stackchk.h and implemented in check.c. Further
  - `_Stack_checker_Reporter_print_details` is a better name for the current reporter function. 
  - `_Stack_checker_Reporter_quiet` can be an alternative reporter implementation that JUST calls fatal error. 
 
Notice the use of _ and a capital letter to indicate grouping and item within that group. In C++, the variable might be Stack_Checker::Reporter.

--
  
Joel Sherrill started a new discussion on cpukit/include/rtems/confdefs/extensions.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108703

 >  #include <rtems/score/userextimpl.h>
 >  #include <rtems/sysinit.h>
 > +#include <rtems/confdefs/inittask.h>

Why was this needed?

--
  
Joel Sherrill started a new discussion on cpukit/include/rtems/confdefs/percpu.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108704

 >  
 > -#endif /* _RTEMS_CONFDEFS_PERCPU_H */
 > +#endif /* _RTEMS_CONFDEFS_PERCPU_H */

What changed here and why is there no newline at the end of the file?

--
  
Joel Sherrill started a new discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108705

 > + * still valid or not.
 > + */
 > +typedef void (*Stack_checker_reporter_extension)(

Put each parameter on its own line and ) on its own. See function prototypes above and follow them.

--
  
Joel Sherrill started a new discussion on cpukit/libmisc/stackchk/check.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108706

 >  }
 >  
 > +void rtems_stack_check_report_blown_task_custom(

This should be provided by the user application. It does not belong here. It could be part of your stackchk test to show it is possible.

--
  
Joel Sherrill started a new discussion on testsuites/libtests/stackchk03/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108707

 > +#ifdef HAVE_CONFIG_H

File header including SPDX, Doxygen, and license. Should be plenty of examples to copy from.

--
  
Joel Sherrill started a new discussion on testsuites/libtests/stackchk03/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108708

 > + */
 > +#define CONFIGURE_STACK_CHECKER_ENABLED
 > +

No need for blank lines between configuration parameters. Use blanks to group similar ones thouogh.

--
  
Joel Sherrill started a new discussion on testsuites/libtests/stackchk03/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108709

 > +#define CONFIGURE_STACK_CHECKER_ENABLED
 > +
 > +#define CONFIGURE_STACK_CHECKER_REPORTER

This should be defining the name of a custom reporter function which is part of this test.

--
  
Joel Sherrill started a new discussion on testsuites/libtests/stackchk03/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108710

 > +#define CONFIGURE_RTEMS_INIT_TASKS_TABLE
 > +
 > +

No double blank lines. No blanks between includes unless changing from POSIX ones to RTEMS ones to application specific one. Again, blank lines to indicate groups.

--
  
Joel Sherrill started a new discussion on testsuites/libtests/stackchk03/stackchk03.doc: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108711

 > +
 > +directives:
 > +  + rtems_stack_checker_switch_extention

Spelling.

--
  
Joel Sherrill started a new discussion on testsuites/libtests/stackchk03/stackchk03.doc: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108712

 > +# POSSIBILITY OF SUCH DAMAGE.
 > +#
 > +This file describes the directives and concepts tested by this test set.

Blank line between license and first text

--
  
Joel Sherrill started a new discussion on testsuites/libtests/stackchk03/stackchk03.scn: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108713

 > +*** TEST STACKCHK03 ***
 > +

No need for blank lines in this output

--
  
Joel Sherrill commented on a discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108714

 > + * still valid or not.
 > + */
 > +typedef void (*Stack_checker_reporter_initialize)(const Thread_Control *, bool);

I think I noted that this should be formatted like a function prototype.

--
  
Joel Sherrill commented on a discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108715

 > + * still valid or not
 > + */
 > +void rtems_custom_stack_check_reporter(

The stack checker should have a custom checker. The user application/test will provide that and it can be named anything. Yours could be "stackchk03_blown_stack_reporter" and it should work.

--
  
Joel Sherrill commented on a discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86#note_108716

 > + * Application provided via <rtems/confdefs.h>
 > + */
 > +extern const Stack_checker_reporter_initialize _Stack_checker_reporter_initialize;

This is still showing the type as `Stack_checker_reporter_initialize`. Hopefully that's just a gitlab artifact. I thought I saw this renamed.




-- 
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/86
You're receiving this email because of your account on gitlab.rtems.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20240705/04320010/attachment-0001.htm>