[MDEV-21362] do something with -fno-builtin-memcmp for rem0cmp.cc Created: 2019-12-19  Updated: 2020-01-29  Resolved: 2019-12-24

Status: Closed
Project: MariaDB Server
Component/s: Compiling, Storage Engine - InnoDB
Fix Version/s: 10.2.31, 10.3.22, 10.4.12, 10.5.1

Type: Task Priority: Major
Reporter: Eugene Kosov (Inactive) Assignee: Eugene Kosov (Inactive)
Resolution: Fixed Votes: 0
Labels: performance


 Description    

  IF (CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64" OR
 
      CMAKE_SYSTEM_PROCESSOR MATCHES "i386")
 
    INCLUDE(CheckCXXCompilerFlag)
 
    CHECK_CXX_COMPILER_FLAG("-fno-builtin-memcmp" HAVE_NO_BUILTIN_MEMCMP)
 
    IF (HAVE_NO_BUILTIN_MEMCMP)
 
      # Work around http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43052
 
      SET_SOURCE_FILES_PROPERTIES(${CMAKE_CURRENT_SOURCE_DIR}/rem/rem0cmp.cc
 
	PROPERTIES COMPILE_FLAGS -fno-builtin-memcmp)
 
    ENDIF()
 
  ENDIF()
 
ENDIF()

This was added to mitigate poor compiler optimization. As I can see, it was fixed in gcc-4.6 https://godbolt.org/z/7G64qo For 10.5 we support at least gcc-4.8 so this CMake code can be removed. For older versions it should be replaced with some preprocessor condition to check compiler name and version.

 Comments   
Comment by Marko Mäkelä [ 2019-12-23 ]

Thank you! It looks good to me.

Comment by Marko Mäkelä [ 2020-01-29 ]

The 10.5 version of the change introduced a WITH_UBSAN failure, by removing the zero check on len before invoking memcmp(). The function parameters can be null pointers in this case, and memcmp(nullptr,nullptr,0) is undefined behaviour, and it will allow GCC 8 (or later) to infer that the pointer arguments are nonnull, and to optimize away further nullness checks for them. That actually happened in MDEV-15587.

diff --git a/storage/innobase/rem/rem0cmp.cc b/storage/innobase/rem/rem0cmp.cc
 
index c0e82bd3ad7..b7e835767df 100644
 
--- a/storage/innobase/rem/rem0cmp.cc
 
+++ b/storage/innobase/rem/rem0cmp.cc
 
@@ -451,8 +451,7 @@ cmp_data(
 
 	}
 
 
 	ulint len = std::min(len1, len2);
 
-
 
-	int cmp = memcmp(data1, data2, len);
 
+	int cmp = len ? memcmp(data1, data2, len) : 0;
 
 
 	if (cmp) {
 
 		return (cmp);

With this change, WITH_UBSAN should be happy. The following tests cover this condition:

vcol.vcol_keys_innodb gcol.gcol_keys_innodb main.func_group_innodb innodb.innodb_bug53592

Comment by Marko Mäkelä [ 2020-01-29 ]

The minor regression (with no observed effects outside WITH_UBSAN) is now fixed in the 10.5 branch. It was not part of any release yet.

Generated at Thu Feb 08 09:06:33 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.