- User Since
- Feb 16 2014, 1:10 AM (230 w, 4 d)
Sat, Jul 14
Yup, LGTM! I'll land this now.
Tue, Jul 10
If you don't have commit access, let me know here if you'd like me to commit this on your behalf.
Thu, Jul 5
This LGTM but I'll just wait for @GorNishanov to accept the patch, just in case I'm missing something. I'd be happy to commit this for you once Gor accepts! :)
Tue, Jul 3
Oops, apologies, I included a line I shouldn't have in the previous diff.
Mon, Jul 2
Tue, Jun 26
Sat, Jun 23
Great, thanks @GorNishanov! I moved the 'can throw' logic into a function called 'memberCallExpressionCanThrow', to convey that some dyn_cast'ing is going on.
Great, thanks for the review! I added a reference to N4499.
Also @GorNishanov I'm curious about your two cents on whether comments like these are valuable. If you think they are I may add a few more with post-commit review.
Jun 13 2018
Great! Thanks for the review :)
Jun 12 2018
Great, thank you!
Thanks for catching this! LGTM.
Jun 11 2018
Thanks for the reviews! I adopted @pcc's suggestion to ignore the option without printing a warning. (I also considered calling llvm::opt::Arg::claim, but it looks like no other arguments are claimed, and lld doesn't print warnings for unclaimed arguments anyway.) I updated the help text and the test as well.
Thanks for the reviews! I'm updating this so that it no longer includes the warning I added in D47994. I'll commit in in a second.
Oops! I fixed a typo in the error limit exceeded message.
Jun 10 2018
Jun 3 2018
Jun 2 2018
May 28 2018
Great! Thanks @GorNishanov!
May 22 2018
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.
May 21 2018
May 19 2018
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.
May 15 2018
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.
May 14 2018
I'm not yet sure why, but this change caused buildbot failures due to a test in Clang for the "did you mean" feature (example: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/29988). 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!
May 11 2018
May 10 2018
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!
May 9 2018
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!
May 8 2018
May 7 2018
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.
May 5 2018
May 4 2018
Thanks again for all the reviews, @GorNishanov! Very much appreciated.
May 2 2018
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!