[compiler-rt] 8ea148d - [Builtins] Fix bug where powerpc builtins specializations didn't remove generic implementations.

[Dan Liew via llvm-commits]

Author: Dan Liew
Date: 2019-10-30T16:20:09-07:00
New Revision: 8ea148dc0cbff33ac3c80cf4273991465479a01e

URL: https://github.com/llvm/llvm-project/commit/8ea148dc0cbff33ac3c80cf4273991465479a01e
DIFF: https://github.com/llvm/llvm-project/commit/8ea148dc0cbff33ac3c80cf4273991465479a01e.diff

LOG: [Builtins] Fix bug where powerpc builtins specializations didn't remove generic implementations.

Summary:
Previously the CMake code looked for filepaths of the form
`<arch>/<filename>` as an indication that `<arch>/<filename>` provided a
specialization of a top-level file `<filename>`. For powerpc there was a
bug because the powerpc specialized implementations lived in `ppc/` but
the architectures were `powerpc64` and `powerpc64le` which meant that
CMake was looking for files at `powerpc64/<filename>` and
`powerpc64le/<filename>`.

The result of this is that for powerpc the builtins library contained a
duplicate symbol for `divtc3` because it had the generic implementation
and the specialized version in the built static library.

Although we could just add similar code to what there is for arm (i.e.
compute `${_arch}`) to fix this, this is extremely error prone (until
r375150 no error was raised). Instead this patch takes a different
approach that removes looking for the architecture name entirely.
Instead this patch uses the convention that a source file in a
sub-directory might be a specialization of a generic implementation and
if a source file of the same name (ignoring extension) exists at the
top-level then it is the corresponding generic implementation. This
approach is much simpler because it doesn't require keeping track of
different architecture names.

This convention already existed in repository but previously it was
implicit.  This change makes it explicit.

This patch is motivated by wanting to revert r375162 which worked around
the powerpc bug found when r375150 landed.

Once it lands we should revert r375162.

Reviewers: phosek, beanz, compnerd, shiva0217, amyk, rupprecht, kongyi, mstorsjo, t.p.northover, weimingz, jroelofs, joerg, sidneym

Subscribers: nemanjai, mgorny, kristof.beyls, jsji, shchenz, steven.zhang, #sanitizers, llvm-commits

Tags: #llvm, #sanitizers

Differential Revision: https://reviews.llvm.org/D69189

Added: 
    

Modified: 
    compiler-rt/lib/builtins/CMakeLists.txt

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/builtins/CMakeLists.txt b/compiler-rt/lib/builtins/CMakeLists.txt
index f42e13714c94..1d3d65a9940a 100644
--- a/compiler-rt/lib/builtins/CMakeLists.txt
+++ b/compiler-rt/lib/builtins/CMakeLists.txt
@@ -586,15 +586,6 @@ else ()
 
   foreach (arch ${BUILTIN_SUPPORTED_ARCH})
     if (CAN_TARGET_${arch})
-      # NOTE: some architectures (e.g. i386) have multiple names.  Ensure that
-      # we catch them all.
-      set(_arch ${arch})
-      if("${arch}" STREQUAL "armv6m")
-        set(_arch "arm|armv6m")
-      elseif("${arch}" MATCHES "^(armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
-        set(_arch "arm")
-      endif()
-
       # For ARM archs, exclude any VFP builtins if VFP is not supported
       if (${arch} MATCHES "^(arm|armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
         string(REPLACE ";" " " _TARGET_${arch}_CFLAGS "${TARGET_${arch}_CFLAGS}")
@@ -608,10 +599,19 @@ else ()
       # architecture specific manner.  This prevents multiple definitions of the
       # same symbols, making the symbol selection non-deterministic.
       foreach (_file ${${arch}_SOURCES})
-        if (${_file} MATCHES ${_arch}/*)
+        get_filename_component(_file_dir "${_file}" DIRECTORY)
+        if (NOT "${_file_dir}" STREQUAL "")
+          # Architecture specific file. We follow the convention that a source
+          # file that exists in a sub-directory (e.g. `ppc/divtc3.c`) is
+          # architecture specific and that if a generic implementation exists
+          # it will be a top-level source file with the same name modulo the
+          # file extension (e.g. `divtc3.c`).
           get_filename_component(_name ${_file} NAME)
           string(REPLACE ".S" ".c" _cname "${_name}")
-          list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
+          if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${_cname}")
+            message(STATUS "For ${arch} builtins preferring ${_file} to ${_cname}")
+            list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
+          endif()
         endif ()
       endforeach ()