This change is trying to address the issues raised in D83154. There are still some open questions:

• Is -fsource-dir the best name for this flag?
• I'm not sure if make_relative should be applied to all source paths, or only paths that start with SourceDir which would exclude system paths outside of the source directory (e.g. it's probably undesirable to relativize paths to /usr/include)?
• If we decide to exclude source paths outside of the source directory, should we support -fsource-dir to be specified more then once to handle multiple source directories?

In D83154#2277712, @phosek wrote:

In D83154#2277539, @rnk wrote:

Yeah, my goal is for the build system to be able to avoid having to embed PWD into the compiler flags. For example, I believe it is a goal of the LLVM gn build system for the build tree to be relocatable. If the compiler command lines need to contain absolute paths to make the paths in the coverage relative, we won't be able to achieve that goal.

Based on all the comments that have gone before, it sounds like we want a -fprofile-compilation-dir= flag. Once we have that, would this logic work?

• if profile compilation dir set, absolutize with that as the root
• if no profile compilation dir, absolutize with CWD
• apply profile prefix map

Should -no-canonical-prefixes also be involved here? Unclear.

I originally considered -fprofile-compilation-dir= and perhaps -ffile-compilation-dir= to also handle cases like __FILE__ macro (-ffile-prefix-map and -fmacro-prefix-map suffer from the same issue as -fprofile-prefix-map), but I'm not sure if specifying compilation dir is the right approach. It makes sense for debug info because -fdebug-compilation-dir is used directly as DW_AT_comp_dir and also because it's a convention for source file paths in debug info to be relative to build directory, but it's not the case for paths in coverage mapping or __FILE__.

So what I'm considering now and started prototyping is a -fsource-dirflag (or maybe -fsource-root-dir, I'm open to suggestions for the name) where the semantics would be that if this flag is set, all source paths will be made relative to this directory. There's an open question whether this should also apply to sources outside of that directory, for example system header paths.

The example use case, assuming source is in /path/to/source and build is in /path/to/build, would be: clang -fdebug-compilation-dir . -fsource-dir ../source ../source/dir/file.c. In this case __FILE__ would expand to dir/file.c and that path would be also stored in coverage mapping.

In D87732#2275943, @jhenderson wrote:

I'm not convinced this is really correct. After all, a path with mixed separators (e.g. '/my/path\to\foo') could be a Windows path rooted at the current drive. This mixed style can sometimes happen when things get concatenated together, so I don't think it's compeletely unreasonable.

I'm also concerned that if we guess wrong, the behaviour will end up breaking. For example, if the path were a Windows path "/my/path\../to/foo", I'd expect a remove_dots call (without worrying about slash normalisation) to result in "/my/to/foo", but if the code is using Posix style, will it actually result in the path being left unchanged?

I can think of two competing alternatives:

1. add an option to remove_dots to not do the separator canonicalization. I'm not sure whether this can be done unambiguously however - which separator should be removed when a directory is removed due to dots? The one before the removed parts? After them?
2. change all '\' to '/' unconditionally. However, this might break Linux paths with '\' in.

In D83154#2277539, @rnk wrote:

Yeah, my goal is for the build system to be able to avoid having to embed PWD into the compiler flags. For example, I believe it is a goal of the LLVM gn build system for the build tree to be relocatable. If the compiler command lines need to contain absolute paths to make the paths in the coverage relative, we won't be able to achieve that goal.

Based on all the comments that have gone before, it sounds like we want a -fprofile-compilation-dir= flag. Once we have that, would this logic work?

• if profile compilation dir set, absolutize with that as the root
• if no profile compilation dir, absolutize with CWD
• apply profile prefix map

Should -no-canonical-prefixes also be involved here? Unclear.

In D87732#2275529, @dblaikie wrote:

Hmm - probably simpler to justify this patch by including refactoring at least one (possibly more) existing uses you mentioned, to use this API - then I probably don't really need to understand what it's doing, just that it's generalizing some existing functionality.

I had to revert D87657 because it broke several tests: the normalization resulted in slashes being converted to native format, but that's incorrect. I'd like to use this function to instead determine the style from the path inside the debug info.

So a trivial example is:

#include <string>
int main() {}

When compiled as:

clang++ test.cc -g3 -S -o test.S

I see paths like these in the output:

"/usr/local/google/home/phosek/fuchsia" "prebuilt/third_party/clang/linux-x64/bin/../include/c++/v1/cstddef"

We could go over driver code and make sure we always invoke sys::path::remove_dots inside methods like AddIncludePaths or addPathIfExists (see for example https://github.com/llvm/llvm-project/blob/b3afad046301d8bb1f4471aceaad704b87de3a69/clang/lib/Driver/ToolChains/Gnu.cpp#L2902 which where the .. in libc++ header path comes from), but maybe it'd be better to do the normalization uniformly when emitting debuginfo metadata?

In D87657#2272775, @dblaikie wrote:

Got any particular examples of this from LLVM's DWARF output? I suspect this sort of thing might come from/lead to path non-canonicalization which would make the line table bigger than it needs to be, so might be worth fixing there too.

While this information can be extracted out of the existing llvm-dwarfdump output, it requires additional post-processing. This became so common in our project that we have implemented a custom Go-based tool for that purpose, but that has other downsides such as the lack of DWARF5 support. I think that supporting this option directly in llvm-dwarfdump might be generally useful and it doesn't add a lot of complexity. I'm open to suggestions for how to improve the output.

LGTM

It's not clear why couldn't we support per-target runtime directory? It uses the standard multiarch layout, so in you'd end up using a directory like [path/to/resource-dir]/lib/armv6m-unknown-eabi/libclang_rt.builtins.a. I'd find this more preferable for consistency with other targets, it'd also enable the use of runtimes build for baremetal.

LGTM

LGTM

LGTM

LGTM

This broke our build with the following error:

FAILED: libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj 
/b/s/w/ir/k/staging/llvm_build/./bin/clang++ --target=aarch64-unknown-fuchsia --sysroot=/b/s/w/ir/k/cipd/sdk/arch/arm64/sysroot  -DHAVE___CXA_THREAD_ATEXIT_IMPL -DLIBCXXABI_USE_LLVM_UNWINDER -D_DEBUG -D_LIBCPP_DISABLE_EXTERN_TEMPLATE -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS -D_LIBCXXABI_BUILDING_LIBRARY -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/k/llvm-project/libcxxabi/include -I/b/s/w/ir/k/llvm-project/libunwind/include -I/b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include --target=aarch64-unknown-fuchsia -I/b/s/w/ir/k/cipd/sdk/pkg/fdio/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/k/staging/llvm_build/runtimes/runtimes-aarch64-unknown-fuchsia-bins=../staging/llvm_build/runtimes/runtimes-aarch64-unknown-fuchsia-bins -ffile-prefix-map=/b/s/w/ir/k/llvm-project/= -no-canonical-prefixes  -O2 -g  -fPIC   -nostdinc++ -Werror=return-type -W -Wall -Wchar-subscripts -Wconversion -Wmismatched-tags -Wmissing-braces -Wnewline-eof -Wunused-function -Wshadow -Wshorten-64-to-32 -Wsign-compare -Wsign-conversion -Wstrict-aliasing=2 -Wstrict-overflow=4 -Wunused-parameter -Wunused-variable -Wwrite-strings -Wundef -Wno-suggest-override -Wno-error -pedantic -fstrict-aliasing -funwind-tables -D_DEBUG -UNDEBUG -std=c++17 -MD -MT libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj -MF libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj.d -o libcxxabi/src/CMakeFiles/cxxabi_shared.dir/cxa_exception_storage.cpp.obj -c /b/s/w/ir/k/llvm-project/libcxxabi/src/cxa_exception_storage.cpp
In file included from /b/s/w/ir/k/llvm-project/libcxxabi/src/cxa_exception_storage.cpp:15:
In file included from /b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include/__threading_support:14:
In file included from /b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include/chrono:827:
/b/s/w/ir/k/llvm-project/libcxxabi/../libcxx/include/ctime:62:10: fatal error: 'sys/cdefs.h' file not found
#include <sys/cdefs.h>
         ^~~~~~~~~~~~~
1 error generated.

LGTM

LGTM

LGTM

This was discussed when LLVM_ENABLE_PER_TARGET_RUNTIME_DIR was introduced and there was a pushback against changing the driver behavior depending on the value of that option, so if we're going to reverse that decision for BareMetal, I think that deserves a broader discussion.

LGTM

This is just a naming suggestion, but how about naming it unnamed_func (instead of unnamedfunc) so it's closer to unnamed_addr.

I have suggested an alternative in https://reviews.llvm.org/D85820#2237357 to try and fix this forward and I was waiting for @haampie's response before landing this. I can implement that today if you're OK fixing this forward? I think it'd be easier to do that than reverting and then relanding all these changes again. @gkistanova how did you check the patch with http://lab.llvm.org:8011/builders/lld-perf-testsuite bot? Is there a way to do a presubmit check on that bot?

In rGda0592e4c8df95efad4e42d63646f8a5336a7edc#943749, @ldionne wrote:

In rGda0592e4c8df95efad4e42d63646f8a5336a7edc#941745, @phosek wrote:

@ldionne this is still failing and it's been several days now, can we revert this change?

Sorry folks, I was gone on vacation. I see you've reverted the change -- that's fine. I'll check it in again with a workaround for older CMakes.

In this case, the workaround will have to be that the libc++ benchmarks are *not* available on older CMakes. The issue is actually deeper than that, it is that the LLVM CMakeLists.txt is setting the CMAKE_CXX_STANDARD to 14 globally: https://github.com/llvm/llvm-project/blob/1c060aa988452508fcfc65357f4a6467f3f88cc1/llvm/CMakeLists.txt#L53.
There's no easy way to work around this AFAICT beyond using proper CMake target features. Note that we should stop setting the CMAKE_CXX_STANDARD (or any global flag really) in LLVM's CMakeLists.txt, but that's another story.

We started seeing assertion failure after rolling a toolchain that contains this change and git bisect identified this change. I have filed a bug with a reproducer as PR47324.

In D85820#2235461, @haampie wrote:

@gkistanova It's true that this change has lead to more issues I could ever imagine, but I think the link you provided is the last remaining problem.

Pinging @phosek for a similar issue w.r.t. zlib: since https://reviews.llvm.org/D79219 zlib gets disabled on static builds of LLVM. The reason is find_package(ZLIB) finds a shared library, and check_symbol_exists(compress2 zlib.h HAVE_ZLIB) tries to statically link to that -- it can't so zlib is disabled. I guess that's a regression too?

What would be the best way forward @phosek? A quick fix for ncurses is to add a simple check_symbol_exists test too, but that would in practice just disable ncurses in static builds. And additionally we could find static libs by adding explicit names like libtinfo.a to find_library when LLVM_BUILD_STATIC=ON. This trick won't help for zlib though, the find_library stuff is hidden inside find_package, and there is no way to target static libs it seems.

In D79219#2231927, @mati865 wrote:

@phosek in MSYS2 (targeting x86_64-w64-windows-gnu) Zlib works properly for LLVM 10 but with master I'm now seeing:

-- Constructing LLVMBuild project information
-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a
CMake Error at lib/Support/CMakeLists.txt:9 (string):
  string sub-command REGEX, mode REPLACE: regex "^(lib|)" matched an empty
  string.
Call Stack (most recent call first):
  lib/Support/CMakeLists.txt:226 (get_system_libname)

-- DEBUG zlib_library=D:/msys64/mingw64/lib/libz.dll.a was printed by my change to help debugging it.
FYI zlib_library is set here: https://github.com/llvm/llvm-project/blob/8e06bf6b3a2e8d25e56cd52dca0cf3ff1b37b5d1/llvm/lib/Support/CMakeLists.txt#L218

In rG31e5f7120bdd2f76337686d9d169b1c00e6ee69c#942622, @haampie wrote:

@phosek do you know what is required to backport this as well as the ncurses detection to the LLVM 11 release branch?

LGTM

LGTM

LGTM

Wasn't this already addressed by https://github.com/llvm/llvm-project/commit/34fe9613dda3c7d8665b609136a8c12deb122382?

LGTM

In D86313#2229323, @smeenai wrote:

In D86313#2229269, @phosek wrote:

I agree that the current situation is not optimal, I was never happy with the default case which is confusing for a lot of people I talked to. I would like to come up with a better solution, but I'm not sure if special casing the x86_64-apple-darwin is sufficient though. With Apple Silicon, I assume we're going to be building runtimes as universal binaries that target multiple architectures.

We could consider completely decoupling runtimes from targets. Originally, each runtime build was identified only by its target, so you'd use RUNTIMES_${target}_${variable} to set target-specific variables. That became insufficient when we introduced the support for multilib, so now you also have a name and you can use RUNTIMES_${target}+${name}_${variable}. We could switch to RUNTIMES_${name}_${variable} and it'd be the responsibility of the user to set the target explicitly if they want it (FWIW it's something we already have to do in our build anyway because of Windows where the CMake behavior when you set CMAKE_${LANG}_COMPILER_TARGET breaks the build). If not, they get the default behavior, matching the default case today. What do you think?

To clarify, what would be setting ${name} in this scenario?

I agree that the current situation is not optimal, I was never happy with the default case which is confusing for a lot of people I talked to. I would like to come up with a better solution, but I'm not sure if special casing the x86_64-apple-darwin is sufficient though. With Apple Silicon, I assume we're going to be building runtimes as universal binaries that target multiple architectures.

In D86308#2228917, @tambre wrote:

I'm pretty sure add_asm_sources() has nothing to do. The ASM language is enabled by compiler-rt anyway and CMake can recognize the files as assembly anyway.

@ldionne we've seen failures in various third party libraries like BoringSSL or ICU that were caused by this change. The problem is that timespec_get and aligned_alloc are only available if __DARWIN_C_LEVEL >= __DARWIN_C_FULL, but many existing libraries define _XOPEN_SOURCE in which case __DARWIN_C_LEVEL == _POSIX_C_SOURCE unless _DARWIN_C_SOURCE or _NONSTD_SOURCE are defined as well.

LGTM

In D86134#2226975, @aeubanks wrote:

In D86134#2226701, @phosek wrote:

@haampie I landed your change but I'm seeing the following failure on sanitizer Windows bot:

CMake Error at lib/Support/CMakeLists.txt:223 (get_system_libname):
  get_system_libname Function invoked with incorrect arguments for function
  named: get_system_libname

The failing build is http://lab.llvm.org:8011/builders/sanitizer-windows/builds/68331

I'm going to revert the change, but I'm happy to reland it once we figure out what the problem is.

I also think this broke the Windows builds for Chrome: https://crbug.com/1119478.
CMake Error at lib/Support/CMakeLists.txt:9 (STRING):

STRING sub-command REGEX, mode REPLACE: regex "^()" matched an empty
string.

Call Stack (most recent call first):

lib/Support/CMakeLists.txt:218 (get_system_libname)

Might need zlib to repro.

@haampie I landed your change but I'm seeing the following failure on sanitizer Windows bot:

This seems to have broken our Mac builders with the following error:

-- Testing: 25226 tests, 24 workers --
Testing:  0.. 10.. 20.
FAIL: Clang :: Frontend/ast-main.c (6834 of 25226)
******************** TEST 'Clang :: Frontend/ast-main.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   env SDKROOT="/" /b/s/w/ir/k/staging/llvm_build/bin/clang -emit-llvm -S -o /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp1.ll -x c - < /b/s/w/ir/k/llvm-project/clang/test/Frontend/ast-main.c
: 'RUN: at line 2';   env SDKROOT="/" /b/s/w/ir/k/staging/llvm_build/bin/clang -emit-ast -o /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp.ast /b/s/w/ir/k/llvm-project/clang/test/Frontend/ast-main.c
: 'RUN: at line 3';   env SDKROOT="/" /b/s/w/ir/k/staging/llvm_build/bin/clang -emit-llvm -S -o /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp2.ll -x ast - < /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp.ast
: 'RUN: at line 4';   diff /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp1.ll /b/s/w/ir/k/staging/llvm_build/tools/clang/test/Frontend/Output/ast-main.c.tmp2.ll
--
Exit Code: 1

@keith do you plan on finishing this? I think we should change the name from -fcoverage-prefix-map to -fprofile-prefix-map, but otherwise it might be good to go.

In D85056#2225093, @MaskRay wrote:

I created https://sourceware.org/bugzilla/show_bug.cgi?id=26404 for the discussion on the syntax.

@christylee @phosek You may consider starting a thread on binutils@sourceware.org to expedite their processing:)

LGTM

D76482 is an example use case for this feature.

LGTM

LGTM

@ldionne this is still failing and it's been several days now, can we revert this change?

Note that this issue isn't manifesting with CMake 3.17 and newer.

LGTM

One concern I have with this change is that we may not always consistently update CMAKE_ASM_FLAGS or set CMAKE_ASM_COMPILER. This wouldn't make a difference before, but it'll with this change. Can you check if these variables are being updated as needed?

CMake already has FindCurses module so we could consider using find_package(Curses), unfortunately it doesn't define the imported target. I'd also prefer replacing HAVE_TERMINFO with LLVM_ENABLE_TERMINFO everywhere for consistency with zlib or libxml2 (see D84563).

LGTM

In D85678#2212326, @plotfi wrote:

In D85678#2211933, @phosek wrote:

I'd be fine with InterfaceStub as a name for the library.

@phosek @haowei Are there any plans in the near term to unify any of the TBD work with InterfaceStubs/elftapi?

@ributzka @cishida