resolve review fee feedback

Author: mingmingl-llvm

compiler-rt/include/profile/InstrProfData.inc

@@ -106,7 +106,7 @@ INSTR_PROF_DATA(const uint32_t, llvm::Type::getInt32Ty(Ctx), NumBitmapBytes, \
 INSTR_PROF_VTABLE_DATA(const uint64_t, llvm::Type::getInt64Ty(Ctx), \
                        VTableNameHash, ConstantInt::get(llvm::Type::getInt64Ty(Ctx), \
                        IndexedInstrProf::ComputeHash(PGOVTableName)))
-INSTR_PROF_VTABLE_DATA(const IntPtrT, llvm::Type::getInt8PtrTy(Ctx), \
+INSTR_PROF_VTABLE_DATA(const IntPtrT, llvm::PointerType::getUnqual(Ctx), \
                        VTablePointer, VTableAddr)
 INSTR_PROF_VTABLE_DATA(const uint32_t, llvm::Type::getInt32Ty(Ctx), VTableSize, \
                        ConstantInt::get(llvm::Type::getInt32Ty(Ctx), \
@@ -210,18 +210,16 @@ VALUE_PROF_KIND(IPVK_MemOPSize, 1, "memory intrinsic functions size")
 /* For virtual table address profiling, the address point of the virtual table
  * (i.e., the address contained in objects pointing to a virtual table) are
  * profiled. Note this may not be the address of the per C++ class virtual table
- * object (e.g., there might be an offset).
+ *  object (e.g., there might be an offset).
  *
  * The profiled addresses are stored in raw profile, together with the following
  * two types of information.
  * 1. The (starting and ending) addresses of per C++ class virtual table objects.
  * 2. The (compressed) virtual table object names.
  * RawInstrProfReader converts profiled virtual table addresses to virtual table
- * objects' MD5 hash.
+ *  objects' MD5 hash.
  */
-VALUE_PROF_KIND(IPVK_VTableTarget, 2, "The address of the compatible vtable (i.e., "
-                                      "there is an offset from this address to a C++ "
-                                      "class virtual table global variable.)")
+VALUE_PROF_KIND(IPVK_VTableTarget, 2, "The profiled address point of the vtable")
 /* These two kinds must be the last to be
  * declared. This is to make sure the string
  * array created with the template can be

compiler-rt/lib/profile/InstrProfilingBuffer.c

@@ -65,6 +65,15 @@ COMPILER_RT_VISIBILITY
 uint64_t __llvm_profile_get_num_data(const __llvm_profile_data *Begin,
                                      const __llvm_profile_data *End) {
   intptr_t BeginI = (intptr_t)Begin, EndI = (intptr_t)End;
+  // `sizeof(__llvm_profile_data) - 1` is required in the numerator when
+  // [Begin, End] represents an inclusive range.
+  // For ELF, [Begin, End) represents the address of linker-inserted
+  // symbols  `__start__<elf-section>` and `__stop_<elf-section>`.
+  // Thereby, `End` is one byte past the inclusive range, and
+  // `sizeof(__llvm_profile_data) - 1` is not necessary in the numerator to get
+  // the correct number of profile data.
+  // FIXME: Consider removing `sizeof(__llvm_profile_data) - 1` if this is true
+  // across platforms.
   return ((EndI + sizeof(__llvm_profile_data) - 1) - BeginI) /
          sizeof(__llvm_profile_data);
 }
@@ -75,11 +84,18 @@ uint64_t __llvm_profile_get_data_size(const __llvm_profile_data *Begin,
   return __llvm_profile_get_num_data(Begin, End) * sizeof(__llvm_profile_data);
 }
 
+// Counts the number of `VTableProfData` elements within the range of [Begin,
+// End). Caller should guarantee that End points to one byte past the inclusive
+// range.
+// FIXME: Add a compiler-rt test to make sure the number of vtables in the
+// raw profile is the same as the number of vtable elements in the instrumented
+// binary.
 COMPILER_RT_VISIBILITY
 uint64_t __llvm_profile_get_num_vtable(const VTableProfData *Begin,
                                        const VTableProfData *End) {
+  // Convert pointers to intptr_t to use integer arithmetic.
   intptr_t EndI = (intptr_t)End, BeginI = (intptr_t)Begin;
-  return (EndI + sizeof(VTableProfData) - 1 - BeginI) / sizeof(VTableProfData);
+  return (EndI - BeginI) / sizeof(VTableProfData);
 }
 
 COMPILER_RT_VISIBILITY
@@ -241,7 +257,7 @@ COMPILER_RT_VISIBILITY int __llvm_profile_write_buffer_internal(
   // Set virtual table arguments to NULL since they are not supported yet.
   return lprofWriteDataImpl(
       &BufferWriter, DataBegin, DataEnd, CountersBegin, CountersEnd,
-      BitmapBegin, BitmapEnd, 0 /* VPDataReader */, NamesBegin, NamesEnd,
-      NULL /* VTableBegin */, NULL /* VTableEnd */, NULL /* VNamesBegin */,
-      NULL /* VNamesEnd */, 0 /* SkipNameDataWrite */);
+      BitmapBegin, BitmapEnd, /*VPDataReader=*/0, NamesBegin, NamesEnd,
+      /*VTableBegin=*/NULL, /*VTableEnd=*/NULL, /*VNamesBegin=*/NULL,
+      /*VNamesEnd=*/NULL, /*SkipNameDataWrite=*/0);
 }

llvm/include/llvm/ProfileData/InstrProfData.inc

@@ -94,19 +94,22 @@ INSTR_PROF_DATA(const uint32_t, llvm::Type::getInt32Ty(Ctx), NumBitmapBytes, \
 #undef INSTR_PROF_DATA
 /* INSTR_PROF_DATA end. */
 
+/* For a virtual table object, record the name hash to associate profiled
+ * addresses with global variables, and record {starting address, size in bytes}
+ * to map the profiled virtual table (which usually have an offset from the
+ * starting address) back to a virtual table object. */
 #ifndef INSTR_PROF_VTABLE_DATA
 #define INSTR_PROF_VTABLE_DATA(Type, LLVMType, Name, Initializer)
 #else
 #define INSTR_PROF_VTABLE_DATA_DEFINED
 #endif
-INSTR_PROF_VTABLE_DATA(
-    const uint64_t, llvm::Type::getInt64Ty(Ctx), VTableNameHash,
-    ConstantInt::get(llvm::Type::getInt64Ty(Ctx),
-                     IndexedInstrProf::ComputeHash(PGOVTableName)))
-INSTR_PROF_VTABLE_DATA(const IntPtrT, llvm::PointerType::getUnqual(Ctx),
+INSTR_PROF_VTABLE_DATA(const uint64_t, llvm::Type::getInt64Ty(Ctx), \
+                       VTableNameHash, ConstantInt::get(llvm::Type::getInt64Ty(Ctx), \
+                       IndexedInstrProf::ComputeHash(PGOVTableName)))
+INSTR_PROF_VTABLE_DATA(const IntPtrT, llvm::PointerType::getUnqual(Ctx), \
                        VTablePointer, VTableAddr)
-INSTR_PROF_VTABLE_DATA(const uint32_t, llvm::Type::getInt32Ty(Ctx), VTableSize,
-                       ConstantInt::get(llvm::Type::getInt32Ty(Ctx),
+INSTR_PROF_VTABLE_DATA(const uint32_t, llvm::Type::getInt32Ty(Ctx), VTableSize, \
+                       ConstantInt::get(llvm::Type::getInt32Ty(Ctx), \
                                         VTableSizeVal))
 #undef INSTR_PROF_VTABLE_DATA
 /* INSTR_PROF_VTABLE_DATA end. */
@@ -204,7 +207,19 @@ VALUE_PROF_FUNC_PARAM(uint32_t, CounterIndex, Type::getInt32Ty(Ctx))
 VALUE_PROF_KIND(IPVK_IndirectCallTarget, 0, "indirect call target")
 /* For memory intrinsic functions size profiling. */
 VALUE_PROF_KIND(IPVK_MemOPSize, 1, "memory intrinsic functions size")
-VALUE_PROF_KIND(IPVK_VTableTarget, 2, "vtable target")
+/* For virtual table address profiling, the address point of the virtual table
+ * (i.e., the address contained in objects pointing to a virtual table) are
+ * profiled. Note this may not be the address of the per C++ class virtual table
+ *  object (e.g., there might be an offset).
+ *
+ * The profiled addresses are stored in raw profile, together with the following
+ * two types of information.
+ * 1. The (starting and ending) addresses of per C++ class virtual table objects.
+ * 2. The (compressed) virtual table object names.
+ * RawInstrProfReader converts profiled virtual table addresses to virtual table
+ *  objects' MD5 hash.
+ */
+VALUE_PROF_KIND(IPVK_VTableTarget, 2, "The profiled address point of the vtable")
 /* These two kinds must be the last to be
  * declared. This is to make sure the string
  * array created with the template can be

llvm/lib/ProfileData/InstrProfWriter.cpp

@@ -609,11 +609,11 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
 
   uint64_t VTableNamesSectionStart = OS.tell();
 
-  // Use an empty string as compressed vtable names and get the necessary
-  // profile format change in place for version 12.
+  // Use a dummy (and uncompressed) string as compressed vtable names and get
+  // the necessary profile format change in place for version 12.
   // TODO: Store the list of vtable names in InstrProfWriter and use the
   // real compressed name.
-  std::string CompressedVTableNames;
+  std::string CompressedVTableNames = "VTableNames";
 
   uint64_t CompressedStringLen = CompressedVTableNames.length();