jgaeddert / liquid-dsp

digital signal processing library for software-defined radios
http://liquidsdr.org
MIT License
1.82k stars 426 forks source link

Linking in a C++ program: warning ... has C-linkage specified, but return ... #229

Open pfeatherstone opened 3 years ago

pfeatherstone commented 3 years ago

When using liquid dsp in a C++ program, and as per the user guide, you include liquid as follows:

#include <complex>
#include <liquid/liquid.h>

you get warnings like the following:

warning: 'polyc_val' has C-linkage specified, but returns user-defined type 'liquid_double_complex' (aka 'complex<double>') which is incompatible with C [-Wreturn-type-c-linkage]
LIQUID_POLY_DEFINE_API(LIQUID_POLY_MANGLE_CDOUBLE,

...

warning: 'polyc_interp_lagrange' has C-linkage specified, but returns user-defined type 'liquid_double_complex' (aka 'complex<double>') which is incompatible with C [-Wreturn-type-c-linkage]
warning: 'polyc_val_lagrange_barycentric' has C-linkage specified, but returns user-defined type 'liquid_double_complex' (aka 'complex<double>') which is incompatible with C [-Wreturn-type-c-linkage]
warning: 'polycf_val' has C-linkage specified, but returns user-defined type 'liquid_float_complex' (aka 'complex<float>') which is incompatible with C [-Wreturn-type-c-linkage]
LIQUID_POLY_DEFINE_API(LIQUID_POLY_MANGLE_CFLOAT,
pfeatherstone commented 3 years ago

I've tried doing the following:

extern "C" {
#include <liquid/liquid.h>
}

But that doesn't work either. __cplusplus is still defined when including that way.

pfeatherstone commented 3 years ago

Thinking about it, liquid is always built as a C library, and therefore always uses R _Complex. You can't change the type of liquid_float_complex when using the library. Isn't that breaking the ABI?? Is it possible that float _Complex and std::complex<float> don't have the same object layout and alignment??

pfeatherstone commented 3 years ago

Actually,

extern "C" {
#include <liquid/liquid.h>
}

Does work, but casting from std::complex<float> to liquid_float_complex is a pain.

pfeatherstone commented 3 years ago

Also, this is only an issue with clang, not gcc

pfeatherstone commented 3 years ago

So i ran the following:

cout << "alignment " << alignof(complex<float>) << endl;
cout << "size      " << sizeof(complex<float>) << endl;

and

typedef float _Complex C;
printf("alignment %li\n", alignof(C));
printf("size      %li\n", sizeof(C));

Both return 4 and 8 for alignment and size respectively under both gcc and clang. No surprises there really. So casting between liquid_float_complex and complex<float> should never be an issue. So it's really just the clang compiler not being happy about linking. I think the best thing to do for C++ users is to use the following functions to keep the compiler happy:

liquid_float_complex complex_to_liquid(complex<float> z);
complex<float>       liquid_to_complex(liquid_float_complex z);

@jgaeddert Have you had a chance to think about this? I compile most of my stuff with clang and not particularly happy about having 100s of compiler warnings about C linkage...

jgaeddert commented 3 years ago

I am 100% with you on this. I also see tons of warnings when using clang but don't have a great solution for it. I always figured the ABIs should always be compatible though. Seems like someone would have solved this at some point.

pfeatherstone commented 3 years ago

I've been watching lots of videos on youtube about breaking ABI from Jason Turner and Marshall Clow. This seems extreme, but it would seem the only safe thing to do when building libraries is to never use the standard library. Just use primitives (bool, int, float, ...) and pointers. But that's crazy, unfeasible and unusable. I wish there was a good solution for communicating across ABIs without having to use conversion functions. Maybe this is more of a C++ problem rather than a C problem. And maybe this is why libraries usually provide a C API, not a C++ API...

In any case, for now i will use conversion functions like the ones above. Hopefully the compiler optimizes this kind of gumpf away. Maybe liquid-dsp could provide another header, called liquid.hpp, which provides a few handy conversion functions. I don't suggest wrapping the entire C API, just certain things.

pfeatherstone commented 3 years ago

As a tangent, I think the only solution for ABIs in C++ is to only ever build header-only libraries, always build everything from source and go to town with static linking.

brian-armstrong commented 3 years ago

An alternate option is to make liquid able to be compiled either as a C or C++ library. I went through this process but my fork is now a few years old and it's a fairly substantial amount of work to keep it up to date. The major changes are to not use variable length arrays (forbidden in C++) and to always use liquid_float_complex everywhere so that it can be typedef'd easily depending on build. Some float literals also have to be defined more precisely. For VLAs you can instead just call alloca so it's not really a big deal.

I went through this process so that liquid would compile under MSVC which does not support C99 and it worked great. Everything still worked as a C lib as well. But after bringing it up to date once after about a year or two's drift I decided not to touch it again. Would be relatively easy change to make if it were included in changes going forward though.

pfeatherstone commented 3 years ago

Doesn't a file need to have the .cpp extension for it to be compiled as C++? I thought that if you ran g++ or clang++ on a .c file, it still compiled it as C code...

jgaeddert commented 3 years ago

Nope. OS-specific behavior on file extensions is pretty much a Windows-only thing AFAIK.

I had thought float complex in C and std::complex in C++ were guaranteed to be binary compatible on a common platform. Is this not correct?

pfeatherstone commented 3 years ago

It is but clang still complains.

pfeatherstone commented 3 years ago

OK, it would be great if this C linkage business was resolved. I now have to #include <liquid/liquid.h> pretty much everywhere, before anything else, to ensure it is included before <complex>, even if i'm not going to use liquid. This is a massive pain and a code smell.

I think first of all, liquid.h should not say at all it's going to use std::complex. Clang is right, you can't pass std::complex<float> to a function when it was compiled to accept another type. It just disobeys the laws of programming. Then maybe there should be a convenience header, liquid.hpp, which has convergence functions from liquid types to std:: types. Then the guidance should be to only ever include liquid.hpp in C++ projects.

Furthermore, if someone fancies adding C++ API wrappers to liquid.hpp in the future, then it is a good place to do so. Has anyone else had enough of seeing literally 1000s of compiler warnings about C linkage?

jgaeddert commented 3 years ago

These are fair points. C++ API wrappers to all C objects is actually in the works (as are python bindings). I do my primary development on macOS, so I see these warnings on clang 12.0.5 all the time, and it is indeed very annoying. I feel your pain.

jgaeddert commented 3 years ago

I will also mention that the ABI for C's float _Complex type is guaranteed compatible with C++'s std::complex<float> type; they are equivalent to float[2]. The library is compiled in C, and clang knows how to link to C externally. The problem is that clang's C++ compiler needs to know that the library was compiled in C (so it can understand the name-mangling that the C compiler does), but then it also knows that std::complex<float> is not a valid type in C. This is why it throws a warning and not an error. I'm at a loss for how to bypass this without just suppressing the warning in clang.

pfeatherstone commented 3 years ago

With regards to C++ API wrappers, that sounds great, though there is nothing wrong with the C API, in fact it's pretty great and has a brilliant object model. With regards to the fix, i would just remove std::complex from the liquid header, just never use it, then have conversion functions like these:

inline liquid_float_complex cpp_to_liquid(const std::complex<float>& z)
{
    liquid_float_complex l;
    memcpy(&l, &z, sizeof(l));
    return l;
}

inline liquid_float_complex* cpp_to_liquid(std::complex<float>* z)
{
    return (liquid_float_complex*)z;
}

inline std::complex<float> liquid_to_cpp(const liquid_float_complex& l)
{
    std::complex<float> z;
    memcpy(&z, &l, sizeof(z));
    return z;
}

inline std::complex<float>* liquid_to_cpp(liquid_float_complex* l)
{
    return (std::complex<float>*)l;
}

Either users can copy paste this into their code, or simply use liquid_float_complex instead of complex<float> (bit weird in C++ but why not)

jgaeddert commented 3 years ago

I think the only issue with the warnings (correct me if I'm wrong) is when a function returns a complex value, right? At least that's been my experience. I strongly hesitate to implement type conversions with something like a memcpy as you have above when the underlying data type is actually the same; the ABI is compatible, it's just the compiler getting confused by the API. For pointers in C++ you can use the reinterpret_cast<T> template and not need to have a separate function.

jgaeddert commented 3 years ago

Out of curiosity, if you add -Wno-return-type-c-linkage (to disable this particular warning) does this resolve the warnings you're getting?

pfeatherstone commented 3 years ago

I could try that tomorrow. But that might hide warnings with other parts of someone's application. I know GNU radio uses liquid under the hood for some of its modules. Imagine if GNU radio was built with that flag. How many bugs would that hide? I would rather just fix the issues without messing with compiler flags. It might also not work on MSVC in which case liquid is no longer portable for C++ in a compiler-warning-free way.

pfeatherstone commented 3 years ago

With regards to pointers, i just wanted to have a uniform API for converting between liquid types and std types regardless of whether the object was a value, reference or pointer. Otherwise it looks ugly if sometimes you have to use a function like liquid_to_cpp and sometimes you use reinterpret_cast. Just have one function with overloads for values, references and pointers. it looks cleaner.

pfeatherstone commented 3 years ago

With regards to type conversion functions, the only way to do that without using type punning (which isn't strictly allowed in C++ because of object lifetime) is to use a memcpy. This is where compiling C code in C++ gets a bit tricky sometimes because C doesn't really have object lifetime (there are no constructors or destructors). Whereas in C++, it doesn't matter if you're using an int, it has object lifetime, and therefore a constructor and destructor must be called. If you use type punning, these are NOT called which is UNDEFINED behaviour. This isn't good. Besides, if you shuv this in CompilerExplorer, with -O3, the memcpy almost certainly gets optimized away. So it doesn't impact performance

pfeatherstone commented 3 years ago

I wouldn't be scared of using memcpy. In fact, the example implementation of C++20 std::bit_cast uses memcpy.

pfeatherstone commented 3 years ago

Just to be clear with regards to type punning. Something like this:

float a;
(*((int*)(&a)))++;

is perfectly fine in C, but this is very much undefined behaviour in C++

jgaeddert commented 3 years ago

Well, in your example float is a 32-bit value while int is platform specific, but I see your point. C and C++ are of course different languages, and treating C++ as "a superset of C" is clearly not true. If it were, C++ would respect the _Complex keyword and allow native API congruency. I see what you mean about type punning here, but that's not really what we're talking about, right? float _Complex in C and std::complex<float> in C++ are the same type (the ABI). It's guaranteed to be so. I'm not trying to do some clever bit masking on the float value which would be platform specific (little vs. big endian). The problem is that there isn't a single set of keywords (e.g. the API) common to both languages.

jgaeddert commented 3 years ago

As an aside, I find this conversation quite interesting. This particular anomaly (C/C++ discrepancies) I have found particularly frustrating when writing code that is supposed to operate between the two languages. If there is a buffer, it's easy to call reinterpret_cast< std::complex<float>* > on the pointer. The issue is only when a function returns a type float _Complex and C++ is expected to understand that this means it is returning std::complex<float>