Great!

Patches that just remove code without breaking functionality are my favourite kind!

This patch certainly looks correct for fixing the symptom, but I'm somewhat concerned that the original logic was incorrect anyway. I believe CMake's FindLibrary and FindPackage work correctly with things installed in /usr/local on FreeBSD, so I don't know why we need these.

I'd like to get this into the 11 point release, so it would be good to have a review...

This was caught with the GNUstep runtime's test suite, which apparently had not been run with anything newer than clang 8 until recently. With this patch, all of the runtime's tests now pass again (a few others failed in 10 but appear to have been fixed in 11 or 12). We've now configured our CI to test with the nightly builds on Linux, so should catch these things sooner in the future.

Does the new plugin work with processes that are created with pdfork? The last time I tried this, it caused the old plugin to lock up the debugger entirely. Please can you ensure that there are tests that cover pdfork and cap_enter in the child? These are currently quite badly broken.

I lost track of the mailing list discussions, but assuming that there is consensus to accept this back end I am happy with the structure of the initial code, the size of the contributed diffs, and the engagement from the maintainer.

Mostly boilerplate that looks fine, a few minor nits.

We don't normally revert commits for downstream build failures. Please can you submit a test case that triggers this issue?

Looks good to me, thank you!

Looks good to me. This bakes in the assumption that function pointers and basic block addresses are always in the same address space. That seems reasonable to me but it might be worth documenting in the DataLayout docs about the program address space.

This feature looks generally useful. A few small suggestions:

Sorry for getting to this late, I assumed it was a runtime-agnostic feature until someone filed a bug against the GNUstep runtime saying that we didn't implement it. It would be polite to tag me in reviews for features that contain runtime-specific things.

This seems to be failing on the Windows pre-merge test run. Unfortunately, I don't have access to those logs and so I can't merge this until this issue is fixed: https://github.com/google/llvm-premerge-checks/issues/187

That makes sense. Do you have commit access yet? If not, I'll merge this tomorrow.

I'm a bit nervous about this. I'm aware of at least one out-of-tree toolchain that uses this mechanism to select their proprietary linker. I'd expect an RFC and for this to be highlighted in LLVM Weekly before I'm happy that this won't break downstream consumers.

Looks good to me. I presume we didn't use this because of CMake version dependencies. Was this feature available in our current minimum CMake version?

In D79846#2036260, @krytarowski wrote:

OK, we will need to live with -Wno*.

@krytarowski , as I read this change you are:

It should already be buildable out of the box. The missing prototypes warning is opt-in. I am not particularly in favour of changes motivated solely by an external build system that explicitly opts in to warnings that don't make sense for a particular compilation unit.

I'm guessing that this is explicitly enabling the missing prototypes warning. I'm not a huge fan of that for compilation units that don't have public headers, it makes the code more complex and doesn't catch any bugs. It's valuable only when you have a public header and want to ensure that it's in sync with the implementation.

I don't object to this, it more correctly expresses the intent. The original int was taken from the GCC docs describing the ABI for these functions. Have the GCC docs been updated?

FreeBSD uses GNU libatomic for the combination with GCC.

To give some background (please correct me if I'm wrong): This is initially motivated by having snmalloc working on NetBSD. Snmalloc relies on two-pointer compare-and-exchange. When passing -mcx16, clang will happily emit these instructions for the atomic builtins used by <atomic> but GCC currently contains a bug that will emit these only for the old-style __sync_* builtins and not for the __atomic_ ones. As a result, the gcc builds of snmalloc include a call to __atomic_compare_exchange_16, which is not found on NetBSD because they don't ship the relevant support functions. NetBSD would like to ship chunks of compiler-rt, compiled with their default toolchain (GCC).

I don't think that this should be including <stdatomic.h>. These routines are used to implement interfaces in <stdatomic.h> and it would be completely valid for a conforming implementation to use _Generic macros to directly call these functions from things in <stdatomic.h> that we then depend call, giving a circular dependency.

Please can we have some perf comparisons for just -emit-llvm (not invoking any of the back-end infrastructure)? Given the numbers so far, I'd expect these to also be in the noise, but even at -O0 a quite significant proportion of the compile time is spent after the stages that use IRBuilder.

We (Microsoft) are interested in participating in this process.

I think this version of the test case is slightly oversimplified and it would be valid for an optimisation to remove the check. If there is any address-taken function that may have escaped, then it is not okay to assume that @llvm.objc.autoreleasePoolPop will definitely not write to it, but in this case there are no stores to an internal global and so GVN is at liberty to eliminate it entirely. The fact that it doesn't is a missed optimisation caused by not having sufficient knowledge of @llvm.objc.autoreleasePoolPop.

In D67867#1686112, @jyknight wrote:

In D67867#1686056, @hfinkel wrote:

Maybe everything is fine, but given this setup, does anyone see any potential problems with compiling these functions for nvptx? I'd like to eventually see a mode where we compile an appropriate subset of these functions for GPU targets -- either in bitcode form for linking with our device-side OpenMP runtime library or as a native object -- to provide a more feature-complete offloading environment.

The one thing that caught by eye was the use of the section attribute, and I was curious what nvptx does with that. As far as I can tell, perhaps the answer is nothing.

Then I think this scheme won't work, since the point of the sections is to enable the creation of the global symbols post-build.

E.g., I think the idea is that the main implementation defines the function with C++ name __llvm_libc::strcpy(char *, const char *), and places the code in the .llvm.libc.entrypoint.strcpy section. And then another tool comes along and iterates the llvm.libc.entrypoint sections, and adds global symbol aliases for each one.

That scheme feels probably over-complex, IMO, but I don't have an concrete counter-proposal in mind.

We don't have anything documenting the usage of reserved identifiers. The C standard reserves identifiers that begin with a double underscore or with an underscore or a capital as reserved for the 'implementation' but assumes that the compiler and library are a single blob. GCC has a documented policy that double-underscore identifiers are reserved for the compiler and underscore-capital identifiers are reserved for the library (but glibc doesn't follow it, so ended up with __block being used as an identifier in unistd.h, which broke many things for a long time). Do we want to have a stricter policy, for example that the only identifiers that we use in public headers that are not standard symbols are __LLVM_LIBC_{FOO} for macros and __llvm_libc_foo for other identifiers?

The changes for the GNU runtimes look correct to me. We're missing a bunch of tests there, unfortunately.

I think this was simply mirroring what gcc 4.something did on Solaris.

Apparently I forgot to hit submit on the comments I made yesterday, sorry!

Thanks for working on this, it's a seriously underspecified part of LLVM.

I think this looks like it will improve codegen for us and not violate any of our C-level guarantees. Hopefully @arichardson can also take a look.

LGTM. I wonder if we have any other ugly GCC bug compatibility parts in clang's Objective-C implementation...

I wonder if a list of comma-separated names is the right approach. Would it make more sense to add a new attribute for each of the helpers, such as #no-runtime-for-memcpy? That should make querying simpler (one lookup in the table, rather than a lookup and a string scan) and also make it easier to add and remove attributes (merging is now just a matter of trying to add all of them, with the existing logic to deduplicate repeated attributes working).

I am not convinced that there is sufficient abstraction here to handle multiple runtimes without significant code duplication. The Mach-O and LLVM IR parsers both appear to duplicate all knowledge of the runtime's data structures, so if you wanted to add a second runtime you'd then need four classes. I would expect to see two abstraction layers:

• Fix ownership with Twine.

• Address Dustin's review comments.
• [objc-gnustep] Use $ in symbol names on Windows.
• Add fix from Dustin.

It would probably benefit from a test so that we don't regress.

Looks good to me. On ELF and Mach-O, I think we get the equivalent behaviour automatically from the ODR linkage type. I'd really like to see linkage type and COMDAT made orthogonal, but that's a bigger change.

This review is mostly so that @DHowett-MSFT can check that I didn't break his patches too badly in the cleanup and test that they still do allow building Objective-C DLLs on Windows.

After some bisection, it appears that this is the revision that introduced the regression in the GNUstep Objective-C runtime test suite that I reported on the list a few weeks ago. In this is the test (compiled with -fobjc-runtime=gnustep-2.0 -O3 and an ELF triple):

Looks like a much cleaner change.

Please can we either merge this or revert the original change that introduced the bug that this is fixing? It's now been 6 months since blocks generated invalid ELF symbols.