Code review ACK

Concept ACK

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• build: avoid repetitions when enabling warnings in configure.ac #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
• test, build: Enable -Werror=sign-compare #18216 (test, build: Enable -Werror=sign-compare by Empact)
• build: Enable -Wsuggest-override if available #16710 (build: Enable -Wsuggest-override if available by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Please don't bundle unrelated changes like fe466a4 into a PR like this.

r1 would be set to 0 on rdrand failure, so initializing it to 0 is a non-functional change.

Can you at least link to documentation that says this is the case.

Please don't bundle unrelated changes like fe466a4 into a PR like this.

Sorry for that. I considered it would be ok as long as it is a separate commit and the main change in this PR steps on top of it. Now I submitted that dedup as a separate PR at #18857, removed it from this PR and made this PR to use the old AX_CHECK_COMPILE_FLAG() so that there is no dependency between the two PRs and either one can be merged first. As a result they conflict but it is trivial to resolve - once one of them gets merged (if that happens!) I will adjust the other one.

r1 would be set to 0 on rdrand failure, so initializing it to 0 is a non-functional change.

Can you at least link to documentation that says this is the case.

Done in the commit message:

From "Intel 64 and IA-32 ArchitecturesSoftware Developer's Manual" [1],
page 1711: "CF=1 indicates that the data in the destination is valid.
Otherwise CF=0 and the data in the destination operand will be returned
as zeros for the specified width."

[1] https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf

Thanks for looking into this!

It would be more readable to use _rdrand64_step() from immintrin.h or __builtin_ia32_rdrand64_step() instead of __asm__ volatile (".byte 0x48, 0x0f, 0xc7, 0xf0; setc %1" : "=a"(r1), "=q"(ok) :: "cc"); reference.

Out of the scope of this PR.

@vasild The annoying part about using intrinsics is that they require moving the code to a separate module and compiling it separately. That's because in order to get access to _rdrand64_step(), you need to compile with -mrdrnd, which makes the compiler assume that rdrand is available throughout the entire module (which could in theory cause the compiler to emit such instructions even in cases where it's running on a system that doesn't have that instruction).

ACK 71f183a

ACK 71f183a