[PATCH] Coverage mapping: Don't map implicit casts and modify "ignore if not extended" flag semantics

[Justin Bogner]

Alex L <arphaman at gmail.com> writes:
> It's ensuring that macros like `define N 1024` don't create mapping regions
> when the integer literal
> is implicitly converted. It also makes sure that literals in ternary
> conditional create mapping regions
> - e.g. there will be a mapping region for 1 and 0 in `cond? 1 : 0` .

Thanks. Please explain that in the commit message when you commit. LGTM.

> 2014-08-20 23:49 GMT-07:00 Justin Bogner <mail at justinbogner.com>:
>
>     Alex L <arphaman at gmail.com> writes:
>     > These two patches remove the mapping for implicit cast expression and
>     > modify the "ignore if not extended" flag to be "ignore if file only
>     > has this region". I'm sending them out together as they both partially
>     > influence a common testcase.
>    
>     Why? What is this accomplishing? As far as I can tell one of these
>     patches removes a CHECK line and the other adds back the exact same
>     check. I guess this changes how that particular construct is modelled?
>     If so, it might be better to just make this one patch so it's clear that
>     you're not removing functionality, but changing how it's done.
>
>
> diff --git a/lib/CodeGen/CoverageMappingGen.cpp b/lib/CodeGen/CoverageMappingGen.cpp
> index 1bfa7cd..0570cd0 100644
> --- a/lib/CodeGen/CoverageMappingGen.cpp
> +++ b/lib/CodeGen/CoverageMappingGen.cpp
> @@ -34,11 +34,12 @@ namespace {
>  /// \brief A region of source code that can be mapped to a counter.
>  struct SourceMappingRegion {
>    enum RegionFlags {
> -    /// \brief This region won't be emitted if it wasn't extended.
> +    /// \brief This region won't be emitted if the "file" that contains this
> +    /// region consists only of this region.
>      /// This is useful so that we won't emit source ranges for single tokens
>      /// that we don't really care that much about, like:
>      ///   the '(' token in #define MACRO (
> -    IgnoreIfNotExtended = 0x0001,
> +    IgnoreIfFileOnlyHasThisRegion = 0x0001,
>    };
>  
>    FileID File, MacroArgumentFile;
> @@ -99,8 +100,8 @@ struct SourceMappingRegion {
>    void mergeByExtendingTo(SourceMappingRegion &R) {
>      LocEnd = R.LocEnd;
>      AlternativeLocEnd = R.LocStart;
> -    if (hasFlag(IgnoreIfNotExtended))
> -      clearFlag(IgnoreIfNotExtended);
> +    if (hasFlag(IgnoreIfFileOnlyHasThisRegion))
> +      clearFlag(IgnoreIfFileOnlyHasThisRegion);
>    }
>  };
>  
> @@ -381,14 +382,31 @@ public:
>    /// \brief Generate the coverage counter mapping regions from collected
>    /// source regions.
>    void emitSourceRegions() {
> +    struct FileRegionInfo {
> +      bool HasMultipleRegions;
> +
> +      FileRegionInfo() : HasMultipleRegions(false) {}
> +    };
> +
> +    llvm::SmallDenseMap<FileID, FileRegionInfo, 8> FileRegionInfoMapping;
> +
> +    for (const auto &R : SourceRegions) {
> +      auto It = FileRegionInfoMapping.find(R.File);
> +      if (It == FileRegionInfoMapping.end()) {
> +        FileRegionInfoMapping.insert(std::make_pair(R.File, FileRegionInfo()));
> +        continue;
> +      }
> +      It->second.HasMultipleRegions = true;
> +    }
> +
>      for (const auto &R : SourceRegions) {
>        SourceLocation LocStart = R.LocStart;
>        SourceLocation LocEnd = R.LocEnd;
>        if (SM.getFileID(LocEnd) != R.File)
>          LocEnd = R.AlternativeLocEnd;
>  
> -      if (R.hasFlag(SourceMappingRegion::IgnoreIfNotExtended) &&
> -          LocStart == LocEnd)
> +      if (R.hasFlag(SourceMappingRegion::IgnoreIfFileOnlyHasThisRegion) &&
> +          !FileRegionInfoMapping[R.File].HasMultipleRegions)
>          continue;
>  
>        LocEnd = getPreciseTokenLocEnd(LocEnd);
> @@ -627,12 +645,13 @@ struct CounterCoverageMappingBuilder
>    void mapToken(SourceLocation LocStart) {
>      CoverageMappingBuilder::mapSourceCodeRange(
>          LocStart, LocStart, CurrentRegionCount,
> -        SourceMappingRegion::IgnoreIfNotExtended);
> +        SourceMappingRegion::IgnoreIfFileOnlyHasThisRegion);
>    }
>  
>    void mapToken(const SourceMappingState &State, SourceLocation LocStart) {
>      CoverageMappingBuilder::mapSourceCodeRange(
> -        State, LocStart, LocStart, SourceMappingRegion::IgnoreIfNotExtended);
> +        State, LocStart, LocStart,
> +        SourceMappingRegion::IgnoreIfFileOnlyHasThisRegion);
>    }
>  
>    void VisitStmt(const Stmt *S) {
> @@ -1021,6 +1040,10 @@ struct CounterCoverageMappingBuilder
>      Visit(E->getSubExpr());
>    }
>  
> +  void VisitImplicitCastExpr(const ImplicitCastExpr *E) {
> +    Visit(E->getSubExpr());
> +  }
> +
>    // Map literals as tokens so that the macros like #define PI 3.14
>    // won't generate coverage mapping regions.
>  
> diff --git a/test/CoverageMapping/casts.c b/test/CoverageMapping/casts.c
> index 94c13dc..ef08c19 100644
> --- a/test/CoverageMapping/casts.c
> +++ b/test/CoverageMapping/casts.c
> @@ -6,6 +6,13 @@ int main() {                                                   // CHECK: File 0,
>    return 0;
>  }
>  
> +#define N 32
>  
> +              // CHECK: foo
> +void foo() {  // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+4]]:2 = #0 (HasCodeBefore = 0)
> +  char c = N;
> +  if (0) {    // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+1]]:4 = #1 (HasCodeBefore = 0)
> +  }
> +}
>  
>  
> diff --git a/test/CoverageMapping/exprregions.c b/test/CoverageMapping/exprregions.c
> new file mode 100644
> index 0000000..c62cc07
> --- /dev/null
> +++ b/test/CoverageMapping/exprregions.c
> @@ -0,0 +1,8 @@
> +// RUN: %clang_cc1 -fprofile-instr-generate -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name exprregions.c %s | FileCheck %s
> +
> +int main() {    // CHECK: File 0, [[@LINE]]:12 -> [[@LINE+5]]:2 = #0 (HasCodeBefore = 0)
> +  int x =   0 ?
> +            1 : // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:14 = #1 (HasCodeBefore = 0)
> +            0;  // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:14 = (#0 - #1) (HasCodeBefore = 0)
> +  return 0;
> +}