LGTM as well, sorry about the delay.

LGTM

This is an alternative solution to D64045, but it seems like !associated metadata, which are lowered to SHF_LINK_ORDER are incompatible with section groups which are used for external symbols, so this solution alone isn't sufficient.

In D64045#1766212, @pcc wrote:

Should this be using !associated metadata on ELF instead of comdats?

Is this patch up to date?

In D76668#1940224, @mcgrathr wrote:

The discussion and the tests should distinguish the COMDAT semantics from the --gc-sections semantics. They are related but distinct.

COMDAT is a standard ELF feature. It's clear how it's supposed to behave on its own: only one definition survives. Anything that prevents two input COMDAT groups with the same signature symbol from being reduced to exactly one of them in the output is clearly a bug.

The __start_*/__stop_* behavior is not formally specified. I was involved with its original creation, so I can speak with some authority on the spirit of its intent. It was never intended to influence what section contents go into the link, merely to provide symbols for the section bounds after all other logic has done so. IIRC this feature predated --gc-sections. It also predated the common use of COMDAT for C++. So its interactions with them were never really considered at the time. But I think the natural expectation of what section group semantics mean with these features is clear enough: a group always travels together. So when --gc-sections decides that .text.f is unreferenced then there is nothing keeping any of the f group in the link, so it all goes. The __start_ and __stop_ references don't affect this--they only resolve to whatever output section is left with that name after all the other logic such as COMDAT rules and --gc-sections has been applied.

LGTM

In D76018#1930398, @leonardchan wrote:

Oh, I think you'll need to add it to llvm/cmake/modules/LLVMExternalProjectUtils.cmake also:

--- a/llvm/cmake/modules/LLVMExternalProjectUtils.cmake
+++ b/llvm/cmake/modules/LLVMExternalProjectUtils.cmake
@@ -221,6 +221,9 @@ function(llvm_ExternalProject_Add name source_dir)
     set(cmake_args ${ARG_CMAKE_ARGS})
   endif()

   ExternalProject_Add(${name}
     DEPENDS ${ARG_DEPENDS} llvm-config
     ${name}-clobber
@@ -239,6 +242,7 @@ function(llvm_ExternalProject_Add name source_dir)
                -DLLVM_HOST_TRIPLE=${LLVM_HOST_TRIPLE}
                -DLLVM_HAVE_LINK_VERSION_SCRIPT=${LLVM_HAVE_LINK_VERSION_SCRIPT}
                -DLLVM_USE_RELATIVE_PATHS_IN_DEBUG_INFO=${LLVM_USE_RELATIVE_PATHS_IN_DEBUG_INFO}
+               -DLLVM_USE_RELATIVE_PATHS_IN_FILES=${LLVM_USE_RELATIVE_PATHS_IN_FILES}
                -DLLVM_SOURCE_PREFIX=${LLVM_SOURCE_PREFIX}
                -DPACKAGE_VERSION=${PACKAGE_VERSION}
                -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}

LGTM

LGTM

LGTM

@smeenai does this look reasonable to you?

This change only covers LLVM, not the runtimes. I'll make a follow up change to do the same for runtimes which should cover the cases you mentioned.

In D75890#1915493, @thakis wrote:

__FILE__ only expands to an absolute path if you pass an absolute path to clang (ctrl-f "FILE" on http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html).

LGTM but you might want to wait a few more hours for other reviewers to see if anyone has objections.

I'd prefer to make this the behavior when just NDEBUG is set, I think it's rather unexpected to still get debug messages even after you set NDEBUG, but since that would be a breaking change, it deserves an email to llvm-dev to ensure that nobody is relying on the current behavior.

In D75818#1911540, @MaskRay wrote:

I think implementing pthread on top of C11 threads is plausible on Fuchsia because it just provides POSIX lite. However, the same idea copying over to Linux may not be practical. For one thing, the performance of pthread_mutex_lock matters. Implementing it on top of mtx_lock costs an extra function call, not to say that some features are not available in C11 mutex. mtx_* and thrd_* are usely so rarely in practice. Implementing them on top of pthread will have an infinitesimal cost.

We're not using this module in Fuchsia at the moment, so I'd be fine not having an alias at all, if we start using it again in the future we'll use the new name.

LGTM

LGTM

LGTM

This is still failing on Windows:

******************** TEST 'Extra Tools Unit Tests :: clang-doc/./ClangDocTests.exe/MDGeneratorTest.emitNamespaceMD' FAILED ********************
Note: Google Test filter = MDGeneratorTest.emitNamespaceMD

Ping again?

In D74106#1862689, @smeenai wrote:

Ah, right, I forgot that "linking" includes header dependencies and so on. This makes sense.

Not this diff, but won't a BUILD_SHARED_LIBS build still have issues here, since then Support will only have a PRIVATE dependency on ZLIB::ZLIB and libraries which depend on Support won't get that dependency? (And if that's not a problem, then I'm wondering why the static case needs to be PUBLIC instead of PRIVATE.)

LGTM

Would it be possible to land the enumeration support separately? These are separate changes and I think it'd be cleaner to land them as two commits.

Wouldn't this break when compile command contains a quoted path with spaces?

In D74176#1862853, @smeenai wrote:

What does the llvm-config --system-libs output look like with this?

LGTM

Ping?

In D74275#1865769, @sbc100 wrote:

Any idea why this file was listed under x86_ARCH_SOURCES? But only seems to active for ARCH_PPC?