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.

This should be fine for the GNUstep runtime (the GCC runtime doesn't support ARC at all). My main concern is that it will break already-released versions of the runtime built with a newer version of clang. I can easily enable a new flag in the next release, but doing so for older ones is more problematic.

I wonder if we want to have an option to elide ObjC type info for all non-POD C++ types. Nothing that you do with the type encoding is likely to be correct (for example, you can see the pointer field in a std::shared_ptr, but you can't see that changes to it need to update reference counts) so it probably does more harm than good.

If we're going to change the default, please at least add a flag to allow callers to opt into dlimport. We can then make that dependent on -static-objc or similar.

It would probably be a good idea to have a similar check on properties, as property encoding strings contain the type encoding (plus extra stuff).

I wonder if it would be cleaner to define a KMP_OS_POSIX and a KMP_OS_BSD and avoid a load of 'if Linux, FreeBSD, NetBSD, DragonFlyBSD, HURD' checks. This would make it easier for the next person to support a UNIX-like or BSD-derived OS.

It's not clear from this whether there's a plan to remove llvm-config, or just to remove llvm-config as a way for clang to detect llvm. I would be strongly opposed to the former. Having tried to maintain out-of-tree code that builds with multiple versions of LLVM, it is more or less possible with llvm-config and totally impossible with the LLVM CMake files. Unless dropping llvm-config is accompanied by a strong commitment to long-term API stability for the LLVM CMake infrastructure, this would have a significant negative impact on downstream consumers of LLVM (and, even then, extracting the CMake information when your downstream build system is not CMake is painful).

Looks good for me, and removing all of the code describing Objective-C 4 as ObjC1 makes me happy.

In D52581#1248306, @smeenai wrote:

The simplest option is something like P8109, where we add a .objc discriminator when mangling the RTTI itself. It would require the GNUStep runtime for Windows to be altered accordingly (e.g. https://github.com/gnustep/libobjc2/blob/master/eh_win32_msvc.cc#L85 would have to use ".objc.PAU" instead of ".PAU.objc_cls_"); would that be acceptable, @theraven and @DHowett-MSFT?

In D47233#1129810, @DHowett-MSFT wrote:

We ran into some critical issues with this approach on x64. The pointers in the exception record are supposed to be image-relative instead of absolute, so I tried to make them absolute to libobjc2's load address, but I never quite solved it.

A slightly better-documented and cleaner version of the code you linked is here.

I would have done the same for the GNUstep RTTI here, except I don't actually
see the code for that anywhere, and no tests seem to break either, so I
believe it's not upstreamed yet.

In D52344#1242530, @rjmccall wrote:

In D52344#1242451, @kristina wrote:

Would you be okay with me renaming cfstring to .cfstring for ELF so it's in line with ELF section naming conventions (I'm not entirely sure if that could cause issues with ObjC stuff)? And yes I think it's best to avoid that code-path altogether if it turns out to be a constant as that's likely from the declaration of the type, I'll write a FileCheck test in a moment and check that I can apply the same logic to ELF aside from the DLL stuff as CoreFoundation needs to export the symbol somehow while preserving the implicit extern attribute for every other TU that comes from the builtin that CFSTR expands to. Is what I'm suggesting making sense? If not, let me know, I may be misunderstanding what's happening here.

Following the ELF conventions seems like the right thing to do, assuming it doesn't cause compatibility problems. David Chisnall might know best here.

In D50783#1221147, @ahatanak wrote:

The GNUstep documentation I found replaces '@' with '\1'. Would that fix the problem?

https://github.com/gnustep/libobjc2/blob/master/ABIDoc/abi.tex