[InstrProf] Implement static profdata registration

[rnk]

Summary

The motivating use case is eliminating duplicate profile data registered
for the same inline function in two object files. Before this change,
users would observe multiple symbol definition errors with VC link, but
links with LLD would succeed.

Users (Mozilla) have reported that PGO works well with clang-cl and LLD,
but when using LLD without this static registration, we would get into a
"relocation against a discarded section" situation. I'm not sure what
happens in that situation, but I suspect that duplicate, unused profile
information was retained. If so, this change will reduce the size of
such binaries with LLD.

Now, Windows uses static registration and is in line with all the other
platforms.

[davidxl]

compiler-rt/lib/profile/InstrProfilingPlatformOther.c

64

register function won't be called with static registration. Why is this change needed?

[rnk]

• Rework InstrProfData.inc to avoid warnings about '$' in identifiers

[rnk]

compiler-rt/lib/profile/InstrProfilingPlatformOther.c

64

This change isn't needed anymore. I suppose Solaris is the only supported platform that uses this now, and perhaps the risk that the linker lays out the profile data discontiguously is negligible.

[rnk]

• Revert unnecessary PlatformOther changes

[davidxl]

compiler-rt/lib/profile/InstrProfilingValue.c

34 ↗

(On Diff #185882)

What is this change for?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

682

Why is const removed?

758

can this code be moved closer to where COFF linkage handling above (i.e. resetting to internal)?

901

insertin --> inserting

[rnk]

compiler-rt/lib/profile/InstrProfilingValue.c

34 ↗

(On Diff #185882)

When I compiled my original change on Linux, I encountered warnings about this code:

#define INSTR_PROF_DATA_COFF .lprfd$M

Clang warns that dollars in identifiers are considered to be an extension. To address that, I quoted the COFF strings, and eliminated the *_STR macros.

The only use of the section names that needs to not be quoted is the use in InstrProfilingPlatformLinux.c, which I modified to use INSTR_PROF_INSTR_PROF_DATA_COMMON to build the start / stop section bounds symbols. This code is already platform specific, so it seemed OK to use the underlying *_COMMON macros.

If you like, I can separate that change out. Would you like me to upload it as a separate review?

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

682

Oops, not intended.

758

I'll try something. I think it might be best to re-inline the code that computes the comdat to use, because the linkage used is very related to how that is done.

[rnk]

• refactor linkage and comdat calculation

[davidxl]

lgtm.

compiler-rt/lib/profile/InstrProfilingValue.c

34 ↗

(On Diff #185882)

ok.

compiler-rt/cmake/base-config-ix.cmake

if (LLVM_TREE_AVAILABLE)
   # Setup the paths where compiler-rt runtimes and headers should be stored.
   set(COMPILER_RT_OUTPUT_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION})
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
   set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests."
          ${LLVM_INCLUDE_TESTS})
   option(COMPILER_RT_ENABLE_WERROR "Fail and stop if warning is triggered"
          ${LLVM_ENABLE_WERROR})
-  # Use just-built Clang to compile/link tests on all platforms, except for
-  # Windows where we need to use clang-cl instead.
-  if(NOT MSVC)
-    set(COMPILER_RT_TEST_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang)
-    set(COMPILER_RT_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++)
-  else()
-    set(COMPILER_RT_TEST_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang.exe)
-    set(COMPILER_RT_TEST_CXX_COMPILER ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++.exe)
-  endif()
+  # Use just-built Clang to compile/link tests on all platforms.
+  set(COMPILER_RT_TEST_COMPILER
+    ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang${CMAKE_EXECUTABLE_SUFFIX})
+  set(COMPILER_RT_TEST_CXX_COMPILER
+    ${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++${CMAKE_EXECUTABLE_SUFFIX})
 else()
     # Take output dir and install path from the user.
   set(COMPILER_RT_OUTPUT_DIR ${CMAKE_CURRENT_BINARY_DIR} CACHE PATH
     "Path where built compiler-rt libraries should be stored.")
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${CMAKE_CURRENT_BINARY_DIR}/bin CACHE PATH
     "Path where built compiler-rt executables should be stored.")
   set(COMPILER_RT_INSTALL_PATH ${CMAKE_INSTALL_PREFIX} CACHE PATH
     "Path where built compiler-rt libraries should be installed.")

compiler-rt/lib/profile/CMakeLists.txt

set(PROFILE_SOURCES
   InstrProfilingMerge.c
   InstrProfilingMergeFile.c
   InstrProfilingNameVar.c
   InstrProfilingWriter.c
   InstrProfilingPlatformDarwin.c
   InstrProfilingPlatformFuchsia.c
   InstrProfilingPlatformLinux.c
   InstrProfilingPlatformOther.c
+  InstrProfilingPlatformWindows.c
   InstrProfilingRuntime.cc
   InstrProfilingUtil.c)
 
 set(PROFILE_HEADERS
   InstrProfData.inc
   InstrProfiling.h
   InstrProfilingInternal.h
   InstrProfilingPort.h

compiler-rt/lib/profile/InstrProfilingPlatformOther.c

 /*===- InstrProfilingPlatformOther.c - Profile data default platform ------===*\
 |*
 |* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 |* See https://llvm.org/LICENSE.txt for license information.
 |* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 |*
 \*===----------------------------------------------------------------------===*/
 
-#if !defined(__APPLE__) && !defined(__linux__) && !defined(__FreeBSD__) && \
-    !(defined(__sun__) && defined(__svr4__)) && !defined(__NetBSD__)
+#if !defined(__APPLE__) && !defined(__linux__) && !defined(__FreeBSD__) &&     \
+    !(defined(__sun__) && defined(__svr4__)) && !defined(__NetBSD__) &&        \
+    !defined(_WIN32)
 
 #include <stdlib.h>
+#include <stdio.h>
 
 #include "InstrProfiling.h"
 
 static const __llvm_profile_data *DataFirst = NULL;
 static const __llvm_profile_data *DataLast = NULL;
 static const char *NamesFirst = NULL;
 static const char *NamesLast = NULL;
 static uint64_t *CountersFirst = NULL;
void __llvm_profile_register_function(void *Data_) {
   }
 
   DataFirst = (const __llvm_profile_data *)getMinAddr(DataFirst, Data);
   CountersFirst = (uint64_t *)getMinAddr(CountersFirst, Data->CounterPtr);
 
   DataLast = (const __llvm_profile_data *)getMaxAddr(DataLast, Data + 1);
   CountersLast = (uint64_t *)getMaxAddr(
       CountersLast, (uint64_t *)Data->CounterPtr + Data->NumCounters);
+
+  /* If the profile data is not properly aligned by the linker, this will cause
+   * problems later. Try to diagnose the problem now. This is often caused by
+   * the Microsoft incremental linker, which inserts extra padding in case
+   * globals grow in subsequent links.
+   */
+  ptrdiff_t DataBytes = (const char *)DataLast - (const char *)DataFirst;
+  ptrdiff_t CounterBytes =
+      (const char *)CountersLast - (const char *)CountersFirst;
+  if (DataBytes % sizeof(__llvm_profile_data) != 0)
+    PROF_ERR("profile data array is corrupt\n");
+  if (CounterBytes % sizeof(uint64_t) != 0)
+    PROF_ERR("profile counter array is corrupt\n");
 }
 
 COMPILER_RT_VISIBILITY
 void __llvm_profile_register_names_function(void *NamesStart,
                                             uint64_t NamesSize) {
   if (!NamesFirst) {
     NamesFirst = (const char *)NamesStart;
     NamesLast = (const char *)NamesStart + NamesSize;
ValueProfNode *__llvm_profile_begin_vnodes(void) {
   return 0;
 }
 COMPILER_RT_VISIBILITY
 ValueProfNode *__llvm_profile_end_vnodes(void) { return 0; }
 
 COMPILER_RT_VISIBILITY ValueProfNode *CurrentVNode = 0;
 COMPILER_RT_VISIBILITY ValueProfNode *EndVNode = 0;
 
-#if defined(_WIN32)
-// Put read-write sections in .data.
-#pragma comment(linker, "/MERGE:.lprfc=.data")
-#pragma comment(linker, "/MERGE:.lprfd=.data")
-// Put read-write sections in .rdata.
-#pragma comment(linker, "/MERGE:.lcovmap=.rdata")
-#pragma comment(linker, "/MERGE:.lprfn=.rdata")
-#endif
-
 #endif

compiler-rt/lib/profile/InstrProfilingPlatformWindows.c

+/*===- InstrProfilingPlatformWindows.c - Profile data on Windows ----------===*\
+|*
+|* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+|* See https://llvm.org/LICENSE.txt for license information.
+|* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+|*
+\*===----------------------------------------------------------------------===*/
+
+#include "InstrProfiling.h"
+
+#if defined(_WIN32)
+
+#if defined(_MSC_VER)
+/* Merge read-write sections into .data. */
+#pragma comment(linker, "/MERGE:" INSTR_PROF_CNTS_SECT_NAME_STR "=.data")
+#pragma comment(linker, "/MERGE:" INSTR_PROF_DATA_SECT_NAME_STR "=.data")
+#pragma comment(linker, "/MERGE:" INSTR_PROF_VALS_SECT_NAME_STR "=.data")
+#pragma comment(linker, "/MERGE:" INSTR_PROF_VNODES_SECT_NAME_STR "=.data")
+/* Merge read-only sections into .data. */
+#pragma comment(linker, "/MERGE:" INSTR_PROF_COVMAP_SECT_NAME_STR "=.rdata")
+#pragma comment(linker, "/MERGE:" INSTR_PROF_NAME_SECT_NAME_STR "=.rdata")
+
+/* Allocate read-only section bounds. */
+#pragma section(".lprfn$A", read)
+#pragma section(".lprfn$Z", read)
+
+/* Allocate read-write section bounds. */
+#pragma section(".lprfd$A", read, write)
+#pragma section(".lprfd$Z", read, write)
+#pragma section(".lprfc$A", read, write)
+#pragma section(".lprfc$Z", read, write)
+#pragma section(".lprfnd$A", read, write)
+#pragma section(".lprfnd$Z", read, write)
+#endif
+
+__llvm_profile_data COMPILER_RT_SECTION(".lprfd$A") DataStart = {0};
+__llvm_profile_data COMPILER_RT_SECTION(".lprfd$Z") DataEnd = {0};
+
+const char COMPILER_RT_SECTION(".lprfn$A") NamesStart = '\0';
+const char COMPILER_RT_SECTION(".lprfn$Z") NamesEnd = '\0';
+
+uint64_t COMPILER_RT_SECTION(".lprfc$A") CountersStart;
+uint64_t COMPILER_RT_SECTION(".lprfc$Z") CountersEnd;
+
+ValueProfNode COMPILER_RT_SECTION(".lprfnd$A") VNodesStart;
+ValueProfNode COMPILER_RT_SECTION(".lprfnd$Z") VNodesEnd;
+
+const __llvm_profile_data *__llvm_profile_begin_data(void) {
+  return &DataStart + 1;
+}
+const __llvm_profile_data *__llvm_profile_end_data(void) { return &DataEnd; }
+
+const char *__llvm_profile_begin_names(void) { return &NamesStart + 1; }
+const char *__llvm_profile_end_names(void) { return &NamesEnd; }
+
+uint64_t *__llvm_profile_begin_counters(void) { return &CountersStart + 1; }
+uint64_t *__llvm_profile_end_counters(void) { return &CountersEnd; }
+
+ValueProfNode *__llvm_profile_begin_vnodes(void) { return &VNodesStart + 1; }
+ValueProfNode *__llvm_profile_end_vnodes(void) { return &VNodesEnd; }
+
+ValueProfNode *CurrentVNode = &VNodesStart + 1;
+ValueProfNode *EndVNode = &VNodesEnd;
+
+#endif

compiler-rt/test/profile/coverage-inline.cpp

+// RUN: %clang_profgen -g -fcoverage-mapping -c -o %t1.o %s -DOBJECT_1
+// RUN: %clang_profgen -g -fcoverage-mapping -c -o %t2.o %s
+// RUN: %clang_profgen -g -fcoverage-mapping %t1.o %t2.o -o %t.exe
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.exe
+// RUN: llvm-profdata show %t.profraw -all-functions | FileCheck %s
+
+// Test that the instrumentation puts the right linkage on the profile data for
+// inline functions.
+
+// CHECK:  {{.*}}foo{{.*}}:
+// CHECK-NEXT:    Hash:
+// CHECK-NEXT:    Counters: 1
+// CHECK-NEXT:    Function count: 1
+// CHECK:  {{.*}}inline_wrapper{{.*}}:
+// CHECK-NEXT:    Hash:
+// CHECK-NEXT:    Counters: 1
+// CHECK-NEXT:    Function count: 2
+// CHECK:  main:
+// CHECK-NEXT:    Hash:
+// CHECK-NEXT:    Counters: 1
+// CHECK-NEXT:    Function count: 1
+
+extern "C" int puts(const char *);
+
+inline void inline_wrapper(const char *msg) {
+  puts(msg);
+}
+
+void foo();
+
+#ifdef OBJECT_1
+void foo() {
+  inline_wrapper("foo");
+}
+#else
+int main() {
+  inline_wrapper("main");
+  foo();
+}
+#endif

compiler-rt/test/profile/lit.cfg

 
 # Setup executable root.
 if hasattr(config, 'profile_lit_binary_dir') and \
         config.profile_lit_binary_dir is not None:
     config.test_exec_root = os.path.join(config.profile_lit_binary_dir, config.name)
 
 if config.host_os in ['Linux']:
   extra_link_flags = ["-ldl"]
+elif config.host_os in ['Windows']:
+  # InstrProf is incompatible with incremental linking. Disable it as a
+  # workaround.
+  extra_link_flags = ["-Wl,-incremental:no"]
 else:
   extra_link_flags = []
 
 # Test suffixes.
 config.suffixes = ['.c', '.cc', '.cpp', '.m', '.mm', '.ll', '.test']
 
 # What to exclude.
 config.excludes = ['Inputs']

llvm/include/llvm/ProfileData/InstrProfData.inc

/* section name strings common to all targets other
    than WIN32 */
 #define INSTR_PROF_DATA_COMMON __llvm_prf_data
 #define INSTR_PROF_NAME_COMMON __llvm_prf_names
 #define INSTR_PROF_CNTS_COMMON __llvm_prf_cnts
 #define INSTR_PROF_VALS_COMMON __llvm_prf_vals
 #define INSTR_PROF_VNODES_COMMON __llvm_prf_vnds
 #define INSTR_PROF_COVMAP_COMMON __llvm_covmap
 /* Win32 */
-#define INSTR_PROF_DATA_COFF .lprfd
-#define INSTR_PROF_NAME_COFF .lprfn
-#define INSTR_PROF_CNTS_COFF .lprfc
-#define INSTR_PROF_VALS_COFF .lprfv
-#define INSTR_PROF_VNODES_COFF .lprfnd
-#define INSTR_PROF_COVMAP_COFF .lcovmap
+#define INSTR_PROF_DATA_COFF .lprfd$M
+#define INSTR_PROF_NAME_COFF .lprfn$M
+#define INSTR_PROF_CNTS_COFF .lprfc$M
+#define INSTR_PROF_VALS_COFF .lprfv$M
+#define INSTR_PROF_VNODES_COFF .lprfnd$M
+#define INSTR_PROF_COVMAP_COFF .lcovmap$M
 
 #ifdef _WIN32
 /* Runtime section names and name strings.  */
 #define INSTR_PROF_DATA_SECT_NAME INSTR_PROF_DATA_COFF
 #define INSTR_PROF_NAME_SECT_NAME INSTR_PROF_NAME_COFF
 #define INSTR_PROF_CNTS_SECT_NAME INSTR_PROF_CNTS_COFF
 /* Array of pointers. Each pointer points to a list
  * of value nodes associated with one value site.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

static inline bool shouldRecordFunctionAddr(Function *F) {
   // be 'addresstaken'. If its address is not recorded here, the profile data
   // with missing address may be picked by the linker leading  to missing
   // indirect call target info.
   return F->hasAddressTaken() || F->hasLinkOnceLinkage();
 }
 
 static inline Comdat *getOrCreateProfileComdat(Module &M, Function &F,
                                                InstrProfIncrementInst *Inc,
-                                               const Triple &TT) {
+                                               Triple &TT) {
   if (!needsComdatForCounter(F, M))
     return nullptr;
 
-  // COFF format requires a COMDAT section to have a key symbol with the same
-  // name. The linker targeting COFF also requires that the COMDAT
-  // a section is associated to must precede the associating section. For this
-  // reason, we must choose the counter var's name as the name of the comdat.
-  StringRef ComdatPrefix =
-      (TT.isOSBinFormatCOFF() ? getInstrProfCountersVarPrefix()
-                              : getInstrProfComdatPrefix());
-  return M.getOrInsertComdat(StringRef(getVarName(Inc, ComdatPrefix)));
+  // There are two cases that need a comdat on COFF:
+  // 1. Functions that already have comdats (standard case)
+  // 2. available_externally functions (dllimport and C99 inline)
+  // In the first case, put all the data in the original function comdat. In the
+  // second case, we don't have a comdat, so choose one of the variables to be
+  // the leader of the group. Today the counter global is chosen as the leader.
+  if (TT.isOSBinFormatCOFF()) {
+    if (Comdat *C = F.getComdat())
+      return C;
+    return M.getOrInsertComdat(
+        StringRef(getVarName(Inc, getInstrProfCountersVarPrefix())));
+  }
+
+  return M.getOrInsertComdat(
+      StringRef(getVarName(Inc, getInstrProfComdatPrefix())));
 }
 
 static bool needsRuntimeRegistrationOfSectionRange(const Triple &TT) {
   // Don't do this for Darwin.  compiler-rt uses linker magic.
   if (TT.isOSDarwin())
     return false;
 
   // Use linker script magic to get data/cnts/name start/end.
   if (TT.isOSLinux() || TT.isOSFreeBSD() || TT.isOSNetBSD() ||
-      TT.isOSFuchsia() || TT.isPS4CPU())
+      TT.isOSFuchsia() || TT.isPS4CPU() || TT.isOSWindows())
     return false;
 
   return true;
 }
 
 GlobalVariable *
 InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   GlobalVariable *NamePtr = Inc->getName();
   auto It = ProfileDataMap.find(NamePtr);
   PerFunctionProfileData PD;
   if (It != ProfileDataMap.end()) {
     if (It->second.RegionCounters)
       return It->second.RegionCounters;
     PD = It->second;
   }
 
   // Move the name variable to the right section. Place them in a COMDAT group
-  // if the associated function is a COMDAT. This will make sure that
-  // only one copy of counters of the COMDAT function will be emitted after
-  // linking.
+  // if the associated function is a COMDAT. This will make sure that only one
+  // copy of counters of the COMDAT function will be emitted after linking.
+  // Match the linkage of the name global, except on COFF, where the linkage
+  // must be local.
   Function *Fn = Inc->getParent()->getParent();
-  Comdat *ProfileVarsComdat = nullptr;
-  ProfileVarsComdat = getOrCreateProfileComdat(*M, *Fn, Inc, TT);
+  Comdat *Cmdt = getOrCreateProfileComdat(*M, *Fn, Inc, TT);
+  GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
+  GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
+  if (TT.isOSBinFormatCOFF()) {
+    Linkage = GlobalValue::InternalLinkage;
+    Visibility = GlobalValue::DefaultVisibility;
+  }
 
   uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();
   LLVMContext &Ctx = M->getContext();
   ArrayType *CounterTy = ArrayType::get(Type::getInt64Ty(Ctx), NumCounters);
 
   // Create the counters variable.
   auto *CounterPtr =
-      new GlobalVariable(*M, CounterTy, false, NamePtr->getLinkage(),
+      new GlobalVariable(*M, CounterTy, false, Linkage,
                          Constant::getNullValue(CounterTy),
                          getVarName(Inc, getInstrProfCountersVarPrefix()));
-  CounterPtr->setVisibility(NamePtr->getVisibility());
+  CounterPtr->setVisibility(Visibility);
   CounterPtr->setSection(
       getInstrProfSectionName(IPSK_cnts, TT.getObjectFormat()));
   CounterPtr->setAlignment(8);
-  CounterPtr->setComdat(ProfileVarsComdat);
+  CounterPtr->setComdat(Cmdt);
+
+  // If we're creating a new comdat group using the counters as the leader, give
+  // the counters an external linkage like linkonce_odr.
+  if (TT.isOSBinFormatCOFF() && Cmdt &&
+      Cmdt != Fn->getComdat())
+    CounterPtr->setLinkage(GlobalValue::LinkOnceODRLinkage);
 
   auto *Int8PtrTy = Type::getInt8PtrTy(Ctx);
   // Allocate statically the array of pointers to value profile nodes for
   // the current function.
   Constant *ValuesPtrExpr = ConstantPointerNull::get(Int8PtrTy);
   if (ValueProfileStaticAlloc && !needsRuntimeRegistrationOfSectionRange(TT)) {
     uint64_t NS = 0;
     for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
       NS += PD.NumValueSites[Kind];
     if (NS) {
       ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
 
       auto *ValuesVar =
-          new GlobalVariable(*M, ValuesTy, false, NamePtr->getLinkage(),
+          new GlobalVariable(*M, ValuesTy, false, Linkage,
                              Constant::getNullValue(ValuesTy),
                              getVarName(Inc, getInstrProfValuesVarPrefix()));
-      ValuesVar->setVisibility(NamePtr->getVisibility());
+      ValuesVar->setVisibility(Visibility);
       ValuesVar->setSection(
           getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
       ValuesVar->setAlignment(8);
-      ValuesVar->setComdat(ProfileVarsComdat);
+      ValuesVar->setComdat(Cmdt);
       ValuesPtrExpr =
           ConstantExpr::getBitCast(ValuesVar, Type::getInt8PtrTy(Ctx));
     }
   }
 
   // Create data variable.
   auto *Int16Ty = Type::getInt16Ty(Ctx);
   auto *Int16ArrayTy = ArrayType::get(Int16Ty, IPVK_Last + 1);
#include "llvm/ProfileData/InstrProfData.inc"
   Constant *Int16ArrayVals[IPVK_Last + 1];
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);
 
   Constant *DataVals[] = {
 #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) Init,
 #include "llvm/ProfileData/InstrProfData.inc"
   };
-  auto *Data = new GlobalVariable(*M, DataTy, false, NamePtr->getLinkage(),
+  auto *Data = new GlobalVariable(*M, DataTy, false, Linkage,
                                   ConstantStruct::get(DataTy, DataVals),
                                   getVarName(Inc, getInstrProfDataVarPrefix()));
-  Data->setVisibility(NamePtr->getVisibility());
+  Data->setVisibility(Visibility);
   Data->setSection(getInstrProfSectionName(IPSK_data, TT.getObjectFormat()));
   Data->setAlignment(INSTR_PROF_DATA_ALIGNMENT);
-  Data->setComdat(ProfileVarsComdat);
+  Data->setComdat(Cmdt);
 
   PD.RegionCounters = CounterPtr;
   PD.DataVar = Data;
   ProfileDataMap[NamePtr] = PD;
 
   // Mark the data variable as used so that it isn't stripped out.
   UsedVars.push_back(Data);
   // Now that the linkage set by the FE has been passed to the data and counter
void InstrProfiling::emitNameData() {
   auto *NamesVal = ConstantDataArray::getString(
       Ctx, StringRef(CompressedNameStr), false);
   NamesVar = new GlobalVariable(*M, NamesVal->getType(), true,
                                 GlobalValue::PrivateLinkage, NamesVal,
                                 getInstrProfNamesVarName());
   NamesSize = CompressedNameStr.size();
   NamesVar->setSection(
       getInstrProfSectionName(IPSK_name, TT.getObjectFormat()));
+  // On COFF, it's important to reduce the alignment down to 1 to prevent the
+  // linker from insertin padding before the start of the names section or
+  // between names entries.
+  NamesVar->setAlignment(1);
   UsedVars.push_back(NamesVar);
 
   for (auto *NamePtr : ReferencedNames)
     NamePtr->eraseFromParent();
 }
 
 void InstrProfiling::emitRegistration() {
   if (!needsRuntimeRegistrationOfSectionRange(TT))

llvm/test/Instrumentation/InstrProfiling/PR23499.ll

 
 ; CHECK-NOT: __profn__Z3barIvEvv
 ; CHECK: @__profc__Z3barIvEvv = linkonce_odr hidden global [1 x i64] zeroinitializer, section "{{.*}}__llvm_prf_cnts", comdat($__profv__Z3barIvEvv), align 8
 ; CHECK: @__profd__Z3barIvEvv = linkonce_odr hidden global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}__llvm_prf_data{{.*}}", comdat($__profv__Z3barIvEvv), align 8
 ; CHECK: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}__llvm_prf_names"
 
 
 ; COFF-NOT: __profn__Z3barIvEvv
-; COFF: @__profc__Z3barIvEvv = linkonce_odr hidden global [1 x i64] zeroinitializer, section "{{.*}}prfc", comdat, align 8
-; COFF: @__profd__Z3barIvEvv = linkonce_odr hidden global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}prfd{{.*}}", comdat($__profc__Z3barIvEvv), align 8
+; COFF: @__profc__Z3barIvEvv = internal global [1 x i64] zeroinitializer, section "{{.*}}prfc$M", comdat($_Z3barIvEvv), align 8
+; COFF: @__profd__Z3barIvEvv = internal global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}prfd{{.*}}", comdat($_Z3barIvEvv), align 8
 
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32) #1
 
 define linkonce_odr void @_Z3barIvEvv() comdat {
 entry:
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([11 x i8], [11 x i8]* @__profn__Z3barIvEvv, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }

llvm/test/Instrumentation/InstrProfiling/comdat.ll

+; RUN: opt < %s -mtriple=x86_64-unknown-linux -instrprof -S | FileCheck %s --check-prefixes=ELF
+; RUN: opt < %s  -mtriple=x86_64-pc-win32-coff -instrprof -S | FileCheck %s --check-prefixes=COFF
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefixes=ELF
+; RUN: opt < %s  -mtriple=x86_64-pc-win32-coff -passes=instrprof -S | FileCheck %s --check-prefixes=COFF
+
+; There are two main cases where comdats are necessary:
+; 1. standard inline functions (weak_odr / linkonce_odr)
+; 2. available externally functions (C99 inline / extern template / dllimport)
+; Check that we do the right thing for the two object formats with comdats, ELF
+; and COFF.
+
+declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
+
+$foo_inline = comdat any
+
+@__profn_foo_inline = linkonce_odr hidden constant [10 x i8] c"foo_inline"
+
+; ELF: @__profc_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat($__profv_foo_inline), align 8
+; ELF: @__profd_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_data", comdat($__profv_foo_inline), align 8
+; COFF: @__profc_foo_inline = internal global{{.*}}, section ".lprfc$M", comdat($foo_inline), align 8
+; COFF: @__profd_foo_inline = internal global{{.*}}, section ".lprfd$M", comdat($foo_inline), align 8
+define weak_odr void @foo_inline() comdat {
+  call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
+  ret void
+}
+
+$foo_extern = comdat any
+
+@__profn_foo_extern = linkonce_odr hidden constant [10 x i8] c"foo_extern"
+
+; ELF: @__profc_foo_extern = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat($__profv_foo_extern)
+; ELF: @__profd_foo_extern = linkonce_odr hidden global{{.*}}, section "__llvm_prf_data", comdat($__profv_foo_extern)
+; COFF: @__profc_foo_extern = linkonce_odr dso_local global{{.*}}, section ".lprfc$M", comdat, align 8
+; COFF: @__profd_foo_extern = internal global{{.*}}, section ".lprfd$M", comdat($__profc_foo_extern), align 8
+define available_externally void @foo_extern() {
+  call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
+  ret void
+}

llvm/test/Instrumentation/InstrProfiling/linkage.ll

 ;; Check that runtime symbols get appropriate linkage.
 
-; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -instrprof -S | FileCheck %s --check-prefixes=COMMON,MACHO
-; RUN: opt < %s -mtriple=x86_64-unknown-linux -instrprof -S | FileCheck %s --check-prefixes=COMMON,LINUX
-; RUN: opt < %s  -mtriple=x86_64-pc-win32-coff -instrprof -S | FileCheck %s --check-prefixes=COMMON,COFF
-; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -passes=instrprof -S | FileCheck %s --check-prefixes=COMMON,MACHO
-; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefixes=COMMON,LINUX
-; RUN: opt < %s  -mtriple=x86_64-pc-win32-coff -passes=instrprof -S | FileCheck %s --check-prefixes=COMMON,COFF
+; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -instrprof -S | FileCheck %s --check-prefixes=POSIX,MACHO
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -instrprof -S | FileCheck %s --check-prefixes=POSIX,LINUX
+; RUN: opt < %s  -mtriple=x86_64-pc-win32-coff -instrprof -S | FileCheck %s --check-prefixes=COFF
+; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -passes=instrprof -S | FileCheck %s --check-prefixes=POSIX,MACHO
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s --check-prefixes=POSIX,LINUX
+; RUN: opt < %s  -mtriple=x86_64-pc-win32-coff -passes=instrprof -S | FileCheck %s --check-prefixes=COFF
 
 ; MACHO: @__llvm_profile_runtime = external global i32
 ; LINUX-NOT: @__llvm_profile_runtime = external global i32
 
 @__profn_foo = hidden constant [3 x i8] c"foo"
 @__profn_foo_weak = weak hidden constant [8 x i8] c"foo_weak"
 @"__profn_linkage.ll:foo_internal" = internal constant [23 x i8] c"linkage.ll:foo_internal"
 @__profn_foo_inline = linkonce_odr hidden constant [10 x i8] c"foo_inline"
 @__profn_foo_extern = linkonce_odr hidden constant [10 x i8] c"foo_extern"
 
-; COMMON: @__profc_foo = hidden global
-; COMMON: @__profd_foo = hidden global
+; POSIX: @__profc_foo = hidden global
+; POSIX: @__profd_foo = hidden global
+; COFF: @__profc_foo = internal global
+; COFF-NOT: comdat
+; COFF: @__profd_foo = internal global
 define void @foo() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
-; COMMON: @__profc_foo_weak = weak hidden global
-; COMMON: @__profd_foo_weak = weak hidden global
+; POSIX: @__profc_foo_weak = weak hidden global
+; POSIX: @__profd_foo_weak = weak hidden global
+; COFF: @__profc_foo_weak = internal global
+; COFF: @__profd_foo_weak = internal global
 define weak void @foo_weak() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @__profn_foo_weak, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
-; COMMON: @"__profc_linkage.ll:foo_internal" = internal global
-; COMMON: @"__profd_linkage.ll:foo_internal" = internal global
+; POSIX: @"__profc_linkage.ll:foo_internal" = internal global
+; POSIX: @"__profd_linkage.ll:foo_internal" = internal global
+; COFF: @"__profc_linkage.ll:foo_internal" = internal global
+; COFF: @"__profd_linkage.ll:foo_internal" = internal global
 define internal void @foo_internal() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([23 x i8], [23 x i8]* @"__profn_linkage.ll:foo_internal", i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
-; COMMON: @__profc_foo_inline = linkonce_odr hidden global
-; COFF-SAME: section ".lprfc", align 8
-; COMMON: @__profd_foo_inline = linkonce_odr hidden global
-; COFF-SAME: section ".lprfd", align 8
+; POSIX: @__profc_foo_inline = linkonce_odr hidden global
+; POSIX: @__profd_foo_inline = linkonce_odr hidden global
+; COFF: @__profc_foo_inline = internal global{{.*}} section ".lprfc$M", align 8
+; COFF: @__profd_foo_inline = internal global{{.*}} section ".lprfd$M", align 8
 define linkonce_odr void @foo_inline() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; LINUX: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat($__profv_foo_extern), align 8
 ; LINUX: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_data", comdat($__profv_foo_extern), align 8
 ; MACHO: @__profc_foo_extern = linkonce_odr hidden global
 ; MACHO: @__profd_foo_extern = linkonce_odr hidden global
-; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc", comdat, align 8
-; COFF: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfd", comdat($__profc_foo_extern), align 8
+; COFF: @__profc_foo_extern = linkonce_odr dso_local global {{.*}}section ".lprfc$M", comdat, align 8
+; COFF: @__profd_foo_extern = internal global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
 
 ; MACHO: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
 ; MACHO:   %[[REG:.*]] = load i32, i32* @__llvm_profile_runtime
 ; MACHO:   ret i32 %[[REG]]
 ; MACHO: }
 ; COFF: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} comdat {
 ; LINUX-NOT: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
 ; LINUX-NOT:   %[[REG:.*]] = load i32, i32* @__llvm_profile_runtime

llvm/test/Instrumentation/InstrProfiling/platform.ll

 
 @__profn_foo = hidden constant [3 x i8] c"foo"
 ; MACHO-NOT: __profn_foo
 ; ELF-NOT: __profn_foo
 ; WINDOWS-NOT: __profn_foo
 
 ; MACHO: @__profc_foo = hidden global [1 x i64] zeroinitializer, section "__DATA,__llvm_prf_cnts", align 8
 ; ELF: @__profc_foo = hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
-; WINDOWS: @__profc_foo = hidden global [1 x i64] zeroinitializer, section ".lprfc", align 8
+; WINDOWS: @__profc_foo = internal global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
 
 ; MACHO: @__profd_foo = hidden {{.*}}, section "__DATA,__llvm_prf_data,regular,live_support", align 8
 ; ELF: @__profd_foo = hidden {{.*}}, section "__llvm_prf_data", align 8
-; WINDOWS: @__profd_foo = hidden {{.*}}, section ".lprfd", align 8
+; WINDOWS: @__profd_foo = internal global {{.*}}, section ".lprfd$M", align 8
 
-; ELF: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}__llvm_prf_names"
+; ELF: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}__llvm_prf_names", align 1
+; WINDOWS: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}lprfn$M", align 1
 
 define void @foo() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
 
 ;; Emit registration functions for platforms that don't find the
 ;; symbols by their sections.
 
 ; MACHO-NOT: define internal void @__llvm_profile_register_functions
 ; LINUX-NOT: define internal void @__llvm_profile_register_functions
 ; FREEBSD-NOT: define internal void @__llvm_profile_register_functions
 ; PS4-NOT: define internal void @__llvm_profile_register_functions
+; WINDOWS-NOT: define internal void @__llvm_profile_register_functions
 
 ;; PR38340: When dynamic registration is used, we had a bug where we'd register
 ;; something that's not a __profd_* variable.
 
-; WINDOWS: define internal void @__llvm_profile_register_functions()
-; WINDOWS-NOT: __llvm_profile_runtime_user
-; WINDOWS: ret void
-
 ; SOLARIS: define internal void @__llvm_profile_register_functions
 ; SOLARIS-NOT: __llvm_profile_runtime_user
 ; SOLARIS: ret void
 
 ; MACHO-NOT: define internal void @__llvm_profile_init
 ; LINUX-NOT: define internal void @__llvm_profile_init
 ; FREEBSD-NOT: define internal void @__llvm_profile_init
 ; PS4-NOT: define internal void @__llvm_profile_init
+; WINDOWS-NOT: define internal void @__llvm_profile_init
 ; SOLARIS: define internal void @__llvm_profile_init
-; WINDOWS: define internal void @__llvm_profile_init