[clang] [clang][Sema] Fix type of an statement expression ending with an atomic type (PR #119711)

[John McCall via cfe-commits]

Alejandro =?utf-8?q?Álvarez_Ayllón?=,
Alejandro =?utf-8?q?Álvarez_Ayllón?=,
Alejandro =?utf-8?q?Álvarez_Ayllón?Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/119711 at github.com>


================
@@ -15919,10 +15919,17 @@ ExprResult Sema::ActOnStmtExprResult(ExprResult ER) {
   if (Cast && Cast->getCastKind() == CK_ARCConsumeObject)
     return Cast->getSubExpr();
 
+  auto Ty = E->getType().getUnqualifiedType();
----------------
rjmccall wrote:

> > As far as this patch goes, my longstanding thought is that _Atomic is clearly a qualifier.
> 
> Errrr, I don't agree. I think `_Atomic` is a type specifier. `_Atomic int` and `int` are not the same type

`const int` and `int` are also not the same type.

> (heck, they can even have different sizes and alignments!).

Size and alignment is the (back-justified) reason we draw the line where we currently do. It is not a particularly sensible way to do it. Most of our existing qualifiers affect code generation and ABI, just in ways that happen to not affect size and alignment. Several qualifiers affect the ABI rules for containing aggregates, for example.

> It's somewhat like a qualifier in that it impacts how you access the underlying object (so in that regard, it is a bit like `volatile int` and `int`), but I think the fundamental thing is that you rarely want `int` semantics directly when the type is spelled `_Atomic int`.

I'm not quite sure what you're saying here. On the implementation level, the compiler can *never* ignore qualifiers when performing an access to an object; the whole point of qualifiers is that they affect the semantics of accesses. On the language design level, I agree with you that programmers should never perform normal accesses to `_Atomic` objects instead of using the intrinsics. However, that is an argument that it was an unfortunate language design decision that `_Atomic` behaves like a qualifier. In the language that we've actually got, `_Atomic` has most of the characteristics of qualifiers, and there is no benefit to the compiler implementation in pretending otherwise.

The common characteristics of qualifiers:
- They convey an aspect of how a value is stored or accessed other than the abstract value itself.
- They are typically disallowed or silently dropped at the top level in type contexts that primarily describe an abstract value rather than storage, such as casts, return values, and parameters (when determining the function type).
- R-value expressions represent an abstract value and are (for the most part) unqualified at the top level. Using a qualified l-value in a context that requires an r-value drops all qualifiers as part of the conversion to an r-value. The qualifiers typically affect the code-generation of the load.
- Assignment consumes an abstract value and therefore expects an r-value on the RHS, which (per above) is of unqualified type. The qualifier typically affects code-generation of the store.
- The qualifier (if it can apply to structs and unions at all) propagates along member accesses, such that the type of `x.member` has all the top-level qualifiers of both `x` and the member.

Most of these apply to `_Atomic`. The most notable exception is the last, which C makes UB. But it is not uncommon for qualifiers to have tweaks to various of these rules, like how C++ allows qualified return values of class type for some reason.

The subtyping rules around CV qualifiers in C++ are clearly a special case of the CV qualifiers and do not apply to anything else. (You can convert an `int**` to `const int * const *`, but you clearly cannot convert it to `__global const int * const *` — address space qualifiers are ABI-affecting in ways that make this infeasible to implement.)

> > So my suggestion is that common functions like getUnqualifiedType() should just be looking through _Atomic, which would have fixed this bug before it happened. If there's a need for a narrower API that only looks through non-_Atomic qualifiers (or perhaps some subset of "non-ABI-affecting" qualifiers?), we can add that with a nice name that suggests when it should be used.
> 
> On the one hand, this specific bug has bitten us so many times I want to agree with your proposed approach.
> 
> On the other hand, that's going to be a massive undertaking that's pretty risky.

Yeah, I don't disagree with this, and I'm not trying to force the problem to be solved immediately. I would like to try to get consensus on the right thing to do, though.

> We have times when we definitely _do not_ want the qualified type to drop the atomic qualifier. For example, when merging function types, we call `getUnqualifiedType()` to determine whether two parameters are compatible. `void func(const int i);` and `void func(int i);` are compatible redeclarations, while `void func(_Atomic int i);` and `void func(int i);` are not. There are other times when we do want the atomic qualifier dropped. It's a frustrating mess and I'm certain we have numerous bugs in this area.

In this case, I have to wonder if the standard is in the same place Clang is — if this was really an intentional allowance for `_Atomic T` as a parameter in function types or just an oversight. It's hard to imagine what an atomic argument could possibly mean. Perhaps it was an oversight that they no longer feel comfortable fixing.

> So the question to me becomes: do we fix this narrow issue and kick the can down the road? Or do we try to find someone willing to undergo the more significant cleanup?

If we agree that the compiler probably ought to be defaulting to treating atomic types as qualifiers, I think the path is:
1. Introduce a function that looks through `_Atomic` as well as the other qualifiers.
2. Make a best effort to switch existing call sites of `getUnqualifiedType()` that immediately also look through `_Atomic` to instead use the common function. Probably ought to be the same patch as (1).
3. Rework this patch to just use the new function.
4. Introduce a function that explicitly does not look through `_Atomic`.
5. Gradually go through the remaining `getUnqualifiedType()` uses and figure out which of the explicit functions they should use, adding tests as necessary.
6. When that's done, decide if we've got compelling evidence that `getUnqualifiedType()` should just default to looking through `_Atomic`.

I think asking Alejandro to do steps 1-3 as part of this bug fix is reasonable, and then the rest can be an open project.

https://github.com/llvm/llvm-project/pull/119711