- User Since
- Aug 15 2014, 7:40 AM (357 w, 3 d)
Fri, Jun 18
Thu, Jun 17
Tue, Jun 15
Assuming I'm answering the correct question that the returns_nonnull is preserved through a PCH, the answer is yes.
Mon, Jun 7
Fri, Jun 4
Thanks for adding more tests! LGTM.
Wed, Jun 2
Tue, Jun 1
@yln sorry, I didn't get notifications for this somehow. This got fixed as part of the more general support in https://github.com/llvm/llvm-project/commit/819e0d105e84c6081cfcfa0e38fd257b6124553a
Thu, May 27
Overall looks good, few remaining nitpicks.
Wed, May 26
Tue, May 25
Mon, May 24
May 21 2021
LGTM. An additional review here would be nice though.
May 20 2021
Thanks for adding the tests. LGTM after some remaining nitpick.
Update after last round of reviews.
Nothing else of substance from me, but I don't feel confident in giving final sign off on this. LG, but please wait until one of the other reviewers accepts as well.
Overall looks good, sounds like it's failing tests though?
Sounds reasonable to me! Can you double check whether this attribute gets correctly serialized/deserialized in face of CXXNewExpr? An example of how to test that would be in clang/test/PCH/cxx-method.cpp.
May 19 2021
May 18 2021
Update patch after @aaron.ballman review!
Thanks for the review @aaron.ballman!
May 17 2021
Make this work for c++14 as well, update testcase to cover all possibilities mentioned. @mdreseler thanks for bringing this up.
May 14 2021
Looks good to me with the remaining clang-format linting on the assert.
May 13 2021
Overall looks good, one more comment below.
You should also update the title with [NFC] or similar.
The approach looks good. There are some test failures - you need to update some bits in clangd and probably anywhere else in the repo that you see uses of getUsingDecl().
May 12 2021
Update patch to reduce code dup as suggested by @dvyukov
May 11 2021
Cool, thanks for the input!
May 10 2021
Thank you @dvyukov for another round of reviews! A have more questions for you.
May 6 2021
Cover more cases in atomic.ll.
Apply more reviewer suggested changes: first evaluate CAS, then decide what order to use.
Forgot to add the Differential tag, my bad. Closed by https://reviews.llvm.org/rG819e0d105e84
LGTM, except that the commit message should say "Lift", not "Lyft". ;)
Reduce the diff in a test.
Update after @rjmccall comments!
May 4 2021
Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.
May 3 2021
Update broken comment in the testcase.
Fix failing tests and clang-format issue.
Apply review comments.
Apr 29 2021
Thanks for keeping a good track of these hacks. Nitpick: before landing, use single instead of double spaces before a new sentence in the comments. LGTM.
This is the same as https://reviews.llvm.org/D80172, right? LGTM.
Apr 28 2021
Address reviewer comments. To prevent false positive on release/consume, this now depends on https://reviews.llvm.org/D101501.
Apr 20 2021
Hi Nathan, thanks for implementing this. Besides formatting nitpicks, few other comments/questions:
Apr 8 2021
Apr 6 2021
Apr 5 2021
Apr 1 2021
Mar 26 2021
Mar 23 2021
Mar 19 2021
Mar 17 2021
Hi Xun, great to see more improvements in this area.
Jan 26 2021
If you have any testcases that would make sense to add to the LLDB testsuite, let me know! It will make it harder for me to break C++ coroutines as I'm working on Swift coroutine support.
Awesome. I've also tested the last version of the patch against some testcases I care about and they all debug fine under lldb!
Jan 25 2021
Hi Adrian, this is nice, I really like the approach! Do you think that this supersedes https://reviews.llvm.org/D90772? I'm curious if we can stop generating the extra dbg.values inserted, or whether those are necessary for your reconstruction work?
Jan 15 2021
Nice correctness improvement!
Jan 12 2021
Dec 9 2020
LGTM, it would be great if someone else stamps this too, in case I missed something.
Dec 4 2020
Dec 2 2020
Hmm... I'm using trunk lldb and gdb8.2, both of them can only print address. Btw, os is centos7.2, FYI.
Thanks for following up with this, I did miss to incorporate this suggestion! Out of curiosity, are you using lldb or gdb to print the variable? In lldb I already get values instead of address before this patch, so I wonder if one debugger needs more specific info than the other.
Nov 19 2020
Looking forward to see m68k support (and hopefully sega genesis toolchain support someday)!
Nov 18 2020
I forgot to follow up, but LGTM too.
Nov 10 2020
Very nice explanation, thanks for improving this!