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

Tue Apr 11 09:42:55 PDT 2023

aaron.ballman added a comment.

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.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073