Skip to content
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

Permitting static constexpr variables in consteval functions #82994

Open
mpusz opened this issue Feb 26, 2024 · 14 comments
Open

Permitting static constexpr variables in consteval functions #82994

mpusz opened this issue Feb 26, 2024 · 14 comments
Labels
c++23 clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party consteval C++20 consteval

Comments

@mpusz
Copy link

mpusz commented Feb 26, 2024

It seems that Clang does not support static constexpr variables in constexpr functions properly:

https://godbolt.org/z/fx1jTModK

The value of a static constexpr variable set in a constexpr function should be available at runtime.

@mpusz
Copy link
Author

mpusz commented Feb 26, 2024

BTW, if I make the functions constexpr (and not consteval) the code works as expected.

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Feb 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 26, 2024

@llvm/issue-subscribers-clang-frontend

Author: Mateusz Pusz (mpusz)

It seems that Clang does not support static constexpr variables in constexpr functions properly:

https://godbolt.org/z/fx1jTModK

The value of a static constexpr variable set in a constexpr function should be available at runtime.

@shafik
Copy link
Collaborator

shafik commented Feb 26, 2024

Confirmed, it would be nice to have a reduction, it is not obvious what is going on here but maybe @cor3ntin might have an idea.

@shafik shafik added confirmed Verified by a second party needs-reduction Large reproducer that should be reduced into a simpler form c++20 labels Feb 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 26, 2024

@llvm/issue-subscribers-c-20

Author: Mateusz Pusz (mpusz)

It seems that Clang does not support static constexpr variables in constexpr functions properly:

https://godbolt.org/z/fx1jTModK

The value of a static constexpr variable set in a constexpr function should be available at runtime.

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Feb 27, 2024

Reduced example (Godbolt link):

#include <cstddef>
#include <cstdio>

template<class T, std::size_t N>
struct my_array {
    T elems_[N];
};

consteval my_array<char, 2> get_array()
{
    my_array<char, 2> result;
    result.elems_[0] = 'a';
    result.elems_[1] = '\0';
    return result;
}

#ifdef CONSTEVAL_DISABLED
constexpr
#else
consteval
#endif
const char* get_buffer_ptr()
{
    static constexpr auto arr = get_array();
    return arr.elems_;
}

int main()
{
    constexpr auto p = get_buffer_ptr();
    static_assert(p[0] == 'a');  // passes with Clang
    static_assert(p[1] == '\0'); // passes with Clang
    // if get_buffer_ptr is consteval, an empty NTBS is printed with Clang,
    // even though static_assert's pass
    std::puts(p);
}

It seems that Clang mistakenly skipped the constant-initialization of static constexpr variables (or mistakenly used zero-initialization only) in consteval functions.

@shafik I think this should be labeled "c++23" as the examples are only well-formed since P2647R1.

@shafik shafik added c++23 and removed c++20 labels Feb 27, 2024
@shafik
Copy link
Collaborator

shafik commented Feb 27, 2024

@cor3ntin @AaronBallman this feels vaguely familiar but I can't the problem it remind me of

@pitust
Copy link

pitust commented Mar 4, 2024

consteval const int* a() {
    static constexpr int b = 3;
    return &b;
}
int c() { return *a(); }

here is an even more minimal reduction (https://godbolt.org/z/5dEr7sdbs)

@EugeneZelenko EugeneZelenko added the constexpr Anything related to constant evaluation label Mar 4, 2024
@cor3ntin cor3ntin changed the title Permitting static constexpr variables in constexpr functions Permitting static constexpr variables in consteval functions Mar 4, 2024
@cor3ntin cor3ntin added clang:codegen consteval C++20 consteval and removed needs-reduction Large reproducer that should be reduced into a simpler form constexpr Anything related to constant evaluation labels Mar 4, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 4, 2024

@llvm/issue-subscribers-clang-codegen

Author: Mateusz Pusz (mpusz)

It seems that Clang does not support static constexpr variables in constexpr functions properly:

https://godbolt.org/z/fx1jTModK

The value of a static constexpr variable set in a constexpr function should be available at runtime.

@cor3ntin
Copy link
Contributor

cor3ntin commented Mar 4, 2024

consteval const int* a() {
    static constexpr int b = 3;
    return &b;
}
int c() { return *a(); }

consteval is an amazing source of runtime bugs!

Because the function a is immediate, its body is not emitted at codegen.
and b would have been emitted by the body of a, so it's not emitted.

Maybe we need to have a visitor to extract static constexpr vars from immediate functions (and emit them)?
That should be enough.

@erichkeane @AaronBallman

@AaronBallman
Copy link
Collaborator

CC @rjmccall @efriedma-quic

@zygoloid
Copy link
Collaborator

zygoloid commented Mar 5, 2024

An extra visitor would be a little expensive. Perhaps local static variables in consteval functions should be handed to the ASTConsumer immediately by Sema, rather than assuming that the consumer will find them indirectly.

@rjmccall
Copy link
Contributor

rjmccall commented Mar 5, 2024

Hmm. I suppose it's not technically as simple as looking for the declarations referenced by the constant value of the call, since you could leak out a reference to a static local via either a template argument or a local / lambda class. I don't love the idea of forcing IRGen to eagerly track every static local in every consteval function just in case one of them leaks out that way, though. An alternative approach would be to just completely defer to CodeGen and teach it to lazily emit these variables without requiring them to ever be handed to the consumer. (The consumer doesn't usually have to be told about static locals, IIRC; it just implicitly knows about them through the containing function.) That might be a nice compile-time optimization for certain code patterns even without constexpr / consteval — it would let us be lazy about emitting global variables for all static locals with constant initializers.

@shafik
Copy link
Collaborator

shafik commented Mar 7, 2024

CC @Fznamznon

@shafik
Copy link
Collaborator

shafik commented Aug 22, 2024

Note clang does not behave the same as edg/MSVC here: https://godbolt.org/z/Gzjjj7dP4

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

No branches or pull requests

10 participants