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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
