The test looks fine to me, I'm quite surprised that it passes.

This seems like a regression for FreeBSD. This function was added to compiler-rt before we officially supported AArch64 and so I'm pretty sure it's shipped in every AArch64 version of FreeBSD, even Tier 2 releases.

@ro, I'm happy to help out on a more up-to-date Solaris port if someone wants to do one.

I generally prefer the approach here. Generality is good when necessary but a source of bugs when unnecessary. For things like stack save and stack restore, the only valid address space is the address space used for the stack. Allowing overloading adds more burden to the verifier (it must check that the address space matches) for no benefit that I can perceive.

In D138680#3971179, @theraven wrote:

tBricks was using this, but I think they've migrated off Solaris now. I'll check. I haven't had a Solaris system for a very long time.

tBricks was using this, but I think they've migrated off Solaris now. I'll check. I haven't had a Solaris system for a very long time.

In D135273#3936297, @al45tair wrote:

@theraven Any chance you could glance over this and reassure us that it isn't going to break the GNU runtime if we do this? (We're adding an extra attribute in the property attribute string so that we can detect @optional properties in ObjC protocols at runtime.)

In D135158#3836839, @jrtc27 wrote:

Except then you can't do unsigned long x = (unsigned long)&y as intrinsics are not constant expressions. Non-integral pointers are too strict for CHERI as things stand, we want a subset of their behaviour.

CHERI capabilities aren't non-integral. Converting a capability to an integer gives you the address, discarding the metadata (which is also stable*), which is as well-defined as it is on normal architectures.

LGTM, a couple of extra comments would help.

I don't believe this version is correct. It unconditionally does the lock. If code using this is mixed with code where the compiler inlines the operation then the two operations will not be atomic with respect to eachother. Worse, an atomic-fetch-nand is not atomic with respect to any other operation (e.g. an atomic increment). This needs to have the same if (lockfree(ptr) guard and do a CAS loop in the is-lock-free case.

I'd like to. The test case from the original bug report was very large but I'm not sure how to trigger this with anything comprehensibly small. You need something where the callee is responsible for destroying the argument and I'm not sure what combination of properties / ABIs results in that. The original test was -(void)foo:(std::string)aString on the MSVC ABI, but I don't know how much of the Visual Studio std::string implementation is necessary to trigger this behaviour. Is it just a non-trivial destructor?

In D68480#3179526, @ldionne wrote:

In D68480#3179355, @theraven wrote:

This appears to have baked in some ABI details that don't permit efficient implementation on some of our supported platforms.

This is unfortunate. Have you folks shipped this yet? Perhaps it's not too late to fix things now if you've only released it very recently.

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?