- User Since
- Apr 18 2013, 6:48 AM (352 w, 2 d)
Additionally, is this something that should be picked to 10.0 after it's committed?
I think this should be put in 10.0, yes.
Cherry-picked to 10.x in a10f87d5695bdd4f1e366c82fd2869f0fe1d4cfe
Cherry-picked to 10.x in ac2c2db1674f200f87b05bee528c761600d87615 thanks!
Thu, Jan 16
Oh, and removed the release note from trunk in 00c74d0b644
It seems that while this commit got pushed to GitHub, it's not actually part of master or any other branches.
Wed, Jan 15
lgtm (with comment)
Tue, Jan 14
This change also caused AppleClang 220.127.116.1100037 to crash while trying to build libcxx, which is problematic for Chromium because that's the version we use for bootstrapping clang/libcxx etc. on Mac.
Mon, Jan 13
Looks good to me (with a minor comment). Thanks!
Fri, Jan 10
Looks good to me. Thanks for pushing this forward!
Thu, Jan 9
Tue, Jan 7
Looks good to me. I'll commit it for you.
Looks good to me. Thanks!
Okay, in that case I would personally just make it an array. But maybe kcc has opinions on this, so let's wait to hear from him.
But was there a bug before, or is this just an optimization? Maybe it could just be an array instead?
Thanks! I just added a few comments.
Do you have commit access, or will you need someone to commit this for you?
Looks okay to me, but maybe expand the comment to explain what's going on and what the patch is doing. "fuzzfork allocating expected number of jobs" sounds more like a bug report than a fix.
Dec 19 2019
Dec 18 2019
Sorry for the breakage, and thanks for reverting! I'll try to follow up and fix this tomorrow.
The way Enable()/Disable() is currently implemented will not work when the tool executes jobs in parallel (ie. llvm-buildozer I presented at LLVM conference; or our re-implementation of /MP which I haven't published yet). It needs refcounting, otherwise one instance might disable the CRC while other threads are running, which effectively disables the crash handlers.
But we can discuss that in a separate review if you prefer. I'll remove it from this patch unless you say otherwise.
I looked over this again, and also studied CrashRecoveryContext some more.
Dec 11 2019
I'm basically happy with this, just some minor comments.
Dec 10 2019
I've reverted in 49da20ddb43 until it can be fixed. Please let me know if you need help with the reproducer.
We're seeing non-determinism in Chromium builds after this commit. You can reproduce using the pre-processed source from https://bugs.chromium.org/p/chromium/issues/detail?id=1032241#c32 and running this (giant, sorry) command:
I see. For some reason I thought the temporary files would be written elsewhere, like in /tmp, but I see that's not the case, and I guess it also makes sense to avoid having to copy it between file systems.
I'm not familiar with this code, but this change looks reasonable to me.
Dec 9 2019
Dec 6 2019
Looks like it was reverted in 055779a9ac11e56442cbcdc73da59f8bce7ce57d.
Our build system does not handle randomly named files created during the build well.
Dec 5 2019
Seems fine to me. I'm not sure how to test the actual "don't write to temp file" functionality, but at least there could be a test to check that the flag gets forwarded to cc1.
lgtm conceptually, just wondering how the "mv" i commented about works.
Dec 4 2019
Thanks for the review!
Dec 3 2019
I'm also supported, even though the gains are smaller than first thought. It depends on D70568 though.
Looks good as far as I can tell. I think it can be simplified by asserting that zlib::compress doesn't fail, though.
Looks fine to me.