Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mbedTLS: Update to new LTS v3.6.0 #90482

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Apr 10, 2024

Keep module compatibility with mbedtls 2.x (old LTS branch).

A patch has been added to allow compiling after removing all the psa_* files from the library folder (will look into upstreaming it).

Note: mbedTLS 3.6 finally enabled TLSv1.3 by default, but it requires some module changes, and to enable PSA crypto (new "standard" API specification), so it might be best done in a separate commit/PR.

Closes #82282

thirdparty/README.md Outdated Show resolved Hide resolved
thirdparty/README.md Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Seems like some warnings in mbedtls code creep up to our own module code, where we don't disable warnings as we want them for Godot code:

cl /Fomodules\mbedtls\tls_context_mbedtls.windows.editor.x86_64.obj /c modules\mbedtls\tls_context_mbedtls.cpp /TP /std:c++17 /nologo /MT /Gd /GR /utf-8 /bigobj /O2 /W4 /wd4100 /wd4127 /wd4201 /wd4244 /wd4245 /wd4267 /wd4305 /wd4514 /wd4714 /wd4820 /WX /DTOOLS_ENABLED /DDEBUG_ENABLED /DNDEBUG /DNO_EDITOR_SPLASH /DWINDOWS_SUBSYSTEM_CONSOLE /DWINDOWS_ENABLED /DWASAPI_ENABLED /DWINMIDI_ENABLED /DTYPED_METHOD_BIND /DWIN32 /DWINVER=0x0601 /D_WIN32_WINNT=0x0601 /DNOMINMAX /D_WIN64 /DVULKAN_ENABLED /DRD_ENABLED /DD3D12_ENABLED /DGLES3_ENABLED /D_HAS_EXCEPTIONS=0 /DMINIZIP_ENABLED /DBROTLI_ENABLED /DTHREADS_ENABLED /DCLIPPER2_ENABLED /DZSTD_STATIC_LINKING_ONLY /DUSE_VOLK /DVK_USE_PLATFORM_WIN32_KHR /DGLAD_ENABLED /DEGL_ENABLED /DGODOT_MODULE /DMBEDTLS_CONFIG_FILE=\"thirdparty/mbedtls/include/godot_module_mbedtls_config.h\" /DTESTS_ENABLED /DDOCTEST_CONFIG_NO_EXCEPTIONS_BUT_WITH_ALL_ASSERTS /Ithirdparty\mbedtls\include /Ithirdparty\libpng /Ithirdparty\glad /Ithirdparty\directx_headers\include\directx /Ithirdparty\volk /Ithirdparty\vulkan /Ithirdparty\vulkan\include /Ithirdparty\zstd /Ithirdparty\zlib /Ithirdparty\clipper2\include /Ithirdparty\brotli\include /Ithirdparty\angle\include /Iplatform\windows /I. /Ithirdparty\d3d12ma
tls_context_mbedtls.cpp
Error: D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): error C2220: the following warning is treated as an error
Error: D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): error C2220: the following warning is treated as an error
Warning: D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): warning C4200: nonstandard extension used: zero-sized array in struct/unionD:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): warning C4200: nonstandard extension used: zero-sized array in struct/union

D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): note: This member will be ignored by a defaulted constructor or copy/move assignment operator
Error: D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): note: This member will be ignored by a defaulted constructor or copy/move assignment operatorD:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): error C2220: the following warning is treated as an error

Warning: D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): warning C4200: nonstandard extension used: zero-sized array in struct/union
D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): note: This member will be ignored by a defaulted constructor or copy/move assignment operator
Error: D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): error C2220: the following warning is treated as an error
Warning: D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): warning C4200: nonstandard extension used: zero-sized array in struct/union
D:\a\godot\godot\thirdparty\mbedtls\include\psa\crypto_struct.h(229): note: This member will be ignored by a defaulted constructor or copy/move assignment operator
scons: *** [modules\mbedtls\tls_context_mbedtls.windows.editor.x86_64.obj] Error 2

If that's the only one we could maybe fix mbedtls and PR the warning fix upstream. If there's more than that, we should look into how to disable warnings for thirdparty code in a more comprehensive manner. Or just use pragrams to disable warnings around the relevant includes in our mbedtls module code.

I just found out that MSVC has some support for an equivalent of -isystem now: https://devblogs.microsoft.com/cppblog/broken-warnings-theory/#external-headers
So we could maybe add our own add_thirdparty_include_path() that wraps around using -isystem or /external stuff somehow. Needs experimentation, it's out of scope of this PR of course.

@Faless
Copy link
Collaborator Author

Faless commented Apr 10, 2024

If that's the only one we could maybe fix mbedtls and PR the warning fix upstream.

This seems already tracked upstream via Mbed-TLS/mbedtls#9020 and since we don't directly use psa_generate_key_ext it should be fixed in 3.6.1 as suggest in Mbed-TLS/mbedtls#7087 (comment) .

In the meantime, I'll see if we can completely drop the psa/* headers, or apply the suggestion in the comment of having a different path C++ builds.

@Faless Faless force-pushed the mbedtls/3.6.0-tls branch 3 times, most recently from 7c36639 to a49ff76 Compare April 10, 2024 13:00
@Faless
Copy link
Collaborator Author

Faless commented Apr 10, 2024

@akien-mga seems like I managed to remove the PSA headers completely, that should fix the warnings/errors too.

EDIT: Nevermind, the build was picking up system headers as substitutes :/ , will need some more work.

@Faless Faless marked this pull request as draft April 10, 2024 13:27
@Faless Faless force-pushed the mbedtls/3.6.0-tls branch from a49ff76 to 2b480cb Compare April 10, 2024 18:22
@Faless
Copy link
Collaborator Author

Faless commented Apr 10, 2024

From #90482 (comment) :

I managed to disable psa completely, and leave the old setup unchanged, we still need the new patch to avoid the bogus inclusion

Well, it seems that at least mbedTLS 3.6.0 is too intertwined with psa headers to remove them, so I've reverted that change.

I'll see if I can find a workaround for the MSVC errors.

Keep module compatibility with mbedtls 2.x (old LTS branch).

A patch has been added to allow compiling after removing all the `psa_*`
files from the library folder (will look into upstreaming it).

Note: mbedTLS 3.6 finally enabled TLSv1.3 by default, but it requires
some module changes, and to enable PSA crypto (new "standard" API
specification), so it might be best done in a separate commit/PR.
@Faless Faless force-pushed the mbedtls/3.6.0-tls branch from 2b480cb to 40fa684 Compare April 10, 2024 19:19
@Faless Faless marked this pull request as ready for review April 10, 2024 19:59
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@akien-mga akien-mga merged commit 58f8a22 into godotengine:master Apr 11, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants