Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

RecoveryExpr incorrectly generated with MS Extension, causes "cannot compile this l-value expression yet" #47659

Open Quuxplusone opened 3 years ago

Quuxplusone commented 3 years ago
Bugzilla Link PR48690
Status NEW
Importance P normal
Reported by Erich Keane (erich.keane@intel.com)
Reported on 2021-01-07 11:16:11 -0800
Last modified on 2021-01-25 00:52:14 -0800
Version 11.0
Hardware PC Windows NT
CC blitzrakete@gmail.com, dgregor@apple.com, douglas_yung@playstation.sony.com, erik.pilkington@gmail.com, hans@chromium.org, hokein@google.com, llvm-bugs@lists.llvm.org, richard-llvm@metafoo.co.uk, rnk@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

See: https://godbolt.org/z/4E56c3

It seems that despite allowing this as an extension, we're still generating a RecoverExpr.

Quuxplusone commented 3 years ago
See also https://llvm.org/pr20249
The phase ordering between MSVC compatibility recovery and typo correction has
been wrong for a while. MSVC checks should go first, but they don't right now.
Quuxplusone commented 3 years ago

_Bug 48834 has been marked as a duplicate of this bug._

Quuxplusone commented 3 years ago

Hans bisected to https://github.com/llvm/llvm-project/commit/efa9aaad703e6b150980ed1a74b4e7c9da7d85a2 in the other issue:


[clang] Suppress "follow-up" diagnostics on recovery call expressions.

Because of typo-correction, the AST can be transformed, and the transformed
AST is marginally useful for diagnostics purpose, the following
diagnostics usually do harm than good (easily cause confusions).

Given the following code:

```
void abcc();
void test() {
  if (abc());
  // diagnostic 1 (for the typo-correction): the typo is correct to `abcc()`, so the code is treate as `if (abcc())` in AST perspective;
  // diagnostic 2 (for mismatch type): we perform an type-analysis on `if`, discover the type is not match
}
```

The secondary diagnostic "convertable to bool" is likely bogus to users.

The idea is to use RecoveryExpr (clang's dependent mechanism) to preserve the
recovery behavior but suppress all follow-up diagnostics.

Differential Revision: https://reviews.llvm.org/D89946

Quuxplusone commented 3 years ago

Thanks for the report.

I didn't come up with a way to fix that, will look at it further. Since we're close to the release cut (next week), I have reverted it in d972d4c749048531953a16b815e07c67e8455a3b.

Quuxplusone commented 3 years ago

Hi, the test that you added in the revert is failing on targets that don't include X86. Please fix the test, or revert it.

See here for an example failure: http://lab.llvm.org:8011/#/builders/43/builds/2549

Quuxplusone commented 3 years ago

Hi, the test that you added in the revert is failing on targets that don't include X86. Please fix the test, or revert it.

Thanks for noticing me, (hopefully) it should be fixed by c6bd6607bf8abfe259fef6a41e695581a88c88f0.