Fix VS2022 build error #7502

daverodgman · 2023-04-26T20:54:24Z

Description

Fixes #7501 .

Gatekeeper checklist

changelog not needed
backport not in 2.28
tests already covered

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

gilles-peskine-arm · 2023-04-26T21:01:44Z

library/bignum_core.h

@@ -109,7 +109,38 @@
 * \return      The number of leading zero bits in \p a, if \p a != 0.
 *              If \p a == 0, the result is undefined.
 */
-size_t mbedtls_mpi_core_clz(mbedtls_mpi_uint a);
+static inline size_t mbedtls_mpi_core_clz(mbedtls_mpi_uint a)


It's weird that arranging to have two copies of the function (since it's used in two different C files) makes the code smaller. How is this happening? Is this maybe only on platforms that have a CLZ instruction — what's the impact on platforms that don't?

Without clz there’s still a 16-byte saving in bignum_core.o, it's just that it is 20 bytes bigger without clz in both development and this PR (code size goes from 2446 to 2466 without clz)

Ah - the clz function is only used in bignum.o if we don't have MBEDTLS_HAVE_UDBL

I suspect a clz instruction is near-universal. E.g., Arm has had it since Armv5. So this function will almost always turn into a single instruction.

It seems to get inlined regardless of marking it static inline (at least on my machine where the clz instruction is present). So the effect of the static inline is really just to eliminate the not-inlined copy of the function, which is small and presumably eliminated during the final link stage. Do we want to simplify and drop the inlining?

static in a header gives you a copy of the function in every module that includes the header. Most compilers are smart enough to eliminate the function if the module doesn't use it at all, but that still leaves two copies in the two modules that do use this particular function (and possibly more later). So no, we shouldn't have static functions in a header.

Err, actually static restricts visibility. If nothing needs an actual not-inlined function, the compiler is free to not include it, and I would expect most modern compilers will do this. An out-of-line function implementation is usually only needed if the address of the function is taken at some point

Sorry, I meant: should we return it to bignum_core.c, as a non-inline, non-static function, and retain a declaration only in the header?

This PR saved bytes over the code in development. I think it is okay

should we return it to bignum_core.c, as a non-inline, non-static function, and retain a declaration only in the header?

Yes, we should do the simple thing (declare in header, define in .c) unless there's a compelling reason to do otherwise. And it seems that there's no compelling reason after all.

tom-cosgrove-arm · 2023-04-26T21:01:55Z

I'd like the know why the CI didn't catch this problem. Is it just Visual Studio 2022? In which case, we really need to update the compiler versions we use...

gilles-peskine-arm · 2023-04-26T21:02:13Z

library/bignum_core.c

@@ -33,39 +33,6 @@
 #include "bn_mul.h"
 #include "constant_time_internal.h"



What could we put in our CI to ensure that if someone tries this again, the CI will complain?

Yes, I'd like to know that too!

tom-cosgrove-arm · 2023-04-27T06:37:37Z

I'd like the know why the CI didn't catch this problem. Is it just Visual Studio 2022? In which case, we really need to update the compiler versions we use...

Hmm, seen in the CI logs this morning

   319>test_suite_bignum_core.generated.obj : error LNK2019: unresolved external symbol _mbedtls_mpi_core_clz referenced in function _test_mpi_core_clz_wrapper [C:\builds\workspace\mbed-tls-nightly-tests\worktrees\tmpxsl0yyyt\cmake_solution\tests\test_suite_bignum_core.generated.vcxproj]
   319>C:\builds\workspace\mbed-tls-nightly-tests\worktrees\tmpxsl0yyyt\cmake_solution\tests\Release\test_suite_bignum_core.generated.exe : fatal error LNK1120: 1 unresolved externals [C:\builds\workspace\mbed-tls-nightly-tests\worktrees\tmpxsl0yyyt\cmake_solution\tests\test_suite_bignum_core.generated.vcxproj]
   319>Done Building Project "C:\builds\workspace\mbed-tls-nightly-tests\worktrees\tmpxsl0yyyt\cmake_solution\tests\test_suite_bignum_core.generated.vcxproj" (Rebuild target(s)) -- FAILED.
:
:
+---------+---------------+--------------+------------+--------------------+------------------+--------------------+----------------+-------------------+
| Version | Configuration | Architecture | Retargeted |   shipped build    | shipped selftest |    cmake build     | cmake selftest | cmake test suites |
+---------+---------------+--------------+------------+--------------------+------------------+--------------------+----------------+-------------------+
|   2015  |    Release    |    Win32     |   False    |        Pass        |       Pass       | Pass with warnings |      Pass      |        Pass       |
|   2015  |    Release    |    Win32     |    True    |        Pass        |       Pass       |        Fail        |    Not Run     |      Not Run      |
|   2015  |    Release    |     x64      |   False    |        Pass        |       Pass       | Pass with warnings |      Pass      |        Pass       |
|   2015  |    Release    |     x64      |    True    |        Pass        |       Pass       |        Fail        |    Not Run     |      Not Run      |
|   2015  |     Debug     |    Win32     |   False    | Pass with warnings |       Pass       | Pass with warnings |      Pass      |        Pass       |
|   2015  |     Debug     |    Win32     |    True    | Pass with warnings |       Pass       | Pass with warnings |      Pass      |        Pass       |
|   2015  |     Debug     |     x64      |   False    | Pass with warnings |       Pass       | Pass with warnings |      Pass      |        Pass       |
|   2015  |     Debug     |     x64      |    True    | Pass with warnings |       Pass       | Pass with warnings |      Pass      |        Pass       |
+---------+---------------+--------------+------------+--------------------+------------------+--------------------+----------------+-------------------+

gilles-peskine-arm · 2023-04-27T08:53:19Z

Hmm, seen in the CI logs this morning

I remember we have a discrepancy in the set of Windows tests we run in nightlies vs in pull requests: PR jobs only test with VS2013, but nightlies also test VS2015 and VS2017. To my recollection this is the first time the VS2013 job passes but not the more recent versions (which is why we didn't really care about the discrepancy until now).

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

tom-cosgrove-arm

This needs to go in to fix the nightlies

gilles-peskine-arm

Keeping things simple, I like it.

Fix VS2022 build error

3b29364

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman requested a review from tom-cosgrove-arm April 26, 2023 20:54

daverodgman added bug needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Apr 26, 2023

daverodgman mentioned this pull request Apr 26, 2023

Undefined mbedtls_mpi_core_clz #7501

Closed

gilles-peskine-arm reviewed Apr 26, 2023

View reviewed changes

gilles-peskine-arm mentioned this pull request Apr 27, 2023

Unify Windows testing between PR and release jobs Mbed-TLS/mbedtls-test#27

Open

Don't explicitly inline mbedtls_mpi_core_clz

914347b

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman removed the needs-ci Needs to pass CI tests label Apr 28, 2023

tom-cosgrove-arm approved these changes Apr 28, 2023

View reviewed changes

tom-cosgrove-arm added the priority-very-high Highest priority - prioritise this over other review work label Apr 28, 2023

gilles-peskine-arm approved these changes Apr 28, 2023

View reviewed changes

gilles-peskine-arm merged commit 7351101 into Mbed-TLS:development Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VS2022 build error #7502

Fix VS2022 build error #7502

daverodgman commented Apr 26, 2023 •

edited

Loading

gilles-peskine-arm Apr 26, 2023

tom-cosgrove-arm Apr 26, 2023

tom-cosgrove-arm Apr 26, 2023 •

edited

Loading

daverodgman Apr 27, 2023

daverodgman Apr 27, 2023

gilles-peskine-arm Apr 27, 2023

tom-cosgrove-arm Apr 27, 2023

daverodgman Apr 27, 2023

tom-cosgrove-arm Apr 27, 2023

gilles-peskine-arm Apr 27, 2023

tom-cosgrove-arm commented Apr 26, 2023

gilles-peskine-arm Apr 26, 2023

tom-cosgrove-arm Apr 26, 2023

tom-cosgrove-arm commented Apr 27, 2023

gilles-peskine-arm commented Apr 27, 2023

tom-cosgrove-arm left a comment

gilles-peskine-arm left a comment

		@@ -33,39 +33,6 @@
		#include "bn_mul.h"
		#include "constant_time_internal.h"

Fix VS2022 build error #7502

Fix VS2022 build error #7502

Conversation

daverodgman commented Apr 26, 2023 • edited Loading

Description

Gatekeeper checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Apr 27, 2023

gilles-peskine-arm commented Apr 27, 2023

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

daverodgman commented Apr 26, 2023 •

edited

Loading

tom-cosgrove-arm Apr 26, 2023 •

edited

Loading