Handle implicit float casting issue (#1878)

An issue occurs when we try to cast a negative float to an unsigned int which is undefined behavior in aarch64. This is why we now pass in -Wfloat-conversion in compiler options. However, some errors are non-issue but we cannot simply pass -Wno-error=float-conversion to suppress them because we want to catch real errors as code are added. Hence we suppress errors only after investigating by using _Pragma(). Task: BABEL-4437 Signed-off-by: Kristian Lejao <[email protected]>
babelfish-for-postgresql · Oct 3, 2023 · 36b5572 · 36b5572
1 parent e515c87
commit 36b5572
Show file tree

Hide file tree

Showing 10 changed files with 68 additions and 17 deletions.
diff --git a/contrib/babelfishpg_common/Makefile b/contrib/babelfishpg_common/Makefile
@@ -12,7 +12,7 @@ PREV_EXTVERSION = 1.0.0
 MODULEPATH = $$libdir/$(EXTENSION)-$(BBFPGCMN_MAJOR_VERSION)
 MODULE_big = $(EXTENSION)
 
-PG_CFLAGS += -g -Werror
+PG_CFLAGS += -g -Werror -Wfloat-conversion
 PG_CFLAGS += -fstack-protector-strong
 ifdef PREV_EXTVERSION
 DATA = sql/$(EXTENSION)--$(PREV_EXTVERSION).sql

diff --git a/contrib/babelfishpg_common/src/babelfishpg_common.h b/contrib/babelfishpg_common/src/babelfishpg_common.h
@@ -2,6 +2,31 @@
 
 #include "fmgr.h"
 
+/*
+ * Casting float < -1.0 to unsigned integer could cause issues on ARM.
+ *
+ * For instance:
+ *     auto fvalue = -176.0;
+ *     auto tvalue = static_cast<uint16_t>(fvalue);
+ *     On Intel, tvalue = 65360 which is correct.
+ *     On ARM, tvalue = 0 which is wrong.
+ *
+ * Hence the compiler flag -Wfloat-conversion has been added to BBF Makefiles
+ * to guard the codebase from this bug.
+ *
+ * However, float-conversion is not too granular enough because it also
+ * flags things like float8 to float4 conversion or conversions where the
+ * original value is always greater than or equal to zero.
+ * For code that are being flagged but are not really an issue, we can suppress
+ * the compilation error by surrounding them with _Pragma().
+ */
+#define BBF_Pragma_IgnoreFloatConversionWarning_Push \
+    _Pragma("GCC diagnostic push") \
+    _Pragma("GCC diagnostic ignored \"-Wfloat-conversion\"")
+
+#define BBF_Pragma_IgnoreFloatConversionWarning_Pop \
+    _Pragma("GCC diagnostic pop")
+
 typedef struct common_utility_plugin
 {
 	/* Function pointers set up by the plugin */

diff --git a/contrib/babelfishpg_common/src/coerce.c b/contrib/babelfishpg_common/src/coerce.c
@@ -28,7 +28,7 @@
 #include "utils/memutils.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
-
+#include "babelfishpg_common.h" // BBF_Pragma_IgnoreFloatConversionWarning_Push
 
 #include <math.h>
 
@@ -53,6 +53,7 @@ dtrunc_(float8 arg1)
 	return result;
 }
 
+BBF_Pragma_IgnoreFloatConversionWarning_Push
 inline static float4
 ftrunc_(float4 arg1)
 {
@@ -65,6 +66,7 @@ ftrunc_(float4 arg1)
 
 	return result;
 }
+BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 /* dtrunci8(X) = dtoi8(dtrunc(X)) */
 PG_FUNCTION_INFO_V1(dtrunci8);
@@ -154,7 +156,9 @@ ftrunci8(PG_FUNCTION_ARGS)
 	 * on just-out-of-range values that would round into range.  Note
 	 * assumption that rint() will pass through a NaN or Inf unchanged.
 	 */
+	BBF_Pragma_IgnoreFloatConversionWarning_Push
 	num = rint(ftrunc_(num));
+	BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 	/* Range check */
 	if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT64(num)))
@@ -166,6 +170,8 @@ ftrunci8(PG_FUNCTION_ARGS)
 }
 
 
+
+
 /* ftrunci4(X) = ftoi4(ftrunc(X)) */
 PG_FUNCTION_INFO_V1(ftrunci4);
 
@@ -179,7 +185,9 @@ ftrunci4(PG_FUNCTION_ARGS)
 	 * on just-out-of-range values that would round into range.  Note
 	 * assumption that rint() will pass through a NaN or Inf unchanged.
 	 */
+	BBF_Pragma_IgnoreFloatConversionWarning_Push
 	num = rint(ftrunc_(num));
+	BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 	/* Range check */
 	if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT32(num)))
@@ -190,7 +198,6 @@ ftrunci4(PG_FUNCTION_ARGS)
 	PG_RETURN_INT32((int32) num);
 }
 
-
 /* ftrunci2(X) = ftoi2(ftrunc(X)) */
 PG_FUNCTION_INFO_V1(ftrunci2);
 
@@ -204,7 +211,9 @@ ftrunci2(PG_FUNCTION_ARGS)
 	 * on just-out-of-range values that would round into range.  Note
 	 * assumption that rint() will pass through a NaN or Inf unchanged.
 	 */
+	BBF_Pragma_IgnoreFloatConversionWarning_Push
 	num = rint(ftrunc_(num));
+	BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 	/* Range check */
 	if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT16(num)))

diff --git a/contrib/babelfishpg_money/Makefile b/contrib/babelfishpg_money/Makefile
@@ -11,7 +11,7 @@ DATA_built = babelfishpg_money--1.1.0.sql
 #include ../Makefile.common
 
 CFLAGS = `$(PG_CONFIG) --includedir-server`
-PG_CFLAGS += -Werror
+PG_CFLAGS += -Werror -Wfloat-conversion
 PG_CFLAGS += -fstack-protector-strong
 TESTS = $(wildcard test/sql/*.sql)
 

diff --git a/contrib/babelfishpg_tds/Makefile b/contrib/babelfishpg_tds/Makefile
@@ -9,7 +9,7 @@ tds_backend = $(tds_top_dir)/src/backend
 tds_include = $(tds_top_dir)/src/include
 TSQL_SRC = ../babelfishpg_tsql
 
-PG_CFLAGS += -Werror
+PG_CFLAGS += -Werror -Wfloat-conversion
 PG_CFLAGS += -fstack-protector-strong
 PG_CPPFLAGS += -I$(TSQL_SRC) -I$(PG_SRC) -I$(tds_top_dir) -DFAULT_INJECTOR
 

diff --git a/contrib/babelfishpg_tds/src/backend/tds/tdstimestamp.c b/contrib/babelfishpg_tds/src/backend/tds/tdstimestamp.c
@@ -377,7 +377,9 @@ GetDatetimeFromDaysTicks(uint32 numDays, uint32 numTicks,
 
 	CalculateTargetDate(y1, &d2, &m2, &y2, numDays);
 
-	numMilli = 3.33333333 * numTicks;
+	Assert((int) numTicks >= 0);
+	numMilli = (int) (3.33333333 * numTicks);
+
 	*fsec = (numMilli % 1000) * 1000;
 	numMilli /= 1000;
 

diff --git a/contrib/babelfishpg_tsql/Makefile b/contrib/babelfishpg_tsql/Makefile
@@ -82,14 +82,14 @@ export ANTLR4_RUNTIME_LIB_DIR=/usr/local/lib
 
 OBJS += src/pltsql_bulkcopy.o
 
-PG_CXXFLAGS += -g -Werror
+PG_CXXFLAGS += -g -Werror -Wfloat-conversion
 PG_CXXFLAGS += -Wno-deprecated -Wno-error=attributes -Wno-suggest-attribute=format # disable some warnings from ANTLR runtime header
 PG_CXXFLAGS += -Wno-undef -Wall -Wcpp
 PG_CXXFLAGS += -Wno-register # otherwise C++17 gags on PostgreSQL headers
 PG_CXXFLAGS += -I$(ANTLR4_RUNTIME_INCLUDE_DIR)
-PG_CFLAGS += -g -Werror
+PG_CFLAGS += -g -Werror -Wfloat-conversion
 PG_CFLAGS += -fstack-protector-strong
-PG_CPPFLAGS += -I$(TSQLSRC) -I$(PG_SRC) -DFAULT_INJECTOR
+PG_CPPFLAGS += -I$(TSQLSRC) -I$(PG_SRC) -DFAULT_INJECTOR -Wfloat-conversion
 
 SHLIB_LINK += -L$(ANTLR4_RUNTIME_LIB_DIR) $(ANTLR4_RUNTIME_LIB) -lcrypto
 

diff --git a/contrib/babelfishpg_tsql/src/hooks.c b/contrib/babelfishpg_tsql/src/hooks.c
@@ -2807,8 +2807,15 @@ pltsql_detect_numeric_overflow(int weight, int dscale, int first_block, int nume
 	 * added to total_digit_count
 	 */
 	if (partially_filled_numeric_block < pow(10, numeric_base - 1))
-		total_digit_count += (partially_filled_numeric_block > 0) ?
-			log10(partially_filled_numeric_block) + 1 : 1;
+	{
+		if (partially_filled_numeric_block > 0)
+		{
+			int log_10 = (int) log10(partially_filled_numeric_block); // keep compiler happy
+			total_digit_count += log_10 + 1;
+		}
+		else
+			total_digit_count += 1;
+	}
 
 	/*
 	 * calculating exact #digits in last block if decimal point exists If

diff --git a/contrib/babelfishpg_tsql/src/pltsql.h b/contrib/babelfishpg_tsql/src/pltsql.h
@@ -48,13 +48,13 @@
 #undef _
 #define _(x) dgettext(TEXTDOMAIN, x)
 
-#define PLTSQL_INSTR_ENABLED()	\
-	(pltsql_instr_plugin_ptr && (*pltsql_instr_plugin_ptr) && \
-	 (*pltsql_instr_plugin_ptr)->pltsql_instr_increment_metric)
+#define PLTSQL_INSTR_ENABLED() \
+       (pltsql_instr_plugin_ptr && (*pltsql_instr_plugin_ptr) && \
+        (*pltsql_instr_plugin_ptr)->pltsql_instr_increment_metric)
 
-#define TSQLInstrumentation(metric)												\
-({	if ((pltsql_instr_plugin_ptr && (*pltsql_instr_plugin_ptr) && (*pltsql_instr_plugin_ptr)->pltsql_instr_increment_metric))		\
-		(*pltsql_instr_plugin_ptr)->pltsql_instr_increment_metric(metric);		\
+#define TSQLInstrumentation(metric)                                                                                            \
+({     if ((pltsql_instr_plugin_ptr && (*pltsql_instr_plugin_ptr) && (*pltsql_instr_plugin_ptr)->pltsql_instr_increment_metric))               \
+               (*pltsql_instr_plugin_ptr)->pltsql_instr_increment_metric(metric);              \
 })
 
 #define TSQL_TXN_NAME_LIMIT 64	/* Transaction name limit */

diff --git a/contrib/babelfishpg_tsql/src/pltsql_coerce.c b/contrib/babelfishpg_tsql/src/pltsql_coerce.c
@@ -1387,6 +1387,7 @@ dtrunc_(float8 arg1)
 	return result;
 }
 
+BBF_Pragma_IgnoreFloatConversionWarning_Push
 inline static float4
 ftrunc_(float4 arg1)
 {
@@ -1400,6 +1401,7 @@ ftrunc_(float4 arg1)
 
 	return result;
 }
+BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 /* dtrunci8(X) = dtoi8(dtrunc(X)) */
 PG_FUNCTION_INFO_V1(dtrunci8);
@@ -1489,7 +1491,9 @@ ftrunci8(PG_FUNCTION_ARGS)
 	 * on just-out-of-range values that would round into range.  Note
 	 * assumption that rint() will pass through a NaN or Inf unchanged.
 	 */
+	BBF_Pragma_IgnoreFloatConversionWarning_Push
 	num = rint(ftrunc_(num));
+	BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 	/* Range check */
 	if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT64(num)))
@@ -1514,7 +1518,9 @@ ftrunci4(PG_FUNCTION_ARGS)
 	 * on just-out-of-range values that would round into range.  Note
 	 * assumption that rint() will pass through a NaN or Inf unchanged.
 	 */
+	BBF_Pragma_IgnoreFloatConversionWarning_Push
 	num = rint(ftrunc_(num));
+	BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 	/* Range check */
 	if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT32(num)))
@@ -1539,7 +1545,9 @@ ftrunci2(PG_FUNCTION_ARGS)
 	 * on just-out-of-range values that would round into range.  Note
 	 * assumption that rint() will pass through a NaN or Inf unchanged.
 	 */
+	BBF_Pragma_IgnoreFloatConversionWarning_Push
 	num = rint(ftrunc_(num));
+	BBF_Pragma_IgnoreFloatConversionWarning_Pop
 
 	/* Range check */
 	if (unlikely(isnan(num) || !FLOAT4_FITS_IN_INT16(num)))