#233 Patch: Clamp float_t to double on s390 to keep ABI stable

[Marius Hillenbrand]

clucene's API makes heavy use of the type float_t. On s390, float_t has historically been defined as double for no good reason. For getting rid of performance overhead in some cases and contradictions with the C standard in others, we discuss plans to clean up that definition - float_t should become float on s390.

As a result of that change, all these places in clucene's ABI would flip from double to float. Existing shared libs of clucene would become incompatible with binaries built with new versions of glibc/gcc and vice versa -- potentially causing very bad update experiences.

To avoid that ABI breakage, I propose to stabilize the use of float_t to always use double on s390x. Please review my attached patch. What do you think of this approach? What alternative may I have missed?

Background

To give you some background on float_t: that type is meant to represent the range and precision that float expressions are evaluated in internally. The s390x architecture supports single-precision float
operations and gcc by default uses them. Though, on s390x float_t has historically been defined as double because the port of glibc to s390x picked up what was then glibc's default.

Since that combination is contradicting wrt the C standard, gcc evaluates expressions of type float in double precision when called in standard-compliant mode (e.g., with -std=c99) -- at the cost of overhead.

To avoid that overhead and be compliant with the C standard (and common sense), we are discussing plans to change float_t to float and s390 to match gcc's default behavior. To avoid breaking the ABI of clucene in the process, this patch changes clucene to always use double in place of float_t on s390, independent of how float_t is defined on the system.

Patch

diff --git a/src/shared/CLucene/SharedHeader.h b/src/shared/CLucene/SharedHeader.h
index d43a1491..d512fd9f 100644
--- a/src/shared/CLucene/SharedHeader.h
+++ b/src/shared/CLucene/SharedHeader.h
@@ -65,6 +65,13 @@
     #include <tchar.h> //required for _T and TCHAR
 #endif

+////////////////////////////////////////////////////////
+//architecture-specific quirks for types
+////////////////////////////////////////////////////////
+#ifdef __s390__
+typedef double float_t;
+#endif
+
 ////////////////////////////////////////////////////////
 //namespace helper
 ////////////////////////////////////////////////////////

[Balint Reczey]

This conflicts with the definition in math.h when building with glibc 2.33.

https://bugs.launchpad.net/ubuntu/+source/libreoffice/+bug/1915927

One possible option is keeping the ABI but changing the API to use clucene_float_t which would be double, or changing the shared library name on s390 with ABI using the float_t as float. IMO the second option is cleaner and simpler.

[Marius Hillenbrand]

Indeed, this patch is just plainly broken.

[Marius Hillenbrand]

Actually, this would work by first decoupling clucene to use a separate type clucene_float_t and then defining that as float_t or double on s390x:

Something like:

#ifdef __s390x__
#define HAVE_SPECIAL_CLUCENE_FLOAT_T
#define SPECIAL_CLUCENE_FLOAT_T double
#endif

#ifdef HAVE_SPECIAL_CLUCENE_FLOAT_T
typedef SPECIAL_CLUCENE_FLOAT_T clucene_float_t;
#else
typedef float_t clucene_float_t;
#endif

The required changes in the rest of the code are trivial (i.e., search and replace) -- updated patch attached.