[PATCH] D32023: [ProfileData] Support coverage for PE binaries

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 13 11:18:29 PDT 2017


vsk added inline comments.


================
Comment at: lib/ProfileData/Coverage/CoverageMappingReader.cpp:651
   // Look for the sections that we are interested in.
-  auto NamesSection = lookupSection(*OF, getInstrProfNameSectionName(false));
+  auto NamesSection = lookupSection(*OF, getInstrProfNameSectionName());
   if (auto E = NamesSection.takeError())
----------------
It doesn't look like getInstrProfNameSectionName() returns the correct section name for COFF files here. With the exception of the ELF-specific change in TargetLoweringObjectFileImpl.cpp, this comment applies everywhere else in this patch where a module is not passed in to the get*SectionName helper.

Do you think we need a mandatory way to tell the get*SectionName functions which file format we're working with?


================
Comment at: lib/ProfileData/InstrProf.cpp:257
+}
+
 void SoftInstrProfErrors::addError(instrprof_error IE) {
----------------
These get*SectionName functions are getting a bit unwieldy. Wdyt of consolidating them? There are two inputs: ObjectFileKind ({MachO, COFF, Other}), and ProfInfoKind ({Counters, Names, Data, Values, Vnodes, Coverage}). We just need one function: `StringRef getInstrProfSectionName(ObjectFileKind, ProfInfoKind)`.


https://reviews.llvm.org/D32023





More information about the llvm-commits mailing list