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