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

Fixed tests on OpenBSD #142

Merged
merged 1 commit into from
Oct 23, 2019
Merged

Fixed tests on OpenBSD #142

merged 1 commit into from
Oct 23, 2019

Conversation

ston1th
Copy link
Contributor

@ston1th ston1th commented Oct 19, 2019

Since OpenBSD 6.0 W^X is enforced.
Added RANDOMX_FLAG_SECURE in tests and benchmarks.

Excluded cpu_set_t since it is not defined on OpenBSD.

@tevador
Copy link
Owner

tevador commented Oct 19, 2019

Do all tests pass on OpenBSD?

@ston1th
Copy link
Contributor Author

ston1th commented Oct 19, 2019

Yes they do.
Except for the AVX2 test, which is skipped because I do not have the required hardware.

@tevador
Copy link
Owner

tevador commented Oct 20, 2019

I'm thinking to add RANDOMX_FLAG_SECURE to randomx_get_flags on OpenBSD, since the purpose of this function is to return flags that will work 100% on the target system. I think if you try mining in monerod it will fail even with this patch.

RandomX/src/randomx.cpp

Lines 42 to 55 in 4296c35

randomx_flags randomx_get_flags() {
randomx_flags flags = RANDOMX_HAVE_COMPILER ? RANDOMX_FLAG_JIT : RANDOMX_FLAG_DEFAULT;
randomx::Cpu cpu;
if (HAVE_AES && cpu.hasAes()) {
flags |= RANDOMX_FLAG_HARD_AES;
}
if (randomx_argon2_impl_avx2() != nullptr && cpu.hasAvx2()) {
flags |= RANDOMX_FLAG_ARGON2_AVX2;
}
if (randomx_argon2_impl_ssse3() != nullptr && cpu.hasSsse3()) {
flags |= RANDOMX_FLAG_ARGON2_SSSE3;
}
return flags;
}

@ston1th
Copy link
Contributor Author

ston1th commented Oct 20, 2019

Good idea. I updated the patch to account for this.

Despite the point I don't think OpenBSD is a system to mine efficiently on :)

@tevador
Copy link
Owner

tevador commented Oct 20, 2019

@ston1th Please also update the comment here

* RANDOMX_FLAG_SECURE

It should say that it sets the flag on OpenBSD because it's enforced by the OS.

Since OpenBSD 6.0 W^X is enforced.
Added `RANDOMX_FLAG_SECURE` in tests and benchmarks.
Updated comment.

Excluded `cpu_set_t` since it is not defined on OpenBSD.
@@ -65,7 +65,7 @@ set_thread_affinity(std::thread::native_handle_type thread,
(thread_policy_t)&policy, 1);
#elif defined(_WIN32) || defined(__CYGWIN__)
rc = SetThreadAffinityMask(reinterpret_cast<HANDLE>(thread), 1ULL << cpuid) == 0 ? -2 : 0;
#else
#elif !defined(__OpenBSD__)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with this on OpenBSD?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, I missed your comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should actually be rewritten for OpenBSD: https://man.openbsd.org/NetBSD-7.0.1/affinity.3
Struct name is cpuset_t there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may be mistaken but you are referring to a man page of NetBSD.
I can't find either one of these in the OpenBSD sources: cpu_set_t, cpuset_t, CPU_ZERO, CPU_SET.

I think there is no thread affinity for OpenBSD: https://man.openbsd.org/pthreads.3

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you're right and it seems to be a design decision for OpenBSD.

@SChernykh
Copy link
Collaborator

SChernykh commented Oct 21, 2019

@ston1th
Excluded cpu_set_t since it is not defined on OpenBSD.
Just add #define cpu_set_t cpuset_t for OpenBSD. It should work fine, just the name is different on OpenBSD.

Edit: nvm, OpenBSD doesn't support it.

@tevador tevador merged commit 34aba9d into tevador:master Oct 23, 2019
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.

4 participants