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

Clang reports an undefined function that has been defined #73232

Open
tbaederr opened this issue Nov 23, 2023 · 13 comments · Fixed by #73463
Open

Clang reports an undefined function that has been defined #73232

tbaederr opened this issue Nov 23, 2023 · 13 comments · Fixed by #73463
Assignees
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" confirmed Verified by a second party rejects-valid

Comments

@tbaederr
Copy link
Contributor

tbaederr commented Nov 23, 2023

Not sure how to title this properly.

The original reproducer is just:

#include <string>
constexpr auto z = std::string("", 0);

with libstdc++.

Using cvise, I get:

template <typename _CharT>
struct basic_string {
  constexpr void _M_construct();

  constexpr basic_string() {
    _M_construct();
  }

};


basic_string<char *> a;


template <typename _CharT>
constexpr void basic_string<_CharT>::_M_construct(){}

constexpr basic_string<char*> z{};

https://godbolt.org/z/7PzreExcf

Clang's output is:

<source>:18:16: error: constexpr variable 'z' must be initialized by a constant expression
   18 | constexpr auto z = basic_string<char *>();
      |                ^   ~~~~~~~~~~~~~~~~~~~~~~
<source>:6:5: note: undefined function '_M_construct' cannot be used in a constant expression
    6 |     _M_construct();
      |     ^
<source>:18:20: note: in call to 'basic_string()'
   18 | constexpr basic_string<char*> z{}
      |                    ^~~~~~~~~~~~~~~~~~~~~~
<source>:3:18: note: declared here
    3 |   constexpr void _M_construct();
      |                  ^

The problem vanishes if a is commented out, or if basic_string is not a template anymore.

@tbaederr tbaederr added clang:frontend Language frontend issues, e.g. anything involving "Sema" libstdc++ GNU libstdc++ C++ standard library labels Nov 23, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 23, 2023

@llvm/issue-subscribers-clang-frontend

Author: Timm Baeder (tbaederr)

Not sure how to title this properly.

The original reproducer is just:

#include &lt;string&gt;
constexpr auto z = std::string("", 0);

with libstdc++.

Using cvise, I get:

template &lt;typename _CharT&gt;
struct basic_string {
  constexpr void _M_construct();

  constexpr basic_string() {
    _M_construct();
  }

};

void operators() {
  basic_string&lt;char *&gt;{};
}

template &lt;typename _CharT&gt;
constexpr void basic_string&lt;_CharT&gt;::_M_construct(){}

constexpr basic_string&lt;char*&gt; z{};

https://godbolt.org/z/7PzreExcf

Clang's output is:

&lt;source&gt;:18:16: error: constexpr variable 'z' must be initialized by a constant expression
   18 | constexpr auto z = basic_string&lt;char *&gt;();
      |                ^   ~~~~~~~~~~~~~~~~~~~~~~
&lt;source&gt;:6:5: note: undefined function '_M_construct' cannot be used in a constant expression
    6 |     _M_construct();
      |     ^
&lt;source&gt;:18:20: note: in call to 'basic_string()'
   18 | constexpr auto z = basic_string&lt;char *&gt;();
      |                    ^~~~~~~~~~~~~~~~~~~~~~
&lt;source&gt;:3:18: note: declared here
    3 |   constexpr void _M_construct();
      |                  ^

The problem vanishes if operators() is commented out, or if basic_string is not a template anymore.

@cor3ntin
Copy link
Contributor

cor3ntin commented Nov 23, 2023

same problem with free functions

template <typename T>
constexpr void g(T);

constexpr int f() {
    g(0);
    return 0;
}

template <typename T> 
constexpr void g(T) {}

constexpr auto z = f();

@cor3ntin cor3ntin added rejects-valid confirmed Verified by a second party and removed libstdc++ GNU libstdc++ C++ standard library labels Nov 23, 2023
@cor3ntin
Copy link
Contributor

We probably want to call Sema::InstantiateFunctionDefinition in CheckConstexprFunction.
Unfortunately, CheckConstexprFunction does not have access to Sema. in general, nothing in ExprConstant has access to Sema.
To fix that we'd need to add a reference to Sema in EvalInfo, probably. A bit tedious. Anyone has a better idea? @erichkeane

@tbaederr
Copy link
Contributor Author

CC @AaronBallman and @zygoloid, maybe they have an idea

@zygoloid
Copy link
Collaborator

Ultimately this is a language bug. There's no point of instantiation after the function template is defined and before the end of the TU -- we only get PoIs at uses and at end of TU.

What we should probably do is instantiate all used specializations at the point where the function template is defined (if it's constexpr). Constant evaluation shouldn't have side effects.

I'm pretty sure this is a duplicate of a (very) old PR.

@zygoloid
Copy link
Collaborator

See CWG2497.

@cor3ntin
Copy link
Contributor

Ultimately this is a language bug. There's no point of instantiation after the function template is defined and before the end of the TU -- we only get PoIs at uses and at end of TU.

What we should probably do is instantiate all used specializations at the point where the function template is defined (if it's constexpr). Constant evaluation shouldn't have side effects.

Hum, I can see how trying to do instantiations in the middle of constant evaluation would not be great, thanks!

Do you think we should wait for core to resolve the issue, or should we try to eagerly instantiate at the end of the definition of constexpr functions?

@zygoloid
Copy link
Collaborator

I think we should eagerly start instantiating at the end of definitions. (Both function and variable templates are affected by this IIRC.)

@cor3ntin cor3ntin self-assigned this Nov 26, 2023
cor3ntin added a commit to cor3ntin/llvm-project that referenced this issue Nov 30, 2023
Despite CWG2497 not being resolved, it is reasonable to expect the following
code to compile (and which is supported by other compilers)

```cpp
  template<typename T> constexpr T f();
  constexpr int g() { return f<int>(); } // #1
  template<typename T> constexpr T f() { return 123; }
  int k[g()];
  // #2
```

To that end, we eagerly instantiate all referenced
specializations of constexpr functions when they are defined.

We maintain a map of (pattern, [instantiations]) independant of
`PendingInstantiations` to avoid having to iterate that list after
each function definition.

We should apply the same logic to constexpr variables,
but I wanted to keep the PR small.

Fixes llvm#73232
cor3ntin added a commit that referenced this issue Nov 30, 2023
…73463)

Despite CWG2497 not being resolved, it is reasonable to expect the
following code to compile (and which is supported by other compilers)

```cpp
  template<typename T> constexpr T f();
  constexpr int g() { return f<int>(); } // #1
  template<typename T> constexpr T f() { return 123; }
  int k[g()];
  // #2
```

To that end, we eagerly instantiate all referenced specializations of
constexpr functions when they are defined.

We maintain a map of (pattern, [instantiations]) independent of
`PendingInstantiations` to avoid having to iterate that list after each
function definition.

We should apply the same logic to constexpr variables, but I wanted to
keep the PR small.

Fixes #73232
@cor3ntin cor3ntin reopened this Dec 2, 2023
@cor3ntin
Copy link
Contributor

cor3ntin commented Dec 2, 2023

Reverted in 19e2174

We need to handle the following code

template <typename> constexpr static void fromType();

void registerConverter() { fromType<int>(); }
template <typename> struct QMetaTypeId  {};
template <typename T> constexpr void fromType() { 
  (void)QMetaTypeId<T>{};
} // #1
template <> struct QMetaTypeId<int> {}; // #20428 

We can't instantiate fromType in #1 because we later specialize an entity that it uses.
GCC seems to do instantiation at evaluation time https://godbolt.org/z/Wz5oMzdnP

@zygoloid
Copy link
Collaborator

zygoloid commented Dec 2, 2023

Normally I'd just say that the code needs to be fixed to give the explicit specialization before it's used or to make sure the instantiation is deferred, but the breakage is probably too much given that this seems to be in Qt core code.

I guess the question is, do we follow GCC's problematic model that evaluation can cause instantiation (which the language rules elsewhere explicitly try to avoid), or do we add a workaround just for Qt (detect that pattern somehow and don't eagerly Instantiate)?

@cor3ntin
Copy link
Contributor

cor3ntin commented Dec 3, 2023

I guess the question is, do we follow GCC's problematic model that evaluation can cause instantiation (which the language rules elsewhere explicitly try to avoid), or do we add a workaround just for Qt (detect that pattern somehow and don't eagerly Instantiate)?

It seems like EDG/MSVC follow the same model. I sent a mail to CWG, but i doubt it will help make progress.

@cor3ntin
Copy link
Contributor

cor3ntin commented Apr 8, 2024

@zygoloid The current direction taking by the reflection proposal is that arbitrary instantiations can emanate from constexpr evaluation. (Both explicitly as there is a proposed method to instantiate and define classes, and implicitly when querying the properties of not yet instantiated definitions).

In that way the model you consider problematic is going to be mandated by the standard

Is that something you gave any thought above? It might impact how we resolve this issue

@zygoloid
Copy link
Collaborator

zygoloid commented Apr 8, 2024

Thinking about the Qt example some more, if you simply reverse the order of the function definitions:

template <typename> constexpr static void fromType();

template <typename> struct QMetaTypeId  {};
template <typename T> constexpr void fromType() { 
  (void)QMetaTypeId<T>{};
}

void registerConverter() { fromType<int>(); }

template <> struct QMetaTypeId<int> {};

... then the example is clearly invalid (albeit no diagnostic required, because there are two points of instantiation for fromType<int> and only one of them results in the use of QMetaTypeId<int> prior to the explicit specialization), and clang correctly rejects. (GCC still doesn't, which is also permissible.) It seems pretty hard to justify why this example should be valid with the use and the template definition in the other order. The explicit specialization is simply declared too late.

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

Successfully merging a pull request may close this issue.

5 participants