Fix multiple definition linker error for operators for C++ (IDFGH-1810)

[X-Ryl669]

In build including FLAG_ATTR'ed enum.

This fix #4016

[Alvin1Zhang]

@X-Ryl669 Thanks for the contribution. We will look into this PR. Thanks.

[ginkgm]

hello @X-Ryl669 ,

thanks for fix this issue. I think inline is not sufficient, since it's not strong enough, there's some case the compiler choose not to inline it. I've amended it to FORCE_INLINE_ATTR (__attribute((always_inline)))

Have pick your commit and start our internal review.

[X-Ryl669]

inline here is not used for actually inlining the code (as you said, it's not like always_inline, more of an hint) but to ensure that the method is not defined twice (that's another side effect/advantage of a global inline function). It could have been static, but each compile unit would have one version (which is dumb since we try to save space).
I think both are required for optimal code, although I don't know how the attribute will conflicts with in a case of constexpr.

[ginkgm]

@X-Ryl669 I guess for such simple operators (binary operators), always_inline is acceptable?

if not, static inline is the only way I think..

Would prefer always_inline

[X-Ryl669]

Don't make them static. If you do that, you'd get:
an instance in A.cpp and an instance in B.cpp (since they are not class member, the compiler will treat them like a static function, that is a function that's local & private for the compilation unit)

always_inline tells the compiler to inline the code in all cases. It does tell to have a single instance of the code (if you only add attr(always-inline) the code is ill formed since both A.cpp and B.cpp will have a definition for the function, even if it does not contain anything), the linker will emit errors with multiple definitions.

Thus, you need both inline operator XX() attr(always_inline) since inline here tells the compiler that only a single definition should be created (and always_inline ensure it's empty).

[ginkgm]

@X-Ryl669 Please check the patch, it's merged by our internal flow. The definition of FORCE_INLINE_ATTR is here: https://github.com/espressif/esp-idf/blob/master/components/xtensa/include/esp_attr.h#L38.

Guess this is what you mean

[X-Ryl669]

FORCE_INLINE_ATTR use static inline so it means that you'll have local function body in each cpp file that'll include that header (it's static). You'll have a

TYPE& operator|=(TYPE& a, TYPE b) { ... }

in a.o and in b.o for both a.cpp and b.cpp

So you'll have X times the function body size in your final binary (one more implementation per .cpp including such header and using the macro, 2 in the example above).

If FORCE_INLINE_ATTR were only inline, then the compiler/linker would ensure it's only defined once (you will have 1x the function body size in your final binary whatever the number of .cpp file that use the macro), so you'd get a smaller binary with just inline than with static inline

That's for the theory. For such simple operator, I doubt the compiler will choose to implement the overhead for a function call and instead insert the 2 asm instructions (operator operation + storage) for each functions (and you instructed the compiler to always_inline anyway). So it'll inline the function body in all cases.

Yet, the macro is somehow wrong as I understand it.

See https://stackoverflow.com/questions/12836171/difference-between-an-inline-function-and-static-inline-function

Yet, I'm ok with the commit.