Skip to content

Add winsock2 header into build_info.h #262

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

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

irwir
Copy link
Contributor

@irwir irwir commented Apr 19, 2025

Description

Ensures proper socket definitions on Windows (as a part of resolving mbedtls issue #10097).

PR checklist

  • changelog not required
  • framework PR not required
  • mbedtls development PR provided #10140
  • mbedtls 3.6 PR not required because: not planned
  • tests not required

@gilles-peskine-arm
Copy link
Contributor

TF-PSA-Crypto needs to build outside of Mbed TLS. So it doesn't seem right to me that the Windows build (with an unchanged mbedtls) is failing here.

@irwir
Copy link
Contributor Author

irwir commented Apr 22, 2025

This is odd if an additional standard header breaks things so badly.

[2025-04-21T19:59:35.604Z] 291>C:/Users/Administrator/AppData/Local/Temp/tmp5cfern6y/tf-psa-crypto/tests/suites/test_suite_ecp.function(19): error C2143: syntax error : missing ')' before 'constant' [C:\Users\Administrator\AppData\Local\Temp\tmp5cfern6y\cmake_solution\tf-psa-crypto\tests\test_suite_ecp.vcxproj]

How to parse this?
The C2143 error code from a compiler and the last word vcxproj indicate a compilation error.
But the source file is test_suite_ecp.function while make build uses tf-psa-crypto/tests/test_suite_ecp.c?

@irwir
Copy link
Contributor Author

irwir commented Apr 22, 2025

[2025-04-21T19:59:36.764Z] cl : Command line warning D9002: ignoring unknown option '/utf-8' [C:\Users\Administrator\AppData\Local\Temp\tmp5cfern6y\cmake_solution\tf-psa-crypto\tests\test_suite_gcm.aes128_en.vcxproj]

Numerous warnings in the log. Maybe VS 2013 did not have this option?
Unlike for mbdetls, tests were never tried with later versions of Visual Studio.

@gilles-peskine-arm
Copy link
Contributor

test_suite_xxx.c is generated from test_suite_xxx.function using framework/scripts/generate_test_code.py. It does a couple of substitutions and adds some boilerplate as well as code to prepare arguments to test functions.

The offending line (more visible in the message from MinGW) is

inline static int mbedtls_ecp_group_cmp(mbedtls_ecp_group *grp1,

It looks like the parser gets confused because it doesn't recognize inline as intended.

We use the inline keyword in our code base. For compilers such as MSVC that don't define it, tf-psa-crypto/build_info.h defines it. This happens shortly before the inclusion of winsock2.h. I have no idea why including winsock2.h breaks this.

As for /utf-8, the warning is annoying but harmless. It was added to fix a warning that does break the build in some environments (those using a non-UTF-8 multibyte encoding by default, if I understand correctly).

@irwir
Copy link
Contributor Author

irwir commented Apr 22, 2025

test_suite_xxx.c is generated from test_suite_xxx.function

The question was why .function instead of the expected .c in the compiler diagnostics. This was explained by #line directive.

I have no idea why including winsock2.h breaks this.

It is difficult to guess without installed VS 2013 and the generated projects and sources.
A trivial inlined function after including build_info.h could be compiled even in VS 2012. The issue should be somewhere deeper.
With VS 2022, I copied and renamed cert_app.vcxproj, changed the source file to test_suite_ecp.c. Some paths had to be added, but finally it compiled.

@irwir
Copy link
Contributor Author

irwir commented Apr 22, 2025

As for /utf-8

Available since VS 2015.

@irwir
Copy link
Contributor Author

irwir commented Apr 23, 2025

As far as I remember, a bug in VS 2015 increased the minimum required version for mbedtls to VS 2017, even though the official documentation still says VS 2013 (also in solution path visualc/VS2013/mbedTLS.sln).

Quote from the current README.md in TF-PSA-Crypto:

The following instructions have been tested on Microsoft Visual Studio Community 2019 Version 16.11.26.

Yet there is nothing but VS 2013 in TF OpenCI: PR tests.

Should this have been changed in development branches?

@irwir
Copy link
Contributor Author

irwir commented May 15, 2025

Two questions.
Could TF OpenCI: PR tests be retried with VS 2017?
Would it be possible to get the failing generated projects and source files, as artifacts maybe?
At least one, like test_suite_ecp.c and test_suite_ecp.vcxproj for investigation in VS 2017.

It looks like the parser gets confused because it doesn't recognize inline as intended.

Text search in TF-PSA-Crypto sources had found 51 files containing the inline word. A few tests had been built successfully before the failure.

@gilles-peskine-arm
Copy link
Contributor

Could TF OpenCI: PR tests be retried with VS 2017?

Sure. Since the last CI run, we've switched our CI from VS2013 to VS2017. I've kicked a new run.

Would it be possible to get the failing generated projects and source files, as artifacts maybe?

Setting this up would require some Groovy code. But I don't really see the point: these files don't depend on the platform, on the library configuration or (in principle) on tool versions, you can generate them locally on any platform.

@irwir
Copy link
Contributor Author

irwir commented May 16, 2025

Setting this up would require some Groovy code. But I don't really see the point: these files don't depend on the platform, on the library configuration or (in principle) on tool versions, you can generate them locally on any platform.

It could be generated provided everything is installed, configured and it is perfectly well known what commands to run.
But there is only VS 2017 for C/C++ builds. Nothing else, and nothing is planned to be added.
There is also Ubuntu 25.04 which is used for generating files with make.
So it was cheaper to manually create a VS project for the test code and then figure out what was wrong.

Build process now is working, but tests are failing.
With make all two versions had been built - with included winsock2.h and without it.
The only difference in the sources and generated files was build_info.h.
Knowing this, I locally created two builds of test_suite_ssl - with winsock2.h and with the #include commented out.
All compiled files were deleted before the rebuild.
The results are exactly the same, some of Session serialization tests had failed.

@irwir
Copy link
Contributor Author

irwir commented May 16, 2025

Every test in test_suite_ssl.datax that contains path name of a certificate file - failed.
Corrected path names had this outcome:
PASSED (854 / 854 tests (146 skipped))

@gilles-peskine-arm
Copy link
Contributor

You need to run the tests suites from the tests directory.

There's code in tests/suites/host_test.function that tries to automatically change into the directory containing the executable. This is relatively new but our scripts might have gotten used to that. But that code isn't implemented for Windows (patches welcome).

@irwir
Copy link
Contributor Author

irwir commented May 17, 2025

/* We might want to allow backslash as well, for Windows. But then we also
* need to consider chdir() vs _chdir(), and different conventions
* regarding paths in argv[0] (naively enabling this code with
* backslash support on Windows leads to chdir into the wrong directory

This code correctly changed directory for me; verified with _getcwd().
Certainly backslash for arv[0] because the path was created by CRT.
chdir is the same as _chdir; deprecation warnings have low level (ignored now, and could be disabled).
strdup (or _strdup) and truncation with null character could give shorter code.

But there is a catch.
Full path for a data file may be specified in command line; and the data may contain paths relative to the file's location.
Which means, chdir should be to the data file's path, not executable's path. Also (forward) slashes might be here.
Unless this line

You need to run the tests suites from the tests directory.

means "executable must be copied to tests directory" - which is not always a great idea.
Another trouble:
const char *default_filename = "./test_suite_ssl.datax";
With the current code this path in command line means not what might be expected:
tests/test_suite_ssl ./test_suite_ssl.datax

@irwir irwir force-pushed the winsock2 branch 2 times, most recently from f033407 to 9cf367f Compare May 30, 2025 19:50
@irwir
Copy link
Contributor Author

irwir commented Jun 4, 2025

To verify that only tests were failing, one final check was attempted with the line #include <winsock2.h> commented out. Build process would not start automatically, hence the commit was reverted.

But that code isn't implemented for Windows (patches welcome).

It is hard to provide useful patches without decent understanding of the testing structure and the future plans.

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

Successfully merging this pull request may close these issues.

2 participants