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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:607
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not 
valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");

MaskRay wrote:
> Is this workaround still needed after 
> b0e61de7075942ef5ac8af9ca1ec918317f62152 (with a test 
> `clang/test/Coverage/unresolved-ctor-expr.cpp`)?
I think this workaround can be removed, verified it no longer crashes if 
reverting this change.


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:607
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not 
valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");

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


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

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

[Zequan Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0529da5b948c: [Coverage] Handle invalid end location of an 
expression/statement. (authored by zequanwu).

Changed prior to commit:
  https://reviews.llvm.org/D147073?vs=512971=513236#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CoverageMappingGen.cpp


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,19 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If either of these locations is invalid, something elsewhere in the
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not 
valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");
+
+// However, we can still recover without crashing.
+// 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;
@@ -624,7 +637,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +705,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -297,6 +297,10 @@
   not a type concept.
 - Fix crash when a doc comment contains a line splicing.
   (`#62054 `_)
+- Work around with a clang coverage crash which happens when visiting 
+  expressions/statements with invalid source locations in non-assert builds. 
+  Assert builds may still see assertions triggered from this.
+  (`#62105 `_)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,19 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If either of these locations is invalid, something elsewhere in the
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");
+
+// However, we can still recover without crashing.
+// 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;
@@ -624,7 +637,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +705,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -297,6 +297,10 @@
   not a type concept.
 - Fix crash when a doc comment contains a 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 512971.
zequanwu edited the summary of this revision.
zequanwu added a comment.

- Filed an github issue:
- Added release note.
- Updated commit message and summary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CoverageMappingGen.cpp


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,19 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If either of these locations is invalid, something elsewhere in the
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not 
valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");
+
+// However, we can still recover without crashing.
+// 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;
@@ -624,7 +637,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +705,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -248,6 +248,10 @@
 - Fix false-positive diagnostic issued for consteval initializers of temporary
   objects.
   (`#60286 `_)
+- Work around with a clang coverage crash which happens when visiting 
+  expressions/statements with invalid source locations in non-assert builds. 
+  Assert builds may still see assertions triggered from this.
+  (`#62105 `_)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,19 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If either of these locations is invalid, something elsewhere in the
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");
+
+// However, we can still recover without crashing.
+// 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;
@@ -624,7 +637,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +705,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -248,6 +248,10 @@
 - Fix false-positive diagnostic issued for consteval initializers of temporary
   objects.
   (`#60286 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 512868.
zequanwu added a comment.

Add assertion on source locations in `pushRegion`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,19 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If either of these locations is invalid, something elsewhere in the
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not 
valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");
+
+// However, we can still recover without crashing.
+// 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;
@@ -624,7 +637,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +705,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,19 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If either of these locations is invalid, something elsewhere in the
+// compiler has broken.
+assert((!StartLoc || StartLoc->isValid()) && "Start location is not valid");
+assert((!EndLoc || EndLoc->isValid()) && "End location is not valid");
+
+// However, we can still recover without crashing.
+// 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;
@@ -624,7 +637,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +705,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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;


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

[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 , @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".


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

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


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

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


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?


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

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

[Hans Wennborg via Phabricator via cfe-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

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)


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 510938.
zequanwu added a comment.

Split to another patch: D147569 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/invalid_location.cpp


Index: clang/test/CoverageMapping/invalid_location.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/invalid_location.cpp
@@ -0,0 +1,40 @@
+// 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  class initializer_list {}; }
+
+template  struct T {
+  T(std::initializer_list, int = int());
+  bool b;
+};
+
+template  struct S1 {
+  static void foo() {
+class C;
+0 ? T{} : T{};
+0 ? 1 : 2;
+  }
+};
+
+void bar() {
+  S1::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{}' 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)
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,13 @@
   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;
@@ -624,7 +631,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +699,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
@@ -891,6 +899,8 @@
   /// Find a valid gap range between \p AfterLoc and \p BeforeLoc.
   std::optional 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()) {


Index: clang/test/CoverageMapping/invalid_location.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/invalid_location.cpp
@@ -0,0 +1,40 @@
+// 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  class initializer_list {}; }
+
+template  struct T {
+  T(std::initializer_list, int = int());
+  bool b;
+};
+
+template  struct S1 {
+  static void foo() {
+class C;
+0 ? T{} : T{};
+0 ? 1 : 2;
+  }
+};
+
+void bar() {
+  S1::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
+//

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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{} : T{};`, 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.


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

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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{} : T{};`, 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.


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 510514.
zequanwu added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/invalid_location.cpp

Index: clang/test/CoverageMapping/invalid_location.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/invalid_location.cpp
@@ -0,0 +1,92 @@
+// 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  class initializer_list {}; }
+
+template  struct T {
+  T(std::initializer_list, int = int());
+  bool b;
+};
+
+template  struct S1 {
+  static void foo() {
+class C;
+0 ? T{} : T{};
+0 ? 1 : 2;
+  }
+};
+
+void bar() {
+  S1::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{}' 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)
+
+// No crash for following example.
+// See https://github.com/llvm/llvm-project/issues/45481
+namespace std {
+class strong_ordering;
+
+// Mock how STD defined unspecified parameters for the operators below.
+struct _CmpUnspecifiedParam {
+  consteval
+  _CmpUnspecifiedParam(int _CmpUnspecifiedParam::*) noexcept {}
+};
+
+struct strong_ordering {
+  signed char value;
+
+  friend constexpr bool operator==(strong_ordering v,
+   _CmpUnspecifiedParam) noexcept {
+return v.value == 0;
+  }
+  friend constexpr bool operator<(strong_ordering v,
+  _CmpUnspecifiedParam) noexcept {
+return v.value < 0;
+  }
+  friend constexpr bool operator>(strong_ordering v,
+  _CmpUnspecifiedParam) noexcept {
+return v.value > 0;
+  }
+  friend constexpr bool operator>=(strong_ordering v,
+   _CmpUnspecifiedParam) noexcept {
+return v.value >= 0;
+  }
+  static const strong_ordering equal, greater, less;
+};
+constexpr strong_ordering strong_ordering::equal = {0};
+constexpr strong_ordering strong_ordering::greater = {1};
+constexpr strong_ordering strong_ordering::less = {-1};
+} // namespace std
+
+struct S {
+friend bool operator<(const S&, const S&);
+friend bool operator==(const S&, const S&);
+};
+
+struct MyStruct {
+friend bool operator==(MyStruct const& lhs, MyStruct const& rhs) = delete;
+friend std::strong_ordering operator<=>(MyStruct const& lhs, MyStruct const& rhs) = default;
+S value;
+};
+
+void foo(MyStruct bar){
+(void)(bar <=> bar);
+}
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,13 @@
   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;
@@ -624,7 +631,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +699,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
  

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu marked an inline comment as done.
zequanwu added a comment.

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{} : T{};`, 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 0x629f3bf0 'const strong_ordering':'const class 
std::strong_ordering' lvalue
  |-BinaryOperator 0x629f3bd0 'const strong_ordering':'const class 
std::strong_ordering' lvalue '<=>'
  | |-MemberExpr 0x629f3848 'const S':'const struct S' lvalue .value 
0x629f34d0
  | | `-DeclRefExpr 0x629f3808 'const MyStruct':'const struct MyStruct' 
lvalue ParmVar 0x629f31b0 'lhs' 'const MyStruct &'
  | `-MemberExpr 0x629f3878 'const S':'const struct S' lvalue .value 
0x629f34d0
  |   `-DeclRefExpr 0x629f3828 'const MyStruct':'const struct MyStruct' 
lvalue ParmVar 0x629f3238 'rhs' 'const MyStruct &'
  |-OpaqueValueExpr 0x629f38a8 'const S':'const struct S' lvalue
  | `-MemberExpr 0x629f3848 'const S':'const struct S' lvalue .value 
0x629f34d0
  |   `-DeclRefExpr 0x629f3808 'const MyStruct':'const struct MyStruct' 
lvalue ParmVar 0x629f31b0 'lhs' 'const MyStruct &'
  |-OpaqueValueExpr 0x629f38c0 'const S':'const struct S' lvalue
  | `-MemberExpr 0x629f3878 'const S':'const struct S' lvalue .value 
0x629f34d0
  |   `-DeclRefExpr 0x629f3828 'const MyStruct':'const struct MyStruct' 
lvalue ParmVar 0x629f3238 'rhs' 'const MyStruct &'
  `-ConditionalOperator 0x629f3ba0 'const strong_ordering':'const class 
std::strong_ordering' lvalue
|-CXXOperatorCallExpr 0x629f3970 '_Bool' '==' adl
| |-ImplicitCastExpr 0x629f3958 '_Bool (*)(const S &, const S &)' 

| | `-DeclRefExpr 0x629f38d8 '_Bool (const S &, const S &)' lvalue 
Function 0x629f2a40 'operator==' '_Bool (const S &, const S &)'
| |-OpaqueValueExpr 0x629f38a8 'const S':'const struct S' lvalue
| | `-MemberExpr 0x629f3848 'const S':'const struct S' lvalue .value 
0x629f34d0
| |   `-DeclRefExpr 0x629f3808 'const MyStruct':'const struct MyStruct' 
lvalue ParmVar 0x629f31b0 'lhs' 'const MyStruct &'
| `-OpaqueValueExpr 0x629f38c0 'const S':'const struct S' lvalue
|   `-MemberExpr 0x629f3878 'const S':'const struct S' lvalue .value 
0x629f34d0
| `-DeclRefExpr 0x629f3828 'const MyStruct':'const struct MyStruct' 
lvalue ParmVar 0x629f3238 'rhs' 'const MyStruct &'
|-DeclRefExpr 0x629f3b80 'const strong_ordering':'const class 
std::strong_ordering' lvalue Var 0x629a4ed8 'equal' 'const 
strong_ordering':'const class std::strong_ordering'
`-ConditionalOperator 0x629f3b50 'const strong_ordering':'const class 
std::strong_ordering' lvalue
  |-CXXOperatorCallExpr 0x629f3ab0 '_Bool' '<' adl
  | |-ImplicitCastExpr 0x629f3a98 '_Bool (*)(const S &, const S &)' 

  | | `-DeclRefExpr 0x629f3a78 '_Bool (const S &, const S &)' lvalue 
Function 0x629f27a0 'operator<' '_Bool (const S &, const S &)'
  | |-OpaqueValueExpr 0x629f38a8 'const S':'const struct S' lvalue
  | | `-MemberExpr 0x629f3848 'const S':'const struct S' lvalue .value 
0x629f34d0
  | |   `-DeclRefExpr 0x629f3808 'const MyStruct':'const struct 
MyStruct' lvalue ParmVar 0x629f31b0 'lhs' 'const MyStruct &'
  | `-OpaqueValueExpr 0x629f38c0 'const S':'const struct S' lvalue
  |   `-MemberExpr 0x629f3878 'const S':'const struct S' lvalue .value 
0x629f34d0
  | `-DeclRefExpr 0x629f3828 'const MyStruct':'const struct 
MyStruct' lvalue ParmVar 0x629f3238 'rhs' 'const MyStruct &'
  |-DeclRefExpr 0x629f3b30 'const strong_ordering':'const class 
std::strong_ordering' lvalue Var 0x629a4c50 'less' 'const 
strong_ordering':'const class std::strong_ordering'
  `-DeclRefExpr 0x629f3ae8 'const strong_ordering':'const class 
std::strong_ordering' lvalue Var 0x629a5388 'greater' 'const 
strong_ordering':'const class std::strong_ordering'




Comment at: clang/test/CoverageMapping/invalid_location.cpp:31
+//now because 'T{}' doesn't have a valid end location for now.
+// CHECK-NETX:   File 0, 13:9 -> 13:14 = #3
+// CHECK-NETX:   File 0, 13:18 -> 13:23 = (#0 - #3)

hans 

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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?




Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:605
 
+// If the either location is invalid, set it to std::nullopt to avoid
+// letting users of RegionStack think that region has a valid start/end

nit: the "the" is not needed



Comment at: clang/test/CoverageMapping/invalid_location.cpp:31
+//now because 'T{}' doesn't have a valid end location for now.
+// CHECK-NETX:   File 0, 13:9 -> 13:14 = #3
+// CHECK-NETX:   File 0, 13:18 -> 13:23 = (#0 - #3)

s/NETX/NEXT/ here and below?


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 509802.
zequanwu added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/invalid_location.cpp

Index: clang/test/CoverageMapping/invalid_location.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/invalid_location.cpp
@@ -0,0 +1,90 @@
+// 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  class initializer_list {}; }
+
+template  struct T {
+  T(std::initializer_list, int = int());
+  bool b;
+};
+
+template  struct S1 {
+  static void foo() {
+class C;
+0 ? T{} : T{};
+0 ? 1 : 2;
+  }
+};
+
+void bar() {
+  S1::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{}' doesn't have a valid end location for now.
+// 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)
+
+// No crash for following example.
+// See https://github.com/llvm/llvm-project/issues/45481
+namespace std {
+class strong_ordering;
+
+// Mock how STD defined unspecified parameters for the operators below.
+struct _CmpUnspecifiedParam {
+  consteval
+  _CmpUnspecifiedParam(int _CmpUnspecifiedParam::*) noexcept {}
+};
+
+struct strong_ordering {
+  signed char value;
+
+  friend constexpr bool operator==(strong_ordering v,
+   _CmpUnspecifiedParam) noexcept {
+return v.value == 0;
+  }
+  friend constexpr bool operator<(strong_ordering v,
+  _CmpUnspecifiedParam) noexcept {
+return v.value < 0;
+  }
+  friend constexpr bool operator>(strong_ordering v,
+  _CmpUnspecifiedParam) noexcept {
+return v.value > 0;
+  }
+  friend constexpr bool operator>=(strong_ordering v,
+   _CmpUnspecifiedParam) noexcept {
+return v.value >= 0;
+  }
+  static const strong_ordering equal, greater, less;
+};
+constexpr strong_ordering strong_ordering::equal = {0};
+constexpr strong_ordering strong_ordering::greater = {1};
+constexpr strong_ordering strong_ordering::less = {-1};
+} // namespace std
+
+struct S {
+friend bool operator<(const S&, const S&);
+friend bool operator==(const S&, const S&);
+};
+
+struct MyStruct {
+friend bool operator==(MyStruct const& lhs, MyStruct const& rhs) = delete;
+friend std::strong_ordering operator<=>(MyStruct const& lhs, MyStruct const& rhs) = default;
+S value;
+};
+
+void foo(MyStruct bar){
+(void)(bar <=> bar);
+}
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,13 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If the 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;
@@ -624,7 +631,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +699,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

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 `` not found.

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


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

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


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

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 509098.
zequanwu added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147073

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/invalid_location.cpp


Index: clang/test/CoverageMapping/invalid_location.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/invalid_location.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -std=c++20 -triple %itanium_abi_triple %s | FileCheck %s
+
+namespace std { template  class initializer_list {}; }
+
+template  struct T {
+  T(std::initializer_list, int = int());
+  bool b;
+};
+
+template  struct S1 {
+  static void foo() {
+class C;
+0 ? T{} : T{};
+0 ? 1 : 2;
+  }
+};
+
+void bar() {
+  S1::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{}' doesn't have a valid end location for now.
+// 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)
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,13 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If the 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;
@@ -624,7 +631,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +699,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
@@ -716,7 +724,8 @@
 
 // The statement may be spanned by an expansion. Make sure we handle a file
 // exit out of this expansion before moving to the next statement.
-if (SM.isBeforeInTranslationUnit(StartLoc, S->getBeginLoc()))
+if (StartLoc.isValid() &&
+SM.isBeforeInTranslationUnit(StartLoc, S->getBeginLoc()))
   MostRecentLocation = EndLoc;
 
 return ExitCount;
@@ -891,6 +900,8 @@
   /// Find a valid gap range between \p AfterLoc and \p BeforeLoc.
   std::optional 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()) {


Index: clang/test/CoverageMapping/invalid_location.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/invalid_location.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -std=c++20 -triple %itanium_abi_triple %s | FileCheck %s
+
+namespace std { template  class initializer_list {}; }
+
+template  struct T {
+  T(std::initializer_list, int = int());
+  bool b;
+};
+
+template  struct S1 {
+  static void foo() {
+class C;
+0 ? T{} : T{};
+0 ? 1 : 2;
+  }
+};
+
+void bar() {
+  S1::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 

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu created this revision.
zequanwu added reviewers: hans, shafik, rsmith, MaggieYi, gulfem.
Herald added a project: All.
zequanwu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

https://github.com/llvm/llvm-project/issues/45481 exposes an issue that an
expression/statement can have valid start location but invalid end location in
some situations.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147073

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,13 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If the 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;
@@ -624,7 +631,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +699,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }


Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -602,6 +602,13 @@
   MostRecentLocation = *StartLoc;
 }
 
+// If the 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;
@@ -624,7 +631,8 @@
 assert(RegionStack.size() >= ParentIndex && "parent not in stack");
 while (RegionStack.size() > ParentIndex) {
   SourceMappingRegion  = RegionStack.back();
-  if (Region.hasStartLoc()) {
+  if (Region.hasStartLoc() &&
+  (Region.hasEndLoc() || RegionStack[ParentIndex].hasEndLoc())) {
 SourceLocation StartLoc = Region.getBeginLoc();
 SourceLocation EndLoc = Region.hasEndLoc()
 ? Region.getEndLoc()
@@ -691,7 +699,7 @@
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
 assert(SpellingRegion(SM, Region).isInSourceOrder());
 SourceRegions.push_back(Region);
-}
+  }
   RegionStack.pop_back();
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits