kernel: Remove unused absolute symbols

[peter-mitsis]

Removes unused absolute symbols (generated via the GEN_ABSOLUTE_SYM() macro) from both common kernel code and architecture specific code.

[carlocaione]

Uhm, not a big fan of this. The GEN_ABSOLUTE_SYM() macro is creating some defines that could (or could not) be used by downstream code that is relaying on those to be present.

I understand the cleanup frenzy but I think that leaving these in place makes no difference in code size or footprint so why deleting those and maybe creating havoc to downstream or out-of-tree projects?

[peter-mitsis]

@carlocaione - To be honest, that potential downstream impact is one that I had not previously considered. That being said, I don't think that the removal of these specific symbols is that large a concern (perhaps I am too close to this to see this objectively). However, should a downstream project require those symbols at assembly level, I would expect that they would most likely employ something similar to the following to get what they need ...

In a 'C' file ...
const size_t _structure_name_SIZE = sizeof(structure_name);

In the asembly file ...
GDATA(_structure_name_SIZE)

In the longer term, my aim is to eliminate the GEN_OFFSET_SYM and GEN_ABSOLUTE_SYM macros. If this can be done cleanly, then it sets the stage for simplifying the build process (as their use requires a separate compilation and custom python script just to create the defines used in assembly).

[carlocaione]

@carlocaione - To be honest, that potential downstream impact is one that I had not previously considered. That being said, I don't think that the removal of these specific symbols is that large a concern (perhaps I am too close to this to see this objectively).

Well, for example I have a downstream project relying on __z_arch_esf_t_SIZEOF so this commit would have definitely impacted me :)

Should we go through a deprecation process for this instead if we really (really) want to progress on this? This is definitely an API breaking change (though not an API per se).

However, should a downstream project require those symbols at assembly level, I would expect that they would most likely employ something similar to the following to get what they need ...

In a 'C' file ... const size_t _structure_name_SIZE = sizeof(structure_name);

In the asembly file ... GDATA(_structure_name_SIZE)

Right, but we still would need to generate _structure_name_SIZE somehow downstream, correct?

In the longer term, my aim is to eliminate the GEN_OFFSET_SYM and GEN_ABSOLUTE_SYM macros. If this can be done cleanly, then it sets the stage for simplifying the build process (as their use requires a separate compilation and custom python script just to create the defines used in assembly).

I really hope that this does not mean to give up on the *_OFFSET symbols given that those are used all over :)

[peter-mitsis]

Fixed the build errors resulting from __z_arch_esf_t_SIZEOF being used in riscv. (For some reason, during the first pass it showed up as not used--I must have searched the wrong directory with grep.) The current failure is, I think, spurious.

One objection thus far is that these symbols may be used in a downstream project. These symbols seem to fall into a bit grey zone. That is, to my knowledge they are not part of the official API, but we do have confirmation that from @carlocaione that some are used there.

I previously stated that one of my longer term goals is to get rid of the GEN_ABSOLUTE_SYM() and GEN_OFFSET_SYM() to allow the build to be simplified a bit. (This could be done using by replacing them with a mixture of global variables and pre-calculated offsets, and it would also preserve the ability to allow that information to be passed downstream--albeit with different names.) I don't know if this additional information would ameliorate any concerns that people may have.

[andyross]

Dropping a +1. I get the argument that there are compatibility concerns, but given that these are fundamentally just an access mechanism for struct fields and sizes, and that (with very few exceptions) we've never documented our struct formats as a proper API, it seems OK to me.

Maybe there's an argument for a few of them to deprecate for a release, but IMHO absent specific arguments this is an OK interface to break.

[carlocaione]

@peter-mitsis can we have a changelog detailing what was removed already in this PR?

[trond-snekvik]

The GEN_ABSOLUTE_SYM() macro is creating some defines that could (or could not) be used by downstream code that is relaying on those to be present.

Here's one✋ We were relying on ___thread_stack_info_t_size_OFFSET to present stack usage during debugging in Nordic's VS Code extensions.

I'll acknowledge that these aren't technically part of the API, so we were on shaky ground here, but these generated symbols have no alternative solution that is covered by the API.

I humbly ask you to please not delete any more of these without providing alternative ways of accomplishing the same use cases. They don't affect production binary sizes and the addition in build time and build system complexity is trivial.

There was a similar discussion in #53951, where some deprecated command line options were removed, then restored. There's definitely value in cleaning up stuff that is actively a burden on the maintainers, but I think this project is big enough that Hyrum's Law should be taken into consideration at this point.

[carlocaione]

lol, I called it.

@trond-snekvik do you have a way to re-introduce GEN_ABSOLUTE_SYM() without having to revert this?

[trond-snekvik]

@carlocaione we actually use nm to read this out of the elf file before launching the debug session. We can't insert our own code into the binary, so as far as I can tell, there are only two solutions: either inspect the type through some C parser or GDB, or hardcode the offset and wait for the day when someone inevitably changes the layout of that struct. The latter is the only realistic option for us now, I'm afraid.

[carlocaione]

@trond-snekvik what about only reintroducing back GEN_ABSOLUTE_SYM()?

[trond-snekvik]

@carlocaione that wouldn't do it, no. The symbol we were relying on was actually generated by GEN_OFFSET_SYM(), and technically it was #54446 that broke our workflow. I commented on this PR since this is where you started the conversation that applies to all these changes.

If anything, I think the offsets should be added to the thread info object here:

zephyr/subsys/debug/thread_info.c

Line 42 in 9f37ae9

size_t _kernel_thread_info_offsets[] = {

The downside is that this can't be read at compile time in the same way. Also, this object isn't part of the API either, and it's not used anywhere else in Zephyr, so I suppose there's nothing stopping anyone from merging a PR that deletes that without deprecation as well.

I don't expect that everything that ever existed in zephyr is kept around forever, I'm just hoping maintainers can keep in mind that any change to the observed behavior (and not API) will disrupt someone's workflow. That doesn't mean that the change shouldn't be made, it just means that it will always come at a cost.

[peter-mitsis]

I never noticed the file zephyr/subsys/debug/thread_info.c before--I learned something new today. :) Adding the value to the _kernel_thread_info_offsets[] array seems reasonable.

[peter-mitsis]

@trond-snekvik - Following up a little on this, I see that there is a pre-processor check that claims that this feature does not work properly with multiple CPUs. I am not sure if this is a make-or-break issue for you, but I thought that I would point it out.

If I am following things correctly, the reason for this limitation is the entry THREAD_INFO_OFFSET_K_CURR_THREAD--offset of the current thread. In an MP system, this entry corresponds only to the offset of the current thread on cpu[0]. Perhaps the list of offsets could be extended to indicate the size of the struct _cpu? This is new code to me, so I assume that there are subtleties about which I am unaware.

If you would like to file an enhancement request, it will ensure that this issue and potential solution is tracked.