[llvm] Stop abusing Twine in DiagnosticInfo (PR #136371)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 18 14:30:15 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
Author: Justin Bogner (bogner)
<details>
<summary>Changes</summary>
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.
---
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
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]
``````````
</details>
https://github.com/llvm/llvm-project/pull/136371
More information about the llvm-commits
mailing list