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

[zequanwu]

Summary

Fix a crash when an expression/statement can have valid start location but invalid end location in some situations. For example: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.1/clang/lib/Sema/SemaExprCXX.cpp#L1536

This confuses CounterCoverageMappingBuilder when popping a region from region
stack as if the end location is a macro or include location.

[zequanwu]

• Handle invalid start location.
• Add a test case.

[zequanwu]

I'm trying to add the test case from: https://github.com/llvm/llvm-project/issues/45481#issuecomment-981028897, but lit test fails in <compare> not found.

[shafik]

In D147073#4228290, @zequanwu wrote:

I'm trying to add the test case from: https://github.com/llvm/llvm-project/issues/45481#issuecomment-981028897, but lit test fails in <compare> not found.

You can just define your own strong_ordering like this test clang/test/SemaCXX/warn-zero-nullptr-cxx20.cpp

[zequanwu]

Add test case from https://github.com/llvm/llvm-project/issues/45481#issuecomment-981028897.

[hans]

I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not?

clang/lib/CodeGen/CoverageMappingGen.cpp

605

nit: the "the" is not needed

clang/test/CoverageMapping/invalid_location.cpp

32

s/NETX/NEXT/ here and below?

[zequanwu]

In D147073#4240017, @hans wrote:

I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not?

Yes.

For 0 ? T<C>{} : T<C>{};, the both branches have valid start location but invalid end location. See comments at https://github.com/llvm/llvm-project/issues/45481#issuecomment-1487267814.

For the std::strong_ordering, I found that DeclRefExpr in the ConditionalOperator's true branch has invalid start and end locations. It might because it's inside a PseudoObjectExpr. Maybe we should simply just skip visiting PseudoObjectExpr, I see that its begin and end location are the same. I'm not familiar with that expression, so, I just handled it by adding checks for validating begin and end locations.

PseudoObjectExpr 0x5555629f3bf0 'const strong_ordering':'const class std::strong_ordering' lvalue
|-BinaryOperator 0x5555629f3bd0 'const strong_ordering':'const class std::strong_ordering' lvalue '<=>'
| |-MemberExpr 0x5555629f3848 'const S':'const struct S' lvalue .value 0x5555629f34d0
| | `-DeclRefExpr 0x5555629f3808 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f31b0 'lhs' 'const MyStruct &'
| `-MemberExpr 0x5555629f3878 'const S':'const struct S' lvalue .value 0x5555629f34d0
|   `-DeclRefExpr 0x5555629f3828 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f3238 'rhs' 'const MyStruct &'
|-OpaqueValueExpr 0x5555629f38a8 'const S':'const struct S' lvalue
| `-MemberExpr 0x5555629f3848 'const S':'const struct S' lvalue .value 0x5555629f34d0
|   `-DeclRefExpr 0x5555629f3808 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f31b0 'lhs' 'const MyStruct &'
|-OpaqueValueExpr 0x5555629f38c0 'const S':'const struct S' lvalue
| `-MemberExpr 0x5555629f3878 'const S':'const struct S' lvalue .value 0x5555629f34d0
|   `-DeclRefExpr 0x5555629f3828 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f3238 'rhs' 'const MyStruct &'
`-ConditionalOperator 0x5555629f3ba0 'const strong_ordering':'const class std::strong_ordering' lvalue
  |-CXXOperatorCallExpr 0x5555629f3970 '_Bool' '==' adl
  | |-ImplicitCastExpr 0x5555629f3958 '_Bool (*)(const S &, const S &)' <FunctionToPointerDecay>
  | | `-DeclRefExpr 0x5555629f38d8 '_Bool (const S &, const S &)' lvalue Function 0x5555629f2a40 'operator==' '_Bool (const S &, const S &)'
  | |-OpaqueValueExpr 0x5555629f38a8 'const S':'const struct S' lvalue
  | | `-MemberExpr 0x5555629f3848 'const S':'const struct S' lvalue .value 0x5555629f34d0
  | |   `-DeclRefExpr 0x5555629f3808 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f31b0 'lhs' 'const MyStruct &'
  | `-OpaqueValueExpr 0x5555629f38c0 'const S':'const struct S' lvalue
  |   `-MemberExpr 0x5555629f3878 'const S':'const struct S' lvalue .value 0x5555629f34d0
  |     `-DeclRefExpr 0x5555629f3828 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f3238 'rhs' 'const MyStruct &'
  |-DeclRefExpr 0x5555629f3b80 'const strong_ordering':'const class std::strong_ordering' lvalue Var 0x5555629a4ed8 'equal' 'const strong_ordering':'const class std::strong_ordering'
  `-ConditionalOperator 0x5555629f3b50 'const strong_ordering':'const class std::strong_ordering' lvalue
    |-CXXOperatorCallExpr 0x5555629f3ab0 '_Bool' '<' adl
    | |-ImplicitCastExpr 0x5555629f3a98 '_Bool (*)(const S &, const S &)' <FunctionToPointerDecay>
    | | `-DeclRefExpr 0x5555629f3a78 '_Bool (const S &, const S &)' lvalue Function 0x5555629f27a0 'operator<' '_Bool (const S &, const S &)'
    | |-OpaqueValueExpr 0x5555629f38a8 'const S':'const struct S' lvalue
    | | `-MemberExpr 0x5555629f3848 'const S':'const struct S' lvalue .value 0x5555629f34d0
    | |   `-DeclRefExpr 0x5555629f3808 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f31b0 'lhs' 'const MyStruct &'
    | `-OpaqueValueExpr 0x5555629f38c0 'const S':'const struct S' lvalue
    |   `-MemberExpr 0x5555629f3878 'const S':'const struct S' lvalue .value 0x5555629f34d0
    |     `-DeclRefExpr 0x5555629f3828 'const MyStruct':'const struct MyStruct' lvalue ParmVar 0x5555629f3238 'rhs' 'const MyStruct &'
    |-DeclRefExpr 0x5555629f3b30 'const strong_ordering':'const class std::strong_ordering' lvalue Var 0x5555629a4c50 'less' 'const strong_ordering':'const class std::strong_ordering'
    `-DeclRefExpr 0x5555629f3ae8 'const strong_ordering':'const class std::strong_ordering' lvalue Var 0x5555629a5388 'greater' 'const strong_ordering':'const class std::strong_ordering'

clang/test/CoverageMapping/invalid_location.cpp

32

These 2 "CHECK-NETX" are misspelled on purpose. Once the FIXME is fixed, we should correct the spelling here and below.

Updated the FIXME.

[zequanwu]

Address comments.

[hans]

In D147073#4240793, @zequanwu wrote:

In D147073#4240017, @hans wrote:

I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not?

Yes.

For 0 ? T<C>{} : T<C>{};, the both branches have valid start location but invalid end location. See comments at https://github.com/llvm/llvm-project/issues/45481#issuecomment-1487267814.

For the std::strong_ordering, I found that DeclRefExpr in the ConditionalOperator's true branch has invalid start and end locations. It might because it's inside a PseudoObjectExpr. Maybe we should simply just skip visiting PseudoObjectExpr, I see that its begin and end location are the same. I'm not familiar with that expression, so, I just handled it by adding checks for validating begin and end locations.

Right, I'm just wondering if it's reasonable that this code should have to handle such situations. (It seems it can't handle it perfectly anyway, since the coverage won't be completely correct.) Maybe Aaron can comment on what the expectations are for these locations.

[aaron.ballman]

In D147073#4242566, @hans wrote:

In D147073#4240793, @zequanwu wrote:

In D147073#4240017, @hans wrote:

I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not?

Yes.

For 0 ? T<C>{} : T<C>{};, the both branches have valid start location but invalid end location. See comments at https://github.com/llvm/llvm-project/issues/45481#issuecomment-1487267814.

For the std::strong_ordering, I found that DeclRefExpr in the ConditionalOperator's true branch has invalid start and end locations. It might because it's inside a PseudoObjectExpr. Maybe we should simply just skip visiting PseudoObjectExpr, I see that its begin and end location are the same. I'm not familiar with that expression, so, I just handled it by adding checks for validating begin and end locations.

Right, I'm just wondering if it's reasonable that this code should have to handle such situations. (It seems it can't handle it perfectly anyway, since the coverage won't be completely correct.) Maybe Aaron can comment on what the expectations are for these locations.

PseudoObjectExpr is a strange little beast in that it's an abstract object in the AST. It gets used when there's is a particular syntax that is modelled directly by the language as a series of expressions (e.g., writing foo->x = 12; in the source, but due to some language feature like __declspec(property), the semantics are foo->setX(12); as a member function call instead). So it has multiple source locations that are of interest and you have to know what information you're after -- the syntactic location or one of the semantic locations. I'm not super familiar with the code coverage machinery, but it looks like it's walking over the semantic expressions rather than the syntactic one, and that seems a bit fishy to me because I'd assume that coverage is intended to describe what the user actually wrote in their source. So I don't think I'd skip the PseudoObjectExpr, but you might need to handle it differently in CounterCoverageMappingBuilder by adding a VisitPseudoObjectExpr() method and not walking over *all* of the children, but only the syntactic form of the expression.

[zequanwu]

Split to another patch: D147569.

[hans]

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)

[aaron.ballman]

In 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 resolved the issue and so this patch is no longer needed?

[zequanwu]

In D147073#4258396, @aaron.ballman wrote:

In 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 resolved the issue and so this patch is no longer needed?

D147569 fixes https://github.com/llvm/llvm-project/issues/45481, which is caused by not visiting PseudoObjectExpr. That makes the visitor visits the semantic expression of PseudoObjectExpr which doesn't have source location.

This one fixes another issue crbug.com/1427933, which is caused by some expressions don't have source location due to some work around [1]. Their stack traces look similar but not caused by the same issue.

Updated the link in summary to: [1]

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

[aaron.ballman]

In D147073#4258426, @zequanwu wrote:

In D147073#4258396, @aaron.ballman wrote:

In 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 resolved the issue and so this patch is no longer needed?

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

[zequanwu]

In D147073#4258529, @aaron.ballman wrote:

In D147073#4258426, @zequanwu wrote:

In D147073#4258396, @aaron.ballman wrote:

In 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 resolved the issue and so this patch is no longer needed?

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

[aaron.ballman]

In D147073#4258664, @zequanwu wrote:

In D147073#4258529, @aaron.ballman wrote:

In D147073#4258426, @zequanwu wrote:

In D147073#4258396, @aaron.ballman wrote:

In 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 resolved the issue and so this patch is no longer needed?

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?

[zequanwu]

In D147073#4258981, @aaron.ballman wrote:

In D147073#4258664, @zequanwu wrote:

In D147073#4258529, @aaron.ballman wrote:

In D147073#4258426, @zequanwu wrote:

In D147073#4258396, @aaron.ballman wrote:

In 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 resolved the issue and so this patch is no longer needed?

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

[aaron.ballman]

In D147073#4261633, @zequanwu wrote:

In D147073#4258981, @aaron.ballman wrote:

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?

Yes, it will, but that's the goal in this case. I think we want the loud failure to tell us when we've

I don't quite understand "do the right thing in non-asserts builds instead of crashing".

For example:

// If either of these locations is invalid, something elsewhere in the compiler has broken...
assert(StartLoc && StartLoc->isInvalid() && "Start location is not valid");
assert(EndLoc && EndLoc->isInvalid() && "End location is not valid");

// ... however, we can still recover without crashing.
if (StartLoc && StartLoc->isInvalid())
  StartLoc = std::nullopt;
if (EndLoc && EndLoc->isInvalid())
  EndLoc = std::nullopt;

[zequanwu]

• Add assertion on source locations in pushRegion.
• Removed test case because that would cause the new assertion failure in assert build.

[aaron.ballman]

Thank you! I think there's only a few small things left then:

• Add a release note about the fact that we worked around this issue in non-asserts builds but that assert builds may see new assertions triggered from this and to file an issue with a reproducer if you hit the assertion.
• File an issue with a simple reproducer for the case you know we're still not handling properly
• Update the commit message to clarify that we're no longer fixing anything with these changes but are instead trying to catch the issues are more loudly than with a crash

[zequanwu]

• Filed an github issue: https://github.com/llvm/llvm-project/issues/62105
• Added release note.
• Updated commit message and summary.

[aaron.ballman]

LGTM!

[MaskRay]

clang/lib/CodeGen/CoverageMappingGen.cpp

607

Is this workaround still needed after b0e61de7075942ef5ac8af9ca1ec918317f62152 (with a test clang/test/Coverage/unresolved-ctor-expr.cpp)?

[zequanwu]

clang/lib/CodeGen/CoverageMappingGen.cpp

607

I think this workaround can be removed, verified it no longer crashes if reverting this change.

clang/lib/CodeGen/CoverageMappingGen.cpp

size_t pushRegion(Counter Count,
                     std::optional<SourceLocation> StartLoc = std::nullopt,
                     std::optional<SourceLocation> EndLoc = std::nullopt,
                     std::optional<Counter> FalseCount = std::nullopt) {
 
     if (StartLoc && !FalseCount) {
       MostRecentLocation = *StartLoc;
     }
 
+    // If either location is invalid, set it to std::nullopt to avoid
+    // letting users of RegionStack think that region has a valid start/end
+    // location.
+    if (StartLoc && StartLoc->isInvalid())
+      StartLoc = std::nullopt;
+    if (EndLoc && EndLoc->isInvalid())
+      EndLoc = std::nullopt;
     RegionStack.emplace_back(Count, FalseCount, StartLoc, EndLoc);
 
     return RegionStack.size() - 1;
   }
 
   size_t locationDepth(SourceLocation Loc) {
     size_t Depth = 0;
     while (Loc.isValid()) {
       Loc = getIncludeOrExpansionLoc(Loc);
       Depth++;
     }
     return Depth;
   }
 
   /// Pop regions from the stack into the function's list of regions.
   ///
   /// Adds all regions from \c ParentIndex to the top of the stack to the
   /// function's \c SourceRegions.
   void popRegions(size_t ParentIndex) {
     assert(RegionStack.size() >= ParentIndex && "parent not in stack");
     while (RegionStack.size() > ParentIndex) {
       SourceMappingRegion &Region = RegionStack.back();
-      if (Region.hasStartLoc()) {
+      if (Region.hasStartLoc() &&
+          (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
         SourceLocation StartLoc = Region.getBeginLoc();
         SourceLocation EndLoc = Region.hasEndLoc()
                                     ? Region.getEndLoc()
                                     : RegionStack[ParentIndex].getEndLoc();
         bool isBranch = Region.isBranch();
         size_t StartDepth = locationDepth(StartLoc);
         size_t EndDepth = locationDepth(EndLoc);
         while (!SM.isWrittenInSameFile(StartLoc, EndLoc)) {
while (RegionStack.size() > ParentIndex) {
           if (StartLoc == getStartOfFileOrMacro(StartLoc) &&
               EndLoc == getEndOfFileOrMacro(EndLoc))
             MostRecentLocation = getIncludeOrExpansionLoc(EndLoc);
         }
 
         assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
         assert(SpellingRegion(SM, Region).isInSourceOrder());
         SourceRegions.push_back(Region);
-        }
       RegionStack.pop_back();
     }
   }
 
   /// Return the currently active region.
   SourceMappingRegion &getRegion() {
     assert(!RegionStack.empty() && "statement has no region");
     return RegionStack.back();
if (!Region.hasEndLoc())
       Region.setEndLoc(EndLoc);
     pushRegion(Counter::getZero());
     HasTerminateStmt = true;
   }
 
   /// Find a valid gap range between \p AfterLoc and \p BeforeLoc.
   std::optional<SourceRange> findGapAreaBetween(SourceLocation AfterLoc,
                                                 SourceLocation BeforeLoc) {
+    if (AfterLoc.isInvalid() || BeforeLoc.isInvalid())
+      return std::nullopt;
     // If AfterLoc is in function-like macro, use the right parenthesis
     // location.
     if (AfterLoc.isMacroID()) {
       FileID FID = SM.getFileID(AfterLoc);
       const SrcMgr::ExpansionInfo *EI = &SM.getSLocEntry(FID).getExpansion();
       if (EI->isFunctionMacroExpansion())
         AfterLoc = EI->getExpansionLocEnd();
     }

clang/test/CoverageMapping/invalid_location.cpp

+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -std=c++20 -stdlib=libc++ -triple %itanium_abi_triple %s | FileCheck %s
+
+namespace std { template <typename> class initializer_list {}; }
+
+template <typename> struct T {
+  T(std::initializer_list<int>, int = int());
+  bool b;
+};
+
+template <typename> struct S1 {
+  static void foo() {
+    class C;
+    0 ? T<C>{} : T<C>{};
+    0 ? 1 : 2;
+  }
+};
+
+void bar() {
+  S1<int>::foo();
+}
+
+
+// CHECK:      _ZN2S1IiE3fooEv:
+// CHECK-NEXT:   File 0, 11:21 -> 15:4 = #0
+// CHECK-NEXT:   File 0, 13:5 -> 13:6 = #0
+// CHECK-NEXT:   Branch,File 0, 13:5 -> 13:6 = 0, 0
+// CHECK-NEXT:   Gap,File 0, 13:8 -> 13:9 = #1
+// FIXME: It's supposed to have two different counters for true and false
+//        branches at line 13 as it has for line 14, shown below. It's missing
+//        now because 'T<C>{}' doesn't have a valid end location for now.
+//        Once it's fixed, correct the following two "CHECK-NETX" and #3 and #2
+//        may need to swap positions.
+// CHECK-NETX:   File 0, 13:9 -> 13:14 = #3
+// CHECK-NETX:   File 0, 13:18 -> 13:23 = (#0 - #3)
+
+// CHECK-NEXT:   File 0, 14:5 -> 14:6 = #0
+// CHECK-NEXT:   Branch,File 0, 14:5 -> 14:6 = 0, 0
+// CHECK-NEXT:   Gap,File 0, 14:8 -> 14:9 = #2
+// CHECK-NEXT:   File 0, 14:9 -> 14:10 = #2
+// CHECK-NEXT:   File 0, 14:13 -> 14:14 = (#0 - #2)