The dependency mechanism for C has been implemented, and we have rolled out this to all internal users, didn't see crashy issues, we consider it is stable enough.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
clang/test/CodeGen/builtins-ppc-error.c | ||
---|---|---|
51 | 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. | |
clang/test/CodeGen/builtins-systemz-zvector-error.c | ||
424 | you may want to make these assertions fuzzier and possibly give a count rather than repeating them, but I guess the owners of these headers would be best to decide | |
clang/test/Index/complete-switch.c | ||
9 | nit: no need for xclang, this is cc1 already I guess this is a crash test (and it also doesn't crash with recovery ast)? | |
clang/test/Sema/__try.c | ||
55 | this seems bad, am I missing something? | |
clang/test/Sema/enum.c | ||
104 | or just move the semicolon to suppress, pretty sure that's not what's being tested here. | |
clang/test/Sema/typo-correction.c | ||
56 | what's up with this change? |
clang/test/CodeGen/builtins-ppc-error.c | ||
---|---|---|
51 | 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'
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? | |
clang/test/Index/complete-switch.c | ||
9 | yes, the crash was caused by accesing a null-like ast node. With recovery ast, it is not true any more. | |
clang/test/Sema/__try.c | ||
55 | 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. | |
clang/test/Sema/typo-correction.c | ||
56 | I'd say this aligns with C++ behavior (the inner typo is not correct in C++ mode neither). The reason why it works here is due to the early typo-correction hack. |
@hubert.reinterpretcast, similar to D78350, could you help to test this patch with your downstream clang? this patch is based on 2c2dc7c392a3f28d4dbec3018e3137d5d4f8c6c8. Thanks!
should be ready to go -- from our internal experiment, we don't see any super crashes.
clang/test/CodeGen/builtins-ppc-error.c | ||
---|---|---|
51 |
OK, that makes sense I guess.
Yep, that definitely looks to be the case. | |
clang/test/Sema/__try.c | ||
55 | Yes, this isn't critical and we were just getting a bit lucky before. No need for changes. |
Hello folks, it looks like this PR is linked to a crash bug: https://github.com/llvm/llvm-project/issues/62711
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.