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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

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

> In D147073#4258664 <https://reviews.llvm.org/D147073#4258664>, @zequanwu wrote:
>
>> 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
>
> Ugh, that workaround is unfortunate.
>
> You are correct that this same situation may come up in the future, but each time it happens, I think it's a bug in the FE that needs to be addressed. I worry that silencing the issues here will 1) lead to the actual bug staying in the code base forever, and 2) be used as justification for making the same workaround elsewhere in the code base, perhaps so often that it becomes "the way things are supposed to work".
>
> Perhaps a way to split the middle would be to assert that the source locations are valid in coverage mapping, but then do the right thing in non-asserts builds instead of crashing. This way, we don't lose the benefit of knowing the issues happen in development builds, but we don't punish users of coverage mapping with the released product. WDYT?

Won't this still cause assertion failure on assert builds? I don't quite understand "do the right thing in non-asserts builds instead of crashing".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073