Bug 232623 - [cmake] Check "cortex" string in the CMAKE_SYSTEM_PROCESSOR for defining WTF_CPU_* vars
Summary: [cmake] Check "cortex" string in the CMAKE_SYSTEM_PROCESSOR for defining WTF_...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Other Linux
: P2 Normal
Assignee: Pablo Saavedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-02 07:12 PDT by Pablo Saavedra
Modified: 2021-11-03 03:37 PDT (History)
10 users (show)

See Also:


Attachments
patch (1.45 KB, patch)
2021-11-02 07:16 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (1.45 KB, patch)
2021-11-02 08:28 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff
patch (1.45 KB, patch)
2021-11-02 13:24 PDT, Pablo Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pablo Saavedra 2021-11-02 07:12:45 PDT
In a native build, in systems with uname support, the CMAKE_SYSTEM_PROCESSOR was defined as the output of `uname -p` by default. That was true in cmake 3.0 [1]. In newer versions of CMake the documentation of this variable changed for highlighting that this variable has not to correspond with the target architecture [2]. For example, lately, in toolchains generated by Yocto it is very common to see the
CMAKE_SYSTEM_PROCESSOR defined using the GCC [3] tune name for compiling the target. Ex: cortexa7t2hf-neon-vfpv4 (rpi3) cortexa9t2hf-neon-vfpv4 (Freescale iMX6q) ...

This change add an additional check to evaluate if the system processor is ARM or ARM64 based on the existence of "cortex" in the string.

[1] https://cmake.org/cmake/help/v3.0/variable/CMAKE_SYSTEM_PROCESSOR.html

    The name of the CPU CMake is building for.

    On systems that support uname, this variable is set to the output of uname -p, on windows it is set to the value of the environment variable PROCESSOR_ARCHITECTURE

[2] https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PROCESSOR.html

    When not cross-compiling, this variable has the same value as the CMAKE_HOST_SYSTEM_PROCESSOR variable. In many cases, this will correspond to the target architecture for the build, but this is not guaranteed. (E.g. on Windows, the host may be AMD64 even when using a MSVC cl compiler with a 32-bit target.)

    When cross-compiling, a CMAKE_TOOLCHAIN_FILE should set the CMAKE_SYSTEM_PROCESSOR variable to match target architecture that it specifies (via CMAKE_<LANG>_COMPILER and perhaps CMAKE_<LANG>_COMPILER_TARGET).

[3] https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html (See: -mtune=name)
Comment 1 Pablo Saavedra 2021-11-02 07:16:56 PDT
Created attachment 443085 [details]
patch
Comment 2 Pablo Saavedra 2021-11-02 08:28:40 PDT
Created attachment 443092 [details]
patch
Comment 3 Carlos Alberto Lopez Perez 2021-11-02 11:21:28 PDT
Comment on attachment 443092 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443092&action=review

> Source/cmake/WebKitCommon.cmake:89
> +    if (LOWERCASE_CMAKE_SYSTEM_PROCESSOR MATCHES "(^aarch64|^arm64|^cortex-?[am][2-7][2-7])")

Maybe is better to use "cortex-?[am][2-7][2-8]" so it matches also Cortex-A78: https://en.wikipedia.org/wiki/ARM_Cortex-A#External_links
Comment 4 Pablo Saavedra 2021-11-02 13:23:13 PDT
(In reply to Carlos Alberto Lopez Perez from comment #3)
> Comment on attachment 443092 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443092&action=review
> 
> > Source/cmake/WebKitCommon.cmake:89
> > +    if (LOWERCASE_CMAKE_SYSTEM_PROCESSOR MATCHES "(^aarch64|^arm64|^cortex-?[am][2-7][2-7])")
> 
> Maybe is better to use "cortex-?[am][2-7][2-8]" so it matches also
> Cortex-A78: https://en.wikipedia.org/wiki/ARM_Cortex-A#External_links

yes. Good point.
Comment 5 Pablo Saavedra 2021-11-02 13:24:09 PDT
Created attachment 443127 [details]
patch
Comment 6 EWS 2021-11-03 03:37:06 PDT
Committed r285201 (243827@main): <https://commits.webkit.org/243827@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443127 [details].