static inline functions are not externally linkable

[axos88]

Summary

This and 15 other functions are defined as static inline in

mbedtls/include/mbedtls/ssl.h

Line 2488 in 1b5c85c

static inline void *mbedtls_ssl_conf_get_user_data_p(

Obviously being inline they are a bit more performant this way, one less function call, one less branch, however they are also static which means they are not externally linkable. Because of this, if mbedtls is used from a language other than c / c++, where one cannot simply include the header file, these functions are not accessible, and in the "private fields" have to be accessed directly - which is totally doable since we're in C, without any real privacy -, but - I would guess - these are subject to change without notice, which means binary or source incompatibility even for patch releases, meaning upgrading mbedtls may be a can of worms.

My use case is calling mbedtls from rust, where bindings are auto-generated from the header files, however static inline wrappers are not supported because these are not externally linkable.

Please consider making these functions non-static (I think they can stay inline?), since they are not ones that would be called too often anyways.

System information

Mbed TLS version (number or commit id): 1b5c85c
Operating system and version: n/a
Configuration (if not default, please attach mbedtls_config.h): n/a
Compiler and options (if you used a pre-built binary, please indicate how you obtained it): n/a
Additional environment information: n/a

Expected behavior

The compiled C library should have externally linkable symbols for these accessors.

Actual behavior

They are not.

Steps to reproduce

compile mbedtls as a library, look at the symbols, I guess.

Additional information

n/a

[axos88]

@mpg , I think you submitted the PR adding some of these.

[gilles-peskine-arm]

The main reason we have these static inline functions is that they save code size (that's more important to us than performance, since we target constrained devices where code size is the bottleneck). Link-time optimization can give the same benefit (and more), but not everyone can use LTO due to compiler or build environment limitations.

I wonder if there's a reasonable way we can provide both inline functions and externally linked functions.

[axos88]

Maybe a compile time config that switches between static inline and not?

[gilles-peskine-arm]

We're trying to cut down on compile-time switches. They're a nightmare to test.

[mpg]

Is the code size difference that significant? I tried the following experiment:

#include "mbedtls/ssl.h"

void conf_dn_hints( const mbedtls_x509_crt *crt,
        mbedtls_ssl_config *conf )
{
    mbedtls_ssl_conf_dn_hints( conf, crt );
}

void non_inline_mbedtls_ssl_conf_dn_hints( mbedtls_ssl_config *conf,
                                const mbedtls_x509_crt *crt );

void ni_conf_dn_hints( const mbedtls_x509_crt *crt,
        mbedtls_ssl_config *conf )
{
    non_inline_mbedtls_ssl_conf_dn_hints( conf, crt );
}

% arm-none-eabi-gcc -Wall -Wextra -Iinclude -Os -mthumb -mcpu=cortex-m0 -c foo.c && arm-none-eabi-objdump -d foo.o
00000000 <mbedtls_ssl_conf_dn_hints>:
   0:   30d4            adds    r0, #212        ; 0xd4
   2:   6001            str     r1, [r0, #0]
   4:   4770            bx      lr

00000006 <conf_dn_hints>:
   6:   31d4            adds    r1, #212        ; 0xd4
   8:   6008            str     r0, [r1, #0]
   a:   4770            bx      lr

0000000c <ni_conf_dn_hints>:
   c:   0003            movs    r3, r0
   e:   b510            push    {r4, lr}
  10:   0008            movs    r0, r1
  12:   0019            movs    r1, r3
  14:   f7ff fffe       bl      0 <non_inline_mbedtls_ssl_conf_dn_hints>
  18:   bd10            pop     {r4, pc}

Basically using the static inline function is 4 bytes (the bx lr at the end doesn't count). Defining the actual linkable function is 6 bytes, and calling it... well it depends in the context, but in my example it's 12 bytes if you count everything, but I think that's a bit of a worst case (for example here I'm counting the initial push but most non-trivial function will have that already, and I purposefully placed the arguments in the wrong registers - if you're calling other ssl_conf functions you like have conf in r0 already).

So, making this function non-inline costs 6 bytes + say 8 bytes per calling site. We have 16 similar functions and it looks to me like most of them (the ssl_conf and ssl_set functions) will often have only one calling site (the getters might be used from more places though). Also, a typical application is unlikely to call all 16 of those functions (for example for user data you'll typically only be using the _p or the _n set, so that's 4 functions you're not using).

So, I'm wondering if the code size impact is large enough to be a problem here.

I was going to say that some of these function are for relatively advanced use cases that are more likely to concern servers handling a variety of clients that typical usage in constrained environments, so I looked at the list of functions to confirm or infirm that:

static inline const mbedtls_ssl_config *mbedtls_ssl_context_get_config(
static inline void mbedtls_ssl_conf_cert_cb( mbedtls_ssl_config *conf,
static inline void mbedtls_ssl_conf_set_user_data_p(
static inline void mbedtls_ssl_conf_set_user_data_n(
static inline void *mbedtls_ssl_conf_get_user_data_p(
static inline uintptr_t mbedtls_ssl_conf_get_user_data_n(
static inline void mbedtls_ssl_set_user_data_p(
static inline void mbedtls_ssl_set_user_data_n(
static inline void *mbedtls_ssl_get_user_data_p(
static inline uintptr_t mbedtls_ssl_get_user_data_n(
static inline const char *mbedtls_ssl_get_srtp_profile_as_string( mbedtls_ssl_srtp_profile profile )
static inline void mbedtls_ssl_conf_max_tls_version( mbedtls_ssl_config *conf,
static inline void mbedtls_ssl_conf_min_tls_version( mbedtls_ssl_config *conf,
static inline mbedtls_ssl_protocol_version mbedtls_ssl_get_version_number(
static inline int mbedtls_ssl_is_handshake_over( mbedtls_ssl_context *ssl )

I think it mostly confirms, but also shows that we're quite inconsistent here: mbedtls_ssl_conf_cert_cb() is static inline why other callback setters are not. It looks like most of the inline function are quite recent too. There's two way to look at it: (1) making the recent functions non-inline will achieve consistency; (2) there are a lot of existing functions that are not inline but could be made inline if we wanted to get maximal code size savings on this front, so when I wrote 16 functions are concerned, that was an under-estimate...

[gilles-peskine-arm]

@mpg The impact on code size is not about a function call vs the body of the function, it's about further optimizations that can be done after inlining, which can't be done if the function is opaque.

[mpg]

Ah, right, that's harder to estimate. However since my estimate above is, I think, a bit pessimistic about function call overhead, and as you pointed out, optimistic about missed optimisation opportunities (in that it completely ignores them), I hope overall it still gives an idea of the order of magnitude.

I want to insist on the last paragraph of my previous comment: I think whatever we decide should be applied in a systematic way, as we're currently very inconsistent:

% grep '^[^*]*mbedtls_ssl.*_cb(' include/mbedtls/ssl.h
void mbedtls_ssl_set_timer_cb( mbedtls_ssl_context *ssl,
static inline void mbedtls_ssl_conf_cert_cb( mbedtls_ssl_config *conf,
void mbedtls_ssl_conf_session_tickets_cb( mbedtls_ssl_config *conf,
void mbedtls_ssl_set_export_keys_cb( mbedtls_ssl_context *ssl,
void mbedtls_ssl_conf_async_private_cb( mbedtls_ssl_config *conf,
void mbedtls_ssl_conf_ca_cb( mbedtls_ssl_config *conf,
void mbedtls_ssl_conf_psk_cb( mbedtls_ssl_config *conf,

(I'm taking the example of setting callbacks as those are all trivial setters. There are probably more trivial setters, I guess most of the mbedtls_ssl_(conf|set)_ functions that return void are trivial setters, and there are 46 such potential candidates.) The current criterion for "is it inline or not" seems to be how recently and by whom it was added, which is not good: it's neither optimal for code size nor for integration with other languages, and gives users no predictability.

[axos88]

@gilles-peskine-arm, @mpg, how about making them inline but not static? I'm not 100% sure, and can't test right now, but as far as I read this thread: https://stackoverflow.com/questions/6312597/is-inline-without-static-or-extern-ever-useful-in-c99,

making the header files inline, and having an external definition in the C files would mean that the functions would be compiled into the resulting library as external symbols, however inside mbedtls and when used from C code that includes the header files with the inline definition would actually let the compiler inline them, (meaning the actual external symbols in the library will be left unused which theoretically can be stripped away if the user wishes - but as @mpg said the code size win is negligible); However when used from outside of C code, or if the header files are not actually correctly included, the it would be possible to call the non-inlined functions using the symbols with external linkage.

[mpg]

To clarify: I think "negligible" is stronger than what I meant. If there are 12 functions concerned, and each of them is called in only one place, that's 168 bytes. That's not negligible (we've already spent time on smaller optimisations in a branch focused on code size in the past, and small optimisations add up) but indeed it's not much and might not be worth the trouble.

If there are more like 40 functions, it's more like 0.5 kB which is already more sizeable. (Of course, these estimates should be taken with a big grain of salt, as Gilles pointed out.)

If making the functions inline but not static works, it could be a good compromise indeed (actually the best kind of compromise: winning on both sides).

[gilles-peskine-arm]

how about making them inline but not static?

That's a good idea. We haven't been doing this because of portability: we only recently started to assume C99 as our baseline. Before that we were assuming C89
plus a few popular extensions. static inline was very widespread and with consistent behavior even with otherwise non-C99 compilers, but non-static inline in C99 is slightly different. I don't know how portable C99 inline and extern inline are, considering we're officially requiring C99, but currently I think our headers are still almost C89 so this could break some officially-supported-but-practically-working platforms.

[gilles-peskine-arm]

https://www.iar.com/knowledge/support/technical-notes/compiler/linker-error-undefined-external-for-inline-functions/ suggests that IAR implements the C99 semantics. https://en.wikipedia.org/wiki/Inline_function#armcc suggests that armcc implements the C++ semantics, which is fine because it's more permissive than C99. I can't find a source for MSVC, I hope that they also implement the C++ semantics since it's generally predominantly a C++ compiler.

[tom-cosgrove-arm]

Broader compatibility could be provided with a new macro MBEDTLS_INLINE which gets #defined to inline by default (for C99) but to extern inline if GNU89 is detected - according to the SO article 'What does extern inline do?' that Gilles linked to above this would give the broadest compatibility. Within the Mbed TLS headers it could only be #defined if not already done, so command-line switches could be used to set a suitable value for esoteric compilers

[gilles-peskine-arm]

We had a team chat and agreed to go with your suggestion of non-static inline @axos88 . Would you be able to make a pull request? If not this will go onto our backlog.

To be clear, the proposed solution is:

• Change all static inline definitions in include/*/*.h to inline. Possibly via a macro MBEDTLS_INLINE which could be overridden in mbedtls_config.h. We're currently defining inline to __inline on MSVC but I don't know if it's required with currently supported versions of MSVC (VS 2013 and above).
• For each such function, add an extern inline declaration in the most logical library/*.c file. Again, possibly via a macro.
• No change in library/*.h.
• Add a paragraph of guidance to https://github.com/Mbed-TLS/mbedtls-docs/blob/main/kb/development/mbedtls-coding-standards.md about the use of inline.

In terms of backward compatibility:

• This doesn't break the API: all the existing functions will still exist.
• I don't think this introduces any ABI change: we're just adding more linkable functions.
• This shouldn't have an impact on code size since the functions can still be inlined wherever they could be inlined before.
• This adds a new platform requirement: the C compiler must conform C99 in a way that we didn't exercise before. This is acceptable since we officially require a C99 compiler. However, we do not intend to make this change to the 2.28 long-time support branch, in case there is a compiler out there with C99 support that's good enough for what we have now but doesn't support this use of inline.