- User Since
- Feb 16 2014, 1:10 AM (223 w, 4 h)
Tue, May 22
Thanks again for the reviews. I removed the extraneous else, and reverted the string switch back to what I had originally.
Sorry for letting this languish a bit. I took some time to experiment. Long story short I think llvm-dwarfdump and lldb aren't able to verify this option at the moment.
Mon, May 21
Sat, May 19
Yes, unfortunately I haven’t been able to reproduce this locally, on any build environment I have access to. There’s a discussion about this in https://reviews.llvm.org/D46776, but I can’t figure out why the buildbots wouldn’t behave the same way. Passing ‘-triple x86_64-scei-ps4-ubuntu’ as suggested there doesn’t make a difference for me locally. And indeed the argument parsing occurs before the triple is taken into account anyway, so it makes sense to me that it wouldn’t make a difference.
Tue, May 15
Hmm... but the >1 condition is such that an edit distance of greater than 1 results in no suggestion, whereas an edit distance of 1 or 0 results in a suggestion. In this case -- and apparently only on certain platforms' buildbots -- the edit distance between -debug-info-macros and -debug-info-macro is 1, and so it should display a suggestion.
Mon, May 14
I'm not yet sure why, but this change caused buildbot failures due to a test in Clang for the "did you mean" feature. I couldn't figure out why the buildbots would fail whereas the Clang tests pass for me locally, and so reverted this change for the time being, in rL332304. I'll try to resubmit once I've figured out and fixed the issue.
Thanks for the reviews, and for pointing out the bug!
Fri, May 11
Thu, May 10
Hooray! Thanks again for the review, @GorNishanov.
I updated this diff to not only document how the ResultFD is set, but also make it a little more consistent across platforms. Previously Windows would only sometimes set it to -1 in the case of failure, now it always does.
Please just bring back the first patch to line 1316 and submit. Thanks!
Thanks for all the review!
Wed, May 9
Thanks for the review! I adopted the changes you suggested. As for validating the debug info, what about using 'llvm-dwarfdump -verify'? In fact running that on the output of this new option resulted in errors, so I'll address those.
Correct! I'll mention that as well.
Thanks for the comments!
Tue, May 8
Mon, May 7
Sorry it took me a while to get around to this, but I think this new approach fits more along the lines of what you were thinking, @GorNishanov. I also confirmed that it optimizes the program you linked to just as well as before.
Sat, May 5
Fri, May 4
Thanks again for all the reviews, @GorNishanov! Very much appreciated.
Wed, May 2
Oops, thanks for testing on release mode, @GorNishanov. Turns out I had a dangling pointer. With this update the tests pass on both release and debug.
Thanks for the review, @GorNishanov. Here's a more correct solution: an i1 is used to keep track of whether await_resume threw. If it did, the coroutine body is skipped, and we go straight to the final suspend point. Otherwise, the coroutine body is executed as normal.
Apr 21 2018
Strange, the revision appeared to have been "Accepted" by @JDevlieghere. I wonder why the message says it wasn't accepted...? Apologies in advance if I did something incorrectly when landing this.
Thanks, @JDevlieghere! I added two tests like the ones you described. Thankfully it doesn't crash because llvm-bcanalyzer verifies the bitcode invariant that input sizes be a multiple of 4.
Sorry for letting this diff languish for so long. I updated the tests to be simple files with just the bytes necessary to be picked up as a specific stream type. I also moved the magic number reading over to a static helper function. I considered adding this function as a member of llvm::BirstreamReader, but chose not to for now, because doing so (as far as I can tell) would mean encoding Clang's magic numbers into LLVM's machinery. Let me know if you have different thoughts. Thanks!
Apr 19 2018
Apr 12 2018
Apr 5 2018
Apr 3 2018
Thank you, @GorNishanov, this is excellent! I've pulled this down and confirmed it fixes some nasty errors in a codebase I work on, related to this usage of std::aligned_storage. The problem also reproduced with this much smaller example: https://reviews.llvm.org/P8076 -- and I've confirmed this change of yours fixes that, too. So, thank you!
Apr 2 2018
Oops! I used arc diff --verbatim and ended up removing reviewers.
Thanks @rnk! I updated this as per your suggestions. As a result it's a smaller and less invasive change -- thank you! Let me know if this looks good.
Apr 1 2018
Thanks for the review, @GorNishanov! I adopted your suggestions and made the commit.
Mar 27 2018
Mar 26 2018
Excellent, thank you! I think this might fix PR34982?
Mar 20 2018
Thanks for tracking down this bug! You may wish to ask @anemet for a review, I think he would have a good idea of what a proper fix would look like. As-is this LGTM, although in my opinion the test case could benefit from being reduced further.
Mar 19 2018
Mar 15 2018
Mar 7 2018
Just tidying up one line with clang-format. I guess everyone's busy preparing for the Jacksonville standards meeting. Please give this a review when you all have a second -- in addition to the reported bug, https://bugs.llvm.org/show_bug.cgi?id=34897, I've also confirmed that this diff fixes ASAN errors that surfaced in a project at work. Thanks! :)
Mar 6 2018
Update formatting with clang-format.
Feb 28 2018
I wasn't sure what the best way to test this would be. The assertion occurs in LLVM, but Clang is responsible for scheduling the passes. If anyone has any suggestions, I'd greatly appreciate them!
Feb 22 2018
Friendly ping! Let me know if this seems totally wrong to you, @GorNishanov :)
Feb 20 2018
Feb 19 2018
Great, thank you both for the reviews!
Thanks for the review! I've updated the diff to use range loops instead.
Feb 16 2018
Feb 15 2018
Thank you for all the reviews, @GorNishanov!
Thanks again, @GorNishanov, for all the reviews!