This appears to have baked in some ABI details that don't permit efficient implementation on some of our supported platforms. In the future, please can you post an RFC from things that need to integrate with platform-specific code? We're now stuck with this interface until we are willing to do an ABI-breaking change. In particular:

I completely agree with @jrtc27 on this, specifically:

Thanks!

Ah, it looks as if you didn't add the __c11_ variant of the builtin? Please can you add this?

Looks good to me, thanks!

This looks cleaner and simpler than mine, which was a quick hack to get it compiling. Thanks!

In D105142#2850247, @anirudhp wrote:

In D105142#2849835, @theraven wrote:

The code looks fine but it would be good to see some docs along with it. We're currently missing docs on inline assembly entirely and the GCC ones are somewhat... opaque when it comes to describing how constraints work.

Thank you for your feedback! By docs do you mind updating/adding some information to the existing LLVM docs(like the langref https://llvm.org/docs/LangRef.html for example), or more comments to the code?

Here's a minimal test:

I don't believe a compiler can ever generate code that hits this code path. The public function is:

The code looks fine but it would be good to see some docs along with it. We're currently missing docs on inline assembly entirely and the GCC ones are somewhat... opaque when it comes to describing how constraints work.

The error message here is very confusing:

To elaborate: The set of possible invalid IR is huge and back ends are permitted to assume that they are only ever provided with valid IR, as per the informal contract between the back end and the rest of the compiler (including requiring function arguments to be arranged in a way that the back end's ABI-lowering logic understands). Ensuring that the back end is only ever given IR that it knows how to consume is the responsibility of whatever is invoking the back end. In an ideal world, every construct that a front end + optimiser pipeline may feed to a back end would have a test in tests/Target/{back end} that would check that it correctly lowers the IR.

Back ends are expected to crash when given invalid IR.

The Morello (AArch64 + CHERI) implementation uses AS 200 for capabilities and does different instruction selection for loads and stores (including atomics) depending on the address space.

In D102292#2757941, @dblaikie wrote:

Yeah, that seems out of scope for this document. https://llvm.org/docs/LangRef.html#pointer-type documents that pointers to functions and pointers to other things are all roughly the same sort of thing - and we're changing all of them over to the opaque pointer type. & other wording generally treats all pointers as having the same size, etc. I think that's best left elsewhere than this doc if there's no interesting distinction between function pointers and data pointers currently (which it doesn't seem like there is).

Please can you ensure that this is tested with some Objective-C code compiled with -fobjc-runtime=gnustep-2.0? If I am reading the intention correctly, it may result in all of the Objective-C code being dropped from the final link.

Thanks!

I'm broadly in favour of this change, but with the GNUstep ABIs this is an ABI-breaking change and so should not be on by default. The type encoding leaks into the ABI in two ways:

Assuming @ezhulenev's description of the desired behaviour is correct (which the comment on line 366 appears to support), this patch looks right. I don't think we have tests for the sets of exported symbols anywhere else, which is a bit unfortunate.

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: