Ping

@dexonsmith and @jansvoboda11 thanks for the fast reply and the extra testing.

Assuming I'm answering the correct question that the returns_nonnull is preserved through a PCH, the answer is yes.

Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side?

Ping

Ping

Thanks for adding more tests! LGTM.

LGTM!

LGTM

Ping.

@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

Overall looks good, few remaining nitpicks.

LGTM

LGTM. An additional review here would be nice though.

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?

LGTM

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.

Update patch after @aaron.ballman review!

Thanks for the review @aaron.ballman!

Make this work for c++14 as well, update testcase to cover all possibilities mentioned. @mdreseler thanks for bringing this up.

Looks good to me with the remaining clang-format linting on the assert.

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().

Update patch to reduce code dup as suggested by @dvyukov

Cool, thanks for the input!

Thank you @dvyukov for another round of reviews! A have more questions for you.

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!

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.

Update broken comment in the testcase.

Fix failing tests and clang-format issue.

Apply review comments.

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.

Address reviewer comments. To prevent false positive on release/consume, this now depends on https://reviews.llvm.org/D101501.

LGTM

Hi Nathan, thanks for implementing this. Besides formatting nitpicks, few other comments/questions:

LGTM

Thanks both of you for the review @delcypher and @yln, will wait for @dvyukov to sign-off!

Ping.

@jfb sure!

Hi Xun, great to see more improvements in this area.

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!

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?

Nice correctness improvement!

Hi Yifeng,

LGTM, it would be great if someone else stamps this too, in case I missed something.

LGTM

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.

Looking forward to see m68k support (and hopefully sega genesis toolchain support someday)!

I forgot to follow up, but LGTM too.

LGTM

Very nice explanation, thanks for improving this!