Merge branch 'main' into gisel-vop3p

Author: Shoreshen

.github/workflows/containers/github-action-ci/Dockerfile

@@ -32,7 +32,7 @@ RUN cmake -B ./build -G Ninja ./llvm \
   -DLLVM_ENABLE_RUNTIMES="compiler-rt" \
   -DCMAKE_INSTALL_PREFIX="$LLVM_SYSROOT" \
   -DLLVM_ENABLE_PROJECTS="bolt;clang;lld;clang-tools-extra" \
-  -DLLVM_DISTRIBUTION_COMPONENTS="lld;compiler-rt;clang-format;scan-build" \
+  -DLLVM_DISTRIBUTION_COMPONENTS="lld;compiler-rt;clang-format;scan-build;llvm-symbolizer" \
   -DCLANG_DEFAULT_LINKER="lld"
 
 RUN ninja -C ./build stage2-clang-bolt stage2-install-distribution && ninja -C ./build install-distribution

.mailmap

@@ -29,7 +29,8 @@
 <[email protected]> <[email protected]>
 <[email protected]> <[email protected]>
 <[email protected]> <[email protected]>
-<[email protected]> <[email protected]>
+Min Hsu <[email protected]> <[email protected]>
+Min Hsu <[email protected]> <[email protected]>
 <[email protected]> <[email protected]> <[email protected]>
 <[email protected]> <[email protected]>
 <[email protected]> <[email protected]>

bolt/lib/Core/BinaryContext.cpp

@@ -579,6 +579,21 @@ bool BinaryContext::analyzeJumpTable(const uint64_t Address,
       TrimmedSize = EntriesAsAddress->size();
   };
 
+  auto printEntryDiagnostics = [&](raw_ostream &OS,
+                                   const BinaryFunction *TargetBF) {
+    OS << "FAIL: function doesn't contain this address\n";
+    if (!TargetBF)
+      return;
+    OS << "  ! function containing this address: " << *TargetBF << '\n';
+    if (!TargetBF->isFragment())
+      return;
+    OS << "  ! is a fragment with parents: ";
+    ListSeparator LS;
+    for (BinaryFunction *Parent : TargetBF->ParentFragments)
+      OS << LS << *Parent;
+    OS << '\n';
+  };
+
   ErrorOr<const BinarySection &> Section = getSectionForAddress(Address);
   if (!Section)
     return false;
@@ -646,25 +661,9 @@ bool BinaryContext::analyzeJumpTable(const uint64_t Address,
 
     // Function or one of its fragments.
     const BinaryFunction *TargetBF = getBinaryFunctionContainingAddress(Value);
-    const bool DoesBelongToFunction =
-        BF.containsAddress(Value) ||
-        (TargetBF && areRelatedFragments(TargetBF, &BF));
-    if (!DoesBelongToFunction) {
-      LLVM_DEBUG({
-        if (!BF.containsAddress(Value)) {
-          dbgs() << "FAIL: function doesn't contain this address\n";
-          if (TargetBF) {
-            dbgs() << "  ! function containing this address: "
-                   << TargetBF->getPrintName() << '\n';
-            if (TargetBF->isFragment()) {
-              dbgs() << "  ! is a fragment";
-              for (BinaryFunction *Parent : TargetBF->ParentFragments)
-                dbgs() << ", parent: " << Parent->getPrintName();
-              dbgs() << '\n';
-            }
-          }
-        }
-      });
+    if (!TargetBF || !areRelatedFragments(TargetBF, &BF)) {
+      LLVM_DEBUG(printEntryDiagnostics(dbgs(), TargetBF));
+      (void)printEntryDiagnostics;
       break;
     }
 
@@ -703,10 +702,7 @@ void BinaryContext::populateJumpTables() {
        ++JTI) {
     JumpTable *JT = JTI->second;
 
-    bool NonSimpleParent = false;
-    for (BinaryFunction *BF : JT->Parents)
-      NonSimpleParent |= !BF->isSimple();
-    if (NonSimpleParent)
+    if (!llvm::all_of(JT->Parents, std::mem_fn(&BinaryFunction::isSimple)))
       continue;
 
     uint64_t NextJTAddress = 0;
@@ -840,33 +836,26 @@ BinaryContext::getOrCreateJumpTable(BinaryFunction &Function, uint64_t Address,
     assert(JT->Type == Type && "jump table types have to match");
     assert(Address == JT->getAddress() && "unexpected non-empty jump table");
 
-    // Prevent associating a jump table to a specific fragment twice.
-    if (!llvm::is_contained(JT->Parents, &Function)) {
-      assert(llvm::all_of(JT->Parents,
-                          [&](const BinaryFunction *BF) {
-                            return areRelatedFragments(&Function, BF);
-                          }) &&
-             "cannot re-use jump table of a different function");
-      // Duplicate the entry for the parent function for easy access
-      JT->Parents.push_back(&Function);
-      if (opts::Verbosity > 2) {
-        this->outs() << "BOLT-INFO: Multiple fragments access same jump table: "
-                     << JT->Parents[0]->getPrintName() << "; "
-                     << Function.getPrintName() << "\n";
-        JT->print(this->outs());
-      }
-      Function.JumpTables.emplace(Address, JT);
-      for (BinaryFunction *Parent : JT->Parents)
-        Parent->setHasIndirectTargetToSplitFragment(true);
-    }
+    if (llvm::is_contained(JT->Parents, &Function))
+      return JT->getFirstLabel();
 
-    bool IsJumpTableParent = false;
-    (void)IsJumpTableParent;
-    for (BinaryFunction *Frag : JT->Parents)
-      if (Frag == &Function)
-        IsJumpTableParent = true;
-    assert(IsJumpTableParent &&
+    // Prevent associating a jump table to a specific fragment twice.
+    auto isSibling = std::bind(&BinaryContext::areRelatedFragments, this,
+                               &Function, std::placeholders::_1);
+    assert(llvm::all_of(JT->Parents, isSibling) &&
            "cannot re-use jump table of a different function");
+    (void)isSibling;
+    if (opts::Verbosity > 2) {
+      this->outs() << "BOLT-INFO: multiple fragments access the same jump table"
+                   << ": " << *JT->Parents[0] << "; " << Function << '\n';
+      JT->print(this->outs());
+    }
+    if (JT->Parents.size() == 1)
+      JT->Parents.front()->setHasIndirectTargetToSplitFragment(true);
+    Function.setHasIndirectTargetToSplitFragment(true);
+    // Duplicate the entry for the parent function for easy access
+    JT->Parents.push_back(&Function);
+    Function.JumpTables.emplace(Address, JT);
     return JT->getFirstLabel();
   }
 

bolt/lib/Core/BinaryEmitter.cpp

@@ -809,52 +809,50 @@ void BinaryEmitter::emitJumpTable(const JumpTable &JT, MCSection *HotSection,
     Streamer.switchSection(JT.Count > 0 ? HotSection : ColdSection);
     Streamer.emitValueToAlignment(Align(JT.EntrySize));
   }
-  MCSymbol *LastLabel = nullptr;
+  MCSymbol *JTLabel = nullptr;
   uint64_t Offset = 0;
   for (MCSymbol *Entry : JT.Entries) {
     auto LI = JT.Labels.find(Offset);
-    if (LI != JT.Labels.end()) {
-      LLVM_DEBUG({
-        dbgs() << "BOLT-DEBUG: emitting jump table " << LI->second->getName()
-               << " (originally was at address 0x"
-               << Twine::utohexstr(JT.getAddress() + Offset)
-               << (Offset ? ") as part of larger jump table\n" : ")\n");
-      });
-      if (!LabelCounts.empty()) {
-        LLVM_DEBUG(dbgs() << "BOLT-DEBUG: jump table count: "
-                          << LabelCounts[LI->second] << '\n');
-        if (LabelCounts[LI->second] > 0)
-          Streamer.switchSection(HotSection);
-        else
-          Streamer.switchSection(ColdSection);
-        Streamer.emitValueToAlignment(Align(JT.EntrySize));
-      }
-      // Emit all labels registered at the address of this jump table
-      // to sync with our global symbol table.  We may have two labels
-      // registered at this address if one label was created via
-      // getOrCreateGlobalSymbol() (e.g. LEA instructions referencing
-      // this location) and another via getOrCreateJumpTable().  This
-      // creates a race where the symbols created by these two
-      // functions may or may not be the same, but they are both
-      // registered in our symbol table at the same address. By
-      // emitting them all here we make sure there is no ambiguity
-      // that depends on the order that these symbols were created, so
-      // whenever this address is referenced in the binary, it is
-      // certain to point to the jump table identified at this
-      // address.
-      if (BinaryData *BD = BC.getBinaryDataByName(LI->second->getName())) {
-        for (MCSymbol *S : BD->getSymbols())
-          Streamer.emitLabel(S);
-      } else {
-        Streamer.emitLabel(LI->second);
-      }
-      LastLabel = LI->second;
+    if (LI == JT.Labels.end())
+      goto emitEntry;
+    JTLabel = LI->second;
+    LLVM_DEBUG({
+      dbgs() << "BOLT-DEBUG: emitting jump table " << JTLabel->getName()
+             << " (originally was at address 0x"
+             << Twine::utohexstr(JT.getAddress() + Offset)
+             << (Offset ? ") as part of larger jump table\n" : ")\n");
+    });
+    if (!LabelCounts.empty()) {
+      const uint64_t JTCount = LabelCounts[JTLabel];
+      LLVM_DEBUG(dbgs() << "BOLT-DEBUG: jump table count: " << JTCount << '\n');
+      Streamer.switchSection(JTCount ? HotSection : ColdSection);
+      Streamer.emitValueToAlignment(Align(JT.EntrySize));
+    }
+    // Emit all labels registered at the address of this jump table
+    // to sync with our global symbol table.  We may have two labels
+    // registered at this address if one label was created via
+    // getOrCreateGlobalSymbol() (e.g. LEA instructions referencing
+    // this location) and another via getOrCreateJumpTable().  This
+    // creates a race where the symbols created by these two
+    // functions may or may not be the same, but they are both
+    // registered in our symbol table at the same address. By
+    // emitting them all here we make sure there is no ambiguity
+    // that depends on the order that these symbols were created, so
+    // whenever this address is referenced in the binary, it is
+    // certain to point to the jump table identified at this
+    // address.
+    if (BinaryData *BD = BC.getBinaryDataByName(JTLabel->getName())) {
+      for (MCSymbol *S : BD->getSymbols())
+        Streamer.emitLabel(S);
+    } else {
+      Streamer.emitLabel(JTLabel);
     }
+  emitEntry:
     if (JT.Type == JumpTable::JTT_NORMAL) {
       Streamer.emitSymbolValue(Entry, JT.OutputEntrySize);
     } else { // JTT_PIC
       const MCSymbolRefExpr *JTExpr =
-          MCSymbolRefExpr::create(LastLabel, Streamer.getContext());
+          MCSymbolRefExpr::create(JTLabel, Streamer.getContext());
       const MCSymbolRefExpr *E =
           MCSymbolRefExpr::create(Entry, Streamer.getContext());
       const MCBinaryExpr *Value =

bolt/lib/Passes/AsmDump.cpp

@@ -134,13 +134,14 @@ void dumpFunction(const BinaryFunction &BF) {
   std::unique_ptr<MCAsmBackend> MAB(
       BC.TheTarget->createMCAsmBackend(*BC.STI, *BC.MRI, MCTargetOptions()));
   int AsmPrinterVariant = BC.AsmInfo->getAssemblerDialect();
-  MCInstPrinter *InstructionPrinter(BC.TheTarget->createMCInstPrinter(
-      *BC.TheTriple, AsmPrinterVariant, *BC.AsmInfo, *BC.MII, *BC.MRI));
+  std::unique_ptr<MCInstPrinter> InstructionPrinter(
+      BC.TheTarget->createMCInstPrinter(*BC.TheTriple, AsmPrinterVariant,
+                                        *BC.AsmInfo, *BC.MII, *BC.MRI));
   auto FOut = std::make_unique<formatted_raw_ostream>(OS);
   FOut->SetUnbuffered();
-  std::unique_ptr<MCStreamer> AsmStreamer(
-      createAsmStreamer(*LocalCtx, std::move(FOut), InstructionPrinter,
-                        std::move(MCEInstance.MCE), std::move(MAB)));
+  std::unique_ptr<MCStreamer> AsmStreamer(createAsmStreamer(
+      *LocalCtx, std::move(FOut), std::move(InstructionPrinter),
+      std::move(MCEInstance.MCE), std::move(MAB)));
   AsmStreamer->initSections(true, *BC.STI);
   std::unique_ptr<TargetMachine> TM(BC.TheTarget->createTargetMachine(
       *BC.TheTriple, "", "", TargetOptions(), std::nullopt));

bolt/lib/Profile/DataAggregator.cpp

@@ -871,18 +871,27 @@ DataAggregator::getFallthroughsInTrace(BinaryFunction &BF,
 
   BinaryContext &BC = BF.getBinaryContext();
 
-  if (!BF.isSimple())
-    return std::nullopt;
-
-  assert(BF.hasCFG() && "can only record traces in CFG state");
-
   // Offsets of the trace within this function.
   const uint64_t From = FirstLBR.To - BF.getAddress();
   const uint64_t To = SecondLBR.From - BF.getAddress();
 
   if (From > To)
     return std::nullopt;
 
+  // Accept fall-throughs inside pseudo functions (PLT/thunks).
+  // This check has to be above BF.empty as pseudo functions would pass it:
+  // pseudo => ignored => CFG not built => empty.
+  // If we return nullopt, trace would be reported as mismatching disassembled
+  // function contents which it is not. To avoid this, return an empty
+  // fall-through list instead.
+  if (BF.isPseudo())
+    return Branches;
+
+  if (!BF.isSimple())
+    return std::nullopt;
+
+  assert(BF.hasCFG() && "can only record traces in CFG state");
+
   const BinaryBasicBlock *FromBB = BF.getBasicBlockContainingOffset(From);
   const BinaryBasicBlock *ToBB = BF.getBasicBlockContainingOffset(To);
 

bolt/test/X86/callcont-fallthru.s

@@ -9,6 +9,7 @@
 # RUN: link_fdata %s %t %t.pa3 PREAGG3
 # RUN: link_fdata %s %t %t.pat PREAGGT1
 # RUN: link_fdata %s %t %t.pat2 PREAGGT2
+# RUN: link_fdata %s %t %t.patplt PREAGGPLT
 
 ## Check normal case: fallthrough is not LP or secondary entry.
 # RUN: llvm-strip --strip-unneeded %t -o %t.strip
@@ -42,6 +43,12 @@
 # RUN: llvm-bolt %t.strip --pa -p %t.pat2 -o %t.out \
 # RUN:   --print-cfg --print-only=main | FileCheck %s --check-prefix=CHECK3
 
+## Check pre-aggregated traces don't report zero-sized PLT fall-through as
+## invalid trace
+# RUN: llvm-bolt %t.strip --pa -p %t.patplt -o %t.out | FileCheck %s \
+# RUN:   --check-prefix=CHECK-PLT
+# CHECK-PLT: traces mismatching disassembled function contents: 0
+
   .globl foo
   .type foo, %function
 foo:
@@ -65,7 +72,10 @@ main:
 	movl	$0x0, -0x4(%rbp)
 	movl	%edi, -0x8(%rbp)
 	movq	%rsi, -0x10(%rbp)
+Ltmp0_br:
 	callq	puts@PLT
+## Check PLT traces are accepted
+# PREAGGPLT: T #Ltmp0_br# #puts@plt# #puts@plt# 3
 ## Target is an external-origin call continuation
 # PREAGG1: B X:0 #Ltmp1# 2 0
 # PREAGGT1: T X:0 #Ltmp1# #Ltmp4_br# 2

bolt/test/X86/split-func-jump-table-fragment-bidirection.s

@@ -10,7 +10,7 @@
 # RUN: %clang %cflags %t.o -o %t.exe -Wl,-q
 # RUN: llvm-bolt -print-cfg -v=3 %t.exe -o %t.out 2>&1 | FileCheck %s
 
-# CHECK: BOLT-INFO: Multiple fragments access same jump table: main; main.cold.1
+# CHECK: BOLT-INFO: multiple fragments access the same jump table: main; main.cold.1
 # CHECK: PIC Jump table JUMP_TABLE1 for function main, main.cold.1 at {{.*}}  with a total count of 0:
 
   .text

bolt/test/link_fdata.py

@@ -8,6 +8,7 @@
 """
 
 import argparse
+import os
 import subprocess
 import sys
 import re
@@ -84,8 +85,16 @@
             exit("ERROR: unexpected input:\n%s" % line)
 
 # Read nm output: <symbol value> <symbol type> <symbol name>
+is_llvm_nm = os.path.basename(args.nmtool) == "llvm-nm"
 nm_output = subprocess.run(
-    [args.nmtool, "--defined-only", args.objfile], text=True, capture_output=True
+    [
+        args.nmtool,
+        "--defined-only",
+        "--special-syms" if is_llvm_nm else "--synthetic",
+        args.objfile,
+    ],
+    text=True,
+    capture_output=True,
 ).stdout
 # Populate symbol map
 symbols = {}

clang-tools-extra/clang-tidy/bugprone/UnintendedCharOstreamOutputCheck.cpp

@@ -7,6 +7,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "UnintendedCharOstreamOutputCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
@@ -35,10 +37,14 @@ AST_MATCHER(Type, isChar) {
 
 UnintendedCharOstreamOutputCheck::UnintendedCharOstreamOutputCheck(
     StringRef Name, ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context), CastTypeName(Options.get("CastTypeName")) {
-}
+    : ClangTidyCheck(Name, Context),
+      AllowedTypes(utils::options::parseStringList(
+          Options.get("AllowedTypes", "unsigned char;signed char"))),
+      CastTypeName(Options.get("CastTypeName")) {}
 void UnintendedCharOstreamOutputCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AllowedTypes",
+                utils::options::serializeStringList(AllowedTypes));
   if (CastTypeName.has_value())
     Options.store(Opts, "CastTypeName", CastTypeName.value());
 }
@@ -50,13 +56,20 @@ void UnintendedCharOstreamOutputCheck::registerMatchers(MatchFinder *Finder) {
                     // with char / unsigned char / signed char
                     classTemplateSpecializationDecl(
                         hasTemplateArgument(0, refersToType(isChar()))));
+  auto IsDeclRefExprFromAllowedTypes = declRefExpr(to(varDecl(
+      hasType(matchers::matchesAnyListedTypeName(AllowedTypes, false)))));
+  auto IsExplicitCastExprFromAllowedTypes = explicitCastExpr(hasDestinationType(
+      matchers::matchesAnyListedTypeName(AllowedTypes, false)));
   Finder->addMatcher(
       cxxOperatorCallExpr(
           hasOverloadedOperatorName("<<"),
           hasLHS(hasType(hasUnqualifiedDesugaredType(
               recordType(hasDeclaration(cxxRecordDecl(
                   anyOf(BasicOstream, isDerivedFrom(BasicOstream)))))))),
-          hasRHS(hasType(hasUnqualifiedDesugaredType(isNumericChar()))))
+          hasRHS(expr(hasType(hasUnqualifiedDesugaredType(isNumericChar())),
+                      unless(ignoringParenImpCasts(
+                          anyOf(IsDeclRefExprFromAllowedTypes,
+                                IsExplicitCastExprFromAllowedTypes))))))
           .bind("x"),
       this);
 }