Skip to content

[C++] Side effect get removed if the return type of a unary operator is std::nullptr_t #64923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ChuanqiXu9 opened this issue Aug 23, 2023 · 8 comments
Labels
c++ clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party

Comments

@ChuanqiXu9
Copy link
Member

This comes from #58459 but I find it is not related to coroutines after I reduce it.

Reproducer: https://godbolt.org/z/TGefxv5e5

From the generated code, we can see the call to DoAnotherThing() is elided incorrectly.

And if we change the return type of operator++, we can find the problem is gone: https://godbolt.org/z/TGefxv5e5

While I tagged codegen, I feel like this is a frontend issue.

CC people since I know not so many people subscribed to the c++ tag: @AaronBallman @cor3ntin @Endilll @erichkeane

@ChuanqiXu9 ChuanqiXu9 added c++ clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Aug 23, 2023
@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2023

@llvm/issue-subscribers-c-1

@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2023

@llvm/issue-subscribers-clang-frontend

@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2023

@llvm/issue-subscribers-clang-codegen

@jacobsa
Copy link
Contributor

jacobsa commented Aug 23, 2023

Thanks for reducing! Here is a corrected link for what I think you meant about changing the return type (your post currently has the same link twice).

@tbaederr
Copy link
Contributor

tbaederr commented Aug 23, 2023

Most likely a problem in the constant interpreter, since this works with -fexperimental-new-constant-interpreter: https://godbolt.org/z/c3zhjdv5r

@cor3ntin cor3ntin added the confirmed Verified by a second party label Aug 23, 2023
@cor3ntin
Copy link
Contributor

We don't bother evaluating either side of the expression when we know the type is nullptr_t
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L13307-L13313

Fix shortly, i just need to write a test!

@cor3ntin
Copy link
Contributor

@EugeneZelenko EugeneZelenko removed the clang:codegen IR generation bugs: mangling, exceptions, etc. label Aug 23, 2023
@jacobsa
Copy link
Contributor

jacobsa commented Aug 25, 2023

Thank you for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party
Projects
None yet
Development

No branches or pull requests

6 participants