Modifying the way has_include is used #519

Frosne · 2022-01-27T16:23:26Z

Summary

The PR modifies the has_include ('config.h') like in order to support the compilers where 'has_include' itself is not defined.

Motivation

We would like to support gcc4.4 and gcc4.8 compilers that fail because of the has_include.

This PR…
-->

Modifies the has_include to have a wrapper around it checking that has_include is supported

…rinsic file for MS Win compilers

msprotz

Thanks Natalia, this looks good. I have two questions.

Can you please explain a little bit more about intrin.h? Which toolchains is it needed on? What functions does it provide that were not in scope already? We've been successfully building on Windows via GitHub actions so it'd be great to document why we need another inclusion, when CI says we're fine.

Second request: the other obvious option is to always require config.h, and to always require that people who integrate the library create a (possibly-empty) config.h. It's less flexible, but has less macro-checking. Can you say a little more about why you prefer to define features via compiler flags?

None of these are issues -- it's just that we have been going back and forth on these things a lot, so if there's a good reason to pick one solution over the other, I want to document for the future. Thanks!

franziskuskiefer

Thanks. Two nits

franziskuskiefer · 2022-01-27T17:57:21Z

lib/c/evercrypt_targetconfig.h

@@ -18,11 +18,13 @@
 #define TARGET_ARCHITECTURE_ID_SYSTEMZ 5
 #define TARGET_ARCHITECTURE_ID_POWERPC64 6

+#if defined(__has_include)
 #if __has_include("config.h")
 #include "config.h"
 #else
 #define TARGET_ARCHITECTURE TARGET_ARCHITECTURE_ID_UNKNOWN


So TARGET_ARCHITECTURE isn't defined at all when __has_include is not available? We might want this defined in any case where we don't have the config.h.

I'm wondering if we should just get rid of TARGET_ARCHITECTURE. It doesn't seem to provide much added value and brings us further away from our goal of thinking exclusively in terms of features (rather than in terms of specific targets)

I wouldn't mind getting rid of it. But this PR changes when and how it gets added. I think we should keep the logic here and then think about removing it.

agreed that this should be a separate PR... it's just that we don't use that thing anywhere else (as far as I can tell) except for hacl_vec128_not_avx () and we could refactor that code to not depend on it...

franziskuskiefer · 2022-01-27T17:59:00Z

lib/c/libintvector.h

@@ -25,6 +27,10 @@
 #include <tmmintrin.h>
 #include <smmintrin.h>

+#if defined(_MSC_VER)
+  #include <intrin.h> 


As @msprotz mentioned, some information on this would be good. Why is this needed now and what wasn't working before?

I think I just remembered: intrin.h breaks the build when using cygwin-packaged, native mingw compilers. We should have CI for that toolchain, too.

Would it then be better to have smth like:

#ifdef _MSC_VER #if _MSC_VER > version #include <intrin.h> #endif #endif

Or will mingw be already filtered out by the #ifdef _MSC_VER clause?

I'm not sure -- I think you would need to check. Basically, I'm fine with any solution, as long as it doesn't break the mingw-based setup that several of us currently rely on.

@tahina-pro do we still have windows CI anywhere in our system that could check Natalia's patch? Or alternatively, could you tell us quickly if #include <intrin.h> works with x86_64-mingw64-gcc?

Right now we have no working Windows CI on our side. We only have a nightly ./everest make test on Windows, which has been failing for months now...
Re. intrin.h with x86_64-mingw64-gcc, I managed to compile a dummy C file containing just that include, but I guess that won't tell us much. I don't fully understand here, but I think the header in question might be https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-headers/crt/intrin.h and the
discussion at https://mingw-w64-public.narkive.com/9uy03D8V/what-is-the-purpose-of-intrin-h might be relevant

thanks Tahina... so intrin.h appears to define MSVC-like macros for mingw... at this point I'm thoroughly confused why we need that header

maybe to re-center the discussion, @Frosne, you can provide us with the compilation error that this include fixes? thanks!

It looks like it's indeed required by mingw

I would suggest pulling this change out of this PR and handling it separately as it appears to need clarification. The __has_include is uncontroversial and could go in.

Looking at the official documentation for MSVC intrin.h should be the only one needed. The others (tmmintrin.h and smmintrin) appear to be outdated.

MinGW supports intrin.h as well such that it should work for both environment.
We will have a Windows CI in a couple weeks to test this on and I might be able to tell you more next week. But I have to set up a Windows environment first.
If someone else has a Windows setup, they could give this a go with intrin.h only.

Frosne · 2022-02-02T17:49:41Z

Thanks Natalia, this looks good. I have two questions.

Can you please explain a little bit more about intrin.h? Which toolchains is it needed on? What functions does it provide that were not in scope already? We've been successfully building on Windows via GitHub actions so it'd be great to document why we need another inclusion, when CI says we're fine.

Small helpful link: https://dev.to/yumetodo/list-of-mscver-and-mscfullver-8nd.

It seems that our windows build requires it. Otherwise, our build returns the following exception: _mm_cmpgt_epi64': intrinsic function not declared. The problem gets solves once we add intrin.h file.

We use 191125547 MSC_VER(sion), HACL* CI runs using 192930139 Version.

franziskuskiefer · 2022-02-03T07:57:33Z

It seems that our windows build requires it. Otherwise, our build returns the following exception: _mm_cmpgt_epi64': intrinsic function not declared. The problem gets solves once we add intrin.h file.

Another issue here is that _mm_cmpgt_epi64 is SSE4.2 but is used when HACL_CAN_COMPILE_VEC128 is set. But HACL_CAN_COMPILE_VEC128 doesn't actually check for SSE4.2 but only for SSE2 and SSE3

Frosne · 2022-02-14T12:25:48Z

Btw, Lib_Memzero0.h header includes #include "libintvector.h" without using it. What do you think about removing it?

msprotz · 2022-02-14T19:17:49Z

So this PR is blocked on @franziskuskiefer and I's lack of understanding of why intrin.h is needed. To move forward, I suggest:

removing the intrin.h change from this PR so that we can merge it
opening a separate PR for intrin.h with a precise description of the build error (including error message), along with a description of the toolchain that needs this.

Regarding libintvector.h being over-included: yes, it would be good to fix. It's relatively easy to fix, see https://github.com/project-everest/hacl-star/blob/master/Makefile#L641-L645 -- libintvector.h is included indiscriminately, rather, it should be included once for each file that needs it like is the case for lib_intrinsics.h just below.

Thanks!

msprotz

All looks good to me, thanks!

msprotz · 2022-02-22T21:20:19Z

@franziskuskiefer this PR now has the intrin.h bit removed and only concerns __has_include

franziskuskiefer

Thanks!

Wrapping has_include with if defined has_include and including an int…

33276bf

…rinsic file for MS Win compilers

Frosne requested review from beurdouche, msprotz and franziskuskiefer January 27, 2022 16:23

msprotz approved these changes Jan 27, 2022

View reviewed changes

franziskuskiefer requested changes Jan 27, 2022

View reviewed changes

polubelova and others added 2 commits January 27, 2022 22:40

Merge branch 'master' into anilam_config

6aa9f82

Merge branch 'master' into anilam_config

79ed08f

Merge branch 'master' into anilam_config

b0de4b4

franziskuskiefer mentioned this pull request Feb 3, 2022

Ensure Toolchain and Platform Checks are correct cryspen/hacl-packages#27

Closed

6 tasks

Merge branch 'master' into anilam_config

297d78b

Frosne and others added 2 commits February 22, 2022 11:48

Merge branch 'master' into anilam_config

4cd69d1

Removing libintvector.h modifications

09bb1c1

Frosne changed the title ~~Modifying the way has_include is used and adding an intrinsic file~~ Modifying the way has_include is used Feb 22, 2022

msprotz approved these changes Feb 22, 2022

View reviewed changes

Merge branch 'master' into anilam_config

971d79a

beurdouche approved these changes Feb 22, 2022

View reviewed changes

franziskuskiefer approved these changes Feb 23, 2022

View reviewed changes

beurdouche merged commit bdcfbe6 into master Feb 23, 2022

karthikbhargavan deleted the anilam_config branch September 6, 2022 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifying the way has_include is used #519

Modifying the way has_include is used #519

Frosne commented Jan 27, 2022 •

edited

msprotz left a comment

franziskuskiefer left a comment

franziskuskiefer Jan 27, 2022

msprotz Jan 27, 2022

franziskuskiefer Jan 27, 2022

msprotz Jan 27, 2022

franziskuskiefer Jan 27, 2022

msprotz Feb 2, 2022

Frosne Feb 2, 2022 •

edited

Frosne Feb 2, 2022

msprotz Feb 2, 2022

tahina-pro Feb 2, 2022 •

edited

msprotz Feb 8, 2022

Frosne Feb 8, 2022

franziskuskiefer Feb 9, 2022

Frosne commented Feb 2, 2022

franziskuskiefer commented Feb 3, 2022

Frosne commented Feb 14, 2022

msprotz commented Feb 14, 2022

msprotz left a comment

msprotz commented Feb 22, 2022

franziskuskiefer left a comment

Modifying the way has_include is used #519

Modifying the way has_include is used #519

Conversation

Frosne commented Jan 27, 2022 • edited

Summary

Motivation

msprotz left a comment

Choose a reason for hiding this comment

franziskuskiefer left a 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

Frosne Feb 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tahina-pro Feb 2, 2022 • edited

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

Frosne commented Feb 2, 2022

franziskuskiefer commented Feb 3, 2022

Frosne commented Feb 14, 2022

msprotz commented Feb 14, 2022

msprotz left a comment

Choose a reason for hiding this comment

msprotz commented Feb 22, 2022

franziskuskiefer left a comment

Choose a reason for hiding this comment

Frosne commented Jan 27, 2022 •

edited

Frosne Feb 2, 2022 •

edited

tahina-pro Feb 2, 2022 •

edited