Stop abusing Twine in DiagnosticInfo #136371

bogner · 2025-04-18T21:29:36Z

This switches the various DiagnosticInfo subclasses that store an un-owned string to use StringRef for this instead of Twine.

Storing a Twine & like most of these were doing makes it very easy to introduce memory corruption bugs:

void f(std::string &Msg) {
  auto Diag = DiagnosticInfoGeneric(Msg);
  Ctx.diagnose(Diag); // Bug! We're referring to the temporary Twine!
  Ctx.diagnose(DiagnosticInfoGeneric(Msg); // this works, I guess...
};

This was sort of mitigated in DiagnosticInfoUnsupported, which actually copies the Twine, but copying Twines is always a questionable practice.

Instead, we store a StringRef, which makes it much more explicit that ownership of the storage is the caller's problem.

This switches the various DiagnosticInfo subclasses that store an un-owned string to use StringRef for this instead of Twine. Storing a `Twine &` like most of these were doing makes it very easy to introduce memory corruption bugs: ```c++ void f(std::string &Msg) { auto Diag = DiagnosticInfoGeneric(Msg); Ctx.diagnose(Diag); // Bug! We're referring to the temporary Twine! Ctx.diagnose(DiagnosticInfoGeneric(Msg); // this works, I guess... }; ``` This was sort of mitigated in DiagnosticInfoUnsupported, which actually copies the Twine, but copying Twines is always a questionable practice. Instead, we store a StringRef, which makes it much more explicit that ownership of the storage is the caller's problem.

llvmbot · 2025-04-18T21:30:09Z

@llvm/pr-subscribers-lto
@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-backend-directx

@llvm/pr-subscribers-pgo

Author: Justin Bogner (bogner)

Changes

This switches the various DiagnosticInfo subclasses that store an un-owned string to use StringRef for this instead of Twine.

Storing a Twine & like most of these were doing makes it very easy to introduce memory corruption bugs:

void f(std::string &amp;Msg) {
  auto Diag = DiagnosticInfoGeneric(Msg);
  Ctx.diagnose(Diag); // Bug! We're referring to the temporary Twine!
  Ctx.diagnose(DiagnosticInfoGeneric(Msg); // this works, I guess...
};

This was sort of mitigated in DiagnosticInfoUnsupported, which actually copies the Twine, but copying Twines is always a questionable practice.

Instead, we store a StringRef, which makes it much more explicit that ownership of the storage is the caller's problem.

Patch is 36.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/136371.diff

11 Files Affected:

(modified) llvm/include/llvm/IR/DiagnosticInfo.h (+40-41)
(modified) llvm/include/llvm/ProfileData/SampleProfReader.h (+3-2)
(modified) llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h (+19-14)
(modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp (+4-3)
(modified) llvm/lib/CodeGen/MachineInstr.cpp (+5-3)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+6-4)
(modified) llvm/lib/IR/DiagnosticInfo.cpp (+5-6)
(modified) llvm/lib/IR/LLVMContext.cpp (+5-2)
(modified) llvm/lib/Target/DirectX/DXILRootSignature.cpp (+2-3)
(modified) llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (+44-47)
(modified) llvm/lib/Transforms/Utils/MisExpect.cpp (+4-5)

diff --git a/llvm/include/llvm/IR/DiagnosticInfo.h b/llvm/include/llvm/IR/DiagnosticInfo.h
index 8743f8058c382..151f323447bfb 100644
--- a/llvm/include/llvm/IR/DiagnosticInfo.h
+++ b/llvm/include/llvm/IR/DiagnosticInfo.h
@@ -18,7 +18,6 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Twine.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/Support/CBindingWrapping.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -139,22 +138,22 @@ class DiagnosticInfo {
 using DiagnosticHandlerFunction = std::function<void(const DiagnosticInfo &)>;
 
 class DiagnosticInfoGeneric : public DiagnosticInfo {
-  const Twine &MsgStr;
+  StringRef MsgStr;
   const Instruction *Inst = nullptr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoGeneric(const Twine &MsgStr,
+  DiagnosticInfoGeneric(StringRef MsgStr,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(MsgStr) {}
 
-  DiagnosticInfoGeneric(const Instruction *I, const Twine &ErrMsg,
+  DiagnosticInfoGeneric(const Instruction *I, StringRef ErrMsg,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(ErrMsg), Inst(I) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
   const Instruction *getInstruction() const { return Inst; }
 
   /// \see DiagnosticInfo::print.
@@ -172,7 +171,7 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// Optional line information. 0 if not set.
   uint64_t LocCookie = 0;
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
   /// Optional origin of the problem.
   const Instruction *Instr = nullptr;
 
@@ -181,7 +180,7 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// \p MsgStr gives the message.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoInlineAsm(uint64_t LocCookie, const Twine &MsgStr,
+  DiagnosticInfoInlineAsm(uint64_t LocCookie, StringRef MsgStr,
                           DiagnosticSeverity Severity = DS_Error);
 
   /// \p Instr gives the original instruction that triggered the diagnostic.
@@ -189,11 +188,11 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
   /// Same for \p I.
-  DiagnosticInfoInlineAsm(const Instruction &I, const Twine &MsgStr,
+  DiagnosticInfoInlineAsm(const Instruction &I, StringRef MsgStr,
                           DiagnosticSeverity Severity = DS_Error);
 
   uint64_t getLocCookie() const { return LocCookie; }
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
   const Instruction *getInstruction() const { return Instr; }
 
   /// \see DiagnosticInfo::print.
@@ -258,15 +257,15 @@ class DiagnosticInfoIgnoringInvalidDebugMetadata : public DiagnosticInfo {
 class DiagnosticInfoSampleProfile : public DiagnosticInfo {
 public:
   DiagnosticInfoSampleProfile(StringRef FileName, unsigned LineNum,
-                              const Twine &Msg,
+                              StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), FileName(FileName),
         LineNum(LineNum), Msg(Msg) {}
-  DiagnosticInfoSampleProfile(StringRef FileName, const Twine &Msg,
+  DiagnosticInfoSampleProfile(StringRef FileName, StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), FileName(FileName),
         Msg(Msg) {}
-  DiagnosticInfoSampleProfile(const Twine &Msg,
+  DiagnosticInfoSampleProfile(StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), Msg(Msg) {}
 
@@ -279,7 +278,7 @@ class DiagnosticInfoSampleProfile : public DiagnosticInfo {
 
   StringRef getFileName() const { return FileName; }
   unsigned getLineNum() const { return LineNum; }
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Name of the input file associated with this diagnostic.
@@ -290,13 +289,13 @@ class DiagnosticInfoSampleProfile : public DiagnosticInfo {
   unsigned LineNum = 0;
 
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 /// Diagnostic information for the PGO profiler.
 class DiagnosticInfoPGOProfile : public DiagnosticInfo {
 public:
-  DiagnosticInfoPGOProfile(const char *FileName, const Twine &Msg,
+  DiagnosticInfoPGOProfile(const char *FileName, StringRef Msg,
                            DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_PGOProfile, Severity), FileName(FileName), Msg(Msg) {}
 
@@ -308,14 +307,14 @@ class DiagnosticInfoPGOProfile : public DiagnosticInfo {
   }
 
   const char *getFileName() const { return FileName; }
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Name of the input file associated with this diagnostic.
   const char *FileName;
 
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 class DiagnosticLocation {
@@ -364,7 +363,7 @@ class DiagnosticInfoWithLocationBase : public DiagnosticInfo {
 
   /// Return the absolute path tot the file.
   std::string getAbsolutePath() const;
-  
+
   const Function &getFunction() const { return Fn; }
   DiagnosticLocation getLocation() const { return Loc; }
 
@@ -379,19 +378,19 @@ class DiagnosticInfoWithLocationBase : public DiagnosticInfo {
 class DiagnosticInfoGenericWithLoc : public DiagnosticInfoWithLocationBase {
 private:
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoGenericWithLoc(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoGenericWithLoc(StringRef MsgStr, const Function &Fn,
                                const DiagnosticLocation &Loc,
                                DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfoWithLocationBase(DK_GenericWithLoc, Severity, Fn, Loc),
         MsgStr(MsgStr) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -404,20 +403,20 @@ class DiagnosticInfoGenericWithLoc : public DiagnosticInfoWithLocationBase {
 class DiagnosticInfoRegAllocFailure : public DiagnosticInfoWithLocationBase {
 private:
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoRegAllocFailure(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoRegAllocFailure(StringRef MsgStr, const Function &Fn,
                                 const DiagnosticLocation &DL,
                                 DiagnosticSeverity Severity = DS_Error);
 
-  DiagnosticInfoRegAllocFailure(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoRegAllocFailure(StringRef MsgStr, const Function &Fn,
                                 DiagnosticSeverity Severity = DS_Error);
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -707,7 +706,7 @@ class DiagnosticInfoIROptimization : public DiagnosticInfoOptimizationBase {
   DiagnosticInfoIROptimization(enum DiagnosticKind Kind,
                                enum DiagnosticSeverity Severity,
                                const char *PassName, const Function &Fn,
-                               const DiagnosticLocation &Loc, const Twine &Msg)
+                               const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoOptimizationBase(Kind, Severity, PassName, "", Fn, Loc) {
     *this << Msg.str();
   }
@@ -764,7 +763,7 @@ class OptimizationRemark : public DiagnosticInfoIROptimization {
   /// Note that this class does not copy this message, so this reference
   /// must be valid for the whole life time of the diagnostic.
   OptimizationRemark(const char *PassName, const Function &Fn,
-                     const DiagnosticLocation &Loc, const Twine &Msg)
+                     const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemark, DS_Remark, PassName,
                                      Fn, Loc, Msg) {}
 };
@@ -810,7 +809,7 @@ class OptimizationRemarkMissed : public DiagnosticInfoIROptimization {
   /// Note that this class does not copy this message, so this reference
   /// must be valid for the whole life time of the diagnostic.
   OptimizationRemarkMissed(const char *PassName, const Function &Fn,
-                           const DiagnosticLocation &Loc, const Twine &Msg)
+                           const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemarkMissed, DS_Remark,
                                      PassName, Fn, Loc, Msg) {}
 };
@@ -863,7 +862,7 @@ class OptimizationRemarkAnalysis : public DiagnosticInfoIROptimization {
 protected:
   OptimizationRemarkAnalysis(enum DiagnosticKind Kind, const char *PassName,
                              const Function &Fn, const DiagnosticLocation &Loc,
-                             const Twine &Msg)
+                             StringRef Msg)
       : DiagnosticInfoIROptimization(Kind, DS_Remark, PassName, Fn, Loc, Msg) {}
 
   OptimizationRemarkAnalysis(enum DiagnosticKind Kind, const char *PassName,
@@ -882,7 +881,7 @@ class OptimizationRemarkAnalysis : public DiagnosticInfoIROptimization {
   /// this class does not copy this message, so this reference must be valid for
   /// the whole life time of the diagnostic.
   OptimizationRemarkAnalysis(const char *PassName, const Function &Fn,
-                             const DiagnosticLocation &Loc, const Twine &Msg)
+                             const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemarkAnalysis, DS_Remark,
                                      PassName, Fn, Loc, Msg) {}
 };
@@ -924,7 +923,7 @@ class OptimizationRemarkAnalysisFPCommute : public OptimizationRemarkAnalysis {
   /// diagnostic.
   OptimizationRemarkAnalysisFPCommute(const char *PassName, const Function &Fn,
                                       const DiagnosticLocation &Loc,
-                                      const Twine &Msg)
+                                      StringRef Msg)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisFPCommute,
                                    PassName, Fn, Loc, Msg) {}
 };
@@ -965,7 +964,7 @@ class OptimizationRemarkAnalysisAliasing : public OptimizationRemarkAnalysis {
   /// diagnostic.
   OptimizationRemarkAnalysisAliasing(const char *PassName, const Function &Fn,
                                      const DiagnosticLocation &Loc,
-                                     const Twine &Msg)
+                                     StringRef Msg)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisAliasing,
                                    PassName, Fn, Loc, Msg) {}
 };
@@ -991,10 +990,10 @@ class DiagnosticInfoMIRParser : public DiagnosticInfo {
 
 /// Diagnostic information for IR instrumentation reporting.
 class DiagnosticInfoInstrumentation : public DiagnosticInfo {
-  const Twine &Msg;
+  StringRef Msg;
 
 public:
-  DiagnosticInfoInstrumentation(const Twine &DiagMsg,
+  DiagnosticInfoInstrumentation(StringRef DiagMsg,
                                 DiagnosticSeverity Severity = DS_Warning)
       : DiagnosticInfo(DK_Instrumentation, Severity), Msg(DiagMsg) {}
 
@@ -1038,7 +1037,7 @@ class DiagnosticInfoOptimizationFailure : public DiagnosticInfoIROptimization {
   /// of the diagnostic.
   DiagnosticInfoOptimizationFailure(const Function &Fn,
                                     const DiagnosticLocation &Loc,
-                                    const Twine &Msg)
+                                    StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationFailure, DS_Warning,
                                      nullptr, Fn, Loc, Msg) {}
 
@@ -1062,7 +1061,7 @@ class DiagnosticInfoOptimizationFailure : public DiagnosticInfoIROptimization {
 /// Diagnostic information for unsupported feature in backend.
 class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
 private:
-  Twine Msg;
+  StringRef Msg;
 
 public:
   /// \p Fn is the function where the diagnostic is being emitted. \p Loc is
@@ -1072,7 +1071,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
   /// copy this message, so this reference must be valid for the whole life time
   /// of the diagnostic.
   DiagnosticInfoUnsupported(
-      const Function &Fn, const Twine &Msg,
+      const Function &Fn, StringRef Msg,
       const DiagnosticLocation &Loc = DiagnosticLocation(),
       DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfoWithLocationBase(DK_Unsupported, Severity, Fn, Loc),
@@ -1082,7 +1081,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
     return DI->getKind() == DK_Unsupported;
   }
 
-  const Twine &getMessage() const { return Msg; }
+  const StringRef getMessage() const { return Msg; }
 
   void print(DiagnosticPrinter &DP) const override;
 };
@@ -1090,7 +1089,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
 /// Diagnostic information for MisExpect analysis.
 class DiagnosticInfoMisExpect : public DiagnosticInfoWithLocationBase {
 public:
-  DiagnosticInfoMisExpect(const Instruction *Inst, Twine &Msg);
+  DiagnosticInfoMisExpect(const Instruction *Inst, StringRef Msg);
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -1099,11 +1098,11 @@ class DiagnosticInfoMisExpect : public DiagnosticInfoWithLocationBase {
     return DI->getKind() == DK_MisExpect;
   }
 
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 static DiagnosticSeverity getDiagnosticSeverity(SourceMgr::DiagKind DK) {
diff --git a/llvm/include/llvm/ProfileData/SampleProfReader.h b/llvm/include/llvm/ProfileData/SampleProfReader.h
index 76c7cecded629..d3f29d52f0c4b 100644
--- a/llvm/include/llvm/ProfileData/SampleProfReader.h
+++ b/llvm/include/llvm/ProfileData/SampleProfReader.h
@@ -449,8 +449,9 @@ class SampleProfileReader {
 
   /// Report a parse error message.
   void reportError(int64_t LineNumber, const Twine &Msg) const {
-    Ctx.diagnose(DiagnosticInfoSampleProfile(Buffer->getBufferIdentifier(),
-                                             LineNumber, Msg));
+    SmallString<128> Storage;
+    Ctx.diagnose(DiagnosticInfoSampleProfile(
+        Buffer->getBufferIdentifier(), LineNumber, Msg.toStringRef(Storage)));
   }
 
   /// Create a sample profile reader appropriate to the file format.
diff --git a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
index d714dba4a9687..1a63d3ed96438 100644
--- a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
@@ -21,6 +21,7 @@
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/LazyCallGraph.h"
 #include "llvm/Analysis/LoopInfo.h"
@@ -39,6 +40,7 @@
 #include "llvm/ProfileData/SampleProf.h"
 #include "llvm/ProfileData/SampleProfReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/GenericDomTree.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Utils/SampleProfileInference.h"
@@ -1104,12 +1106,13 @@ void SampleProfileLoaderBaseImpl<BT>::emitCoverageRemarks(FunctionT &F) {
     unsigned Used = CoverageTracker.countUsedRecords(Samples, PSI);
     unsigned Total = CoverageTracker.countBodyRecords(Samples, PSI);
     unsigned Coverage = CoverageTracker.computeCoverage(Used, Total);
+    SmallString<128> Msg =
+        formatv("{0} of {1} available profile records ({2}%) were applied",
+                Used, Total, Coverage);
     if (Coverage < SampleProfileRecordCoverage) {
-      Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-          Func.getSubprogram()->getFilename(), getFunctionLoc(F),
-          Twine(Used) + " of " + Twine(Total) + " available profile records (" +
-              Twine(Coverage) + "%) were applied",
-          DS_Warning));
+      Func.getContext().diagnose(
+          DiagnosticInfoSampleProfile(Func.getSubprogram()->getFilename(),
+                                      getFunctionLoc(F), Msg, DS_Warning));
     }
   }
 
@@ -1117,12 +1120,13 @@ void SampleProfileLoaderBaseImpl<BT>::emitCoverageRemarks(FunctionT &F) {
     uint64_t Used = CoverageTracker.getTotalUsedSamples();
     uint64_t Total = CoverageTracker.countBodySamples(Samples, PSI);
     unsigned Coverage = CoverageTracker.computeCoverage(Used, Total);
+    SmallString<128> Msg =
+        formatv("{0} of {1} available profile samples ({2}%) were applied",
+                Used, Total, Coverage);
     if (Coverage < SampleProfileSampleCoverage) {
-      Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-          Func.getSubprogram()->getFilename(), getFunctionLoc(F),
-          Twine(Used) + " of " + Twine(Total) + " available profile samples (" +
-              Twine(Coverage) + "%) were applied",
-          DS_Warning));
+      Func.getContext().diagnose(
+          DiagnosticInfoSampleProfile(Func.getSubprogram()->getFilename(),
+                                      getFunctionLoc(F), Msg, DS_Warning));
     }
   }
 }
@@ -1149,10 +1153,11 @@ unsigned SampleProfileLoaderBaseImpl<BT>::getFunctionLoc(FunctionT &F) {
 
   // If the start of \p F is missing, emit a diagnostic to inform the user
   // about the missed opportunity.
-  Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-      "No debug information found in function " + Func.getName() +
-          ": Function profile not used",
-      DS_Warning));
+  SmallString<128> Msg = formatv(
+      "No debug information found in function {0}: Function profile not used",
+      Func.getName());
+
+  Func.getContext().diagnose(DiagnosticInfoSampleProfile(Msg, DS_Warning));
   return 0;
 }
 
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
index 59fc4cfc23e10..217ce0a298e19 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
@@ -34,6 +34,7 @@
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"
@@ -313,9 +314,9 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
         }
         if (Error) {
           const Function &Fn = MI->getMF()->getFunction();
-          Fn.getContext().diagnose(DiagnosticInfoInlineAsm(
-              LocCookie,
-              "invalid operand in inline asm: '" + Twine(AsmStr) + "'"));
+          SmallString<128> Msg =
+              formatv("invalid operand in inline asm: '{0}'", AsmStr);
+          Fn.getContext().diagnose(DiagnosticInfoInlineAsm(LocCookie, Msg));
         }
   ...
[truncated]

llvmbot · 2025-04-18T21:30:09Z

@llvm/pr-subscribers-llvm-selectiondag

Author: Justin Bogner (bogner)

Changes

This switches the various DiagnosticInfo subclasses that store an un-owned string to use StringRef for this instead of Twine.

Storing a Twine & like most of these were doing makes it very easy to introduce memory corruption bugs:

void f(std::string &amp;Msg) {
  auto Diag = DiagnosticInfoGeneric(Msg);
  Ctx.diagnose(Diag); // Bug! We're referring to the temporary Twine!
  Ctx.diagnose(DiagnosticInfoGeneric(Msg); // this works, I guess...
};

This was sort of mitigated in DiagnosticInfoUnsupported, which actually copies the Twine, but copying Twines is always a questionable practice.

Instead, we store a StringRef, which makes it much more explicit that ownership of the storage is the caller's problem.

Patch is 36.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/136371.diff

11 Files Affected:

(modified) llvm/include/llvm/IR/DiagnosticInfo.h (+40-41)
(modified) llvm/include/llvm/ProfileData/SampleProfReader.h (+3-2)
(modified) llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h (+19-14)
(modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp (+4-3)
(modified) llvm/lib/CodeGen/MachineInstr.cpp (+5-3)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+6-4)
(modified) llvm/lib/IR/DiagnosticInfo.cpp (+5-6)
(modified) llvm/lib/IR/LLVMContext.cpp (+5-2)
(modified) llvm/lib/Target/DirectX/DXILRootSignature.cpp (+2-3)
(modified) llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (+44-47)
(modified) llvm/lib/Transforms/Utils/MisExpect.cpp (+4-5)

diff --git a/llvm/include/llvm/IR/DiagnosticInfo.h b/llvm/include/llvm/IR/DiagnosticInfo.h
index 8743f8058c382..151f323447bfb 100644
--- a/llvm/include/llvm/IR/DiagnosticInfo.h
+++ b/llvm/include/llvm/IR/DiagnosticInfo.h
@@ -18,7 +18,6 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Twine.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/Support/CBindingWrapping.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -139,22 +138,22 @@ class DiagnosticInfo {
 using DiagnosticHandlerFunction = std::function<void(const DiagnosticInfo &)>;
 
 class DiagnosticInfoGeneric : public DiagnosticInfo {
-  const Twine &MsgStr;
+  StringRef MsgStr;
   const Instruction *Inst = nullptr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoGeneric(const Twine &MsgStr,
+  DiagnosticInfoGeneric(StringRef MsgStr,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(MsgStr) {}
 
-  DiagnosticInfoGeneric(const Instruction *I, const Twine &ErrMsg,
+  DiagnosticInfoGeneric(const Instruction *I, StringRef ErrMsg,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(ErrMsg), Inst(I) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
   const Instruction *getInstruction() const { return Inst; }
 
   /// \see DiagnosticInfo::print.
@@ -172,7 +171,7 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// Optional line information. 0 if not set.
   uint64_t LocCookie = 0;
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
   /// Optional origin of the problem.
   const Instruction *Instr = nullptr;
 
@@ -181,7 +180,7 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// \p MsgStr gives the message.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoInlineAsm(uint64_t LocCookie, const Twine &MsgStr,
+  DiagnosticInfoInlineAsm(uint64_t LocCookie, StringRef MsgStr,
                           DiagnosticSeverity Severity = DS_Error);
 
   /// \p Instr gives the original instruction that triggered the diagnostic.
@@ -189,11 +188,11 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
   /// Same for \p I.
-  DiagnosticInfoInlineAsm(const Instruction &I, const Twine &MsgStr,
+  DiagnosticInfoInlineAsm(const Instruction &I, StringRef MsgStr,
                           DiagnosticSeverity Severity = DS_Error);
 
   uint64_t getLocCookie() const { return LocCookie; }
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
   const Instruction *getInstruction() const { return Instr; }
 
   /// \see DiagnosticInfo::print.
@@ -258,15 +257,15 @@ class DiagnosticInfoIgnoringInvalidDebugMetadata : public DiagnosticInfo {
 class DiagnosticInfoSampleProfile : public DiagnosticInfo {
 public:
   DiagnosticInfoSampleProfile(StringRef FileName, unsigned LineNum,
-                              const Twine &Msg,
+                              StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), FileName(FileName),
         LineNum(LineNum), Msg(Msg) {}
-  DiagnosticInfoSampleProfile(StringRef FileName, const Twine &Msg,
+  DiagnosticInfoSampleProfile(StringRef FileName, StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), FileName(FileName),
         Msg(Msg) {}
-  DiagnosticInfoSampleProfile(const Twine &Msg,
+  DiagnosticInfoSampleProfile(StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), Msg(Msg) {}
 
@@ -279,7 +278,7 @@ class DiagnosticInfoSampleProfile : public DiagnosticInfo {
 
   StringRef getFileName() const { return FileName; }
   unsigned getLineNum() const { return LineNum; }
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Name of the input file associated with this diagnostic.
@@ -290,13 +289,13 @@ class DiagnosticInfoSampleProfile : public DiagnosticInfo {
   unsigned LineNum = 0;
 
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 /// Diagnostic information for the PGO profiler.
 class DiagnosticInfoPGOProfile : public DiagnosticInfo {
 public:
-  DiagnosticInfoPGOProfile(const char *FileName, const Twine &Msg,
+  DiagnosticInfoPGOProfile(const char *FileName, StringRef Msg,
                            DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_PGOProfile, Severity), FileName(FileName), Msg(Msg) {}
 
@@ -308,14 +307,14 @@ class DiagnosticInfoPGOProfile : public DiagnosticInfo {
   }
 
   const char *getFileName() const { return FileName; }
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Name of the input file associated with this diagnostic.
   const char *FileName;
 
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 class DiagnosticLocation {
@@ -364,7 +363,7 @@ class DiagnosticInfoWithLocationBase : public DiagnosticInfo {
 
   /// Return the absolute path tot the file.
   std::string getAbsolutePath() const;
-  
+
   const Function &getFunction() const { return Fn; }
   DiagnosticLocation getLocation() const { return Loc; }
 
@@ -379,19 +378,19 @@ class DiagnosticInfoWithLocationBase : public DiagnosticInfo {
 class DiagnosticInfoGenericWithLoc : public DiagnosticInfoWithLocationBase {
 private:
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoGenericWithLoc(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoGenericWithLoc(StringRef MsgStr, const Function &Fn,
                                const DiagnosticLocation &Loc,
                                DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfoWithLocationBase(DK_GenericWithLoc, Severity, Fn, Loc),
         MsgStr(MsgStr) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -404,20 +403,20 @@ class DiagnosticInfoGenericWithLoc : public DiagnosticInfoWithLocationBase {
 class DiagnosticInfoRegAllocFailure : public DiagnosticInfoWithLocationBase {
 private:
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoRegAllocFailure(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoRegAllocFailure(StringRef MsgStr, const Function &Fn,
                                 const DiagnosticLocation &DL,
                                 DiagnosticSeverity Severity = DS_Error);
 
-  DiagnosticInfoRegAllocFailure(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoRegAllocFailure(StringRef MsgStr, const Function &Fn,
                                 DiagnosticSeverity Severity = DS_Error);
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -707,7 +706,7 @@ class DiagnosticInfoIROptimization : public DiagnosticInfoOptimizationBase {
   DiagnosticInfoIROptimization(enum DiagnosticKind Kind,
                                enum DiagnosticSeverity Severity,
                                const char *PassName, const Function &Fn,
-                               const DiagnosticLocation &Loc, const Twine &Msg)
+                               const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoOptimizationBase(Kind, Severity, PassName, "", Fn, Loc) {
     *this << Msg.str();
   }
@@ -764,7 +763,7 @@ class OptimizationRemark : public DiagnosticInfoIROptimization {
   /// Note that this class does not copy this message, so this reference
   /// must be valid for the whole life time of the diagnostic.
   OptimizationRemark(const char *PassName, const Function &Fn,
-                     const DiagnosticLocation &Loc, const Twine &Msg)
+                     const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemark, DS_Remark, PassName,
                                      Fn, Loc, Msg) {}
 };
@@ -810,7 +809,7 @@ class OptimizationRemarkMissed : public DiagnosticInfoIROptimization {
   /// Note that this class does not copy this message, so this reference
   /// must be valid for the whole life time of the diagnostic.
   OptimizationRemarkMissed(const char *PassName, const Function &Fn,
-                           const DiagnosticLocation &Loc, const Twine &Msg)
+                           const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemarkMissed, DS_Remark,
                                      PassName, Fn, Loc, Msg) {}
 };
@@ -863,7 +862,7 @@ class OptimizationRemarkAnalysis : public DiagnosticInfoIROptimization {
 protected:
   OptimizationRemarkAnalysis(enum DiagnosticKind Kind, const char *PassName,
                              const Function &Fn, const DiagnosticLocation &Loc,
-                             const Twine &Msg)
+                             StringRef Msg)
       : DiagnosticInfoIROptimization(Kind, DS_Remark, PassName, Fn, Loc, Msg) {}
 
   OptimizationRemarkAnalysis(enum DiagnosticKind Kind, const char *PassName,
@@ -882,7 +881,7 @@ class OptimizationRemarkAnalysis : public DiagnosticInfoIROptimization {
   /// this class does not copy this message, so this reference must be valid for
   /// the whole life time of the diagnostic.
   OptimizationRemarkAnalysis(const char *PassName, const Function &Fn,
-                             const DiagnosticLocation &Loc, const Twine &Msg)
+                             const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemarkAnalysis, DS_Remark,
                                      PassName, Fn, Loc, Msg) {}
 };
@@ -924,7 +923,7 @@ class OptimizationRemarkAnalysisFPCommute : public OptimizationRemarkAnalysis {
   /// diagnostic.
   OptimizationRemarkAnalysisFPCommute(const char *PassName, const Function &Fn,
                                       const DiagnosticLocation &Loc,
-                                      const Twine &Msg)
+                                      StringRef Msg)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisFPCommute,
                                    PassName, Fn, Loc, Msg) {}
 };
@@ -965,7 +964,7 @@ class OptimizationRemarkAnalysisAliasing : public OptimizationRemarkAnalysis {
   /// diagnostic.
   OptimizationRemarkAnalysisAliasing(const char *PassName, const Function &Fn,
                                      const DiagnosticLocation &Loc,
-                                     const Twine &Msg)
+                                     StringRef Msg)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisAliasing,
                                    PassName, Fn, Loc, Msg) {}
 };
@@ -991,10 +990,10 @@ class DiagnosticInfoMIRParser : public DiagnosticInfo {
 
 /// Diagnostic information for IR instrumentation reporting.
 class DiagnosticInfoInstrumentation : public DiagnosticInfo {
-  const Twine &Msg;
+  StringRef Msg;
 
 public:
-  DiagnosticInfoInstrumentation(const Twine &DiagMsg,
+  DiagnosticInfoInstrumentation(StringRef DiagMsg,
                                 DiagnosticSeverity Severity = DS_Warning)
       : DiagnosticInfo(DK_Instrumentation, Severity), Msg(DiagMsg) {}
 
@@ -1038,7 +1037,7 @@ class DiagnosticInfoOptimizationFailure : public DiagnosticInfoIROptimization {
   /// of the diagnostic.
   DiagnosticInfoOptimizationFailure(const Function &Fn,
                                     const DiagnosticLocation &Loc,
-                                    const Twine &Msg)
+                                    StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationFailure, DS_Warning,
                                      nullptr, Fn, Loc, Msg) {}
 
@@ -1062,7 +1061,7 @@ class DiagnosticInfoOptimizationFailure : public DiagnosticInfoIROptimization {
 /// Diagnostic information for unsupported feature in backend.
 class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
 private:
-  Twine Msg;
+  StringRef Msg;
 
 public:
   /// \p Fn is the function where the diagnostic is being emitted. \p Loc is
@@ -1072,7 +1071,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
   /// copy this message, so this reference must be valid for the whole life time
   /// of the diagnostic.
   DiagnosticInfoUnsupported(
-      const Function &Fn, const Twine &Msg,
+      const Function &Fn, StringRef Msg,
       const DiagnosticLocation &Loc = DiagnosticLocation(),
       DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfoWithLocationBase(DK_Unsupported, Severity, Fn, Loc),
@@ -1082,7 +1081,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
     return DI->getKind() == DK_Unsupported;
   }
 
-  const Twine &getMessage() const { return Msg; }
+  const StringRef getMessage() const { return Msg; }
 
   void print(DiagnosticPrinter &DP) const override;
 };
@@ -1090,7 +1089,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
 /// Diagnostic information for MisExpect analysis.
 class DiagnosticInfoMisExpect : public DiagnosticInfoWithLocationBase {
 public:
-  DiagnosticInfoMisExpect(const Instruction *Inst, Twine &Msg);
+  DiagnosticInfoMisExpect(const Instruction *Inst, StringRef Msg);
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -1099,11 +1098,11 @@ class DiagnosticInfoMisExpect : public DiagnosticInfoWithLocationBase {
     return DI->getKind() == DK_MisExpect;
   }
 
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 static DiagnosticSeverity getDiagnosticSeverity(SourceMgr::DiagKind DK) {
diff --git a/llvm/include/llvm/ProfileData/SampleProfReader.h b/llvm/include/llvm/ProfileData/SampleProfReader.h
index 76c7cecded629..d3f29d52f0c4b 100644
--- a/llvm/include/llvm/ProfileData/SampleProfReader.h
+++ b/llvm/include/llvm/ProfileData/SampleProfReader.h
@@ -449,8 +449,9 @@ class SampleProfileReader {
 
   /// Report a parse error message.
   void reportError(int64_t LineNumber, const Twine &Msg) const {
-    Ctx.diagnose(DiagnosticInfoSampleProfile(Buffer->getBufferIdentifier(),
-                                             LineNumber, Msg));
+    SmallString<128> Storage;
+    Ctx.diagnose(DiagnosticInfoSampleProfile(
+        Buffer->getBufferIdentifier(), LineNumber, Msg.toStringRef(Storage)));
   }
 
   /// Create a sample profile reader appropriate to the file format.
diff --git a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
index d714dba4a9687..1a63d3ed96438 100644
--- a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
@@ -21,6 +21,7 @@
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/LazyCallGraph.h"
 #include "llvm/Analysis/LoopInfo.h"
@@ -39,6 +40,7 @@
 #include "llvm/ProfileData/SampleProf.h"
 #include "llvm/ProfileData/SampleProfReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/GenericDomTree.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Utils/SampleProfileInference.h"
@@ -1104,12 +1106,13 @@ void SampleProfileLoaderBaseImpl<BT>::emitCoverageRemarks(FunctionT &F) {
     unsigned Used = CoverageTracker.countUsedRecords(Samples, PSI);
     unsigned Total = CoverageTracker.countBodyRecords(Samples, PSI);
     unsigned Coverage = CoverageTracker.computeCoverage(Used, Total);
+    SmallString<128> Msg =
+        formatv("{0} of {1} available profile records ({2}%) were applied",
+                Used, Total, Coverage);
     if (Coverage < SampleProfileRecordCoverage) {
-      Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-          Func.getSubprogram()->getFilename(), getFunctionLoc(F),
-          Twine(Used) + " of " + Twine(Total) + " available profile records (" +
-              Twine(Coverage) + "%) were applied",
-          DS_Warning));
+      Func.getContext().diagnose(
+          DiagnosticInfoSampleProfile(Func.getSubprogram()->getFilename(),
+                                      getFunctionLoc(F), Msg, DS_Warning));
     }
   }
 
@@ -1117,12 +1120,13 @@ void SampleProfileLoaderBaseImpl<BT>::emitCoverageRemarks(FunctionT &F) {
     uint64_t Used = CoverageTracker.getTotalUsedSamples();
     uint64_t Total = CoverageTracker.countBodySamples(Samples, PSI);
     unsigned Coverage = CoverageTracker.computeCoverage(Used, Total);
+    SmallString<128> Msg =
+        formatv("{0} of {1} available profile samples ({2}%) were applied",
+                Used, Total, Coverage);
     if (Coverage < SampleProfileSampleCoverage) {
-      Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-          Func.getSubprogram()->getFilename(), getFunctionLoc(F),
-          Twine(Used) + " of " + Twine(Total) + " available profile samples (" +
-              Twine(Coverage) + "%) were applied",
-          DS_Warning));
+      Func.getContext().diagnose(
+          DiagnosticInfoSampleProfile(Func.getSubprogram()->getFilename(),
+                                      getFunctionLoc(F), Msg, DS_Warning));
     }
   }
 }
@@ -1149,10 +1153,11 @@ unsigned SampleProfileLoaderBaseImpl<BT>::getFunctionLoc(FunctionT &F) {
 
   // If the start of \p F is missing, emit a diagnostic to inform the user
   // about the missed opportunity.
-  Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-      "No debug information found in function " + Func.getName() +
-          ": Function profile not used",
-      DS_Warning));
+  SmallString<128> Msg = formatv(
+      "No debug information found in function {0}: Function profile not used",
+      Func.getName());
+
+  Func.getContext().diagnose(DiagnosticInfoSampleProfile(Msg, DS_Warning));
   return 0;
 }
 
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
index 59fc4cfc23e10..217ce0a298e19 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
@@ -34,6 +34,7 @@
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"
@@ -313,9 +314,9 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
         }
         if (Error) {
           const Function &Fn = MI->getMF()->getFunction();
-          Fn.getContext().diagnose(DiagnosticInfoInlineAsm(
-              LocCookie,
-              "invalid operand in inline asm: '" + Twine(AsmStr) + "'"));
+          SmallString<128> Msg =
+              formatv("invalid operand in inline asm: '{0}'", AsmStr);
+          Fn.getContext().diagnose(DiagnosticInfoInlineAsm(LocCookie, Msg));
         }
   ...
[truncated]

llvmbot · 2025-04-18T21:30:09Z

@llvm/pr-subscribers-llvm-transforms

Author: Justin Bogner (bogner)

Changes

This switches the various DiagnosticInfo subclasses that store an un-owned string to use StringRef for this instead of Twine.

Storing a Twine & like most of these were doing makes it very easy to introduce memory corruption bugs:

void f(std::string &amp;Msg) {
  auto Diag = DiagnosticInfoGeneric(Msg);
  Ctx.diagnose(Diag); // Bug! We're referring to the temporary Twine!
  Ctx.diagnose(DiagnosticInfoGeneric(Msg); // this works, I guess...
};

This was sort of mitigated in DiagnosticInfoUnsupported, which actually copies the Twine, but copying Twines is always a questionable practice.

Instead, we store a StringRef, which makes it much more explicit that ownership of the storage is the caller's problem.

Patch is 36.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/136371.diff

11 Files Affected:

(modified) llvm/include/llvm/IR/DiagnosticInfo.h (+40-41)
(modified) llvm/include/llvm/ProfileData/SampleProfReader.h (+3-2)
(modified) llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h (+19-14)
(modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp (+4-3)
(modified) llvm/lib/CodeGen/MachineInstr.cpp (+5-3)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+6-4)
(modified) llvm/lib/IR/DiagnosticInfo.cpp (+5-6)
(modified) llvm/lib/IR/LLVMContext.cpp (+5-2)
(modified) llvm/lib/Target/DirectX/DXILRootSignature.cpp (+2-3)
(modified) llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (+44-47)
(modified) llvm/lib/Transforms/Utils/MisExpect.cpp (+4-5)

diff --git a/llvm/include/llvm/IR/DiagnosticInfo.h b/llvm/include/llvm/IR/DiagnosticInfo.h
index 8743f8058c382..151f323447bfb 100644
--- a/llvm/include/llvm/IR/DiagnosticInfo.h
+++ b/llvm/include/llvm/IR/DiagnosticInfo.h
@@ -18,7 +18,6 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Twine.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/Support/CBindingWrapping.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -139,22 +138,22 @@ class DiagnosticInfo {
 using DiagnosticHandlerFunction = std::function<void(const DiagnosticInfo &)>;
 
 class DiagnosticInfoGeneric : public DiagnosticInfo {
-  const Twine &MsgStr;
+  StringRef MsgStr;
   const Instruction *Inst = nullptr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoGeneric(const Twine &MsgStr,
+  DiagnosticInfoGeneric(StringRef MsgStr,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(MsgStr) {}
 
-  DiagnosticInfoGeneric(const Instruction *I, const Twine &ErrMsg,
+  DiagnosticInfoGeneric(const Instruction *I, StringRef ErrMsg,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(ErrMsg), Inst(I) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
   const Instruction *getInstruction() const { return Inst; }
 
   /// \see DiagnosticInfo::print.
@@ -172,7 +171,7 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// Optional line information. 0 if not set.
   uint64_t LocCookie = 0;
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
   /// Optional origin of the problem.
   const Instruction *Instr = nullptr;
 
@@ -181,7 +180,7 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// \p MsgStr gives the message.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoInlineAsm(uint64_t LocCookie, const Twine &MsgStr,
+  DiagnosticInfoInlineAsm(uint64_t LocCookie, StringRef MsgStr,
                           DiagnosticSeverity Severity = DS_Error);
 
   /// \p Instr gives the original instruction that triggered the diagnostic.
@@ -189,11 +188,11 @@ class DiagnosticInfoInlineAsm : public DiagnosticInfo {
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
   /// Same for \p I.
-  DiagnosticInfoInlineAsm(const Instruction &I, const Twine &MsgStr,
+  DiagnosticInfoInlineAsm(const Instruction &I, StringRef MsgStr,
                           DiagnosticSeverity Severity = DS_Error);
 
   uint64_t getLocCookie() const { return LocCookie; }
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
   const Instruction *getInstruction() const { return Instr; }
 
   /// \see DiagnosticInfo::print.
@@ -258,15 +257,15 @@ class DiagnosticInfoIgnoringInvalidDebugMetadata : public DiagnosticInfo {
 class DiagnosticInfoSampleProfile : public DiagnosticInfo {
 public:
   DiagnosticInfoSampleProfile(StringRef FileName, unsigned LineNum,
-                              const Twine &Msg,
+                              StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), FileName(FileName),
         LineNum(LineNum), Msg(Msg) {}
-  DiagnosticInfoSampleProfile(StringRef FileName, const Twine &Msg,
+  DiagnosticInfoSampleProfile(StringRef FileName, StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), FileName(FileName),
         Msg(Msg) {}
-  DiagnosticInfoSampleProfile(const Twine &Msg,
+  DiagnosticInfoSampleProfile(StringRef Msg,
                               DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_SampleProfile, Severity), Msg(Msg) {}
 
@@ -279,7 +278,7 @@ class DiagnosticInfoSampleProfile : public DiagnosticInfo {
 
   StringRef getFileName() const { return FileName; }
   unsigned getLineNum() const { return LineNum; }
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Name of the input file associated with this diagnostic.
@@ -290,13 +289,13 @@ class DiagnosticInfoSampleProfile : public DiagnosticInfo {
   unsigned LineNum = 0;
 
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 /// Diagnostic information for the PGO profiler.
 class DiagnosticInfoPGOProfile : public DiagnosticInfo {
 public:
-  DiagnosticInfoPGOProfile(const char *FileName, const Twine &Msg,
+  DiagnosticInfoPGOProfile(const char *FileName, StringRef Msg,
                            DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_PGOProfile, Severity), FileName(FileName), Msg(Msg) {}
 
@@ -308,14 +307,14 @@ class DiagnosticInfoPGOProfile : public DiagnosticInfo {
   }
 
   const char *getFileName() const { return FileName; }
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Name of the input file associated with this diagnostic.
   const char *FileName;
 
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 class DiagnosticLocation {
@@ -364,7 +363,7 @@ class DiagnosticInfoWithLocationBase : public DiagnosticInfo {
 
   /// Return the absolute path tot the file.
   std::string getAbsolutePath() const;
-  
+
   const Function &getFunction() const { return Fn; }
   DiagnosticLocation getLocation() const { return Loc; }
 
@@ -379,19 +378,19 @@ class DiagnosticInfoWithLocationBase : public DiagnosticInfo {
 class DiagnosticInfoGenericWithLoc : public DiagnosticInfoWithLocationBase {
 private:
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoGenericWithLoc(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoGenericWithLoc(StringRef MsgStr, const Function &Fn,
                                const DiagnosticLocation &Loc,
                                DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfoWithLocationBase(DK_GenericWithLoc, Severity, Fn, Loc),
         MsgStr(MsgStr) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -404,20 +403,20 @@ class DiagnosticInfoGenericWithLoc : public DiagnosticInfoWithLocationBase {
 class DiagnosticInfoRegAllocFailure : public DiagnosticInfoWithLocationBase {
 private:
   /// Message to be reported.
-  const Twine &MsgStr;
+  StringRef MsgStr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoRegAllocFailure(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoRegAllocFailure(StringRef MsgStr, const Function &Fn,
                                 const DiagnosticLocation &DL,
                                 DiagnosticSeverity Severity = DS_Error);
 
-  DiagnosticInfoRegAllocFailure(const Twine &MsgStr, const Function &Fn,
+  DiagnosticInfoRegAllocFailure(StringRef MsgStr, const Function &Fn,
                                 DiagnosticSeverity Severity = DS_Error);
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  StringRef getMsgStr() const { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -707,7 +706,7 @@ class DiagnosticInfoIROptimization : public DiagnosticInfoOptimizationBase {
   DiagnosticInfoIROptimization(enum DiagnosticKind Kind,
                                enum DiagnosticSeverity Severity,
                                const char *PassName, const Function &Fn,
-                               const DiagnosticLocation &Loc, const Twine &Msg)
+                               const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoOptimizationBase(Kind, Severity, PassName, "", Fn, Loc) {
     *this << Msg.str();
   }
@@ -764,7 +763,7 @@ class OptimizationRemark : public DiagnosticInfoIROptimization {
   /// Note that this class does not copy this message, so this reference
   /// must be valid for the whole life time of the diagnostic.
   OptimizationRemark(const char *PassName, const Function &Fn,
-                     const DiagnosticLocation &Loc, const Twine &Msg)
+                     const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemark, DS_Remark, PassName,
                                      Fn, Loc, Msg) {}
 };
@@ -810,7 +809,7 @@ class OptimizationRemarkMissed : public DiagnosticInfoIROptimization {
   /// Note that this class does not copy this message, so this reference
   /// must be valid for the whole life time of the diagnostic.
   OptimizationRemarkMissed(const char *PassName, const Function &Fn,
-                           const DiagnosticLocation &Loc, const Twine &Msg)
+                           const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemarkMissed, DS_Remark,
                                      PassName, Fn, Loc, Msg) {}
 };
@@ -863,7 +862,7 @@ class OptimizationRemarkAnalysis : public DiagnosticInfoIROptimization {
 protected:
   OptimizationRemarkAnalysis(enum DiagnosticKind Kind, const char *PassName,
                              const Function &Fn, const DiagnosticLocation &Loc,
-                             const Twine &Msg)
+                             StringRef Msg)
       : DiagnosticInfoIROptimization(Kind, DS_Remark, PassName, Fn, Loc, Msg) {}
 
   OptimizationRemarkAnalysis(enum DiagnosticKind Kind, const char *PassName,
@@ -882,7 +881,7 @@ class OptimizationRemarkAnalysis : public DiagnosticInfoIROptimization {
   /// this class does not copy this message, so this reference must be valid for
   /// the whole life time of the diagnostic.
   OptimizationRemarkAnalysis(const char *PassName, const Function &Fn,
-                             const DiagnosticLocation &Loc, const Twine &Msg)
+                             const DiagnosticLocation &Loc, StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationRemarkAnalysis, DS_Remark,
                                      PassName, Fn, Loc, Msg) {}
 };
@@ -924,7 +923,7 @@ class OptimizationRemarkAnalysisFPCommute : public OptimizationRemarkAnalysis {
   /// diagnostic.
   OptimizationRemarkAnalysisFPCommute(const char *PassName, const Function &Fn,
                                       const DiagnosticLocation &Loc,
-                                      const Twine &Msg)
+                                      StringRef Msg)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisFPCommute,
                                    PassName, Fn, Loc, Msg) {}
 };
@@ -965,7 +964,7 @@ class OptimizationRemarkAnalysisAliasing : public OptimizationRemarkAnalysis {
   /// diagnostic.
   OptimizationRemarkAnalysisAliasing(const char *PassName, const Function &Fn,
                                      const DiagnosticLocation &Loc,
-                                     const Twine &Msg)
+                                     StringRef Msg)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisAliasing,
                                    PassName, Fn, Loc, Msg) {}
 };
@@ -991,10 +990,10 @@ class DiagnosticInfoMIRParser : public DiagnosticInfo {
 
 /// Diagnostic information for IR instrumentation reporting.
 class DiagnosticInfoInstrumentation : public DiagnosticInfo {
-  const Twine &Msg;
+  StringRef Msg;
 
 public:
-  DiagnosticInfoInstrumentation(const Twine &DiagMsg,
+  DiagnosticInfoInstrumentation(StringRef DiagMsg,
                                 DiagnosticSeverity Severity = DS_Warning)
       : DiagnosticInfo(DK_Instrumentation, Severity), Msg(DiagMsg) {}
 
@@ -1038,7 +1037,7 @@ class DiagnosticInfoOptimizationFailure : public DiagnosticInfoIROptimization {
   /// of the diagnostic.
   DiagnosticInfoOptimizationFailure(const Function &Fn,
                                     const DiagnosticLocation &Loc,
-                                    const Twine &Msg)
+                                    StringRef Msg)
       : DiagnosticInfoIROptimization(DK_OptimizationFailure, DS_Warning,
                                      nullptr, Fn, Loc, Msg) {}
 
@@ -1062,7 +1061,7 @@ class DiagnosticInfoOptimizationFailure : public DiagnosticInfoIROptimization {
 /// Diagnostic information for unsupported feature in backend.
 class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
 private:
-  Twine Msg;
+  StringRef Msg;
 
 public:
   /// \p Fn is the function where the diagnostic is being emitted. \p Loc is
@@ -1072,7 +1071,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
   /// copy this message, so this reference must be valid for the whole life time
   /// of the diagnostic.
   DiagnosticInfoUnsupported(
-      const Function &Fn, const Twine &Msg,
+      const Function &Fn, StringRef Msg,
       const DiagnosticLocation &Loc = DiagnosticLocation(),
       DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfoWithLocationBase(DK_Unsupported, Severity, Fn, Loc),
@@ -1082,7 +1081,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
     return DI->getKind() == DK_Unsupported;
   }
 
-  const Twine &getMessage() const { return Msg; }
+  const StringRef getMessage() const { return Msg; }
 
   void print(DiagnosticPrinter &DP) const override;
 };
@@ -1090,7 +1089,7 @@ class DiagnosticInfoUnsupported : public DiagnosticInfoWithLocationBase {
 /// Diagnostic information for MisExpect analysis.
 class DiagnosticInfoMisExpect : public DiagnosticInfoWithLocationBase {
 public:
-  DiagnosticInfoMisExpect(const Instruction *Inst, Twine &Msg);
+  DiagnosticInfoMisExpect(const Instruction *Inst, StringRef Msg);
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -1099,11 +1098,11 @@ class DiagnosticInfoMisExpect : public DiagnosticInfoWithLocationBase {
     return DI->getKind() == DK_MisExpect;
   }
 
-  const Twine &getMsg() const { return Msg; }
+  StringRef getMsg() const { return Msg; }
 
 private:
   /// Message to report.
-  const Twine &Msg;
+  StringRef Msg;
 };
 
 static DiagnosticSeverity getDiagnosticSeverity(SourceMgr::DiagKind DK) {
diff --git a/llvm/include/llvm/ProfileData/SampleProfReader.h b/llvm/include/llvm/ProfileData/SampleProfReader.h
index 76c7cecded629..d3f29d52f0c4b 100644
--- a/llvm/include/llvm/ProfileData/SampleProfReader.h
+++ b/llvm/include/llvm/ProfileData/SampleProfReader.h
@@ -449,8 +449,9 @@ class SampleProfileReader {
 
   /// Report a parse error message.
   void reportError(int64_t LineNumber, const Twine &Msg) const {
-    Ctx.diagnose(DiagnosticInfoSampleProfile(Buffer->getBufferIdentifier(),
-                                             LineNumber, Msg));
+    SmallString<128> Storage;
+    Ctx.diagnose(DiagnosticInfoSampleProfile(
+        Buffer->getBufferIdentifier(), LineNumber, Msg.toStringRef(Storage)));
   }
 
   /// Create a sample profile reader appropriate to the file format.
diff --git a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
index d714dba4a9687..1a63d3ed96438 100644
--- a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
@@ -21,6 +21,7 @@
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/LazyCallGraph.h"
 #include "llvm/Analysis/LoopInfo.h"
@@ -39,6 +40,7 @@
 #include "llvm/ProfileData/SampleProf.h"
 #include "llvm/ProfileData/SampleProfReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/GenericDomTree.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Utils/SampleProfileInference.h"
@@ -1104,12 +1106,13 @@ void SampleProfileLoaderBaseImpl<BT>::emitCoverageRemarks(FunctionT &F) {
     unsigned Used = CoverageTracker.countUsedRecords(Samples, PSI);
     unsigned Total = CoverageTracker.countBodyRecords(Samples, PSI);
     unsigned Coverage = CoverageTracker.computeCoverage(Used, Total);
+    SmallString<128> Msg =
+        formatv("{0} of {1} available profile records ({2}%) were applied",
+                Used, Total, Coverage);
     if (Coverage < SampleProfileRecordCoverage) {
-      Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-          Func.getSubprogram()->getFilename(), getFunctionLoc(F),
-          Twine(Used) + " of " + Twine(Total) + " available profile records (" +
-              Twine(Coverage) + "%) were applied",
-          DS_Warning));
+      Func.getContext().diagnose(
+          DiagnosticInfoSampleProfile(Func.getSubprogram()->getFilename(),
+                                      getFunctionLoc(F), Msg, DS_Warning));
     }
   }
 
@@ -1117,12 +1120,13 @@ void SampleProfileLoaderBaseImpl<BT>::emitCoverageRemarks(FunctionT &F) {
     uint64_t Used = CoverageTracker.getTotalUsedSamples();
     uint64_t Total = CoverageTracker.countBodySamples(Samples, PSI);
     unsigned Coverage = CoverageTracker.computeCoverage(Used, Total);
+    SmallString<128> Msg =
+        formatv("{0} of {1} available profile samples ({2}%) were applied",
+                Used, Total, Coverage);
     if (Coverage < SampleProfileSampleCoverage) {
-      Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-          Func.getSubprogram()->getFilename(), getFunctionLoc(F),
-          Twine(Used) + " of " + Twine(Total) + " available profile samples (" +
-              Twine(Coverage) + "%) were applied",
-          DS_Warning));
+      Func.getContext().diagnose(
+          DiagnosticInfoSampleProfile(Func.getSubprogram()->getFilename(),
+                                      getFunctionLoc(F), Msg, DS_Warning));
     }
   }
 }
@@ -1149,10 +1153,11 @@ unsigned SampleProfileLoaderBaseImpl<BT>::getFunctionLoc(FunctionT &F) {
 
   // If the start of \p F is missing, emit a diagnostic to inform the user
   // about the missed opportunity.
-  Func.getContext().diagnose(DiagnosticInfoSampleProfile(
-      "No debug information found in function " + Func.getName() +
-          ": Function profile not used",
-      DS_Warning));
+  SmallString<128> Msg = formatv(
+      "No debug information found in function {0}: Function profile not used",
+      Func.getName());
+
+  Func.getContext().diagnose(DiagnosticInfoSampleProfile(Msg, DS_Warning));
   return 0;
 }
 
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
index 59fc4cfc23e10..217ce0a298e19 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
@@ -34,6 +34,7 @@
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"
@@ -313,9 +314,9 @@ static void EmitInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
         }
         if (Error) {
           const Function &Fn = MI->getMF()->getFunction();
-          Fn.getContext().diagnose(DiagnosticInfoInlineAsm(
-              LocCookie,
-              "invalid operand in inline asm: '" + Twine(AsmStr) + "'"));
+          SmallString<128> Msg =
+              formatv("invalid operand in inline asm: '{0}'", AsmStr);
+          Fn.getContext().diagnose(DiagnosticInfoInlineAsm(LocCookie, Msg));
         }
   ...
[truncated]

arsenm

Can we just get a warning on the bad usage instead?

arsenm

If we do this the DiagnosticPrinter twine overload of << is probably dead?

bogner · 2025-04-19T22:56:47Z

Can we just get a warning on the bad usage instead?

I'd be perfectly happy for this to just warn on misuse, but I don't think it's possible. Essentially we'd need to be able to warn on creating any of these DiagnosticInfo* classes on the stack, as the problematic pattern is as follows:

DiagnosticInfo Foo(...);
Ctx.diagnose(Foo);

The only way to correctly use these diagnostic classes is within the expression that calls LLVMContext::diagnose. This is the same as the problem with Twine itself, where we also need to ensure we're never doing this. IIRC we've discussed creating a warning for that too, but nobody came up with anything satisfactory. The only real difference is really that it's well known how Twine works and where it's dangerous, so people tend to catch misuses in code review.

If we want to make these interfaces "safe", we either need to move them away from Twine: either via stringref or making them actually copy and store the string. If we don't want to make these safe because we care about the small amount of extra stack space this approach uses in error paths, then I guess we could drop this and live with the status quo, but there are really too many times I’ve seen people struggle to debug the subtle build configuration and platform specific bugs that occur with these types.

bogner · 2025-04-21T07:13:56Z

If we do this the DiagnosticPrinter twine overload of << is probably dead?

It should be, yes. Updated the patch to catch the target specific diags I missed that removing this caught.

github-actions · 2025-04-21T07:15:28Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- llvm/include/llvm/IR/DiagnosticInfo.h llvm/include/llvm/IR/DiagnosticPrinter.h llvm/include/llvm/ProfileData/SampleProfReader.h llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp llvm/lib/CodeGen/MachineInstr.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp llvm/lib/IR/DiagnosticInfo.cpp llvm/lib/IR/DiagnosticPrinter.cpp llvm/lib/IR/LLVMContext.cpp llvm/lib/LTO/LTOCodeGenerator.cpp llvm/lib/LTO/ThinLTOCodeGenerator.cpp llvm/lib/Linker/IRMover.cpp llvm/lib/Linker/LinkDiagnosticInfo.h llvm/lib/Linker/LinkModules.cpp llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp llvm/lib/Target/BPF/BPFISelLowering.cpp llvm/lib/Target/BPF/BPFPreserveStaticOffset.cpp llvm/lib/Target/DirectX/DXILRootSignature.cpp llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp llvm/lib/Transforms/Instrumentation/KCFI.cpp llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp llvm/lib/Transforms/Utils/MisExpect.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/LTO/LTOCodeGenerator.cpp b/llvm/lib/LTO/LTOCodeGenerator.cpp
index 037344b77..6c2ea35de 100644
--- a/llvm/lib/LTO/LTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -745,6 +745,7 @@ LTOCodeGenerator::setDiagnosticHandler(lto_diagnostic_handler_t DiagHandler,
 namespace {
 class LTODiagnosticInfo : public DiagnosticInfo {
   StringRef Msg;
+
 public:
   LTODiagnosticInfo(StringRef DiagMsg, DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Linker, Severity), Msg(DiagMsg) {}
diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index fce154b0d..926f12e80 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -166,6 +166,7 @@ static void promoteModule(Module &TheModule, const ModuleSummaryIndex &Index,
 namespace {
 class ThinLTODiagnosticInfo : public DiagnosticInfo {
   StringRef Msg;
+
 public:
   ThinLTODiagnosticInfo(StringRef DiagMsg,
                         DiagnosticSeverity Severity = DS_Error)

These uses were fine, but the comment that claimed the string isn't copied are just incorrect.

arsenm · 2025-04-21T12:37:06Z

Can we just get a warning on the bad usage instead?

I'd be perfectly happy for this to just warn on misuse, but I don't think it's possible. Essentially we'd need to be able to warn on creating any of these DiagnosticInfo* classes on the stack, as the problematic pattern is as follows:

Seems to work if we add [[clang::lifetimebound]] to all of these constructor parameters

diff --git a/llvm/include/llvm/IR/DiagnosticInfo.h b/llvm/include/llvm/IR/DiagnosticInfo.h
index 8743f8058c38..badc1cdb6cac 100644
--- a/llvm/include/llvm/IR/DiagnosticInfo.h
+++ b/llvm/include/llvm/IR/DiagnosticInfo.h
@@ -146,15 +146,15 @@ public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoGeneric(const Twine &MsgStr,
+  DiagnosticInfoGeneric(const Twine &MsgStr [[clang::lifetimebound]] ,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(MsgStr) {}
 
-  DiagnosticInfoGeneric(const Instruction *I, const Twine &ErrMsg,
+  DiagnosticInfoGeneric(const Instruction *I, const Twine &ErrMsg [[clang::lifetimebound]] ,
                         DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Generic, Severity), MsgStr(ErrMsg), Inst(I) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  const Twine &getMsgStr() const [[clang::lifetimebound]] { return MsgStr; }
   const Instruction *getInstruction() const { return Inst; }
 
   /// \see DiagnosticInfo::print.
@@ -193,7 +193,7 @@ public:
                           DiagnosticSeverity Severity = DS_Error);
 
   uint64_t getLocCookie() const { return LocCookie; }
-  const Twine &getMsgStr() const { return MsgStr; }
+  const Twine &getMsgStr() const [[clang::lifetimebound]] { return MsgStr; }
   const Instruction *getInstruction() const { return Instr; }
 
   /// \see DiagnosticInfo::print.
@@ -364,7 +364,7 @@ public:
 
   /// Return the absolute path tot the file.
   std::string getAbsolutePath() const;
-  
+
   const Function &getFunction() const { return Fn; }
   DiagnosticLocation getLocation() const { return Loc; }
 
@@ -391,7 +391,7 @@ public:
       : DiagnosticInfoWithLocationBase(DK_GenericWithLoc, Severity, Fn, Loc),
         MsgStr(MsgStr) {}
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  const Twine &getMsgStr() const [[clang::lifetimebound]] { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
@@ -417,7 +417,7 @@ public:
   DiagnosticInfoRegAllocFailure(const Twine &MsgStr, const Function &Fn,
                                 DiagnosticSeverity Severity = DS_Error);
 
-  const Twine &getMsgStr() const { return MsgStr; }
+  const Twine &getMsgStr() const [[clang::lifetimebound]] { return MsgStr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;

 warning: temporary whose address is used as value of local variable 'Diag' will be destroyed at the end of the full-expression [-Wdangling]
 2863 |     auto Diag = DiagnosticInfoGeneric("bug");

bogner · 2025-04-21T16:46:06Z

Can we just get a warning on the bad usage instead?

I'd be perfectly happy for this to just warn on misuse, but I don't think it's possible. Essentially we'd need to be able to warn on creating any of these DiagnosticInfo* classes on the stack, as the problematic pattern is as follows:

Seems to work if we add [[clang::lifetimebound]] to all of these constructor parameters

This looks promising, I’ll look into doing that instead.

bogner · 2025-04-25T21:15:07Z

Superceded by #137397, which opts to warn on misuse instead.

llvmbot added PGO Profile Guided Optimizations backend:DirectX llvm:SelectionDAG SelectionDAGISel as well llvm:ir llvm:transforms labels Apr 18, 2025

bogner requested a review from nikic April 18, 2025 21:29

github-project-automation bot added this to HLSL Support Apr 18, 2025

bogner requested a review from arsenm April 18, 2025 21:29

bogner mentioned this pull request Apr 18, 2025

[DirectX] add Function name to DiagnosticInfoUnsupported Msg in DXILOpLowering #136234

Merged

farzonl approved these changes Apr 18, 2025

View reviewed changes

arsenm requested changes Apr 19, 2025

View reviewed changes

arsenm reviewed Apr 19, 2025

View reviewed changes

fixup: Fix target specific diags that were missed

1695f88

llvmbot added backend:AMDGPU LTO Link time optimization (regular/full LTO or ThinLTO) labels Apr 21, 2025

bogner added 3 commits April 21, 2025 01:47

fixup: clean up uses where the string *was* copied

c454daf

These uses were fine, but the comment that claimed the string isn't copied are just incorrect.

fixup: adjacent clang-format to appease the bot

736d2df

fixup: typos

7fe9dc0

bogner marked this pull request as draft April 21, 2025 16:44

bogner closed this Apr 25, 2025

github-project-automation bot moved this to Closed in HLSL Support Apr 25, 2025

damyanp removed this from HLSL Support Jun 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stop abusing Twine in DiagnosticInfo #136371

Stop abusing Twine in DiagnosticInfo #136371

Uh oh!

bogner commented Apr 18, 2025

Uh oh!

llvmbot commented Apr 18, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 18, 2025

Uh oh!

llvmbot commented Apr 18, 2025

Uh oh!

arsenm left a comment

Uh oh!

arsenm left a comment

Uh oh!

bogner commented Apr 19, 2025 •

edited

Loading

Uh oh!

bogner commented Apr 21, 2025

Uh oh!

github-actions bot commented Apr 21, 2025 •

edited

Loading

Uh oh!

arsenm commented Apr 21, 2025

Uh oh!

bogner commented Apr 21, 2025

Uh oh!

bogner commented Apr 25, 2025

Uh oh!

Uh oh!

Stop abusing Twine in DiagnosticInfo #136371

Stop abusing Twine in DiagnosticInfo #136371

Uh oh!

Conversation

bogner commented Apr 18, 2025

Uh oh!

llvmbot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 18, 2025

Uh oh!

llvmbot commented Apr 18, 2025

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

bogner commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogner commented Apr 21, 2025

Uh oh!

github-actions bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm commented Apr 21, 2025

Uh oh!

bogner commented Apr 21, 2025

Uh oh!

bogner commented Apr 25, 2025

Uh oh!

Uh oh!

llvmbot commented Apr 18, 2025 •

edited

Loading

bogner commented Apr 19, 2025 •

edited

Loading

github-actions bot commented Apr 21, 2025 •

edited

Loading