Page MenuHomePhabricator

[C++20] [P1825] Fix bugs with implicit-move from variables of reference type
ClosedPublic

Authored by Quuxplusone on Mar 19 2021, 11:05 AM.

Details

Summary

Review D88220 turns out to have some pretty severe bugs, but I *think* this patch fixes them.

Paper P1825 is supposed to enable implicit move from "non-volatile objects
and rvalue references to non-volatile object types." Instead, what was committed
seems to have enabled implicit move from "non-volatile things of all kinds,
except that if they're rvalue references then they must also refer to non-volatile
things." In other words, D88220 accidentally enabled implicit move from
lvalue object references (super yikes!) and also from non-object references
(such as references to functions).

These two cases are now fixed and regression-tested.

(For better and worse, D88220 was present in trunk for more than a month before anyone noticed the (very major!) bug — my thanks to @sberg for reporting it! — so we may be able to take our time fixing it, but also it may be another month before someone spots the next bug. I have no strong opinion on the process here.)

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Mar 19 2021, 11:05 AM
Quuxplusone created this revision.
mizvekov added inline comments.Mar 19 2021, 11:51 AM
clang/lib/Sema/SemaStmt.cpp
3095

A drive by fix here would be that we already have a VDType in this context, might as well use it even though original for some reason missed it in this part.

Per @mizvekov, use VDType instead of VD->getType() wherever possible.

Quuxplusone marked an inline comment as done.Mar 19 2021, 12:06 PM
mizvekov added inline comments.Mar 19 2021, 12:17 PM
clang/lib/Sema/SemaStmt.cpp
3095

This whole block is also logically equivalent to the much simpler:

if (VDType.isReferenceType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDType = VDType.getNonReferenceType()
}
if (!VDType.isObjectType() || VDType.isVolatileQualified()) 
  return false;

But you do have to adjust the comments there and adjust the rest to use VDType consistently :)
Also, I think it might be possible to even remove the !VDType.isObjectType() || part from my suggestion above, because it might be the only option left if it is not a reference anyway.

mizvekov added inline comments.Mar 19 2021, 12:27 PM
clang/lib/Sema/SemaStmt.cpp
3095
bool isObjectType() const {
  // C++ [basic.types]p8:
  //   An object type is a (possibly cv-qualified) type that is not a
  //   function type, not a reference type, and not a void type.
  return !isReferenceType() && !isFunctionType() && !isVoidType();
}

So yeah I think you can just make my suggestion be:

if (VDType.isReferenceType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDType = VDType.getNonReferenceType()
}
if (VDType.isVolatileQualified()) 
  return false;
Quuxplusone marked 2 inline comments as done.Mar 19 2021, 1:00 PM
Quuxplusone added inline comments.
clang/lib/Sema/SemaStmt.cpp
3095

Yeah but I reaally don't want to
(1) change the meaning of VDType in the middle of this function (mantra: "one name = one meaning")
(2) get "clever". I don't want to have to think about "Are there any types that are neither object types nor reference types?" (What about function types? What about block types? What about, I dunno, bit-fields?) I want the code to be obviously correct, and also to casewise match exactly what the standard says. It says object or rvalue reference type — let's write code that expresses that wording exactly.

mizvekov added inline comments.Mar 19 2021, 1:12 PM
clang/lib/Sema/SemaStmt.cpp
3095

How about:

QualType VDObjType = VDType;
if (!VDType.isObjectType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDObjType = VDType.getNonReferenceType();
}
if (VDObjType .isVolatileQualified()) 
  return false;

And then s/VDType/VDObjType/ from here on.
I think this expresses the meaning of the standard clearly.

aaronpuchert added inline comments.Mar 19 2021, 5:36 PM
clang/lib/Sema/SemaStmt.cpp
3095

That seems like a sensible simplification, the proposed code is indeed a bit repetitive. I'd go with the original suggestion plus the new variable:

QualType VDNonRefType = VDType;
if (VDType.isReferenceType()) {
    if (!(CESK & CES_AllowRValueReferenceType) || !VDType.isRValueReferenceType())
      return false;
    VDNonRefType = VDType.getNonReferenceType()
}
if (!VDNonRefType.isObjectType() || VDNonRefType.isVolatileQualified()) 
  return false;

or whatever name you find appropriate. Actually it's the type of the DeclRefExpr, isn't it? So maybe DREType?

The initialization might be a bit misleading, an alternative would be to not initialize and have an assignment VDNonRefType = VDType in the else branch instead.

3102

This is probably over the line limit, maybe try to reflow this as suggested.

mizvekov added inline comments.Mar 19 2021, 5:58 PM
clang/lib/Sema/SemaStmt.cpp
3095

@aaronpuchert Yeah that combination is good to me also, and I liked the name you suggested better. So +1 :)

Quuxplusone marked 3 inline comments as done.Mar 19 2021, 6:01 PM
Quuxplusone added inline comments.
clang/lib/Sema/SemaStmt.cpp
3095

Actually it's the type of the DeclRefExpr, isn't it? So maybe DREType?

The fact that we don't know what it is gives me pause, re introducing it. ;) If I were going to introduce a local synonym for VDType.getNonReferenceType() on lines 3105-3108, I guess VDReferencedType would be the best name for it; but I don't think there's any reason to introduce another name here.

aaronpuchert added inline comments.Mar 19 2021, 6:48 PM
clang/lib/Sema/SemaStmt.cpp
3095

I think it's just the expression type. It should be precisely the type of E in Sema::getCopyElisionCandidate. (Since moving doesn't even affect the expression type, just its value category, you can either think of the original expression type or the implicitly moved expression type.)

Why the expression type? Well, in the end we're still returning an expression and not a declaration, even if it's a special kind of expression. That's why we want to know if the expression has non-volatile object type. That's how I would look at it.

sberg added a comment.Mar 23 2021, 6:50 AM

FWIW, LibreOffice make check (which started to consistently fail with D88220) succeeds with this change.

Shrink the code by one line, by introducing another local named variable.
Still hoping for an "accept" here.

aaronpuchert accepted this revision.Mar 23 2021, 8:01 AM

I thought maybe you wanted to follow @mizvekov's proposal to simplify, but I understand you want to stick close to the standard language.

So LGTM.

This revision is now accepted and ready to land.Mar 23 2021, 8:01 AM

Shrink the code by one line, by introducing another local named variable.
Still hoping for an "accept" here.

My two cents:

Functionality-wise I think it is OK, but like I said before, I think the extra repetition / verbosity hurts more than helps.

I think the suggested shortening captures the intention of the standard better: The object itself should be non-volatile, no matter if we have it by value or rvalue reference.

It does not look like to me that the clang code is in general styled to spell the wording that precisely, or else those comments quoting the standard wording would not be of much use.

This revision was landed with ongoing or failed builds.Mar 23 2021, 11:12 AM
This revision was automatically updated to reflect the committed changes.