[BOLT] Fix issue related to missing symbol name

[nhuhuan]

Summary

Add symbol names that previously not recorded in BinaryFunction.
Test cases are updated accordingly.

Test Plan:

ninja check-bolt

[nhuhuan]

Clang-formatted

[Amir]

bolt/include/bolt/Core/BinaryFunction.h

958

We need to avoid this breaking change.
Do we end up with way more alternative names than before for all symbols? Or does it only affect hand-written assembly tests, where main also gets the extra .text symbol? If it's the latter, we may avoid STT_SECTION symbols.

bolt/lib/Rewrite/RewriteInstance.cpp

1050–1052

Can you explain the effect of this condition? Did we skip certain ISyms without this check?

[nhuhuan]

bolt/lib/Rewrite/RewriteInstance.cpp

1050–1052

Yes we skipped some symbol names.
If symbol address points to an existing BF, update BF.Symbols
We have that update after, but since it was previously continue, that update is not executed.

bolt/include/bolt/Core/BinaryFunction.h

const JumpTable *getJumpTableContainingAddress(uint64_t Address) const {
     return const_cast<BinaryFunction *>(this)->getJumpTableContainingAddress(
         Address);
   }
 
   /// Return the name of the function if the function has just one name.
   /// If the function has multiple names - return one followed
   /// by "(*#<numnames>)".
   ///
-  /// We should use getPrintName() for diagnostics and use
+  /// We should use getPrintNameWithNum() for diagnostics and use
   /// hasName() to match function name against a given string.
   ///
   /// NOTE: for disambiguating names of local symbols we use the following
   ///       naming schemes:
   ///           primary:     <function>/<id>
   ///           alternative: <function>/<file>/<id2>
-  std::string getPrintName() const {
+  std::string getPrintNameWithNum() const {
     const size_t NumNames = Symbols.size() + Aliases.size();
     return NumNames == 1
                ? getOneName().str()
                : (getOneName().str() + "(*" + std::to_string(NumNames) + ")");
   }
 
+  std::string getPrintName() const {
+    return getOneName().str();
+  }
+
   /// The function may have many names. For that reason, we avoid having
   /// getName() method as most of the time the user needs a different
   /// interface, such as forEachName(), hasName(), hasNameRegex(), etc.
   /// In some cases though, we need just a name uniquely identifying
   /// the function, and that's what this method is for.
   StringRef getOneName() const { return Symbols[0]->getName(); }
 
   /// Return the name of the function as getPrintName(), but also trying

bolt/lib/Rewrite/RewriteInstance.cpp

if (PreviousFunction && SymbolType != SymbolRef::ST_Function) {
         } else if (Address == PreviousFunction->getAddress() && !SymbolSize) {
           LLVM_DEBUG(dbgs() << "BOLT-DEBUG: ignoring symbol as a marker\n");
         } else if (opts::Verbosity > 1) {
           errs() << "BOLT-WARNING: symbol " << UniqueName
                  << " seen in the middle of function " << *PreviousFunction
                  << ". Could be a new entry.\n";
         }
         registerName(SymbolSize);
+        if (BC->getBinaryFunctions().find(Address) ==
+            BC->getBinaryFunctions().end())
           continue;
       } else if (PreviousFunction->getSize() == 0 &&
                  PreviousFunction->isSymbolValidInScope(Symbol, SymbolSize)) {
         LLVM_DEBUG(dbgs() << "BOLT-DEBUG: symbol is a function local symbol\n");
         registerName(SymbolSize);
+        if (BC->getBinaryFunctions().find(Address) ==
+            BC->getBinaryFunctions().end())
           continue;
       }
     }
 
     if (PreviousFunction && PreviousFunction->containsAddress(Address) &&
         PreviousFunction->getAddress() != Address) {
       if (PreviousFunction->isSymbolValidInScope(Symbol, SymbolSize)) {
         if (opts::Verbosity >= 1)
           outs() << "BOLT-INFO: skipping possibly another entry for function "
if (BFI != BC->getBinaryFunctions().end()) {
             errs() << "BOLT-WARNING: size mismatch for duplicate entries "
                    << *BF << " and " << UniqueName << '\n';
           outs() << "BOLT-INFO: adjusting size of function " << *BF << " old "
                  << BF->getSize() << " new " << SymbolSize << "\n";
         }
         BF->setSize(std::max(SymbolSize, BF->getSize()));
         BC->setBinaryDataSize(Address, BF->getSize());
       }
+      // Avoid adding duplicate symbol name
+      if (!BF->hasName(UniqueName))
         BF->addAlternativeName(UniqueName);
     } else {
       ErrorOr<BinarySection &> Section = BC->getSectionForAddress(Address);
       // Skip symbols from invalid sections
       if (!Section) {
         errs() << "BOLT-WARNING: " << UniqueName << " (0x"
                << Twine::utohexstr(Address) << ") does not have any section\n";
         continue;
       }

bolt/test/X86/split-func-icf.s

 # This reproduces an issue where two cold fragments are folded into one, so the
 # fragment has two parents.
 # The fragment is only reachable through a jump table, so all functions must be
 # ignored.
 
 # REQUIRES: system-linux
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
 # RUN: llvm-strip --strip-unneeded %t.o
 # RUN: %clang %cflags %t.o -o %t.exe -Wl,-q
 # RUN: llvm-bolt %t.exe -o %t.out -v=1 --print-only=main2.cold.1 --print-disasm 2>&1 | FileCheck %s
 
 # CHECK-NOT: unclaimed PC-relative relocations left in data
-# CHECK-DAG: BOLT-INFO: marking main2.cold.1(*2) as a fragment of main
-# CHECK-DAG: BOLT-INFO: marking main2.cold.1(*2) as a fragment of main2
-# CHECK: Binary Function "main2.cold.1(*2)" after disassembly
-# CHECK: End of Function "main2.cold.1(*2)"
+# CHECK-DAG: BOLT-INFO: marking main2.cold.1 as a fragment of main
+# CHECK-DAG: BOLT-INFO: marking main2.cold.1 as a fragment of main2
+# CHECK: Binary Function "main2.cold.1" after disassembly
+# CHECK: End of Function "main2.cold.1"
 # CHECK-DAG: BOLT-WARNING: Ignoring main2
 # CHECK-DAG: BOLT-WARNING: Ignoring main
-# CHECK-DAG: BOLT-WARNING: Ignoring main2.cold.1(*2)
+# CHECK-DAG: BOLT-WARNING: Ignoring main2.cold.1
 # CHECK: BOLT-WARNING: skipped 3 functions due to cold fragments
   .text
   .globl main
   .type main, %function
   .p2align 2
 main:
 LBB0:
   andl $0xf, %ecx

bolt/test/runtime/X86/fdata-escape-chars.ll

 ; INSTR_CHECK: Exec Count  : 4
 ; INSTR_CHECK: Binary Function "symb backslash\"
 ; INSTR_CHECK: Exec Count  : 2
 ; INSTR_CHECK: {{([[:xdigit:]]+)}}:   callq   "symb whitespace" # Count: 2
 ; INSTR_CHECK: Binary Function "main"
 ; INSTR_CHECK: Exec Count  : 1
 ; INSTR_CHECK: {{([[:xdigit:]]+)}}:   callq   "symb whitespace" # Count: 1
 ; INSTR_CHECK: {{([[:xdigit:]]+)}}:   callq   "symb backslash\" # Count: 2
-; INSTR_CHECK: Binary Function "static symb backslash\/1(*2)"
+; INSTR_CHECK: Binary Function "static symb backslash\/1"
 ; INSTR_CHECK: Exec Count  : 1
 ; INSTR_CHECK: {{([[:xdigit:]]+)}}:   callq   "symb whitespace" # Count: 1
 
 ; PREAGR_FDATA_CHECK: 1 symb\ backslash\\ 0 1 symb\ whitespace 0 0 2
 ; PREAGR_FDATA_CHECK: 1 main 0 1 static\ symb\ backslash\\/1 0 0 1
 ; PREAGR_FDATA_CHECK: 1 main 0 1 symb\ whitespace 0 0 1
 ; PREAGR_FDATA_CHECK: 1 main 0 1 symb\ backslash\\ 0 0 2
 ; PREAGR_FDATA_CHECK: 1 static\ symb\ backslash\\/1 0 1 symb\ whitespace 0 0 1
 
 ; PREAGR_CHECK: Binary Function "symb whitespace"
 ; PREAGR_CHECK-DAG: Exec Count  : 4
 ; PREAGR_CHECK: Binary Function "symb backslash\"
 ; PREAGR_CHECK-DAG: Exec Count  : 2
-; PREAGR_CHECK: Binary Function "static symb backslash\/1(*2)"
+; PREAGR_CHECK: Binary Function "static symb backslash\/1"
 ; PREAGR_CHECK-DAG: Exec Count  : 1