- User Since
- Feb 16 2014, 1:10 AM (247 w, 2 d)
Mon, Nov 5
Sorry to have let this languish! LGTM.
Sat, Nov 3
Tue, Oct 30
Mon, Oct 29
Here's a compiler explorer link demonstrating that GCC 8.2 and Clang 7.0.0 compile the example code, but Clang trunk emits an error: https://godbolt.org/z/l3baI_
Mon, Oct 15
Oh, I'm sorry I let this languish! I'll address your comments later this week, @vsk. Thanks so much for the review!
Oct 7 2018
Sep 26 2018
This is great, thanks! Sorry for letting it languish. I defer to @GorNishanov, but I don't see why this couldn't go in now and if there're any edge cases I'm missing we can address those in another patch. I pulled this down and played around with it, and everything seemed OK to me.
Sep 3 2018
Excellent, I think pushing this along with D50410 revealed the true error, as an MSAN buildbot tells use there's a use of an uninitialized value: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/23176/steps/check-clang%20msan/logs/stdio
Nice! This seems like a plain improvement in terms of debugging, thanks.
Thanks! I'll land D50410, then this, and monitor the buildbots to see what happens. If they fail again I may revert this once again. Thanks again for looking into this, sorry for the delay in landing it.
Aug 28 2018
This looks good to me, but maybe some people who've modified this part of the codebase before could review this as well? @aaron.ballman added a fix-it for angled/quoted strings in rL160406, and more recently @erikjv modified some of this code in rL285057. If anyone could suggest other reviewers that would be great as well!
Aug 15 2018
I don't have access to the PS4 SDK, but this is the most plausible explanation I've seen for why I was experiencing issues on these platforms. Thanks for this!
Jul 14 2018
Yup, LGTM! I'll land this now.
Jul 10 2018
If you don't have commit access, let me know here if you'd like me to commit this on your behalf.
Jul 5 2018
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! :)
Jul 3 2018
Oops, apologies, I included a line I shouldn't have in the previous diff.
Jul 2 2018
Jun 26 2018
Jun 23 2018
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.