[RFC] [Clang] Not assuming there is at most one definition in a redeclaration chain

Clang Frontend
1

See [Serialization] Stop demote var definition as declaration by ChuanqiXu9 · Pull Request #172430 · llvm/llvm-project · GitHub for background

Cite @zygoloid 's comment:

The purpose of this mechanism was to maintain an AST invariant that other parts of Clang may reasonably be relying on – specifically that there is at most one definition in a redeclaration chain for a variable.
Ultimately we have a choice: either we make the modules code attempt to build an AST that “looks like” a non-modules AST, for example with at most one definition per variable, function, or class, and the rest of Clang gets to assume that normal traditional C++ rules are in effect, or we allow the AST to directly represent things like a variable that has multiple definitions (or a class with multiple definitions or an inline function with multiple definitions) and the complexity associated with dealing with those things gets distributed across all parts of Clang and tools that consume its ASTs.
So far we’e largely picked the first option. In part that’s because Clang’s modules system was originally a language extension, so keeping its impact contained made sense, and in part that’s because the intent of the modules system has historically mostly been around getting to the same state that a non-modules compilation would reach, but faster. Maybe it’s time to change that, given that modules is now a standard language feature. But I think this is probably something the Clang Area Team should consider and make a conscious decision on.
(If we choose to directly represent the post-modules state in the AST – for example, including the possibility of there being multiple definitions of entities in different TUs in the same AST – I’d encourage that you also model the multiple TUs themselves explicitly, by creating multiple distinct TranslationUnitDecls, one per module.)

Previously we can assume there is at most a definition in a redeclaration chain in a compilation unit. But after modules are introduced, technically we can have multiple same definition in the redeclaration chain.

Previously, for clang header modules, since it is an extension, clang header modules’s solution is, pretending there is at most a definition in the redeclaration chain.

But with C++20 modules, given it is a standard feature, maybe we should change the assumption. That is, there may be multiple definitions within a redeclaration chain, but these definitions must be the same.

This affects #172241, #64034, #149404 and not verified, but I suspected, #171548

Proposal

For clang developers, we can’t no longer assume there is at most one definition in a redeclaration chain. We need to assume there may be multiple definitions in a redeclaration chain, but we can assume these definitions are the same.

CC @AaronBallman @cor3ntin @rnk

2

FWIW, we have this need outside of C++ modules as well. WG14 added a feature to C23 which allows you to redefine the same type within the same TU. e.g.,

struct S { int x; };

void func(struct S { int x; } s) { /* Error pre-C23, fine now */
}

So this also impacts #151394 as well as other C23 issues I’ve been fixing for the past while.

2 Likes
3

Yeah, so it seems like we can have basic assumption.

I’d like to land [Serialization] Stop demote var definition as declaration by ChuanqiXu9 · Pull Request #172430 · llvm/llvm-project · GitHub in next week if no more objection comes in. I want to land it in clang22 as it affects a lot issue reports.

4

That’s a risky change to land so late in the cycle and would prefer to land it immediately after the branch point so that we can get more bake time for it. It’s changing an invariant that’s been in Clang forever, so it’s not something we should rush IMO.

5

Got it. Then I’ll land it after the branch cut.

6

How multiple definitions affect the CodeGen? Do the variables refer to the same memory or does each definition have its own memory allocated for it?

7

See my analysis in C++20 modules and `std::jthread`: link failed with clang & libc++ v21.1.x · Issue #172241 · llvm/llvm-project · GitHub and they have distinct allocation.

8

If different definitions have different allocations how do you know which allocation is used when you refer to a variable?

For example, let’s take a global variable “gVar” that has multiple definitions in different modules. Can you predict which allocation is going to be changed for “gVar = 3;”?

9

Can you predict which allocation is going to be changed for “gVar = 3;”?

What do you mean by “change”? These definitions are not allowed to be changed seperately. Or they have to be changed all together.

BTW, I think change is not allowed generally. We can “add” something. But change may be bad.

10

From the previous discussion my understanding is that different definitions correspond to different memory locations. So if module A has int gVar = 0; and module B has int gVar = 1; these result in 2 different memory locations having values 0 and 1. And by “change” I mean a change in a value stored at some memory address. So if there is gVar = 3; which memory location should store value 3?

11

When we talking about memory allocations, we were talking about different things. I was talking about the concept in clang’s AST. And it looks like you’re talking about the memory at Runtime. As these definitions are the same, their runtime address should be the same.

So if module A has int gVar = 0; and module B has int gVar = 1; these result in 2 different memory locations having values 0 and 1

Then it is invalid. This is an ODR violation. Compiler should diagnose it.

12

I wasn’t able to reproduce any problems with multiple variable definitions in different modules because it’s not allowed. So I’d like to step back and clarify what definitions across modules you have in mind when saying

But after modules are introduced, technically we can have multiple same definition in the redeclaration chain.

Because with declaring and exporting the same global variable in multiple modules I get

error: declaration ‘gVar’ attached to named module ‘a’ cannot be attached to other modules

and with declaring a global variable in a shared header I get

error: redefinition of 'gVar'

Based on these experiments it doesn’t look like we need to support multiple variable definitions in the redeclaration chain.

13

I wasn’t able to reproduce any problems with multiple variable definitions in different modules because it’s not allowed.

Please see the issues I mentioned.

So I’d like to step back and clarify what definitions across modules you have in mind when saying

Please see basic.def p16:

For any other definable item D with definitions in multiple translation units,
…

That literally allow the same declaration have multiple definitions in multiple TU.

14

Personally, I’m not convinced those issues are caused by multiple variable definitions as there are no traces of the investigation, no mentions of alternative approaches. But I haven’t debugged those issues, so I cannot claim your assessment is wrong.

As far as I understand, these definitions should be the same, so it’s unclear what is the benefit of having multiple objects in memory to represent the same entity.

Is there a plan to have multiple definitions for other entities as well? For example, structs, unions, enums, classes.

15

In general, I think the RFC process is lacking a few important pieces

  • What are the consequences of the decision? Are there any risks or disadvantages?
  • What is the strategy to discover bugs caused by the RFC? In Clang codebase pointer comparison is used fairly often as a substitute for equality comparison and multiple definitions aren’t compatible with this approach. Is there any way to detect these problems?
16

Please look at the example in C++20 modules and `std::jthread`: link failed with clang & libc++ v21.1.x · Issue #172241 · llvm/llvm-project · GitHub

The reason why the symbol is missing is, in use.cc, the CodeGen incorrectly treat the definition as a declaration.

And please look at [C++ Modules] "import std" conflict with module that includes STL headers: std::println compilation failure · Issue #174858 · llvm/llvm-project · GitHub

the reason for the failure is, since one of the definition pretends to be a declaration, then the ODR check mechanism fail.

so it’s unclear what is the benefit of having multiple objects in memory to represent the same entity.

The benefit is: previously, there already multiple definitions within modules, but we pretended that there is only one definition and ask all other definitions to pretend as declaration. It simply makes the logic more complex.

Is there a plan to have multiple definitions for other entities as well? For example, structs, unions, enums, classes.

Again, there is already multiple definitions in the same declaration chain.

What the RFC ask for is, we shouldn’t pretend anymore. And what this RFC for cfe dev is, we shouldn’t assume in a redeclaration chain, there is at most a definition. There may be multiple definition. But these definitions should be the same.

CC @zygoloid if you have more insights.

17

What are the consequences of the decision?

As said again, it is the cfe dev can’t assume in a redeclaration chain, there are at most a definition.

e.g.,

int definition_count = 0;
for (auto *RD : D->redecls())
    if (RD->isDefinition())
       RD++;
assert(RD <= 1);

Previous we can safely assume the assertion is true, otherwise it is a problem elsewhere. But then, the assertion itself is wrong.

Are there any risks or disadvantages?

I didn’t see any such things.

What is the strategy to discover bugs caused by the RFC?

The RFC itself won’t cause bugs. As the RFC just introduces a more relax assumption. The judgement for particular bug reports will be, if they wrongly assumes the number of definitions.

In Clang codebase pointer comparison is used fairly often as a substitute for equality comparison and multiple definitions aren’t compatible with this approach. Is there any way to detect these problems?

I don’t think the RFC changes that.

Previously, it is already incorrectly to think two Decl* are not the same declaration by their different address. Previously, we already need to compare the Canonical Decl to test that. This RFC doesn’t change that.

18

+1

We discussed this at the Clang Area Team meeting on Jan 8 and came to the conclusion this isn’t really even an RFC; it’s just asking the community to admit reality of both language standards.

That said, there may be technical details in the approach of how we admit reality. For example, Richard suggested maybe we want to model multiple modules as multiple translation unit decls. However, we leave that to the modules maintainers to decide whether that design makes the right tradeoffs (it seems like a reasonable approach but it also seems like it could be a potentially significant refactoring and still have surprises for maintainers and downstreams).

FWIW, I brought this post up in the Clang C and C++ Language Working Group meeting on Jan 7 to alert other folks doing standards conformance work. There were some questions, but no opposition to the idea either.

3 Likes
19

Ok, looks like everyone is eager to have multiple definitions in the redeclaration chain, I won’t argue that. Therefore no point-by-point responses as I don’t think it is useful to anyone.

Can we limit the change only to C++20 modules? In some cases multiple definitions can lead to miscompilations and I’d like to avoid that for Clang modules at least.

20

In some cases multiple definitions can lead to miscompilations and I’d like to avoid that for Clang modules at least.

What’s the case? It will be better to look at the specific issue.

Can we limit the change only to C++20 modules?

If possible, it would be better to not do such limitations. It will be better to have a uniform implementation as much as possible. It will reduce complexity and maintain burden. For my understanding to modules, I don’t think there are unfixable bugs if we admit for the redefinitions along the redeclaration chain.

To be honest, I feel the original approach “pretending the definition is a declaration”, is not pretty and it is a workaround.

1 Like