Re: Portability: common-src/amcrc32chw.c & config/amanda/amanda_configure.m4

Jean-Louis Martineau <[email protected]>
Newsgroups gmane.comp.archivers.amanda.devel
Message-ID <[email protected]> 
On 03/01/17 05:14 PM, Eric Schnoebelen wrote:
> Jean-Louis Martineau writes:
> - amanda detect at run time if the sse4.2 instruction are available and=20
> - use them only if they are available.
> - different code is used if the sse4.2 instruction are not available.
> - What's wrong with that?
>
> Backing up.
>
> The original code *assumed* that if you were using GCC and it
> was at least 4.3 on any x86 processor architecture, the
> __builtin_ia32_* functions would be available.
>
> However, as you've stated, that is only true if the compliler is
> called with the ``-msse4.2'' (or other -msse* flags?).
>
> And amanda *assumed* it was being compiled with the ``-msse4.2''
> (or similar) flag.
That's the problem, you removed the flag
>
> pkgsrc explicitly strips such flags from compile tests for
> maximal portability for it's binary builds across systems.
As I already said, even if the binary have sse4.2 instructions, they are 
executed only if the CPU have it, there is a runtime check for there 
availability.
>
> Given that gcc wasn't given ``-msse4.2'' on the complile/load
> command line, the __builtin_ia32_* functions were unable to be
> resolved at link time.
>
> Summary: amanda was blindly using (builtin) library routines
> with out insuring that they were resolvable at compile/link
> time.  My original patch at least tested for their existence and
> didn't try to use that code path if a compile test for the
> functions failed.
Your patch never detected them.
>
> - If I understand, you want an option to completely disable sse4.2?
> - You do not want newer CPU to use sse4.2
> - I don't understand since amanda detect sse4.2 at run time.
>
> See above about amanda in pkgsrc not getting the compile time
> compiler flag detection.
>
> Effectively, pkgsrc isn't allowing the ``-msse4.2'' flag through
> to the test cases to expose the associated builtins.
>
> - On 03/01/17 02:45 PM, Eric Schnoebelen wrote:
> - > Jean-Louis Martineau writes:
> - > - I think I can replace the #ifdef line with:
> - > - #ifdef __SSE_4_2_
> - > -
> - > - Because if __SSE_4_2_ is defined then the __builtin_ia32_crc32* function
> - > - must be defined
> - >
> - > Depending on a compiler specific internal macro definition seems
> - > fraught with danger.  Imagine the case of xycc (random non-gcc
> - > compiler) defining __SSE_4_2_ for some purpose, but not
> - > providing these particular functions.  Feature testing for the
> - > functions used is almost always a better idea.
>
> - If the xycc compiler define __SSE4_2__ for a different meaning then that
> - compiler is broken.
> -
> - Both gcc and clang define it
>
> While I would hope that the developers of xycc would look to
> existing practice in defining a __SSE4_2__ macro today.  Of course,
> xycc's definition of __SSE4_2__ may predate Intel's definition of
> the sse4.2 instruction set, and GCC's addition of said macro to
> their compiler.
>
> Hoever, being prefixed with a double underscore, it is a macro in
> the compiler implementation name space, and is not guarenteed to
> be unique across all compilers, or have the same meaning on all
> compilers.  (at least that is my memory of reading the ANSI C
> standard back when I was implementing an ANSI C/POSIX.1 libc circa
> 1991.)
>
> If you want to use __SSE4_2__ in a more robust, portable fashion,
> I would recommend testing it along with the macro describing the
> compiler that is known to support it, eg
>
>    #if defined __GNUC__ && GCC_VERSION > 40300 && defined __SSE4_2__
>    #endif
>
> or
>
>    #if (defined __GNUC__ || defined __CLANG__ ) && defined __SSE4_2__
>    #endif
>
> (assuming __CLANG__ is the macro clang defines to describe itself,
> and that gcc before 4.3 wouldn't ever define __SSE4_2__ )
>
I prefer to allow __SSE4_2__ for all compiler and disable xycc when we 
found it is broken with something like:

#if defined __SSE4_2 && !defined __xycc__

I committed the attached patch, try it and tell me if it fix your issue

Jean-Louis

Disclaimer

The information contained in this communication from the sender is confidential. It is intended solely for use by the recipient and others authorized to receive it. If you are not the recipient, you are hereby notified that any disclosure, copying, distribution or taking action in relation of the contents of this information is strictly prohibited and may be unlawful.

This email has been scanned for viruses and malware, and may have been automatically archived by Mimecast Ltd, an innovator in Software as a Service (SaaS) for business. Providing a safer and more useful place for your human generated data. Specializing in; Security, archiving and compliance. To find out more visit the Mimecast website.
sse4_2.diff (text/x-patch, 5.3 KB) 
diff --git a/common-src/Makefile.am b/common-src/Makefile.am
index 221f7ba..2559e44 100644
--- a/common-src/Makefile.am
+++ b/common-src/Makefile.am
@@ -255,6 +255,8 @@ amsemaphore_test_LDADD = libamanda.la libtestutils.la
 
 crc32_test_SOURCES = crc32-test.c
 crc32_test_LDADD = libamanda.la libtestutils.la
+crc32-test.o: AM_CFLAGS += $(SSE42_CFLAGS)
+crc32-test.lo: AM_CFLAGS += $(SSE42_CFLAGS)
 
 quoting_test_SOURCES = quoting-test.c
 quoting_test_LDADD = libamanda.la libtestutils.la
diff --git a/common-src/amcrc32chw.c b/common-src/amcrc32chw.c
index 0a2ac14..994cfe2 100644
--- a/common-src/amcrc32chw.c
+++ b/common-src/amcrc32chw.c
@@ -29,7 +29,8 @@
 #include <amutil.h>
 #include <amcrc32chw.h>
 
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
+#ifdef __SSE4_2__
+gboolean compiled_with_sse4_2 = TRUE;
 #define POLY 0x82F63B78
 
 /* Multiply a matrix times a vector over the Galois field of two elements,
@@ -140,7 +141,8 @@ static uint32_t crc32c_short[4][256];
 static uint32_t crc32c_low[4][256];
 
 /* Initialize tables for shifting crcs. */
-void crc32c_init_hw(void)
+void
+crc32c_init_hw(void)
 {
     crc32c_zeros(crc32c_long, LONG);
     crc32c_zeros(crc32c_short, SHORT);
@@ -340,8 +342,10 @@ void crc32c_add_hw(uint8_t *buf, size_t len, crc_t *crc)
 }
 
 #else
+gboolean compiled_with_sse4_2 = FALSE;
 
-void crc32c_init_hw(void)
+void
+crc32c_init_hw(void)
 {
    g_error("crc32c_init_hw is not defined");
 }
diff --git a/common-src/amcrc32chw.h b/common-src/amcrc32chw.h
index cf28fbd..a90a4b7 100644
--- a/common-src/amcrc32chw.h
+++ b/common-src/amcrc32chw.h
@@ -30,6 +30,7 @@
 #include <amanda.h>
 #include <amutil.h>
 
+extern gboolean compiled_with_sse4_2;
 void crc32c_init_hw(void);
 void crc32c_add_hw(uint8_t *buf, size_t len, crc_t *crc);
 
diff --git a/common-src/amutil.c b/common-src/amutil.c
index 55d64c1..60ba136 100644
--- a/common-src/amutil.c
+++ b/common-src/amutil.c
@@ -2071,7 +2071,7 @@ make_amanda_tmpdir(void)
 }
 
 #define POLY 0x82F63B78
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
+#if defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__
 static int get_sse42(void)
 {
     uint32_t op, eax, ebx, ecx, edx;
@@ -2100,12 +2100,10 @@ static int get_sse42(void)
 
 static uint32_t crc_table[16][256];
 static gboolean crc_initialized = FALSE;
-int have_sse42 = 0;
+gboolean have_sse42 = FALSE;
 void (* crc32_function)(uint8_t *buf, size_t len, crc_t *crc);
 
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
   #include "amcrc32chw.h"
-#endif
 
 /* Run this function previously */
 void
@@ -2116,14 +2114,15 @@ make_crc_table(void)
     int slice;
 
     if (!crc_initialized) {
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
-	have_sse42 = get_sse42();
+	if (compiled_with_sse4_2) {
+	    have_sse42 = get_sse42();
+	}
 	if (have_sse42) {
 	    crc32c_init_hw();
 	    crc32_function = &crc32c_add_hw;
-	} else
-#endif
+	} else {
             crc32_function = &crc32_add_16bytes;
+	}
 
         for (i = 0; i < 256; i++) {
             uint32_t c = i;
diff --git a/common-src/crc32-test.c b/common-src/crc32-test.c
index be9cb19..afa7718 100644
--- a/common-src/crc32-test.c
+++ b/common-src/crc32-test.c
@@ -49,25 +49,25 @@ test_size(
 {
     crc_t crc1;
     crc_t crc16;
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
+#ifdef __SSE4_2__
     crc_t crchw;
 #endif
 
     crc32_init(&crc1);
     crc32_init(&crc16);
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
+#ifdef __SSE4_2__
     crc32_init(&crchw);
 #endif
 
     crc32_add_1byte(test_buf, size, &crc1);
     crc32_add_16bytes(test_buf, size, &crc16);
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
+#ifdef __SSE4_2__
     if (have_sse42) {
 	crc32c_add_hw(test_buf, size, &crchw);
     }
 #endif
 
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
+#ifdef __SSE4_2__
     g_fprintf(stderr, " %08x:%lld  %08x:%lld  %08x:%lld\n", crc32_finish(&crc1), (long long)crc1.size, crc32_finish(&crc16), (long long)crc16.size, crc32_finish(&crchw), (long long)crchw.size);
 #else
     g_fprintf(stderr, " %08x:%lld  %08x:%lld\n", crc32_finish(&crc1), (long long)crc1.size, crc32_finish(&crc16), (long long)crc16.size);
@@ -78,7 +78,7 @@ test_size(
 	g_fprintf(stderr, " CRC16 %zu %08x:%lld != %08x:%lld\n", size, crc32_finish(&crc1), (long long)crc1.size, crc32_finish(&crc16), (long long)crc16.size);
 	return FALSE;
     }
-#if defined __GNUC__ && GCC_VERSION > 40300 && (defined __x86_64__ || defined __i386__ || defined __i486__ || defined __i586__ || defined __i686__)
+#ifdef __SSE4_2__
     if (have_sse42) {
 	if (crc1.crc != crchw.crc ||
 	    crc1.size != crchw.size) {
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.