Unable to build simple c++ example with Visual Studio 15 2017 #7087
Comments
|
If I build Mbed TLS itself with cmake and "args I note you have Mbed TLS in We do ask that people reporting issues give us enough information to replicate the problem, which this isn't. Can you give a bit more information about exactly what you are doing, and (a link to) a full transcript of the build? In addition, if 3.2.1 is working for you, and 3.3 isn't, you could actually determine what has caused this breakage, making it easier for us to fix! |
|
C projects builds and works ok, but C++ fails, with strange bugreport.zip mkdir build
cd build
cmake .. -G"Visual Studio 15 2017" -Tv141_xp
cmake --build . --target bugreport |
|
Does it work without Checking on Linux, Given that you're observing not just link errors but also syntax error, there's more to it than a missing |
|
No, it does not. It fails with the same messages with just |
|
Okay, a simpler reproducer |
|
And more information. This simple but THIS one doesn't: complaining: |
|
And of course, the problem is returning a structure (rather than a pointer-to-structure) that the compiler hasn't seen at that point. Pointer-to-structure forward declarations do compile without complaint |
|
So the issue in Mbed TLS is (at least - there may be other problems hiding behind this) functions such as and which are returning structures that, at the time the compiler sees the function definition, haven't been fully defined, only forward-declared. |
|
... and this seems to be a Visual Studio problem, rather than a generic C++ problem (at least, this isn't a problem on macOS with |
|
And finally, the syntax errors are coming from the following, where the definition of leads to where line 1960 is the This is a bit weird, since Visual Studio is supposed to support designated initialisers since VS2013. Maybe a problem because it's a union element? |
|
... and a demonstrator for the initialiser problem which leads to (what silences the compiler here is |
|
And here is a diff which silences all of the errors. Probably not exactly what we'd want to commit, though |
tom-cosgrove-arm
added
bug
size-m
|
... and to use that diff, write it to a file With this change, the very simple |
|
@tom-cosgrove-arm Thanks for investigating! Is there a bug in our code or not? As far as I can tell it's conforming C99, which is all we promise. We also try to make it compliant to reasonably modern C++, but I don't know C++ well enough: are we using features that aren't in modern C++, is it a bug in old-but-still-supported versions of Visual Studio when compiling C++ code, or is that a bug in some old runtime that we don't care about (sorry, but Windows XP is not a target we care about anymore)? Getting rid of named initializers for the INIT macros wouldn't be a big deal. We have the On the other hand, your second patch doesn't look right. |
Honestly, I'm not really sure! The minimal reproducer ( It's nothing to do with XP - it's still an issue when building just with
Oh, I'm not suggesting that as a change we make - it would have been a PR in that case. I don't grok the PSA header structure well enough yet to do that. What Visual Studio requires is: any function that returns a structure (rather than a pointer to a structure) must have the structure fully defined before the declaration of that function. (It thinks any forward-declared structure is going to be a C++ class!) Moving the include achieves this as a proof-of-concept, but I doubt would be the best solution. |
|
Designated initializers have long existed as a GCC extension, but they were only added to C++ in C++20. We don't have an explicit minimum version of the C++ language to support our header files, but C++20 is too recent. So this is a bug in our code, and we can check for it on the CI: (We might choose GCC doesn't have any problem with the linkage involving a forward declaration, but Clang does. So this is a bug in our code, and we can check it on the CI. Therefore the goals of this issue are:
|
gilles-peskine-arm
added
component-psa
|
At this stage of the analysis, I think the fix will be in the high size-S to low size-M range. |
|
I'm also facing this issue. I made an attempt at fixing this in PR #7174, similar to your solution @tom-cosgrove-arm of moving the crypto_struct.h include to the start, but this introduced other issues with clang which the CI tests highlighted. Is there a fix expected for this? |
|
We definitely intend to fix this, however I can't give a timeline. For now I'm adding this issue to our task list for the next quarter, but we haven't planned the next quarter yet and I can't guarantee that it'll remain planned. (But conversely we might get around to it sooner depending on how we advance with this quarter's bugfix queue.) Please don't hesitate to submit a PR if you have even a partial fix, as long as it doesn't break the CI (different versions of GCC and Clang have different sets of warnings). |
|
I'm trying to reproduce the forward declaration linkage issue with clang on Linux. |
There is a known issue with building 3.3.0 using visual studio - bugtracker: Mbed-TLS/mbedtls#7087
feat: Downgrade mbedtls to 3.2.1 from 3.3.0 There is a known issue with building 3.3.0 using visual studio - bugtracker: Mbed-TLS/mbedtls#7087 Co-authored-by: Ron <45816308+rjaegers@users.noreply.github.com>
Cannot update to newer versions due to Mbed-TLS/mbedtls#7087.
|
This issue is the reason why people can't upgrade from 3.2.1 to anything newer unfortunately 😢 |
|
I'm afraid I don't have any bandwidth for this at the moment. If you have a solution that doesn't break anything else, please submit a pull request. |
|
If interested, here's the fix that worked for me, just minor headers tuning |
Incidentally, #6567 removed the Designated Initializer in |
Cannot update to newer versions due to Mbed-TLS/mbedtls#7087.
|
The problem seems to persist in mbedTLS 3.4.1, released yesterday. (Visual Studio 2019 here) |
Upgrade? I just tried to re-use a build of 3.2.1 I made for open62541 (still using VS2015) in my C++ app, and ran into the same issues including the header. Managed to simplify the workaround patch of yours a bit further to get it to compile: |
|
Related (but not in scope here) — since 3.6.0 there is a flexible array member in a header ( |
|
I've filed the flexible array member as a separate issue: #9020 We (Arm) are going to look for a workaround for the flexible array member issue. The API is going to stay in Mbed TLS 3.6.x, but we'll look at changing it in 4.0. In C++, if worst comes to worst, maybe we'll just disable Please note that we still have no fix for the other C++ issues mentioned in this thread, and we would welcome a pull request if someone can fix them (without changing the API or ABI for C builds of course). |
... including in the default external version to use when explicitly directed to do so (currently possible only under the traditional Unix build system). Formally adjust existing tuneups (whose files have notably been renamed) and ensure forward declarations for structs returned by value(!) are in place early enough to avoid errors when directly using Mbed TLS from C++ under Visual Studio, per Mbed-TLS/mbedtls#7087. JIRA: CXX-13565. git-svn-id: https://anonsvn.ncbi.nlm.nih.gov/repos/v1/trunk/c++@102330 78c7ea69-d796-4a43-9a09-de51944f1b03
Fix taken from here: Mbed-TLS#7087 (comment)
Summary
Its impossible to build simple ssl_client based single cpp file project with 3.3.0. 3.2.1 works ok!
System information
Mbed TLS version v3.3.0:
Operating system and version: windows xp and later
Configuration (if not default, please attach
mbedtls_config.h): platform defaultCompiler and options (if you used a pre-built binary, please indicate how you obtained it): Visual Studio 15 2017 -Tv141_xp so its MSVC with xp toolchain.
Additional environment information:
buld with cpp17 standard
Expected behavior
success build!
Actual behavior
bunch of error messages:
Steps to reproduce
Additional information
cmake project standard - c++17
cmake args:
-G"Visual Studio 15 2017" -Tv141_xpThe text was updated successfully, but these errors were encountered: