Resolve FIXME: Look at E, not M

[AtariDreams]

No description provided.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: AtariDreams (AtariDreams)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/85541.diff

1 Files Affected:

• (modified) clang/lib/CodeGen/CGExpr.cpp (+1-3)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 59a7fe8925001c..c826663955fecf 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -273,9 +273,7 @@ pushTemporaryCleanup(CodeGenFunction &CGF, const MaterializeTemporaryExpr *M,
   // Objective-C++ ARC:
   //   If we are binding a reference to a temporary that has ownership, we
   //   need to perform retain/release operations on the temporary.
-  //
-  // FIXME: This should be looking at E, not M.
-  if (auto Lifetime = M->getType().getObjCLifetime()) {
+  if (auto Lifetime = E->getType().getObjCLifetime()) {
     switch (Lifetime) {
     case Qualifiers::OCL_None:
     case Qualifiers::OCL_ExplicitNone:

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[efriedma-quic]

I assume the FIXME is referring to some deeper change here? If it were that easy, the FIXME wouldn't be there in the first place.

CC @zygoloid @rjmccall

[rjmccall]

Well, sometimes we do leave FIXMEs that end up being suprisingly easy to fix. But what FIXME are we talking about here? The patch doesn't remove any FIXMEs, or for that matter include any tests.

[efriedma-quic]

llvm-project/clang/lib/CodeGen/CGExpr.cpp

Line 277 in c0f8be4

// FIXME: This should be looking at E, not M.

[rjmccall]

Hmm. Well, you'd have to ask @zygoloid what he was thinking in r183721/r183859, but I don't think it's correct in general to be looking at E (the MTE sub-expression) for most of the logic here. IIRC, the MTE sub-expression is essentially the initializer for the materialized temporary, so it should be a pr-value, which means that for ARC (and any similar features in the future that we might add with qualifier-specific ownership) it won't have the right type information to set up the destructor for the temporary. That is, M is going to tell us the type of the temporary, but E is only going to tell us the type of the initializer. I assume that's why the proposed patch in this PR ends up ignoring the half of the function that handles the ARC qualifiers. Maybe Richard had some better representation in mind, though.

[zygoloid]

IIRC the issue here is that in C++98, the MTE is in a "weird" place in the AST, because of the different rules governing how and when temporaries are formed. For example, for const int &r = C().a[0];, we'll form an MTE wrapping just the C() expression, converting it to an xvalue, in C++11 onwards. But in C++98, xvalues mostly don't exist, the MTE wraps the whole C().a[0], and you need to call skipRValueSubobjectAdjustments to get the actual expression that initializes the temporary, which I guess is what E is here.

So I think the idea is that E is always the expression that initializes the temporary, and the type of M in C++98 might be some subobject of that, with a type that doesn't reflect properties of the temporary. So we should generally look at properties of E, not M, to understand properties of the temporary object. But in C++11 onwards I think it doesn't matter, because we always materialize immediately.

(That's mostly from memory, I might have got some details wrong.)

[rjmccall]

Hmm. The best solution there is probably to use a consistent representation but introduce some sort of GLValueThatTheStandardAbsurdlyPretendsIsAnRValue (definitely the only and best word for this, ship it) that we can use as the value category. IIRC, something similar happens in C where the standard pretends that function l-values don't exist.