This is an archive of the discontinued LLVM Phabricator instance.

[AST] Build recovery expression by default for all language.
ClosedPublic

Authored by hokein on Oct 8 2020, 7:09 AM.

Details

Summary

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.

Diff Detail

Event Timeline

hokein created this revision.Oct 8 2020, 7:09 AM
hokein requested review of this revision.Oct 8 2020, 7:09 AM
hokein updated this revision to Diff 296968.Oct 8 2020, 7:23 AM

fix unexpected format changes.

sammccall added inline comments.Oct 9 2020, 4:24 AM
clang/test/CodeGen/builtins-ppc-error.c
51–52

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?
Do we see the LHS is dependent/contains-errors and then give up on correcting typos in the RHS?
Should we?

hokein updated this revision to Diff 300931.Oct 27 2020, 3:09 AM
hokein marked an inline comment as done.

rebase and address comments.

hokein added inline comments.Oct 27 2020, 3:10 AM
clang/test/CodeGen/builtins-ppc-error.c
51–52

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?

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.

sammccall accepted this revision.Nov 20 2020, 11:42 AM
sammccall added inline comments.
clang/test/CodeGen/builtins-ppc-error.c
51–52

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.

clang/test/Sema/__try.c
55

Yes, this isn't critical and we were just getting a bit lucky before. No need for changes.

This revision is now accepted and ready to land.Nov 20 2020, 11:42 AM
hokein updated this revision to Diff 306992.Nov 23 2020, 1:49 AM

rebase and update

hokein edited the summary of this revision. (Show Details)Nov 23 2020, 1:51 AM
This revision was landed with ongoing or failed builds.Nov 23 2020, 2:08 AM
This revision was automatically updated to reflect the committed changes.
shafik added a subscriber: shafik.May 15 2023, 8:22 AM

Hello folks, it looks like this PR is linked to a crash bug: https://github.com/llvm/llvm-project/issues/62711

Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2023, 8:22 AM