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

[David Li via Phabricator via llvm-commits]

davidxl 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())
----------------
vsk wrote:
> 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?
You are right here.

However this patch is about cross compilation support, not about host tool support for other platforms. I can add a comment and TODO here that the binary format is assumed to be the native format on the host platform.


================
Comment at: lib/ProfileData/InstrProf.cpp:257
+}
+
 void SoftInstrProfErrors::addError(instrprof_error IE) {
----------------
vsk wrote:
> 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)`.
True. I will see if a refactoring is doable.


https://reviews.llvm.org/D32023