[PATCH] D105756: [clang] C++98 implicit moves are back with a vengeance

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D105756#2990495 <https://reviews.llvm.org/D105756#2990495>, @mizvekov wrote:

> Hello! Thanks for reporting this.

Thanks for fixing!

In D105756#2990495 <https://reviews.llvm.org/D105756#2990495>, @mizvekov wrote:

> The double implicit lvalue-to-rvalue cast is an unfortunate consequence of some technical debt around the different ways we handle the implicit return type on blocks.
> But I don't think it's incorrect as in it actually producing program with different semantics.
> Does it actually affect you in your use cases besides the AST? Is this important to you for external semantic analysis reasons or something?

I guess I'm confused about the exact meaning of the AST. I'm not sure whether it's actively incorrect or just changed meaning. I'll definitely be debugging this further.

My backstory here is that the static analyzer can be thought of as (primitivized down to) an AST "interpreter". It arranges AST nodes in the order in which they're "executed" at run-time (aka "Clang CFG") and then processes them one node at a time in that order - unlike CodeGen that processes the nodes more or less recursively. This causes us static analyzer people to have an exotic taste for the good AST as we're required to understand every node literally in isolation from other nodes. Every time the nodes make sense together/recursively but not individually our life becomes much harder.

So in the new AST (which wouldn't exist with your fix so we may be talking about vacuous truths here, as I'm still to look at the C++ variant of the same AST) I'm confused about the seemingly brand-new AST node that you introduced, `ImplicitCastExpr xvalue <NoOp>` - a cast that changes value kind from prvalue to glvalue and in this regard appears to behave exactly like `MaterializeTemporaryExpr` - which was hard enough to support the first time (due to its deep recursive nature where it extracts the storage address that was already "lost" in the operand), I really don't want to do it again :) And I'm also confused by the situation where `ImplicitCastExpr <LValueToRValue>` does not correspond to a memory load like it typically does.

If I understand correctly, in C++ the innermost lvalue-to-rvalue cast wouldn't exist, therefore the double memory load concern will be eliminated. I.e., it'll probably be something like

  ReturnStmt
  `- ImplicitCastExpr <LValueToRValue>
     `- ImplicitCastExpr <NoOp> xvalue  // MaterializeTemporaryExpr xvalue?
        `- CXXConstructExpr

which makes perfect sense to me (assuming `MaterializeTemporaryExpr` is actually used). I'm assuming the old AST would be

  ReturnStmt
  `- CXXConstructExpr

which also makes perfect sense. So if my understanding is correct then we're probably mostly good.

In D105756#2990495 <https://reviews.llvm.org/D105756#2990495>, @mizvekov wrote:

> The nrvo is a separate thing, and again I thin it's harmless, or at least a little beneficial.

I definitely don't mind it.

In D105756#2990495 <https://reviews.llvm.org/D105756#2990495>, @mizvekov wrote:

> Removing the xvalue cast on non CplusPlus is simple, the bigger issue here is why we are doing this, specially as it will affect what test cases we create for this change.
> If this is an issue, is it serious enough to consider getting the fix into 13.0.0 at this stage?

I think your current fix is sufficient. It eliminates the known crash and we're defended against xvalues in C. I'll see what I can do to investigate this further.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105756/new/

https://reviews.llvm.org/D105756