From a7a7387575b87a7ae551307a24835f549d530d14 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 3 May 2018 11:32:57 -0400 Subject: [PATCH] Further improve code for probing the availability of ARM CRC instructions. Andrew Gierth pointed out that commit 1c72ec6f4 would yield the wrong answer on big-endian ARM systems, because the data being CRC'd would be different. To fix that, and avoid the rather unsightly hard-wired constant, simply compare the hardware and software implementations' results. While we're at it, also log the resulting decision at DEBUG1, and error out if the hw and sw results unexpectedly differ. Also, since this file must compile for both frontend and backend, avoid incorrect dependencies on backend-only headers. In passing, add a comment to postmaster.c about when the CRC function pointer will get initialized. Thomas Munro, based on complaints from Andrew Gierth and Tom Lane Discussion: https://postgr.es/m/HE1PR0801MB1323D171938EABC04FFE7FA9E3110@HE1PR0801MB1323.eurprd08.prod.outlook.com --- src/backend/postmaster/postmaster.c | 10 +++++++- src/port/pg_crc32c_armv8_choose.c | 37 +++++++++++++++++++++++------ 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index d948369f3e..a4b53b33cd 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -957,7 +957,15 @@ PostmasterMain(int argc, char *argv[]) */ CreateDataDirLockFile(true); - /* read control file (error checking and contains config) */ + /* + * Read the control file (for error checking and config info). + * + * Since we verify the control file's CRC, this has a useful side effect + * on machines where we need a run-time test for CRC support instructions. + * The postmaster will do the test once at startup, and then its child + * processes will inherit the correct function pointer and not need to + * repeat the test. + */ LocalProcessControlFile(false); /* diff --git a/src/port/pg_crc32c_armv8_choose.c b/src/port/pg_crc32c_armv8_choose.c index d0d3a3da78..c339af7f16 100644 --- a/src/port/pg_crc32c_armv8_choose.c +++ b/src/port/pg_crc32c_armv8_choose.c @@ -18,11 +18,15 @@ *------------------------------------------------------------------------- */ -#include "c.h" +#ifndef FRONTEND +#include "postgres.h" +#else +#include "postgres_fe.h" +#endif #include +#include -#include "libpq/pqsignal.h" #include "port/pg_crc32c.h" @@ -33,7 +37,7 @@ static sigjmp_buf illegal_instruction_jump; * isn't available, we expect to get SIGILL, which we can trap. */ static void -illegal_instruction_handler(int signo) +illegal_instruction_handler(SIGNAL_ARGS) { siglongjmp(illegal_instruction_jump, 1); } @@ -42,16 +46,35 @@ static bool pg_crc32c_armv8_available(void) { uint64 data = 42; - bool result; + int result; + /* + * Be careful not to do anything that might throw an error while we have + * the SIGILL handler set to a nonstandard value. + */ pqsignal(SIGILL, illegal_instruction_handler); if (sigsetjmp(illegal_instruction_jump, 1) == 0) - result = (pg_comp_crc32c_armv8(0, &data, sizeof(data)) == 0xdd439b0d); + { + /* Rather than hard-wiring an expected result, compare to SB8 code */ + result = (pg_comp_crc32c_armv8(0, &data, sizeof(data)) == + pg_comp_crc32c_sb8(0, &data, sizeof(data))); + } else - result = false; + { + /* We got the SIGILL trap */ + result = -1; + } pqsignal(SIGILL, SIG_DFL); - return result; +#ifndef FRONTEND + /* We don't expect this case, so complain loudly */ + if (result == 0) + elog(ERROR, "crc32 hardware and software results disagree"); + + elog(DEBUG1, "using armv8 crc32 hardware = %d", (result > 0)); +#endif + + return (result > 0); } /* -- 2.40.0