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

#issue1446 : 修复x86 ubuntu16 链接_rdseed64_step找不到问题 #1447

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alittlehorse
Copy link

修复x86 ubuntu16 链接_rdseed64_step找不到问题
#1446

@alittlehorse
Copy link
Author

@guanzhi hello, please run workflow and see cmake result in ios, android or other archs.

@alittlehorse
Copy link
Author

alittlehorse commented Mar 15, 2023

Only using gcc and in x86 or x86_64 arch ,we use x86intrin.h. Please review and run workflow.

@zzl360
Copy link

zzl360 commented Mar 15, 2023

@alittlehorse the above line #include "immintrin.h" is also only in x86 or x86_64.
I believe they also need to be under the macro

@alittlehorse
Copy link
Author

@zzl360 you are right. done

@zzl360
Copy link

zzl360 commented Mar 15, 2023

my mistake, according to

GmSSL/CMakeLists.txt

Lines 259 to 267 in 2167dda

option(ENABLE_RDRND "Enable Intel RDRND instructions" OFF)
if (${CMAKE_SYSTEM_PROCESSOR} MATCHES x86_64)
set(ENABLE_RDRND ON)
endif()
if (ENABLE_RDRND)
message(STATUS "ENABLE_RDRND")
list(APPEND src src/rdrand.c)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -mrdrnd -mrdseed")
endif()

this file will be compiled only on x86_64,so no need to use macro

@alittlehorse
Copy link
Author

yes, the option ENABLE_RDRND is matched with x86_64.

the link error in #1446 is the same with https://gcc.gnu.org/legacy-ml/gcc-bugs/2015-11/msg01051.html, so the fix as below will work.

#include <immintrin.h>
#if defined(__GNUC__) 
#include <x86intrin.h>
#endif

@zzl360
Copy link

zzl360 commented Mar 16, 2023

weidai11/cryptopp@951d8b9
I think this is better

@alittlehorse
Copy link
Author

we should add gcc version check . This is what you mean, right?

#include <immintrin.h>
#if defined(__GNUC__) && (_GNUC_MAJOR__ > 4 || (__GNUC_MAJOR__ == 4 && __GNUC_MINOR__ >= 6))
#include <x86intrin.h>
#endif

@zzl360
Copy link

zzl360 commented Mar 16, 2023

we should add gcc version check . This is what you mean, right?

#include <immintrin.h>
#if defined(__GNUC__) && (_GNUC_MAJOR__ > 4 || (__GNUC_MAJOR__ == 4 && __GNUC_MINOR__ >= 6))
#include <x86intrin.h>
#endif

yes

@alittlehorse
Copy link
Author

@zzl360 please review and run workflows

@zzl360
Copy link

zzl360 commented Mar 16, 2023

@zzl360 please review and run workflows

LGTM
I'm not the maintainer of GMSSL,I think the workflow will be run after approvalled.

@alittlehorse
Copy link
Author

alittlehorse commented Mar 16, 2023

We'd better replace __GNUC_MAJOR__ with __GNUC__.

#include <immintrin.h>
#if defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))
#include <x86intrin.h>
#endif

@alittlehorse
Copy link
Author

@guanzhi hello, please review and run workflows. @zzl360 thanks to your review

@alittlehorse
Copy link
Author

@guanzhi hello, professor guan, please review. thanks a lot

@alittlehorse
Copy link
Author

Hello, this issue has appeared several times before, please review and provide corresponding feedback

@alittlehorse
Copy link
Author

please merge this pull request. it has appeared several times

@Gitxiaozhu-oss
Copy link

请问大佬们怎么解决这个问题的?

@zzl360
Copy link

zzl360 commented May 5, 2023

请问大佬们怎么解决这个问题的?

参考这个pr加头文件

@alittlehorse
Copy link
Author

@guanzhi Hello, professor guan! Please review and merge this pr. We have seen several issues related to it in the pr.

@alittlehorse
Copy link
Author

@guanzhi plz merge this pr to fix issue: #1446

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.

3 participants