[cfe-dev] Do we intentionally 'transform' non-dependent expressions?

[Keane, Erich via cfe-dev]

> But AtomicExpr is "special", because when parsing it, it's just a CallExpr, but it becomes a different kind of AST node after semantic analysis. I think we need to either decompose an AtomicExpr back into a CallExpr, or factor out the checking and building code for atomic builtins into a Build* function that TreeTransform can call, in order to be able to rebuild it properly.

I am currently trying to create an overload of SemaAtomicOpsOverloaded that doesn’t depend on it being a CallExpr to do exactly this.  I likely should rename it BuildAtomicOpsOverloaded or something before review for consistency however.  Sadly, AtomicExpr is still going to be a special case, since it doesn’t have an ActOn or anything either.

From: Richard Smith <richard at metafoo.co.uk>
Sent: Friday, September 20, 2019 10:31 AM
To: Keane, Erich <erich.keane at intel.com>
Cc: Clang Dev <cfe-dev at lists.llvm.org>
Subject: Re: [cfe-dev] Do we intentionally 'transform' non-dependent expressions?

On Fri, 20 Sep 2019, 10:39 Keane, Erich via cfe-dev, <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:
> It is necessary in some cases (including this one): we need to rewrite the reference to 'v' to refer to the instantiated variable rather than the variable in the template.

I see, thank you. Then this was the wrong path here.  I can see how that would break in the case of magic-statics in a template.
Not just for static locals; we need the declarations named in DeclRefExprs to be correct in general so that we know what they're referencing. (Also, TreeTransform is used in non-instantiation contexts that can require rebuilding arbitrary expressions.)
It seems here then that the solution is more AtomicExpr specific than this.  The solution in CodeGen is actually quite trivial (change a getPointeeType to a getPointeeOrArrayElementType), but it seems to me the problem should be solved way before then (somewhere in Sema to properly get the AST).
Generally, TreeTransform needs to be able to recreate arbitrary expressions and statements. The semantic analysis for building an expression, including forming suitable conversions, should be done by the Sema::Build* functions that are called by both TreeTransform and the Sema::ActOn* functions that the parser calls, ensuring that you get the same behaviour from parsing an expression and instantiating it.

But AtomicExpr is "special", because when parsing it, it's just a CallExpr, but it becomes a different kind of AST node after semantic analysis. I think we need to either decompose an AtomicExpr back into a CallExpr, or factor out the checking and building code for atomic builtins into a Build* function that TreeTransform can call, in order to be able to rebuild it properly.
Thanks!
-Erich

From: Richard Smith <richard at metafoo.co.uk<mailto:richard at metafoo.co.uk>>
Sent: Friday, September 20, 2019 9:32 AM
To: Keane, Erich <erich.keane at intel.com<mailto:erich.keane at intel.com>>
Cc: Clang Dev <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>>
Subject: Re: [cfe-dev] Do we intentionally 'transform' non-dependent expressions?

On Fri, 20 Sep 2019, 10:14 Keane, Erich via cfe-dev, <cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>> wrote:
I decided to investigate https://bugs.llvm.org/show_bug.cgi?id=43370

First, this code is in a dependent context (but is not dependent itself):
uint64_t v[2];
    __atomic_store_n(v, 0, 0);

The function template declaration in the AST shows the first parameter as having an ArrayToPointerDecay implicit cast.

However, the AtomicExpr transform calls “TransformExpr” on this, which then strips off all implicit casts. The expression then is NOT checked later, since it was already checked the first time.  If the expression is dependent, it gets properly checked, and the cast is added back in.

It seems to me that transforming a dependent expression should be unnecessary at best, and a risk of crashing at worst (due to running out of Transform stack space).  SO, I put a test in the beginning of TransformExpr to just return the expr.
It is necessary in some cases (including this one): we need to rewrite the reference to 'v' to refer to the instantiated variable rather than the variable in the template.
However, check-clang resulted in 600+ test failures.  I evaluated a couple, and all seem to be because they are re-checking a statement that doesn’t need to be (since it isn’t dependent!).  There is unfortunately no good way it seems to determine whether a Stmt is dependent, so I can’t find anything to put into TransformStmt to skip like before.  So, I think I will have to manage these tests 1 by 1.

Before I deep-dive into this, I’d like to hear whether I’m missing something obvious and would just be wasting my time?  Does anyone know whether there is a good reason to still transform non-dependent expressions?

Thanks,
Erich

_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org<mailto:cfe-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190920/2eda0a98/attachment.html>