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

[delcypher]

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.

[delcypher]

Comment update.

[delcypher]

Ping.

[delcypher]

@phosek @amyk : ping

[nemanjai]

One of us here at IBM would gladly try this out on ppc64/ppc64le if that is desired. We are certainly interested in helping to ensure our target does not put undue burden on the community, but I don't think anyone here has much CMake expertise.
So if you let us know if you'd like us to test something out for PPC, we'll be happy to do it.

Just as an example, with my very limited knowledge of CMake, I don't actually understand how this ensures we actually build the ppc/<builtin_name>.c on powerpc/powerpc64/powerpc64le rather than some other one (for example arm/<builtin_name>.c).

[delcypher]

In D69189#1724545, @nemanjai wrote:

One of us here at IBM would gladly try this out on ppc64/ppc64le if that is desired. We are certainly interested in helping to ensure our target does not put undue burden on the community, but I don't think anyone here has much CMake expertise.
So if you let us know if you'd like us to test something out for PPC, we'll be happy to do it.

If you could test it that would be great. My current understanding of the bug I found is that on the master branch when the powerpc builtin library is built it will accidently contain two implementations of the divtc3 builtin. This just so happens to workout because we're producing a static archive and the ar tool doesn't care that there are duplicate symbols. When the linker uses the archive it'll just pick the first symbol it finds. I think the first symbol will be the powerpc version but I'm not sure because I don't have any powerpc hardware to test this.

If you apply this patch you should hopefully find that the duplicate divtc3 builtin symbol goes away and the only one left is the ppc/divtc3.c implementation.

Just as an example, with my very limited knowledge of CMake, I don't actually understand how this ensures we actually build the ppc/<builtin_name>.c on powerpc/powerpc64/powerpc64le rather than some other one (for example arm/<builtin_name>.c).

This patch doesn't actually ensure this explicitly. The current version in master does but its very error prone because it does the wrong thing for powerpc and nobody noticed. This patch relies on everyone following the convention that the provided source files for a target are either the generic implementations or the implementations intended for that target. In my opinion this is reasonable and AFAICT this convention is already followed by the source code.
If this convention is broken I consider this to be a bug because it doesn't make sense to build a builtin library that contains both the arm and powerpc specific implementations for example.

Once the powerpc build is fixed I plan to revert dc748816e2aec8941d63f8ad07fb82aff6be8af7 which will mean if someone breaks the convention it will actually cause testing to fail.

[nemanjai]

I have confirmed that we no longer get duplicate files for dictc3 in the archive with this patch. Furthermore, the symbol we do get is from .../ppc/ according to build.ninja.
Before the patch, I observed the behaviour Dan mentioned.
So this patch LGTM.

[kongyi]

Thanks for the patch! I'm developing something for X86 that happens to also need this feature.

[delcypher]

Thanks for the review committed as 8ea148dc0cbff33ac3c80cf4273991465479a01e

lib/builtins/CMakeLists.txt

else ()
   set(BUILTIN_DEFS "")
 
   if(NOT ANDROID)
     append_list_if(COMPILER_RT_HAS_VISIBILITY_HIDDEN_FLAG VISIBILITY_HIDDEN BUILTIN_DEFS)
   endif()
 
   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}")
         check_compile_definition(__VFP_FP__ "${CMAKE_C_FLAGS} ${_TARGET_${arch}_CFLAGS}" COMPILER_RT_HAS_${arch}_VFP)
         if(NOT COMPILER_RT_HAS_${arch}_VFP)
           list(REMOVE_ITEM ${arch}_SOURCES ${arm_Thumb1_VFPv2_SOURCES} ${arm_Thumb1_SjLj_EH_SOURCES})
         endif()
       endif()
 
       # Filter out generic versions of routines that are re-implemented in
       # 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 (e.g. `divtc3.c`).
           get_filename_component(_name ${_file} NAME)
           string(REPLACE ".S" ".c" _cname "${_name}")
+          if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${_cname}")
+            message(STATUS "For ${arch} builtins preferring ${_file} to ${_cname}")
             list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
-        endif ()
+          endif()
+        endif ()
       endforeach ()
 
       # Needed for clear_cache on debug mode, due to r7's usage in inline asm.
       # Release mode already sets it via -O2/3, Debug mode doesn't.
       if (${arch} STREQUAL "armhf")
         list(APPEND BUILTIN_CFLAGS -fomit-frame-pointer -DCOMPILER_RT_ARMHF_TARGET)
       endif()