User Details
- User Since
- Nov 19 2012, 2:56 AM (426 w, 19 h)
Tue, Jan 12
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.
Dec 1 2020
Nov 30 2020
I'd like to get this into the 11 point release, so it would be good to have a review...
Nov 20 2020
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.
Nov 6 2020
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.
Oct 9 2020
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.
Oct 8 2020
Mostly boilerplate that looks fine, a few minor nits.
Oct 2 2020
We don't normally revert commits for downstream build failures. Please can you submit a test case that triggers this issue?
Oct 1 2020
Sep 24 2020
Looks good to me, thank you!
Sep 2 2020
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.
Aug 7 2020
This feature looks generally useful. A few small suggestions:
Jul 26 2020
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.
Jun 19 2020
Jun 4 2020
May 26 2020
May 21 2020
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
May 20 2020
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.
May 19 2020
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?
May 14 2020
@krytarowski , as I read this change you are:
May 13 2020
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?
May 12 2020
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).
May 11 2020
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.
Feb 11 2020
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.
Jan 10 2020
Jan 8 2020
We (Microsoft) are interested in participating in this process.
Dec 16 2019
Dec 10 2019
Nov 14 2019
Nov 9 2019
Nov 4 2019
Nov 3 2019
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.
Oct 30 2019
Oct 28 2019
Oct 4 2019
Sep 29 2019
Sep 27 2019
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.
Sep 3 2019
Jul 16 2019
I think this was simply mirroring what gcc 4.something did on Solaris.
Jun 27 2019
Jun 20 2019
Apparently I forgot to hit submit on the comments I made yesterday, sorry!
Jun 19 2019
Thanks for working on this, it's a seriously underspecified part of LLVM.
Jun 11 2019
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.
May 29 2019
LGTM. I wonder if we have any other ugly GCC bug compatibility parts in clang's Objective-C implementation...
May 17 2019
May 16 2019
May 7 2019
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).
Apr 29 2019
Apr 1 2019
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:
Mar 31 2019
- Fix ownership with Twine.
Mar 1 2019
- 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.
Feb 28 2019
Feb 27 2019
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.
Feb 24 2019
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):
Feb 3 2019
Dec 29 2018
Looks like a much cleaner change.
Dec 28 2018
Dec 27 2018
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.