[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.
> 
> Maybe I'm being imprecise, sorry for that. `const int` and `int` are both "morally" of type `int` in that the allowed accesses to either type will give the same code generation and behavior. There's no difference between read accesses to `int` and `const int`: https://godbolt.org/z/GzhKWr8W6

That is mostly a special case of `const`. Even `volatile` requires code-gen differences, at least when generating a high-level IR subject to optimization.

> > > (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.
> 
> Do you have an example of impacting the ABI rules in C?

If you mean qualifiers officially blessed in any way by WG14, I believe that's just `const`, `volatile`, `restrict`, and the address-space qualifiers from TR 18037.  Address space qualifiers do affect ABI, but they don't specifically affect struct layout because they are not allowed on struct members.

> Loading an int to get its value is a different operation than loading an atomic int to get its value; the latter may require entirely different instruction selection. Probably `int` is a bad example, perhaps `struct` values makes the distinction more clear?

Needing different instruction selection is common for extended qualifiers. Even among the official qualifiers, address spaces typically require different instruction selection.

> Another notable exception is that `_Atomic` is not silently dropped at the top level in type for return values and parameters when determining the function type, as mentioned above.

It actually is dropped for return types; it's just parameters that are different.

> > 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.
> 
> Doesn't it matter in the same way it matters for `const` parameters: it impacts the accesses to the parameter within the function body itself?

Sure, but it is not in any way necessary for this to be part of the function type, just as it isn't part of the function type for `const`. The parameter is an independent object and has no relation to anything in the caller, so making the atomic-ness part of the signature is pointless.
> > 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).
> > ```
> 
> I think we've already done this though. We have `getAtomicUnqualifiedType()` which we use when we want to get a non-atomic, unqualified type.
> 
> > ```
> > 3. Rework this patch to just use the new function.
> > 
> > 4. Introduce a function that explicitly does not look through `_Atomic`.
> > ```
> 
> Which is `getUnqualifiedType()` currently.

Right, it would have the same behavior as `getUnqualifiedType()` (and presumably would be implemented as a call to it). Making it a separate function lets you keep track of the fact that you considered a caller and deliberately did not want it to look through `_Atomic`, though.

> > 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.
> 
> So I think what I'm getting is: we should audit all existing calls to `getUnqualifiedType()` and `getAtomicUnqualifiedType()` to determine whether we're correctly handling atomics or not, and if there are more uses of `getAtomicUnqualifiedType()` after correcting issues, then maybe we want `getUnqualifiedType()` to look through atomics and rename `getAtomicUnqualifiedType()` to something else?

Right, with the tweak that the audit *always* switches away from `getUnqualifiedType()`: each call either becomes `getAtomicUnqualifiedType()` (I forgot that this already exists) or `getUnqualifiedTypePreservingAtomic()`, and then the audit is done when there are no remaining calls to `getUnqualifiedType()`. At that point, we decide if having a `getUnqualifiedType()` is a good idea or if we should just force all callers to explicitly choose.

And as mentioned, there's a short-term subset of the plan that unblocks this fix.

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