Page MenuHome

Clang/GCC: use relative path in __FILE__ macro
ClosedPublic

Authored by Ankit Meel (ankitm) on Oct 29 2020, 8:23 PM.

Details

Summary

This change removes the user-specific information from
macros like __FILE__ and keeps it relative to top level
source or build (for generated files) directory.
It makes traces concise.


MVSC is not handled here. I didn't do any macro modifications to avoid editing source files.

This needs testing by linux users.. I don't have either of linux or gcc.

Diff Detail

Repository
rB Blender

Event Timeline

Ankit Meel (ankitm) requested review of this revision.Oct 29 2020, 8:23 PM
Ankit Meel (ankitm) retitled this revision from macOS/Linux: use relative path in debug info and file macro to Clang/GCC: use relative path in debug info and file macro.Oct 29 2020, 8:25 PM
Campbell Barton (campbellbarton) requested changes to this revision.EditedOct 30 2020, 1:36 AM
  • Since this is specific to the compiler, not the platform, I think it'd be better to have this in the Extra compile flags section of ./CMakeLists.txt (or below that if this becomes optional).
  • Adding CMAKE_BINARY_DIR may be OK for release builds, but for development it means the path is ambiguous which will be confusing in some situations.
  • We could make this optional (eg advanced option WITH_COMPILER_SHORT_PATHS) so developers can disable if they like to be able to select and open paths from anywhere on their system.
build_files/cmake/platform/platform_apple.cmake
431 ↗(On Diff #30512)

Should add if(CMAKE_C_COMPILER_ID MATCHES "Clang" OR CMAKE_COMPILER_IS_GNUCC) here too.

434–437 ↗(On Diff #30512)

While I don't use macOS, this seems like the kind of thing that could trip developers up, wasting time only to find they missed this message.

Maybe this shouldn't be used at all for older versions since it doesn't work properly.

build_files/cmake/platform/platform_unix.cmake
684 ↗(On Diff #30512)

Missing version checks, even though GCC-9.3 is documented as the minimum version. This is only for pre-compiled libraries, checks for older versions remain elsewhere. Unless we move to fail completely with older versions, a version check should be added here.

685 ↗(On Diff #30512)

*picky*, set(PLATFORM_LINKFLAGS "${PLATFORM_LINKFLAGS} ...") is currently being done everywhere, adding it in one place makes the reader wonder why it's only used here, checking on this it was added in CMake 3.4x, which is fine since the minimum is 3.10.

Prefer to move to string(APPEND, ...) in a separate refactor.

685 ↗(On Diff #30512)

No need for 2x calls.

This revision now requires changes to proceed.Oct 30 2020, 1:36 AM
  • disable flags on xcode < 11, gcc < 8.4; add option.
Ankit Meel (ankitm) edited the summary of this revision. (Show Details)EditedOct 30 2020, 11:42 AM
Ankit Meel (ankitm) marked 2 inline comments as done.

Since this is specific to the compiler, not the platform, I think it'd be better to have this in the Extra compile flags section of ./CMakeLists.txt (or below that if this becomes optional).

Platform specific if-checks pull it towards platform files.

Adding CMAKE_BINARY_DIR may be OK for release builds, but for development it means the path is ambiguous which will be confusing in some situations.

I don't understand this bit. Works for me (TM). Why is the path ambiguous ?

We could make this optional (eg advanced option WITH_COMPILER_SHORT_PATHS) so developers can disable if they like to be able to select and open paths from anywhere on their system.

Good point. Some terminal emulators allow clicking on file links. Made it an option.

build_files/cmake/platform/platform_apple.cmake
431 ↗(On Diff #30512)

We don't really expect any other compiler on macOS. The GCC shipped with Xcode is just an alias to. clang.

434–437 ↗(On Diff #30512)

I am being a bit selfish here. I have Xcode 10.2 which is shipped with AppleClang 10.1 which doesn't support this flag. But I compiled LLVM toolchain with clang 12 and made some changes to have Xcode use clang 12.
It works fine for compiling, linking etc. But those menu items are somehow hardcoded to use AppleClang 10.

I'll make changes to not add those flags on (< Xcode 11) and make some changes locally to my initial cache file to add those complier flags.

build_files/cmake/platform/platform_unix.cmake
685 ↗(On Diff #30512)

The single line had become too long. Hope this is correct.

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Oct 30 2020, 11:44 AM

Since this is specific to the compiler, not the platform, I think it'd be better to have this in the Extra compile flags section of ./CMakeLists.txt (or below that if this becomes optional).

Platform specific if-checks pull it towards platform files.

Isn't this only about compiler versions? As long as someone is building with GCC/Clang, the platform shouldn't matter.

If we add new platform files this will add needless copy-pasting.

Adding CMAKE_BINARY_DIR may be OK for release builds, but for development it means the path is ambiguous which will be confusing in some situations.

I don't understand this bit. Works for me (TM). Why is the path ambiguous ?

One file is in the build dir another in the source dir, while I didn't test this, I assume both will have their prefix trimmed.

  • source/blender/makesdna/intern/dna_verify.c
  • source/blender/makesdna/intern/dna_utils.c

On the other hand there aren't many cases like this, so I'm not so worried. Probably people who find this an issue can turn the option off.

CMakeLists.txt
574

This can be marked as advanced.

build_files/cmake/platform/platform_unix.cmake
685 ↗(On Diff #30535)

Version checks should be paired with the compiler they're checking the version of.

693 ↗(On Diff #30535)

The white-space is being added in the command line, this can be done instead.

    set(PLATFORM_CFLAGS
      "${PLATFORM_CFLAGS} \
-ffile-prefix-map=\"${CMAKE_SOURCE_DIR}/\"=\"\" \
-ffile-prefix-map=\"${CMAKE_BINARY_DIR}/\"=\"\"")
Campbell Barton (campbellbarton) requested changes to this revision.Oct 30 2020, 12:07 PM
This revision now requires changes to proceed.Oct 30 2020, 12:07 PM
Ankit Meel (ankitm) marked 6 inline comments as done.
  • mark as advanced; move code to main cmakelists file.

Use -fmacro, not -ffile because it enables -fdebug for which gcc docs say:

This can give more reproducible builds, which are location independent,
but may require an extra
command to tell GDB where to find the source files.

Ankit Meel (ankitm) retitled this revision from Clang/GCC: use relative path in debug info and file macro to Clang/GCC: use relative path in __FILE__ macro.Oct 30 2020, 12:45 PM
Ankit Meel (ankitm) edited the summary of this revision. (Show Details)
  • rename WITH_DEBUG_REL_PATHS to WITH_MACRO_REL_PATHS
Campbell Barton (campbellbarton) requested changes to this revision.Oct 30 2020, 3:02 PM
Campbell Barton (campbellbarton) added inline comments.
CMakeLists.txt
574

WITH_* are often used for Blender features (FFMPEG, FLUID... etc). For tweaking compiler flags I'd prefer to group this with other compiler flags. It could be called WITH_COMPILER_MACRO_REL_PATHS ?

1663

Logic in this block seems a bit odd.

I find this easier to follow, where turning off the feature is an exception instead of the body of the first check.

if(WITH_MACRO_REL_PATHS)
  # Use '-fmacro-prefix-map' for Clang and GCC (MSVC doesn't support this).
  if((CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 8.4) OR
     (CMAKE_C_COMPILER_ID MATCHES "Clang" AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 10.0))

    if(APPLE AND XCODE AND ${XCODE_VERSION} VERSION_LESS 11.0)
      message(WARNING
        "-fmacro-prefix-map flag is not supported by Clang shipped with Xcode-${XCODE_VERSION}."
      )
      set(WITH_MACRO_REL_PATHS OFF)
    endif()

    if(WITH_MACRO_REL_PATHS)
      set(PLATFORM_CFLAGS "${PLATFORM_CFLAGS} \
-fmacro-prefix-map=\"${CMAKE_SOURCE_DIR}/\"=\"\" \
-fmacro-prefix-map=\"${CMAKE_BINARY_DIR}/\"=\"\"")
    endif()
  endif()
endif()
This revision now requires changes to proceed.Oct 30 2020, 3:02 PM
CMakeLists.txt
1664

I would make this logic purely depend on the compiler version, not Xcode version.

(CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 8.4) OR
(CMAKE_C_COMPILER_ID EQUAL "Clang" AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 10.0) OR
(CMAKE_C_COMPILER_ID EQUAL "AppleClang" AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 11.0)

Also, it's inconsistent that for Xcode it shows a warning message when unsupported, while for Linux it silently ignores.

You might as well check the compiler versions earlier and if not supported, simply don't expose the option.

This seems close to being ready, a few things:

The name should help developers quickly understand what the feature does, that __FILE__ is a macro isn't so important in naming IMHO.

Suggest either WITH_COMPILER_SHORT_FILE_MACRO or WITH_COMPILER_FILE_MACRO_RELATIVE since it has the term FILE in it, use RELATIVE since REL is an abbreviation for Release in the context of CMake.

Assume it's fine for this to be "on" by default.

CMakeLists.txt
1664

From an earlier version of this patch the issue seems to be XCode not properly handling the paths. (as in - it breaks some XCode features, while the compiler works).

if(XCODE AND ${XCODE_VERSION} VERSION_LESS 11.0)
  message_first_run(WARNING "Some actions under Product menu like assembling or preprocessing"
                            " individual files may not work due to -ffile-prefix-map.")
endif()

I don't have strong opinions on this, It just seemed like the kind of thing developers could spend a long time looking into only to realize they they missed this this message, so disabling makes more sense so as not to trip developers up.

CMakeLists.txt
1664

To clarify, that would happen in a case where Xcode say 10.2 is being used with a new toolchain containing llvm clang 10 or 11 or 12 etc.
The condition (CMAKE_C_COMPILER_ID EQUAL "Clang" AND CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL 10.0) will be met, flag will be passed and then some menu items will break.
Though I expect the person doing that much to be able to navigate around Xcode and see what the issue is in build logs (:

  • rename to WITH_COMPILER_SHORT_FILE_MACRO; use compiler version to add flag. Show a warning on Xcode < 11.
  • use EQUAL for Clang too: otherwise AppleClang 10.0 also passes the check.

Just found out that clang 11.0.3 doesn't support the flag. Bumping minimum AppleClang to 12.0.
Is there a more reliable way of finding AppleClang release notes, than collecting command
outputs from friends?

If the version checks are too complicated ADD_CHECK_C_COMPILER_FLAG, ADD_CHECK_C_COMPILER_FLAG could be used.

Although it seems like it should be possible to check the compiler version to know if this feature is available.

CMakeLists.txt
1670–1676

On Linux this is giving me an error.

CMake Error at CMakeLists.txt:1671 (if):
  if given arguments:

    "XCODE" "AND" "VERSION_LESS" "12.0"

  Unknown arguments specified

Check APPLE before the version of xcode.

if(APPLE)
  if((DEFINED XCODE) AND ...)
  endif()
endif()

Then there is the issue of this being disabled or not, suggest to disable so developers don't loose time investigating broken features, since we can't rely on them to see this message, but I'm not an XCode user.

  • move XCODE_VERSION inside XCODE block.
  • disable for llvm-clang and AppleClang version mismatch.
  • Use STREQUAL instead of EQUAL.
This revision is now accepted and ready to land.Nov 2 2020, 9:53 AM
CMakeLists.txt
1670

I'd prefer if(APPLE) wrap the entire block, it means non-apple devs can ignore the contents entirely and avoids changes to the if-check impacting other platforms.

  • use APPLE check