I can confirm this fixes both the minimized test case from https://bugs.llvm.org/show_bug.cgi?id=41417, and the full original test case from https://bugs.freebsd.org/237074.

• Only set OPENMP_TEST_COMPILER_OPENMP_FLAGS once.
• For standalone builds, call find_package(Threads) and use the result unconditionally.
• Fix else -> else().

So, any thoughts on the current state of the review?

• Use CMAKE_THREAD_LIBS_INIT verbatim, if it is not "-lpthread".
• Add similar logic to DetectTestCompiler for standalone build.

Readdress the issue in a different way: add -pthread in the
OpenMPTesting.cmake file instead. The top-level LLVM config-ix.cmake file
mangles the value of the needed CMAKE_THREAD_LIBS_INIT variable, so derive the
flag from this, instead of directly using it.

Using CMake's own FindThreads package is obviously the correct solution for finding the right compiler and linker options for enabling threading.

Split off the __kmp_gettid() fix to D59735, the <alloca.h> header fix to D59736. I will rebase this review and update it to add some CMake logic for pthread flag detection.

In D59451#1432417, @krytarowski wrote:

it's not with libc++ and newer libstdc++.

Some OSs merged libpthread into libc and/or implemented it almost fully in the kernel. It's not the case on NetBSD and apparently FreeBSD.

In libc there are just stubs for libpthread to allow linking but without defined interfaces, unless someone will link with the POSIX threading library.

In D59451#1432386, @Hahnfeld wrote:

In D59451#1432251, @dim wrote:

In D59451#1432201, @protze.joachim wrote:

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads?

How would the compiler know up-front what is in the code? I have not often seen compilers that influence linking flags from the code that was compiled, unless maybe the Microsoft specific #pragma lib feature (which somebody is now adding to clang, I believe). Usually, these kinds of flags are specified by the user, and passed from the frontend to the linker.

The point is that the test case is using std::thread and the compiler needs to link all libraries that are needed for the stdlib to work. If this includes libpthread that's up to the compiler, so I agree with Joachim that adding linking flags does not seem right.

In D59451#1432201, @protze.joachim wrote:

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads?

Update to use -pthread instead.

In D59451#1431899, @krytarowski wrote:

If we want to depend on pthread, please go for canonical -pthread.

Btw @jonpa, for me it's still interesting as to why the compiler hang only occurred for builds targeting i386 (i.e 32 bit x86), while x86_64 worked just fine. Is it due to the lower number of physical registers on i386?

@hans it's really late but if you're going to spin 8.0.0 rc5, it would be nice if this one gets merged too, after it's been committed. Or if not, I'll probably merge into the FreeBSD version of clang anyway, since we hit this issue first. :)

This helps quite a lot, in the sense that my original test case from PR40986 now at least compiles within a finite time. There is still a performance loss between clang 7.0.1 and clang 8.0.0 with this patch, though (tested with 10 runs each):

Fixed in rCXX355006. (@ldionne it would have been handy to include the "Differential Revision:" line in your commit message, then Phabricator would have picked it up. :)

I will update and commit.

In the case of <__locale>, __throw_runtime_error is already used in the file before its declaration, but via transitive includes, <stdexcept> is also included earlier, so it gets the duplicate declaration of __throw_runtime_error from there. I think we can just delete the __throw_runtime_error declaration from <__locale> altogether.

For what it's worth, this builds and tests correctly for me, and also appears to solve the test cases from bug 40206.

Updated as @ruiu suggested.

Rebased.

Updated to address comments:

• Replaced tuple with BfdDesc struct
• Parse -freebsd suffix to determine OSABI
• Add new case for elf-aarch64 (which used to be elf-aarch64-freebsd)

I can't vouch for the NetBSD part, but this definitely seems like a good idea to me. I'm unsure if the sysctl is already fixed on the FreeBSD side. @emaste any idea?

I'm not sure yet why, but this revision breaks unrolling of some loops in FreeBSD's lib/msun/ld80/e_powl.c, specifically the one in __polevll here, which gets expanded in two places, one instance with a count of 3:

In D55576#1332370, @mgorny wrote:

@dim, thanks for the review. Should I also try removing the following restriction?

#if !defined(__APPLE__) && !defined(__FreeBSD__) // No support for domain sockets
        {env.create_socket("socket"), file_type::socket},
#endif

LGTM.

In D54724#1303844, @phosek wrote:

In D54724#1303809, @dim wrote:

I think this is the wrong direction, placing "common" code in addCXXStdlibLinkDeps, which then has all kinds of ugly ifs and switches for different OSes? Let different OSes handle this in their own way, maybe.

You mean handling this in individual ToolChain subclasses? I've considered doing that. The reason I went with a single method is because we already use this approach for other types of runtimes like sanitizers, XRay, etc. There'd be still some duplication for handling different C++ libraries, but I'd be fine changing the implementation to use that approach if there's a strong preference for doing so.

I think this is the wrong direction, placing "common" code in addCXXStdlibLinkDeps, which then has all kinds of ugly ifs and switches for different OSes? Let different OSes handle this in their own way, maybe.

On FreeBSD, there is no libterminfo either, so LGTM.

In rL325887#291580, @grimar wrote:

I think that the inconsistency of linkers behavior shown above shows that LLD behavior is
not incorrect. It is different. But we never tried to be fully bug compatible with bfd/gold.
First of all the problem is caused by a weak assumption in the linker script, which assumes
that orphans are placed at the particular place before the assignment command,
and my feeling it should be fixed.

Here is the shortest test case I could come up with. (This uses ld.lld-r325886, but it could also use ld.lld 6.0.1.)

In rL325887#291458, @grimar wrote:

I am still debugging it, but one observation to mention:

If you remove the .data1 : { *(.data1) } line from the script,
(https://github.com/freebsd/freebsd/blob/master/sys/conf/ldscript.i386#L141)
you'll get the same result as after this patch

.data1 is the empty section and it is anyways removed. But with this patch, it is done a bit earlier, what does not
allows our orphan placement algorithm to see/use it for finding the insertion point.

So it is just a luck that the script worked before this revision.

In rL325887#290552, @grimar wrote:

My guess it happens because of set_sysinit_set, set_sysuninit_set, set_pcpu, set_vnet are orphans (not listed in the linker script),
and our algorithm of placing the orphans started to place them in between of
(https://github.com/freebsd/freebsd/blob/master/sys/conf/ldscript.i386#L142)

_edata = .; PROVIDE (edata = .);
__bss_start = .;

for some reason. And honestly, I would not call this incorrect behavior, though I do not understand now how this patch could affect this.

In rL325887#290840, @grimar wrote:

In rL325887#290689, @dim wrote:

Furthermore, we have read-only and read/write set_ sections. BFD ld and older lld seem to figure these out, then place the read-only ones just after .rodata, and the read/write ones after .data. I am not sure if there is a way to distinguish between these in a linker script? The names always match set_*, but there is no different naming scheme for r/o and r/w sets.

What I meant is to specify them explicitly. You do not need to match the 'set_*' for that.
I modified the script from the reproduce file in the following way (added all set_* r/w sections before _edata symbol assignment):

...
  .data1          : { *(.data1) }
  
  set_sysinit_set    : { *(set_sysinit_set) }
  set_sysuninit_set  : { *(set_sysuninit_set ) }
  set_pcpu           : { *(set_pcpu) }
  set_vnet           : { *(set_sysinit_set) }
  
  _edata = .; PROVIDE (edata = .);
...

And that resolves the issue (at least _edata == 01e2ef78 now, I did not check the booting).
I think the same should be done for the rest (r/o) set_* sections for the safety and overall correctness of the script.

Speaking about some kind of auto distinguishing between r/o and r/w, I do not think it is possible.
The script may have ONLY_IF_RO and ONLY_IF_RW commands, but that is
about different thing and will not help here I believe.

Will the suggested solution work for you?

In rL325887#290552, @grimar wrote:

Two questions:

1. Why don't you just list them in the script at the expected place? This will solve the issue and make the behavior explicit and cleaner I think.

This change breaks booting an lld-linked i386-freebsd kernel, apparently because it moves symbols around, while not updating the sections? For instance, with lld r325886, I have the following readelf output:

Ah, sorry about that! I should have realized this, but my CMake-fu is weak. :)

In D52240#1242341, @dim wrote:

Hmm, I tried on macOS 10.13.6 with Xcode 9.4.1 and Apple LLVM version 9.1.0 (clang-902.0.39.2), using stock libc++ r342744, but that already failed:

ninja: Entering directory `/Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1'
[1/1] Testing ABI compatibility...
FAILED: projects/libcxx/lib/abi/CMakeFiles/check-cxx-abilist
cd /Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1/projects/libcxx/lib/abi && /Users/dim/tmp/llvm/projects/libcxx/utils/sym_diff.py --only-stdlib-symbols --strict /Users/dim/tmp/llvm/projects/libcxx/lib/abi/x86_64-apple-darwin.v1.abilist /Users/dim/obj/llvm-342744-trunk-darwin17-x86_64-ninja-rel-1/lib/libc++.1.dylib
Symbol added: __ZNSt3__16__itoa8__u32toaEjPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u32toaEjPc'}

Symbol added: __ZNSt3__16__itoa8__u64toaEyPc
    {'type': 'FUNC', 'is_defined': True, 'name': '__ZNSt3__16__itoa8__u64toaEyPc'}

Summary
    Added:   2
    Removed: 0
    Changed: 0
Symbols added.

So I assume the ABI is already broken in some way...

In D52240#1239509, @ldionne wrote:

Marshall, can you confirm that the change does not break check-cxx-abilist on OS X. I'm fine with the change if that's not broken.

Built with mkdir build && cd build && cmake -G Ninja .. && ninja, the resulting libc++.so.1 has the following additional symbols:

So, any more actions to take, or can I commit this?

In D50297#1216935, @ruiu wrote:

Oh, I didn't notice that you are trying to make a change for i386, not for x86-64. What is your motivation to use superpages for 32-bit applications? I believe that the applications you want to use superpages are large programs and naturally be 64-bit, so I wonder if this change is actually useful.

Update all tests to match current output.

LGTM.