[PATCH] D89046: [AST] Build recovery expression by default for all language.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/test/CodeGen/builtins-ppc-error.c:51
 void testCTF(int index) {
-  vec_ctf(vsi, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}}
-  vec_ctf(vui, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}}
+  vec_ctf(vsi, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}} expected-error {{argument to '__builtin_altivec_vcfux' must be a constant integer}}
+  vec_ctf(vui, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}} expected-error {{argument to '__builtin_altivec_vcfux' must be a constant integer}}
----------------
hokein wrote:
> sammccall wrote:
> > hmm, this doesn't exactly look right to me - we know the type of `vsi` so we should only be considering the signed case I thought.
> > 
> > However the existing diagnostic for `vui` indicates that it's considering the **signed** case, so I guess this is already broken/bad.
> yeah, we should know this is the signed case.
> 
> after the macro expansion, the code looks like 
> 
> ```
> _Generic((vsi), vector int
>            : (vector float)__builtin_altivec_vcfsx((vector int)(vsi), (index)), vector unsigned int
>            : (vector float)__builtin_altivec_vcfux((vector unsigned int)(vsi), (index))); 
> ```
> 
> it is a `GenericSelectionExpr` which contains a switch-like structure (see the AST below), so when we traverse it, we will traverse both `__builtin_altivec_vcfsx` and `__builtin_altivec_vcfux`.
> 
> ```
> `-GenericSelectionExpr 0x9527238 <line:31:3, line:33:88> '__vector float' contains-errors
>     |-ImplicitCastExpr 0x9527220 <line:31:12, col:16> '__vector int' <LValueToRValue>
>     | `-ParenExpr 0x9526980 <col:12, col:16> '__vector int' lvalue
>     |   `-DeclRefExpr 0x9526960 <col:13> '__vector int' lvalue Var 0x9526568 'vsi' '__vector int' non_odr_use_unevaluated
>     |-VectorType 0x91923c0 '__vector int' altivec 4
>     | `-BuiltinType 0x91296e0 'int'
>     |-case  '__vector int' selected
>     | |-VectorType 0x91923c0 '__vector int' altivec 4
>     | | `-BuiltinType 0x91296e0 'int'
>     | `-CStyleCastExpr 0x9526db8 <line:32:14, col:78> '__vector float' contains-errors <Dependent>
>     |   `-RecoveryExpr 0x9526d70 <col:28, col:78> '<dependent type>' contains-errors lvalue
>     |     |-DeclRefExpr 0x9526bc0 <col:28> '<builtin fn type>' Function 0x95269e8 '__builtin_altivec_vcfsx' '__attribute__((__vector_size__(4 * sizeof(float)))) float (__attribute__((__vector_size__(4 * sizeof(int)))) int, int)'
>     |     |-CStyleCastExpr 0x9526c68 <col:52, col:68> '__vector int' <BitCast>
>     |     | `-ImplicitCastExpr 0x9526c50 <col:64, col:68> '__vector int' <LValueToRValue> part_of_explicit_cast
>     |     |   `-ParenExpr 0x9526c30 <col:64, col:68> '__vector int' lvalue
>     |     |     `-DeclRefExpr 0x9526be0 <col:65> '__vector int' lvalue Var 0x9526568 'vsi' '__vector int'
>     |     `-ParenExpr 0x9526cb0 <col:71, col:77> 'const int' lvalue
>     |       `-DeclRefExpr 0x9526c90 <col:72> 'const int' lvalue ParmVar 0x95267c8 'index' 'const int'
>     `-case  '__vector unsigned int'
>       |-VectorType 0x9192700 '__vector unsigned int' altivec 4
>       | `-BuiltinType 0x9129780 'unsigned int'
>       `-CStyleCastExpr 0x95271f8 <line:33:14, col:87> '__vector float' contains-errors <Dependent>
>         `-RecoveryExpr 0x95271b0 <col:28, col:87> '<dependent type>' contains-errors lvalue
>           |-DeclRefExpr 0x9527000 <col:28> '<builtin fn type>' Function 0x9526e28 '__builtin_altivec_vcfux' '__attribute__((__vector_size__(4 * sizeof(float)))) float (__attribute__((__vector_size__(4 * sizeof(unsigned int)))) unsigned int, int)'
>           |-CStyleCastExpr 0x95270a8 <col:52, col:77> '__vector unsigned int' <BitCast>
>           | `-ImplicitCastExpr 0x9527090 <col:73, col:77> '__vector int' <LValueToRValue> part_of_explicit_cast
>           |   `-ParenExpr 0x9527070 <col:73, col:77> '__vector int' lvalue
>           |     `-DeclRefExpr 0x9527020 <col:74> '__vector int' lvalue Var 0x9526568 'vsi' '__vector int'
>           `-ParenExpr 0x95270f0 <col:80, col:86> 'const int' lvalue
>             `-DeclRefExpr 0x95270d0 <col:81> 'const int' lvalue ParmVar 0x95267c8 'index' 'const int'
> ```
> 
> 
> > However the existing diagnostic for vui indicates that it's considering the signed case, so I guess this is already broken/bad.
> 
> hmm, `vsi` and `vui` have the same type `vector signed int`, so I think there is a typo for `vui`, it should be `vector unsigned int`?
> it is a GenericSelectionExpr which contains a switch-like structure (see the AST below), so when we traverse it, we will traverse both __builtin_altivec_vcfsx and __builtin_altivec_vcfux.

OK, that makes sense I guess.

> I think there is a typo for vui, it should be vector unsigned int?

Yep, that definitely looks to be the case.


================
Comment at: clang/test/Sema/__try.c:55
+    // expected-error{{too few arguments to function call, expected 1, have 0}} \
+    // expected-error{{expected ';' after expression}}
   }
----------------
hokein wrote:
> sammccall wrote:
> > this seems bad, am I missing something?
> agree, `__except` makes it very confusing intuitively. 
> 
> what's happening? - while at here, there is no preceding `__try`, clang parses `__execept` as a function call to an implicit function, so it expects a `;` after ` __except (FilterExpression());`
> 
> why don't we hit this before? -- because the `FilterExpression()` is invalid (missing an argument), and the whole function call expr is being dropped. With recovery-ast, we preserve the whole function call, then we emit the `;` diagnostic.
> 
> not quite sure on how to make it clearer. I think this is not super critical.
> 
> 
Yes, this isn't critical and we were just getting a bit lucky before. No need for changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89046