[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Vedant Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG99317124e1c7: [Coverage] Revise format to reduce binary size 
(authored by vsk).

Changed prior to commit:
  https://reviews.llvm.org/D69471?vs=245756=247396#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69471

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/abspath.cpp
  clang/test/CoverageMapping/ir.c
  clang/test/Profile/def-assignop.cpp
  clang/test/Profile/def-ctors.cpp
  clang/test/Profile/def-dtors.cpp
  compiler-rt/include/profile/InstrProfData.inc
  llvm/docs/CoverageMappingFormat.rst
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingWriter.h
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfData.inc
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
  llvm/test/tools/llvm-cov/Inputs/binary-formats.v3.macho64l
  llvm/test/tools/llvm-cov/binary-formats.c
  llvm/unittests/ProfileData/CoverageMappingTest.cpp

Index: llvm/unittests/ProfileData/CoverageMappingTest.cpp
===
--- llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -895,4 +895,29 @@
   std::pair({true, false}),
   std::pair({true, true})),);
 
+TEST(CoverageMappingTest, filename_roundtrip) {
+  std::vector Paths({"a", "b", "c", "d", "e"});
+
+  for (bool Compress : {false, true}) {
+std::string EncodedFilenames;
+{
+  raw_string_ostream OS(EncodedFilenames);
+  CoverageFilenamesSectionWriter Writer(Paths);
+  Writer.write(OS, Compress);
+}
+
+std::vector ReadFilenames;
+RawCoverageFilenamesReader Reader(EncodedFilenames, ReadFilenames);
+BinaryCoverageReader::DecompressedData Decompressed;
+EXPECT_THAT_ERROR(Reader.read(CovMapVersion::CurrentVersion, Decompressed),
+  Succeeded());
+if (!Compress)
+  ASSERT_EQ(Decompressed.size(), 0U);
+
+ASSERT_EQ(ReadFilenames.size(), Paths.size());
+for (unsigned I = 0; I < Paths.size(); ++I)
+  ASSERT_TRUE(ReadFilenames[I] == Paths[I]);
+  }
+}
+
 } // end anonymous namespace
Index: llvm/test/tools/llvm-cov/binary-formats.c
===
--- llvm/test/tools/llvm-cov/binary-formats.c
+++ llvm/test/tools/llvm-cov/binary-formats.c
@@ -7,5 +7,6 @@
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32b -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
+// RUN: llvm-cov show %S/Inputs/binary-formats.v3.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 
 // RUN: llvm-cov export %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata | FileCheck %S/Inputs/binary-formats.canonical.json
Index: llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
===
--- llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
+++ llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
@@ -1,6 +1,8 @@
 ;; Ensure that SHF_ALLOC section flag is not set for the __llvm_covmap section on Linux.
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
 
+@covfun = linkonce_odr hidden constant i32 0, section "__llvm_covfun"
 @__llvm_coverage_mapping = internal constant i32 0, section "__llvm_covmap"
 
+; CHECK-DAG: .section	__llvm_covfun,""
 ; CHECK-DAG: .section	__llvm_covmap,""
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -74,10 +74,6 @@
 
 namespace {
 
-cl::opt DoNameCompression("enable-name-compression",
-cl::desc("Enable name string compression"),
-cl::init(true));
-
 cl::opt DoHashBasedCounterSplit(
 "hash-based-counter-split",
 cl::desc("Rename counter variable of a comdat function based on cfg hash"),
@@ -973,7 +969,7 @@
 
   

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.

Sounds like a plan, thanks!


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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Vedant Kumar via Phabricator via cfe-commits]

vsk updated this revision to Diff 245756.
vsk added a comment.
This revision is now accepted and ready to land.

Get rid of multiple inheritance in the coverage::accessors namespace.


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

https://reviews.llvm.org/D69471

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/abspath.cpp
  clang/test/CoverageMapping/ir.c
  clang/test/Profile/def-assignop.cpp
  clang/test/Profile/def-ctors.cpp
  clang/test/Profile/def-dtors.cpp
  compiler-rt/include/profile/InstrProfData.inc
  llvm/docs/CoverageMappingFormat.rst
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingWriter.h
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfData.inc
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
  llvm/test/tools/llvm-cov/Inputs/binary-formats.v3.macho64l
  llvm/test/tools/llvm-cov/binary-formats.c
  llvm/unittests/ProfileData/CoverageMappingTest.cpp

Index: llvm/unittests/ProfileData/CoverageMappingTest.cpp
===
--- llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -895,4 +895,29 @@
   std::pair({true, false}),
   std::pair({true, true})),);
 
+TEST(CoverageMappingTest, filename_roundtrip) {
+  std::vector Paths({"a", "b", "c", "d", "e"});
+
+  for (bool Compress : {false, true}) {
+std::string EncodedFilenames;
+{
+  raw_string_ostream OS(EncodedFilenames);
+  CoverageFilenamesSectionWriter Writer(Paths);
+  Writer.write(OS, Compress);
+}
+
+std::vector ReadFilenames;
+RawCoverageFilenamesReader Reader(EncodedFilenames, ReadFilenames);
+BinaryCoverageReader::DecompressedData Decompressed;
+EXPECT_THAT_ERROR(Reader.read(CovMapVersion::CurrentVersion, Decompressed),
+  Succeeded());
+if (!Compress)
+  ASSERT_EQ(Decompressed.size(), 0U);
+
+ASSERT_EQ(ReadFilenames.size(), Paths.size());
+for (unsigned I = 0; I < Paths.size(); ++I)
+  ASSERT_TRUE(ReadFilenames[I] == Paths[I]);
+  }
+}
+
 } // end anonymous namespace
Index: llvm/test/tools/llvm-cov/binary-formats.c
===
--- llvm/test/tools/llvm-cov/binary-formats.c
+++ llvm/test/tools/llvm-cov/binary-formats.c
@@ -7,5 +7,6 @@
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32b -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
+// RUN: llvm-cov show %S/Inputs/binary-formats.v3.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 
 // RUN: llvm-cov export %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata | FileCheck %S/Inputs/binary-formats.canonical.json
Index: llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
===
--- llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
+++ llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
@@ -1,6 +1,8 @@
 ;; Ensure that SHF_ALLOC section flag is not set for the __llvm_covmap section on Linux.
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
 
+@covfun = linkonce_odr hidden constant i32 0, section "__llvm_covfun"
 @__llvm_coverage_mapping = internal constant i32 0, section "__llvm_covmap"
 
+; CHECK-DAG: .section	__llvm_covfun,""
 ; CHECK-DAG: .section	__llvm_covmap,""
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -74,10 +74,6 @@
 
 namespace {
 
-cl::opt DoNameCompression("enable-name-compression",
-cl::desc("Enable name string compression"),
-cl::init(true));
-
 cl::opt DoHashBasedCounterSplit(
 "hash-based-counter-split",
 cl::desc("Rename counter variable of a comdat function based on cfg hash"),
@@ -973,7 +969,7 @@
 
   std::string CompressedNameStr;
   if (Error E = collectPGOFuncNameStrings(ReferencedNames, CompressedNameStr,

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Vedant Kumar via Phabricator via cfe-commits]

vsk planned changes to this revision.
vsk added a comment.

@rnk Thanks for chasing this down. I'll update the function record structs to 
use free functions instead of multiple inheritance.

I don't plan on getting rid of the awkward method calls at this point. The 
coverage reader is still templated by CovMapFunctionRecordX via CovMapTraits, 
so we'd need to untangle that first.


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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D69471#1884043 , @dexonsmith wrote:

> In D69471#1883912 , @rnk wrote:
>
> > Everything is off-by-one because the empty bases are not zero sized. The 
> > MSVC record layout algorithm is just different in this area. =/
>
>
> Do all the MSVCs we support building with support `__declspec(empty_bases)`?
>  
> https://devblogs.microsoft.com/cppblog/optimizing-the-layout-of-empty-base-classes-in-vs2015-update-2-3/


Yes, we can use it. Clang supports it too.

---

However, you might want this class to follow the rules for a standard layout 
type:
http://www.cplusplus.com/reference/type_traits/is_standard_layout/
" ...  has  at most one base class with non-static data members, or has no 
base classes with non-static data members."
So the use of multiple inheritance makes a type non-standard layout. Nevermind 
that MSVC accepts the following program:

  #include 
  struct EmptyA {};
  struct EmptyB {};
  struct Foo : EmptyA, EmptyB { int x, y; };
  static_assert(std::is_standard_layout::value, "asdf");

Why not use free function templates like these?

  template  uint64_t getFoo(T *p) { return p->Foo; }
  template  uint64_t getBar(T *p) { return p->Bar; }

These accessors don't seem like they have to be methods. It actually makes them 
a bit more awkward to use:

  uint32_t DataSize = CFR->template getDataSize();


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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D69471#1883912 , @rnk wrote:

> Everything is off-by-one because the empty bases are not zero sized. The MSVC 
> record layout algorithm is just different in this area. =/


Do all the MSVCs we support building with support `__declspec(empty_bases)`?
https://devblogs.microsoft.com/cppblog/optimizing-the-layout-of-empty-base-classes-in-vs2015-update-2-3/

> So, I think this patch would be fine if you refactor it to avoid the accessor 
> classes. I took a stab at it, but it's not straightforward.

Another option is to chain the accessor classes:

  template  struct MyAccessor1 : Base { /* ... */ };
  template  struct MyAccessor1 { /* ... */ };
  template  struct MyAccessor2 : Base { /* ... */ };
  template  struct MyAccessor2 { /* ... */ };

Then you can:

  struct MyFormat : MyAccessor1> { /* ... */ };


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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Compiling with -fdump-record-layouts revealed the problem:

  *** Dumping AST Record Layout
   0 | struct llvm::coverage::CovMapFunctionRecordV3
   0 |   struct llvm::coverage::accessors::FuncHashAndDataSize (base) (empty)
   1 |   struct llvm::coverage::accessors::HashedNameRef (base) (empty)
   1 |   const int64_t NameRef
   9 |   const uint32_t DataSize
  13 |   const uint64_t FuncHash
  21 |   const uint64_t FilenamesRef
  29 |   const char CoverageMapping
 | [sizeof=30, align=1,
 |  nvsize=30, nvalign=1]

Everything is off-by-one because the empty bases are not zero sized. The MSVC 
record layout algorithm is just different in this area. =/

So, I think this patch would be fine if you refactor it to avoid the accessor 
classes. I took a stab at it, but it's not straightforward.


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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Vedant Kumar via Phabricator via cfe-commits]

vsk updated this revision to Diff 244790.
vsk added a comment.

Rebase.

Sorry this hasn't seen any update: at this point in our release cycle I've had 
to put this on the back burner.


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

https://reviews.llvm.org/D69471

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/abspath.cpp
  clang/test/CoverageMapping/ir.c
  clang/test/Profile/def-assignop.cpp
  clang/test/Profile/def-ctors.cpp
  clang/test/Profile/def-dtors.cpp
  compiler-rt/include/profile/InstrProfData.inc
  llvm/docs/CoverageMappingFormat.rst
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingWriter.h
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfData.inc
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
  llvm/test/tools/llvm-cov/Inputs/binary-formats.v3.macho64l
  llvm/test/tools/llvm-cov/binary-formats.c
  llvm/unittests/ProfileData/CoverageMappingTest.cpp

Index: llvm/unittests/ProfileData/CoverageMappingTest.cpp
===
--- llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -895,4 +895,29 @@
   std::pair({true, false}),
   std::pair({true, true})),);
 
+TEST(CoverageMappingTest, filename_roundtrip) {
+  std::vector Paths({"a", "b", "c", "d", "e"});
+
+  for (bool Compress : {false, true}) {
+std::string EncodedFilenames;
+{
+  raw_string_ostream OS(EncodedFilenames);
+  CoverageFilenamesSectionWriter Writer(Paths);
+  Writer.write(OS, Compress);
+}
+
+std::vector ReadFilenames;
+RawCoverageFilenamesReader Reader(EncodedFilenames, ReadFilenames);
+BinaryCoverageReader::DecompressedData Decompressed;
+EXPECT_THAT_ERROR(Reader.read(CovMapVersion::CurrentVersion, Decompressed),
+  Succeeded());
+if (!Compress)
+  ASSERT_EQ(Decompressed.size(), 0U);
+
+ASSERT_EQ(ReadFilenames.size(), Paths.size());
+for (unsigned I = 0; I < Paths.size(); ++I)
+  ASSERT_TRUE(ReadFilenames[I] == Paths[I]);
+  }
+}
+
 } // end anonymous namespace
Index: llvm/test/tools/llvm-cov/binary-formats.c
===
--- llvm/test/tools/llvm-cov/binary-formats.c
+++ llvm/test/tools/llvm-cov/binary-formats.c
@@ -7,5 +7,6 @@
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32b -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
+// RUN: llvm-cov show %S/Inputs/binary-formats.v3.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 
 // RUN: llvm-cov export %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata | FileCheck %S/Inputs/binary-formats.canonical.json
Index: llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
===
--- llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
+++ llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
@@ -1,6 +1,8 @@
 ;; Ensure that SHF_ALLOC section flag is not set for the __llvm_covmap section on Linux.
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
 
+@covfun = linkonce_odr hidden constant i32 0, section "__llvm_covfun"
 @__llvm_coverage_mapping = internal constant i32 0, section "__llvm_covmap"
 
+; CHECK-DAG: .section	__llvm_covfun,""
 ; CHECK-DAG: .section	__llvm_covmap,""
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -74,10 +74,6 @@
 
 namespace {
 
-cl::opt DoNameCompression("enable-name-compression",
-cl::desc("Enable name string compression"),
-cl::init(true));
-
 cl::opt DoHashBasedCounterSplit(
 "hash-based-counter-split",
 cl::desc("Rename counter variable of a comdat function based on cfg hash"),
@@ -973,7 +969,7 @@
 
   std::string CompressedNameStr;
   if (Error E = collectPGOFuncNameStrings(ReferencedNames, 

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I haven't forgotten about this, even though it's been two months. Hopefully I 
get to it soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I have a Windows build directory and am motivated to debug this. I'll try to do 
it tomorrow, but I have a couple of other deadlines so I can't make a very firm 
promise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Sorry for the delay here. An update: I tested this patch with a sanitized build 
on a Linux install, but could not reproduce the "malformed coverage data" 
errors seen on the Windows bots. At this point, it seems likely that there is a 
bug in CoverageMappingReader (this would explain why the .covmapping testing 
bundles fail to parse), and it may be that there is a separate issue on the 
producer side (this could explain why the 'instrprof-merging.cpp' test from 
`check-profile` failed). I think I need a Windows set up to debug further, and 
will try to find one. Meanwhile, if anyone has the bandwidth to attempt to 
reproduce on Windows, and to share a backtrace from where the 
`CoverageMapError` constructor is called, I would greatly appreciate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Vedant Kumar via Phabricator via cfe-commits]

vsk reopened this revision.
vsk marked an inline comment as done.
vsk added a comment.
This revision is now accepted and ready to land.

I reverted this due to failures on Windows that I did not encounter in local 
testing. I suspect that there's an error in the coverage parsing logic, as the 
same binary coverage data parses successfully locally, but not on some armv7 
bots.




Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:53-56
+Error E =
+zlib::compress(FilenamesStr, CompressedStr, zlib::BestSizeCompression);
+if (E)
+  llvm_unreachable("Unexpected failure in zlib::compress");

efriedma wrote:
> vsk wrote:
> > rnk wrote:
> > > I think the shorter pattern for this is `cantFail(zlib::compress(...))`.
> > Both sound reasonable, I'll pick the shorter spelling.
> cantFail and report_bad_alloc_error have substantially different semantics. 
> In particular, for non-Asserts builds, cantFail is a no-op, which doesn't 
> seem appropriate for an API that fails on out-of-memory.
I see, I'll revise this to use report_bad_alloc_error in the next update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:53-56
+Error E =
+zlib::compress(FilenamesStr, CompressedStr, zlib::BestSizeCompression);
+if (E)
+  llvm_unreachable("Unexpected failure in zlib::compress");

vsk wrote:
> rnk wrote:
> > I think the shorter pattern for this is `cantFail(zlib::compress(...))`.
> Both sound reasonable, I'll pick the shorter spelling.
cantFail and report_bad_alloc_error have substantially different semantics. In 
particular, for non-Asserts builds, cantFail is a no-op, which doesn't seem 
appropriate for an API that fails on out-of-memory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69471



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Vedant Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe18531595bba: [Coverage] Revise format to reduce binary size 
(authored by vsk).
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69471

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/CoverageMapping/abspath.cpp
  clang/test/CoverageMapping/ir.c
  clang/test/Profile/def-assignop.cpp
  clang/test/Profile/def-ctors.cpp
  clang/test/Profile/def-dtors.cpp
  compiler-rt/include/profile/InstrProfData.inc
  llvm/docs/CoverageMappingFormat.rst
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingWriter.h
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfData.inc
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
  llvm/test/tools/llvm-cov/Inputs/binary-formats.v3.macho64l
  llvm/test/tools/llvm-cov/binary-formats.c
  llvm/unittests/ProfileData/CoverageMappingTest.cpp

Index: llvm/unittests/ProfileData/CoverageMappingTest.cpp
===
--- llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -895,4 +895,29 @@
   std::pair({true, false}),
   std::pair({true, true})),);
 
+TEST(CoverageMappingTest, filename_roundtrip) {
+  std::vector Paths({"a", "b", "c", "d", "e"});
+
+  for (bool Compress : {false, true}) {
+std::string EncodedFilenames;
+{
+  raw_string_ostream OS(EncodedFilenames);
+  CoverageFilenamesSectionWriter Writer(Paths);
+  Writer.write(OS, Compress);
+}
+
+std::vector ReadFilenames;
+RawCoverageFilenamesReader Reader(EncodedFilenames, ReadFilenames);
+BinaryCoverageReader::DecompressedData Decompressed;
+EXPECT_THAT_ERROR(Reader.read(CovMapVersion::CurrentVersion, Decompressed),
+  Succeeded());
+if (!Compress)
+  ASSERT_EQ(Decompressed.size(), 0U);
+
+ASSERT_EQ(ReadFilenames.size(), Paths.size());
+for (unsigned I = 0; I < Paths.size(); ++I)
+  ASSERT_TRUE(ReadFilenames[I] == Paths[I]);
+  }
+}
+
 } // end anonymous namespace
Index: llvm/test/tools/llvm-cov/binary-formats.c
===
--- llvm/test/tools/llvm-cov/binary-formats.c
+++ llvm/test/tools/llvm-cov/binary-formats.c
@@ -7,5 +7,6 @@
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 // RUN: llvm-cov show %S/Inputs/binary-formats.macho32b -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
+// RUN: llvm-cov show %S/Inputs/binary-formats.v3.macho64l -instr-profile %t.profdata -path-equivalence=/tmp,%S %s | FileCheck %s
 
 // RUN: llvm-cov export %S/Inputs/binary-formats.macho64l -instr-profile %t.profdata | FileCheck %S/Inputs/binary-formats.canonical.json
Index: llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
===
--- llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
+++ llvm/test/Instrumentation/InstrProfiling/X86/alloc.ll
@@ -1,6 +1,8 @@
 ;; Ensure that SHF_ALLOC section flag is not set for the __llvm_covmap section on Linux.
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
 
+@covfun = linkonce_odr hidden constant i32 0, section "__llvm_covfun"
 @__llvm_coverage_mapping = internal constant i32 0, section "__llvm_covmap"
 
+; CHECK-DAG: .section	__llvm_covfun,""
 ; CHECK-DAG: .section	__llvm_covmap,""
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -74,10 +74,6 @@
 
 namespace {
 
-cl::opt DoNameCompression("enable-name-compression",
-cl::desc("Enable name string compression"),
-cl::init(true));
-
 cl::opt DoHashBasedCounterSplit(
 "hash-based-counter-split",
 cl::desc("Rename counter variable of a comdat function based on cfg hash"),
@@ -916,7