LGTM

In D86170#2224841, @dblaikie wrote:

Rather than adding a new option, could we generalize LLVM_DISABLE_CRASH_REPORT to work on all platforms & wire that up to not --crash as you've done, without adding another name for this functionality?

Actually it looks like if we removed the Apple-specific filtering of ENABLE_CRASH_OVERRIDE, then the handling in Signals.inc which also supports the runtime API parameter "DisableCrashReporting" would also do what I was proposing for unit tests (disabling crash reporting on gtest) - which has been the default on MacOS builds for 5 years now? I'm not sure I feel perfectly good about disabling crash reporting for all unit test failures, rather than only disabling it around death tests - but I think it's probably good to make the perf tradeoffs the same on different platforms, and that this has been the default for 5 years for a good chunk of the LLVM developers (those on mac) it's probably good enough to ship.

Seems reasonable to me too.

LGTM

LGTM

This shouldn't affect the final binary size as the linker is going to probably recreate the relocations and symbol table anyway, but seems fine to align this more in the object files.

LGTM.

LGTM

LGTM

LGTM

LGTM.

Looks like the tests are extremely comprehensive. LGTM.

LGTM.

I'm trusting the binary blobs are good. Phab doesn't have a built in mach-o parser :)

LGTM.

Looks good. That’s nice to see more lines removed than added :)

Wow. That must be a huge test case for this to be a problem!

LGTM.

LGTM.

LGTM.

LGTM. Thanks!

Looks like we can have print_protocol_list64_t also call the new print_protocol_64_t, but that can be done in a later NFC commit.

LGTM. Thanks for fixing this!

LGTM

Thanks for the review. Submitted as r350284.

In D55869#1339537, @rjmccall wrote:

It sounds like it's fine.

Thanks for all the feedback so far. Is there anything else you'd like me to change before I can land this?

In D55869#1337723, @js wrote:

In D55869#1337711, @dexonsmith wrote:

In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Yes, the idea is that the specialized runtime functions are fast paths for the common case, but they still fall back if the object has overridden them.

I am guessing not just overridden, but also a root class providing one? IOW, the fast path is used when the class does not have the retain or release method at all? In that case, LGTM as is.

In D55869#1335783, @rjmccall wrote:

So, once upon a time this was a problem because we were rewriting the method calls in the runtime itself. Can you explain how that's not a problem now? Do we just expect the runtime to be compiled with the right switch?

In D55802#1334008, @rjmccall wrote:

You're making intrinsics with weak_external linkage? I feel like that's going to be unnecessarily awkward in the future, but okay.

In D55349#1323926, @rjmccall wrote:

LGTM.

Added test for integer argument and updated code to only accept pointer types to allocWithZone.

I added the other objc* intrinsics used by ARC in r348646.

Thanks for the review. Have fixed the comments as suggested.

LGTM

Discovered that objc_storeStrong is actually wrong in the documentation. Changing it from returning id to returning void. Will also update the docs.

In D55233#1318897, @ahatanak wrote:

LGTM

Updated to add a comment to Intrinsics.td, remove the ReturnedValue<> and added all of the ARC methods from https://clang.llvm.org/docs/AutomaticReferenceCounting.html#runtime-support

Sorry for the delay. LGTM.

Nice catch. This LGTM.

In D30882#1075461, @dexonsmith wrote:

In D30882#1075407, @ddunbar wrote:

In D30882#1074822, @dexonsmith wrote:

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

Of course, yes, I was wrong. Thanks for the correction.

I agree we might want to tread cautiously on how we change the .d output, but in this case I think it might be safe.

If we decide this isn't safe, then we may want to consider a new flag which tracks *all* anti-dependencies (file's for which Clang checked existence but did not exist), and include that here. The major concern with that approach is it is a much larger list, and while it would support being substantially more correct, it is also well beyond what people currently expect out of the build system + compiler generated deps approaches.

Even though this is likely safe, it seems really noisy. Consider the case where someone has __has_include(<missing>) -- we'll get an entry for every -I, -F, -isystem, and -iframework. If we're going to add that noise, I prefer a separate flag that covers all anti-dependencies.

This all looks reasonable. Its a nice localized change to this pass. LGTM.

Good catch! LGTM.

This is a great improvement over what we had. LGTM.

Sorry for forgetting to review this.

I scrolled through fairly quickly, but everything looked fine. Looks like basically renaming and fixing formatting along the way.

In D30723#695798, @jordan_rose wrote:

Thanks, Pete! Will land soon, then.

I think we could support SpecificBumpPtrAllocator but it would require walking the objects by 'sizeof(T) + RedZoneSize'. RedZoneSize itself would also have to be set to make sure we continue to have appropriate alignment. Not something you need to do now, but could be interesting in future.

FWIW, this doesn't work as is because SpecificBumpPtrAllocator lets you allocate *arrays* of objects as well as single objects, and the arrays obviously can't have red zones between elements.

Good point. Never thought of that. Wouldn't be surprised if there's a whole bunch of users who never use arrays and could benefit, but still something to think about more.

Very cool. Justin and I looked at ASan on BumpPtrAllocator a while ago and I think this would have been useful then too. I think the reason you didn't find issues is that he fixed many of them already.

Whats the behavior here in terms of implicit conversions and overloading?