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

[Aaron Ballman 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();
----------------
AaronBallman 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 (heck, they can even have different sizes and alignments!). 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`.

> 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. 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.

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?

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