[ProfileData] Support cross target binary reading for coverage tool

[davidxl]

Summary

This is a follow up to the patch that added the coverage support for PE binary. This patch also fixes the rest of test failures.

[davidxl]

Remove stale TODO

[vsk]

I'm not aware of what the test failures are, but if they are affecting CI I'd prefer to revert everything temporarily.

As it stands this patch looks functionally OK, but I'm concerned about having overloads of the get*SectionName functions that do vs. do not include segment prefixes. I.e it's confusing to me that getInstrProfCoverageSectionName(/*isCoff*/false) and getInstrProfCoverageSectionName(MachOModule) return different results.

Wdyt of removing all of the get*SectionName helpers, and just exposing: getInstrProfSectionName(InstrProfSectKind IPSK, ObjectFileKind OFK, bool AddSegmentPrefix). It will make client code more verbose but very clear / unambiguous.

[davidxl]

The failures are only on windows platform. Since the change touches three repos and and there were also incremental fixes applied later, it is better to roll forward than rolling back.

[davidxl]

The main client (the compiler) should not need to know the details so a higher level interface taking 'Module' should be better. It is the host tool that need to use the lower level interface. For the later use, there was never a need for segment prefix, so adding the flexibility is not necessary.

[vsk]

In D32073#727016, @davidxl wrote:

The main client (the compiler) should not need to know the details so a higher level interface taking 'Module' should be better.

I see your point, but taking a 'Module' doesn't seem to be nicer, because it forces us to expose more APIs which are very slightly different from each other. This leaks details because in order to pick the right API, you need to know the low level details (e.g whether or not segment prefixes are needed).

Wdyt of removing all of the get*SectionName helpers, and just exposing: getInstrProfSectionName(InstrProfSectKind IPSK, ObjectFileKind OFK, bool AddSegmentPrefix). It will make client code more verbose but very clear / unambiguous.

It is the host tool that need to use the lower level interface. For the later use, there was never a need for segment prefix, so adding the flexibility is not necessary.

We could set 'bool AddSegmentPrefix = true' by default. This still provides some extra flexibility, but the right option is picked by default, so there should be less confusion.

If you'd prefer to extend the set of get*SectionName() functions, I just ask that the names of the "getXSectionName(bool)" and "getXSectionName(Module *)" functions be made different.

[davidxl]

Changed the new interface name according to review feedback.

[vsk]

Thanks, LGTM. I will send a patch out with the alternate take on the API for your consideration -- it may turn out that this is simpler.

include/llvm/ProfileData/InstrProf.h

 /// the host machine, and the segment prefix will not be added.
 std::string getInstrProfCountersSectionName(const Module *M = nullptr);
 
 /// Return the name of data section containing names of instrumented
 /// functions. If M is null, the target platform is assumed to be the same as
 /// the host machine, nor will segment prefix be added.
 std::string getInstrProfNameSectionName(const Module *M = nullptr);
 
+/// Similar to the above, but used by host tool (e.g, coverage) which has
+/// object format information. The section name returned is not prefixed
+/// with segment name.
+std::string getInstrProfNameSectionName(bool isCoff);
+
 /// Return the name of the data section containing per-function control
 /// data. If M is null, the target platform is assumed to be the same as
 /// the host machine, and the segment prefix will not be added.
 std::string getInstrProfDataSectionName(const Module *M = nullptr);
 
+/// Similar to the above, but used by host tool (e.g, coverage) which has
+/// object format information. The section name returned is not prefixed
+/// with segment name.
+std::string getInstrProfDataSectionName(bool isCoff);
+
 /// Return the name of data section containing pointers to value profile
 /// counters/nodes. If M is null, the target platform is assumed to be
 /// the same as the host machine, and the segment prefix will not be added.
 std::string getInstrProfValuesSectionName(const Module *M = nullptr);
 
 /// Return the name of data section containing nodes holdling value
 /// profiling data. If M is null, the target platform is assumed to be
 /// the same as the host machine, and the segment prefix will not be added.
 std::string getInstrProfVNodesSectionName(const Module *M = nullptr);
 
 /// Return the name profile runtime entry point to do value profiling
 /// for a given site.
 inline StringRef getInstrProfValueProfFuncName() {
   return INSTR_PROF_VALUE_PROF_FUNC_STR;
 }
 
 /// Return the name profile runtime entry point to do value range profiling.
 inline StringRef getInstrProfValueRangeProfFuncName() {
   return INSTR_PROF_VALUE_RANGE_PROF_FUNC_STR;
 }
 
 /// Return the name of the section containing function coverage mapping
 /// data.
 std::string getInstrProfCoverageSectionName(const Module *M = nullptr);
+/// Similar to the above, but used by host tool (e.g, coverage) which has
+/// object format information. The section name returned is not prefixed
+/// with segment name.
+std::string getInstrProfCoverageSectionName(bool isCoff);
 
 /// Return the name prefix of variables containing instrumented function names.
 inline StringRef getInstrProfNameVarPrefix() { return "__profn_"; }
 
 /// Return the name prefix of variables containing per-function control data.
 inline StringRef getInstrProfDataVarPrefix() { return "__profd_"; }
 
 /// Return the name prefix of profile counter variables.

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

 static SectionKind
 getELFKindForNamedSection(StringRef Name, SectionKind K) {
   // N.B.: The defaults used in here are no the same ones used in MC.
   // We follow gcc, MC follows gas. For example, given ".section .eh_frame",
   // both gas and MC will produce a section with no flags. Given
   // section(".eh_frame") gcc will produce:
   //
   //   .section   .eh_frame,"a",@progbits
-  
-  // TODO: to support Win->ELF cross compilation with coverage properly,
-  // need to pass the module pointer to the following call.
-  if (Name == getInstrProfCoverageSectionName())
+  if (Name == getInstrProfCoverageSectionName(false /*not coff*/))
     return SectionKind::getMetadata();
 
   if (Name.empty() || Name[0] != '.') return K;
 
   // Some lame default implementation based on some magic section names.
   if (Name == ".bss" ||
       Name.startswith(".bss.") ||
       Name.startswith(".gnu.linkonce.b.") ||

lib/ProfileData/Coverage/CoverageMappingReader.cpp

                                 : support::endianness::big;
 
   // Look for the sections that we are interested in.
   // TODO: with the current getInstrProfXXXSectionName interfaces, the
   // the coverage reader host tool is limited to read coverage section on
   // binaries with compatible profile section naming scheme as the host
   // platform. Currently, COFF format binaries have different section
   // naming scheme from the all the rest.
-  auto NamesSection = lookupSection(*OF, getInstrProfNameSectionName());
+  bool isCoff = (cast<COFFObjectFile>(OF.get()) != nullptr);
+  auto NamesSection = lookupSection(*OF, getInstrProfNameSectionName(isCoff));
   if (auto E = NamesSection.takeError())
     return E;
-  auto CoverageSection = lookupSection(*OF, getInstrProfCoverageSectionName());
+  auto CoverageSection =
+      lookupSection(*OF, getInstrProfCoverageSectionName(isCoff));
   if (auto E = CoverageSection.takeError())
     return E;
 
   // Get the contents of the given sections.
   if (auto EC = CoverageSection->getContents(CoverageMapping))
     return errorCodeToError(EC);
   if (Error E = ProfileNames.create(*NamesSection))
     return E;

lib/ProfileData/InstrProf.cpp

 
 const char *InstrProfSectNamePrefix[] = {
 #define INSTR_PROF_SECT_ENTRY(Kind, SectName, SectNameCommon, SectNameCoff,    \
                               Prefix)                                          \
   Prefix,
 #include "llvm/ProfileData/InstrProfData.inc"
 };
 
-std::string getInstrProfSectionName(const Module *M, InstrProfSectKind Kind) {
+std::string getInstrProfSectionName(bool isCoff, InstrProfSectKind Kind) {
+  return isCoff ? InstrProfSectNameCoff[Kind]
+                : InstrProfSectNameCommon[Kind];
+}
 
+std::string getInstrProfSectionName(const Module *M, InstrProfSectKind Kind) {
   if (!M)
     return InstrProfSectName[Kind];
 
   bool AddSegment = Triple(M->getTargetTriple()).isOSBinFormatMachO();
   std::string SectName;
   if (Triple(M->getTargetTriple()).isOSBinFormatCOFF())
     SectName = InstrProfSectNameCoff[Kind];
   else

 std::string getInstrProfCountersSectionName(const Module *M) {
   return getInstrProfSectionName(M, IPSK_cnts);
 }
 
 std::string getInstrProfNameSectionName(const Module *M) {
   return getInstrProfSectionName(M, IPSK_name);
 }
 
+std::string getInstrProfNameSectionName(bool isCoff) {
+  return getInstrProfSectionName(isCoff, IPSK_name);
+}
+
 std::string getInstrProfDataSectionName(const Module *M) {
   return getInstrProfSectionName(M, IPSK_data);
 }
 
+std::string getInstrProfDataSectionName(bool isCoff) {
+  return getInstrProfSectionName(isCoff, IPSK_data);
+}
+
 std::string getInstrProfValuesSectionName(const Module *M) {
   return getInstrProfSectionName(M, IPSK_vals);
 }
 
 std::string getInstrProfVNodesSectionName(const Module *M) {
   return getInstrProfSectionName(M, IPSK_vnodes);
 }
 
 std::string getInstrProfCoverageSectionName(const Module *M) {
   return getInstrProfSectionName(M, IPSK_covmap);
 }
 
+std::string getInstrProfCoverageSectionName(bool isCoff) {
+  return getInstrProfSectionName(isCoff, IPSK_covmap);
+}
+
 void SoftInstrProfErrors::addError(instrprof_error IE) {
   if (IE == instrprof_error::success)
     return;
 
   if (FirstError == instrprof_error::success)
     FirstError = IE;
 
   switch (IE) {