Add symbols to library

[ollie-etl]

Work to address #156

In addition to #defines, static inline functions have no symbols in compiled library. This PR also addresses the more obvious candidates here.

[arnopo]

Please complete the commit message correctly as pointed out by the compliance checks.
The commit message has to explain your patch to help reviewer to understand it, and must contain a signed-off-by.

[edmooring]

Please update the commit message to meet the checkpatch requirements and to explain why these changes have been made.

As I see it, this pull request has two parts.

The first part brings the code into the 21st century, by replacing macros with inline functions. This is obviously a good idea.

The second part is what needs explanation. By making a bunch of functions globally visible, it changes the existing API and could cause problems for existing code. Please explain why this change is useful.

[ollie-etl]

I'll refactor the commits shortly to meet the compiance checks. Let me explain the rational here.

I use libmetal in a FFI environment (linking from rust). Functions defined as #defines, or as static inlines in headers are useless in this context. The compiled library simply does not contain these symbols.

So, this PR does 2 things

• it defines the #defines as functions where sensible to do so.
• for some of the functions in io.h, it marks some functions as inline instead of static inline, but crucially, also declares those functions as extern in io.c. This still provides a C compiler with sufficient info to inline the function, even if linking to a shared library, because the function is declared inline in the header. However, the significant difference is that the symbol is now actually present in the library also, which makes it possible to link to it.

I'm certain there is further scope for refactorings of the second kind. These are just the functions I've personally hit linkage problems with.

[arnopo]

I'll refactor the commits shortly to meet the compiance checks. Let me explain the rational here.

I use libmetal in a FFI environment (linking from rust). Functions defined as #defines, or as static inlines in headers are useless in this context. The compiled library simply does not contain these symbols.

I have never developed in a Rust environment so I would like to beter understand your issue.
Do you generate your libmetal and openamp libraries in your FFI environment or only link them?

So, this PR does 2 things

• it defines the #defines as functions where sensible to do so.
• for some of the functions in io.h, it marks some functions as inline instead of static inline, but crucially, also declares those functions as extern in io.c. This still provides a C compiler with sufficient info to inline the function, even if linking to a shared library, because the function is declared inline in the header. However, the significant difference is that the symbol is now actually present in the library also, which makes it possible to link to it.

• Since the functions are defined in the io.h, it seems normal to me that the symbol does not exist in the generated libs because the inline function code is inserted in the caller's code stream. So I guess you have a specific way to include the C API in Rust and you can't directly include the io.h to use the inline static functions, right?
• It seems to me that you remove the static so that the "inline" is ignored and a function symbol is included in the library. In this case, seems to me that your real need is: no inline function in the API but an external symbol declaration for embedding C code in Rust.
• declares those functions as extern in io.c while io.c include io.h does not seem very clean to me.

[ollie-etl]

I'll refactor the commits shortly to meet the compiance checks. Let me explain the rational here.
I use libmetal in a FFI environment (linking from rust). Functions defined as #defines, or as static inlines in headers are useless in this context. The compiled library simply does not contain these symbols.

I have never developed in a Rust environment so I would like to beter understand your issue.
Do you generate your libmetal and openamp libraries in your FFI environment or only link them?

I compile a vanilla libmetal library via a c toolchain. I then link to its symbols from rust. There are tools to automate the discovery of these symbols by parsing headers, but fundamentally thats what's happening.

This is the same for all the FFI's i've ever used (python, rust, Haskell). Code in C headers are not compiled by other languages compilers (unsurprisingly).

So, this PR does 2 things

• it defines the #defines as functions where sensible to do so.
• for some of the functions in io.h, it marks some functions as inline instead of static inline, but crucially, also declares those functions as extern in io.c. This still provides a C compiler with sufficient info to inline the function, even if linking to a shared library, because the function is declared inline in the header. However, the significant difference is that the symbol is now actually present in the library also, which makes it possible to link to it.

• Since the functions are defined in the io.h, it seems normal to me that the symbol does not exist in the generated libs because the inline function code is inserted in the caller's code stream. So I guess you have a specific way to include the C API in Rust and you can't directly include the io.h to use the inline static functions, right?

To do this, Rust compilers would have to compile C. Headers may be arbitrarily complex.

• It seems to me that you remove the static so that the "inline" is ignored and a function symbol is included in the library. In this case, seems to me that your real need is: no inline function in the API but an external symbol declaration for embedding C code in Rust.
• declares those functions as extern in io.c while io.c include io.h does not seem very clean to me.

If you accept that it'd be helpful for a library to actually contain the symbols (I'm very aware of header only libraries, I've worked a lot in C++), you've 2 choices:

• provide the declaration in the header, and the definition in the source. The effect of this can be considered based on compilation strategy. If compiled to a static library, link time optimisation can be performed to achive inline functionality. Linkers are good at this - your inline flags are likely ignored anyway - they are not pragmas.
  If you're compiling to a shared library and linking to your executable, this isn't possible. Therefore, users who were previously benefiting from the inlining of some of these small functions now pay the penalty of a function call in there code.

or (and this is what i've done in this PR)

• provide an inline definition the header, and an extern declaration in the source. The extern declaration in the source forces the symbol to be included in the compiled library. However, for existing users of the library, thecompiler now has access to the inline declaration in the header as before, and is able to choose to either call the function or inline the code in the users application. This is true for either sort of library. It therefore doesn't penalise existing users.

[arnopo]

• It seems to me that you remove the static so that the "inline" is ignored and a function symbol is included in the library. In this case, seems to me that your real need is: no inline function in the API but an external symbol declaration for embedding C code in Rust.
• declares those functions as extern in io.c while io.c include io.h does not seem very clean to me.

If you accept that it'd be helpful for a library to actually contain the symbols (I'm very aware of header only libraries, I've worked a lot in C++), you've 2 choices:

• provide the declaration in the header, and the definition in the source. The effect of this can be considered based on compilation strategy. If compiled to a static library, link time optimisation can be performed to achive inline functionality. Linkers are good at this - your inline flags are likely ignored anyway - they are not pragmas.
  If you're compiling to a shared library and linking to your executable, this isn't possible. Therefore, users who were previously benefiting from the inlining of some of these small functions now pay the penalty of a function call in there code.

or (and this is what i've done in this PR)

• provide an inline definition the header, and an extern declaration in the source. The extern declaration in the source forces the symbol to be included in the compiled library. However, for existing users of the library, thecompiler now has access to the inline declaration in the header as before, and is able to choose to either call the function or inline the code in the users application. This is true for either sort of library. It therefore doesn't penalise existing users.

Thanks for your description of the alternatives, that's what I have in mind too.
The first option (remove the inline and move the function into .c file) seems more reliable.

• Compilers know better than us what needs to be done.
• Make sure it is also compatible with a potential environment that would not support inline.
• Having a penalty for calling an extra function call using the dynamic library seems reasonable. If the performance optimization is the crieteria, the static library would probably be used.

@edmooring: any opinion on this?

[edmooring]

• It seems to me that you remove the static so that the "inline" is ignored and a function symbol is included in the library. In this case, seems to me that your real need is: no inline function in the API but an external symbol declaration for embedding C code in Rust.
• declares those functions as extern in io.c while io.c include io.h does not seem very clean to me.

If you accept that it'd be helpful for a library to actually contain the symbols (I'm very aware of header only libraries, I've worked a lot in C++), you've 2 choices:

• provide the declaration in the header, and the definition in the source. The effect of this can be considered based on compilation strategy. If compiled to a static library, link time optimisation can be performed to achive inline functionality. Linkers are good at this - your inline flags are likely ignored anyway - they are not pragmas.
  If you're compiling to a shared library and linking to your executable, this isn't possible. Therefore, users who were previously benefiting from the inlining of some of these small functions now pay the penalty of a function call in there code.

or (and this is what i've done in this PR)

• provide an inline definition the header, and an extern declaration in the source. The extern declaration in the source forces the symbol to be included in the compiled library. However, for existing users of the library, thecompiler now has access to the inline declaration in the header as before, and is able to choose to either call the function or inline the code in the users application. This is true for either sort of library. It therefore doesn't penalise existing users.

Thanks for your description of the alternatives, that's what I have in mind too.
The first option (remove the inline and move the function into .c file) seems more reliable.
el>

• Compilers know better than us what needs to be done.
• Make sure it is also compatible with a potential environment that would not support inline.
• Having a penalty for calling an extra function call using the dynamic library seems reasonable. If the performance optimization is the crieteria, the static library would probably be used.

@edmooring: any opinion on this?

Lots of opinions, but no concrete answer at this point.

First, making libmetal friendly to Foreign Function Interface libraries in other languages is highly desirable. Right now, it's mostly for Linux; but I'm seeing some interest in Rust on the more constrained Cortex-R and Cortex-M processors as well.

Second, the effect of this effort (regardless of the way in which we do it) is to add a bunch of libmetal-related symbols to those already visible to the linker at program scope. I'm not sure how much practical effect this has.

Third, the 'static inline' idiom in C libraries is well-established, and both compilers and coders know what it means. It is used as a compile-time indirection mechanism. Because it is explicit, and it can be shown that compilers obey it, it can be relied on for safety and security evaluations of products that use libmetal.

Fourth, I'm concerned about the effects on the more resource-constrained environments that libmetal runs in (some of them have a total of 64K memory available),. Inlining reduces the requirements somewhat. Does anybody have some real data?

[arnopo]

Lots of opinions, but no concrete answer at this point.

First, making libmetal friendly to Foreign Function Interface libraries in other languages is highly desirable. Right now, it's mostly for Linux; but I'm seeing some interest in Rust on the more constrained Cortex-R and Cortex-M processors as well.

Second, the effect of this effort (regardless of the way in which we do it) is to add a bunch of libmetal-related symbols to those already visible to the linker at program scope. I'm not sure how much practical effect this has.

Third, the 'static inline' idiom in C libraries is well-established, and both compilers and coders know what it means. It is used as a compile-time indirection mechanism. Because it is explicit, and it can be shown that compilers obey it, it can be relied on for safety and security evaluations of products that use libmetal.

Fourth, I'm concerned about the effects on the more resource-constrained environments that libmetal runs in (some of them have a total of 64K memory available),. Inlining reduces the requirements somewhat. Does anybody have some real data?

Thanks @edmooring

The answer seems not simple...

• the inline can duplicate the code, so the effect should be positive in term of footprint,
• a project with strong memory constraints probably doesn't use the metal_io_* APIs apart from using the open-amp library,
• it seem that the compiler can ignore the inline anyway.

So i propose to go ahead with option 1 and check effect on few projects (Zephyr, Linux, baremetal) and few compilers (gcc, IAR, armcc) to have real data.

@ollie-etl: Please could you update the PR in this way ( without c09c50b commit which breaks the API)
I will then have look on effect for projects using the static library.

[arnopo]

Hi @ollie-etl
Do you plan to propose a PR update, based on option 1 that we measure the impact?

[ollie-etl]

I've just realised this is unresolved. I do plan on returning and fixing this up. As ever, there are not enough hours in the day.

[ollie-etl]

Fourth, I'm concerned about the effects on the more resource-constrained environments that libmetal runs in (some of them have a total of 64K memory available),. Inlining reduces the requirements somewhat. Does anybody have some real data?

In such environments, would you not be compiling binaries via a static libraries, and either performing link time optimization, and stripping your binary pretty ruthlessly?

[arnopo]

Hello @ollie-etl
sorry for the late answer.
I started something on my side to understand impacts, the work is avalaible on my github: arnopo#17
For the moment I have tried the solution which consists in defining the functions in .c.
After encountering some build/link issues I tested the size of Zephyr project showing an increase of the footprint.

• without PR ROM size of the zephyr samples/subsys/ipc/openamp_rsc_table: 29838

• without PR ROM size of the zephyr samples/subsys/ipc/openamp_rsc_table: 29952

I have not analyzed the code in detail, but the distribution of the footprint shows that some functions that were previously forced inline are no longer inline.
So Keeping inline in .h and appending extern in * .c as you suggested should probably have less impact (although the syntax doesn't seem the most readable to me)

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.