[Modules] Incorrect ODR checks caused by `preferred_name` attribute.

[ChuanqiXu9]

I met an incorrect ODR checks caused by preferred_name. Here is a reduced example.

// string_view.h
template<class _CharT>
class basic_string_view;

typedef basic_string_view<char> string_view;

template<class _CharT>
class
__attribute__((__preferred_name__(string_view)))
basic_string_view {
public:
    basic_string_view() 
    {
    }
};

inline basic_string_view<char> foo()
{
  return basic_string_view<char>();
}
// A.cppm
module;
#include "string_view.h"
export module A;

// Use.cppm
module;
#include "string_view.h"
export module Use;
import A;

The input command line is:

clang++ -std=c++20 --precompile A.cppm -o A.pcm
clang++ -std=c++20 --precompile Use.cppm -fprebuilt-module-path=.  -fsyntax-only

The output would be the incorrect ODR warning:

string_view.h:11:5: error: 'basic_string_view<char>::basic_string_view' from module 'A.<global>' is not present in definition of 'string_view' provided earlier

I've been fighting with the problem for several days. My current process is that when the ASTReader reads the preferred_name attribute, it would read the string_view type and find an existing one. However, the current behavior would create another RecordType for string_view despite there is already one. So here is the problem.

[llvmbot]

@llvm/issue-subscribers-clang-modules

[ChuanqiXu9]

I guess I located the problem. But it is really hard to handle. The problem here is that when the compiler reads __attribute__((__preferred_name__(string_view))), it would like to read the type of string_view. And when the compiler reads the type of string_view, the compiler would try to read basic_string_view and the compiler find that basic_string_view has an attribute __preferred_name__ so the compiler would read the attribute again...

Although due to the design of ASTReader, it wouldn't fall into the infinite reading loop. But the reading result is not correct somehow.

I don't find a good solution to fix this.

[ChuanqiXu9]

Sent a2772fc as a workaround but we still need to fix it.

[zygoloid]

It seems like the problem here is that reading string_view should not result in us immediately reading the attributes on basic_string_view. Generally, reading a declaration shouldn't cause us to immediately read things that appear later, because that can result in cycles.

So either reading the basic_string_view<char> from string_view should only read the forward declaration of basic_string_view, without attributes, or we should defer loading attributes until we're done with recursive deserializations.

[ChuanqiXu9]

We would read the local redeclarations for TagDecl. So the first suggestion might not be straight forward. The second suggestion looks promising.

[ChuanqiXu9]

I did some initial experiments. And it shows it at least not straight forward since we may read some attributes during the deserialization. So if we continue this direction, we may have to divide the attributes as lazy attributes and eager attributes..

[ilya-biryukov]

The same error should be happening for header modules, I'm surprised we're not hitting it at Google with libc++ already.
I guess everything would be fine if all the types would come from the same module, but I'm not sure.

[ilya-biryukov]

Answering my own question here: we're not hitting it at Google because the preferred_name is only used in targets that are modularized themselves, so we never need to merge two independent types with preferred_name into one.

I've checked that if that actually happens, we also hit the same error with header modules.

[VitaNuo]

I've tried to compare the deserialisation of A in the versions of code with and without the preferred_name attribute.

The primary difference is that the version without the attribute does not visit the typedef decl for string_view. I don't know the exact reason for this, but it might be that module deserialization is lazy and as long as the typedef is not used, it's not loaded either.

During the merge step, the typedef imported from A cannot be found
in the decl context. The decl context is the translation unit decl for string_view.h. It does contain the string_view typedef, but the two typedefs (imported and existing) don't pass the same entity check.

Type of the existing typedef:

RecordType 0x555569490520 'string_view'
`-ClassTemplateSpecialization 0x555569490428 'basic_string_view'

Type of the imported typedef:

RecordType 0x5555694c5560 'string_view' imported
`-ClassTemplateSpecialization 0x555569490428 'basic_string_view'

They seem identical, but pointers are not the same, and so the typedefs aren't merged.

In the code version with the attribute, the basic_string_view constructor merge step seems to be triggered twice. The first time it seems to be associated with merging ClassTemplateDecl for basic_string_view.
This first constructor merge is the same both in the version with and without the attribute, and succeeds in both cases (without going through the ODR check mechanism at all).

In the code version with the attribute, the constructor merge step is then triggered again for this constructor decl

CXXConstructorDecl 0x5555694c5928 parent 0x555569490428 <string_view.h:11:5, col:18> col:5 imported in A.<global> hidden used basic_string_view 'void ()' implicit_instantiation implicit-inline instantiated_from 0x5555694c59e0

which I therefore assume is connected to the typedef merge step.
This merge triggers the failed ODR check.

These are probably still all symptoms rather than the actual problem.

So far I haven't identified the exact location where deserialization attempts to go on a loop, as described in the previous comments.

[ilya-biryukov]

The primary difference is that the version without the attribute does not visit the typedef decl for string_view. I don't know the exact reason for this, but it might be that module deserialization is lazy and as long as the typedef is not used, it's not loaded either.

This seems totally expected as string_view is not mentioned anywhere in the template itself if you don't have the attribute specified.

They seem identical, but pointers are not the same, and so the typedefs aren't merged.

The fact that pointers are different should be the curlprit of the problem. This should be stemming from the fact that if we have the attribute, we need to deserialize the typedef when deserializing the template itself. I suspect we just don't have the information that

RecordType 0x5555694c5560 'foo' imported

is the same as (previously created)

RecordType 0x555569490520 'foo'

and so we end up with wrong results of merging.

From a different standpoint, without the attribute:

1. we can independently deserialize the template and its pattern RecordDecl,
2. deserializing the typedef requires deserializing the template and its pattern RecordDecl.

With the attribute, the (2) still holds, but now we also need to deserialize the typedef when deserializing the template itself. And I believe it's this new aspect that screws us up. The merging of the type inside typedef fails to canonicalize the type to point to the previously parsed RecordType 0x555569490520 'foo' (non-imported) and we end up failing the ODR check because the attributes are different?

To confirm this theory, I'd recommend looking at:

• why the ODR check fails (I expect its the type inside the attribute being different),
• tracing if the merging sees the previously parsed RecordType properly in all the internal attributes when deserializing the typedef.

Intuitively, this recursion coming from the attributes should be avoided and we could probably fix this by postponing the merging of this specific attribute to happen after the deserialization step for the corresponding RecordDecl finishes. But it'd be nice to get to the bottom of the issue too.