[PATCH] D147073: [Coverage] Handle invalid end location of an expression/statement.

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D147073#4258529 <https://reviews.llvm.org/D147073#4258529>, @aaron.ballman wrote:

> In D147073#4258426 <https://reviews.llvm.org/D147073#4258426>, @zequanwu wrote:
>
>> In D147073#4258396 <https://reviews.llvm.org/D147073#4258396>, @aaron.ballman wrote:
>>
>>> In D147073#4258384 <https://reviews.llvm.org/D147073#4258384>, @hans wrote:
>>>
>>>> Again not an expert here, but lgtm.
>>>>
>>>> (Nit: the https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExprCXX.cpp#L1528-L1530 link in the description seems to point to the wrong code now, since main changed. Here is a link for 16.0.1: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.1/clang/lib/Sema/SemaExprCXX.cpp#L1536)
>>>
>>> I'm confused -- I thought D147569 <https://reviews.llvm.org/D147569> resolved the issue and so this patch is no longer needed?
>>
>> D147569 <https://reviews.llvm.org/D147569> fixes https://github.com/llvm/llvm-project/issues/45481. This one fixes another issue crbug.com/1427933. Their stack trace look similar but not caused by the same issue.
>>
>> Updated the link in summary to: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.1/clang/lib/Sema/SemaExprCXX.cpp#L1536
>
> Thank you for clarifying, I was confused. :-)
>
> I don't think the changes here are correct either -- it's glossing over an issue that we're not properly tracking the source location in the AST. Playing around with the reduced example is interesting though. If you remove the default argument in the `T` constructor, the issue goes away. If you stop using a forward declaration in the instantiation of `T`, the issue goes away. If `S1` isn't a template, the issue goes away (but the issue will come back if you then make `foo()` a template instead of `S1`). So it seems that something about template instantiation is dropping the source location information (perhaps) and we should be trying to track down where that is to fix the root cause rather than work around it here for coverage mapping alone. (The end location being incorrect can impact other things that are harder to test because it'll be for things like fix-its that don't work properly, which are easy to miss.)

I agree that the real fix is to fix the source location information. If I just change [1] to `SourceRange Locs = SourceRange(LParenOrBraceLoc, RParenOrBraceLoc);`, the crash is gone. However, as the "FIXME" comment in [1] says, it's an intended work-around to drop source locations here. So, I'm assuming this kind of source location work-around could happen in multiple places in clang, and could happen in the future as a temporary work-around. Instead of crashing clang coverage for those work-around, we can at least skip coverage info for those expressions/statements.

[1]: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.1/clang/lib/Sema/SemaExprCXX.cpp#L1538


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073