[ELF] Add context-aware diagnostic functions #112319
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lld-macho @llvm/pr-subscribers-lld-wasm Author: Fangrui Song (MaskRay) ChangesThe current diagnostic functions log/warn/error/fatal lack a context This patch introduces context-aware replacements:
Example:
(For the ELF port, the long term goal is to eliminate Full diff: https://github.com/llvm/llvm-project/pull/112319.diff 8 Files Affected:
diff --git a/lld/Common/ErrorHandler.cpp b/lld/Common/ErrorHandler.cpp
index 4e3a1bc31ade50..21755481665961 100644
--- a/lld/Common/ErrorHandler.cpp
+++ b/lld/Common/ErrorHandler.cpp
@@ -335,3 +335,21 @@ void ErrorHandler::fatal(const Twine &msg) {
error(msg);
exitLld(1);
}
+
+SyncStream::~SyncStream() {
+ os.flush();
+ switch (level) {
+ case DiagLevel::Log:
+ e.log(buf);
+ break;
+ case DiagLevel::Warn:
+ e.warn(buf);
+ break;
+ case DiagLevel::Err:
+ e.error(buf);
+ break;
+ case DiagLevel::Fatal:
+ e.fatal(buf);
+ break;
+ }
+}
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index f5d65d26ef4479..59bae75794571b 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -547,6 +547,8 @@ struct Ctx {
LinkerScript *script;
std::unique_ptr<TargetInfo> target;
+ ErrorHandler *e;
+
// These variables are initialized by Writer and should not be used before
// Writer is initialized.
uint8_t *bufferStart;
@@ -679,6 +681,25 @@ static inline void internalLinkerError(StringRef loc, const Twine &msg) {
llvm::getBugReportMsg());
}
+struct ELFSyncStream : SyncStream {
+ Ctx &ctx;
+ ELFSyncStream(Ctx &ctx, DiagLevel level)
+ : SyncStream(*ctx.e, level), ctx(ctx) {}
+};
+
+template <typename T>
+std::enable_if_t<!std::is_pointer_v<std::remove_reference_t<T>>,
+ const ELFSyncStream &>
+operator<<(const ELFSyncStream &s, T &&v) {
+ s.os << std::forward<T>(v);
+ return s;
+}
+
+ELFSyncStream Log(Ctx &ctx);
+ELFSyncStream Warn(Ctx &ctx);
+ELFSyncStream Err(Ctx &ctx);
+ELFSyncStream Fatal(Ctx &ctx);
+
} // namespace lld::elf
#endif
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index afacbbfe9099cc..4774047de8442d 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -91,6 +91,13 @@ void elf::errorOrWarn(const Twine &msg) {
error(msg);
}
+ELFSyncStream elf::Log(Ctx &ctx) { return {ctx, DiagLevel::Log}; }
+ELFSyncStream elf::Warn(Ctx &ctx) { return {ctx, DiagLevel::Warn}; }
+ELFSyncStream elf::Err(Ctx &ctx) {
+ return {ctx, ctx.arg.noinhibitExec ? DiagLevel::Warn : DiagLevel::Err};
+}
+ELFSyncStream elf::Fatal(Ctx &ctx) { return {ctx, DiagLevel::Fatal}; }
+
Ctx::Ctx() : driver(*this) {}
void Ctx::reset() {
@@ -101,6 +108,8 @@ void Ctx::reset() {
script = nullptr;
target.reset();
+ e = nullptr;
+
bufferStart = nullptr;
mainPart = nullptr;
tlsPhdr = nullptr;
@@ -169,6 +178,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
Ctx &ctx = elf::ctx;
LinkerScript script(ctx);
ctx.script = &script;
+ ctx.e = &context->e;
ctx.symAux.emplace_back();
ctx.symtab = std::make_unique<SymbolTable>(ctx);
@@ -2780,8 +2790,9 @@ static void readSecurityNotes(Ctx &ctx) {
if (ctx.arg.zForceBti && !(features & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)) {
features |= GNU_PROPERTY_AARCH64_FEATURE_1_BTI;
if (ctx.arg.zBtiReport == "none")
- warn(toString(f) + ": -z force-bti: file does not have "
- "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property");
+ Warn(ctx) << f
+ << ": -z force-bti: file does not have "
+ "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property";
} else if (ctx.arg.zForceIbt &&
!(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
if (ctx.arg.zCetReport == "none")
@@ -3048,7 +3059,7 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
oldFilenames.insert(f->getName());
for (InputFile *newFile : newInputFiles)
if (!oldFilenames.contains(newFile->getName()))
- errorOrWarn("input file '" + newFile->getName() + "' added after LTO");
+ Err(ctx) << "input file '" << newFile->getName() << "' added after LTO";
}
// Handle --exclude-libs again because lto.tmp may reference additional
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 3d02ef8b77abaa..a3e9bb9daa52ff 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -66,6 +66,12 @@ std::string lld::toString(const InputFile *f) {
return std::string(f->toStringCache);
}
+const ELFSyncStream &elf::operator<<(const ELFSyncStream &s,
+ const InputFile *f) {
+ s << toString(f);
+ return s;
+}
+
static ELFKind getELFKind(MemoryBufferRef mb, StringRef archiveName) {
unsigned char size;
unsigned char endian;
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index f80413b215047d..e00e2c83d017c2 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -386,6 +386,8 @@ ELFFileBase *createObjFile(Ctx &, MemoryBufferRef mb,
std::string replaceThinLTOSuffix(Ctx &, StringRef path);
+const ELFSyncStream &operator<<(const ELFSyncStream &, const InputFile *);
+
} // namespace elf
} // namespace lld
diff --git a/lld/ELF/Target.cpp b/lld/ELF/Target.cpp
index 96d132a2a8f369..b38446f287bf71 100644
--- a/lld/ELF/Target.cpp
+++ b/lld/ELF/Target.cpp
@@ -45,6 +45,15 @@ std::string lld::toString(RelType type) {
return std::string(s);
}
+const ELFSyncStream &elf::operator<<(const ELFSyncStream &s, RelType type) {
+ StringRef buf = getELFRelocationTypeName(s.ctx.arg.emachine, type);
+ if (buf == "Unknown")
+ s << ("Unknown (" + Twine(type) + ")").str();
+ else
+ s << buf;
+ return s;
+}
+
void elf::setTarget(Ctx &ctx) {
switch (ctx.arg.emachine) {
case EM_386:
diff --git a/lld/ELF/Target.h b/lld/ELF/Target.h
index 27986b0a127e8e..250a29d6184313 100644
--- a/lld/ELF/Target.h
+++ b/lld/ELF/Target.h
@@ -327,6 +327,8 @@ inline uint64_t overwriteULEB128(uint8_t *bufLoc, uint64_t val) {
*bufLoc = val;
return val;
}
+
+const ELFSyncStream &operator<<(const ELFSyncStream &, RelType);
} // namespace elf
} // namespace lld
diff --git a/lld/include/lld/Common/ErrorHandler.h b/lld/include/lld/Common/ErrorHandler.h
index 0b69bb6202b32c..6bc9d81c1741a8 100644
--- a/lld/include/lld/Common/ErrorHandler.h
+++ b/lld/include/lld/Common/ErrorHandler.h
@@ -73,11 +73,11 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileOutputBuffer.h"
+#include "llvm/Support/raw_ostream.h"
#include <mutex>
namespace llvm {
class DiagnosticInfo;
-class raw_ostream;
}
namespace lld {
@@ -152,6 +152,19 @@ void message(const Twine &msg, llvm::raw_ostream &s = outs());
void warn(const Twine &msg);
uint64_t errorCount();
+enum class DiagLevel { Log, Warn, Err, Fatal };
+
+class SyncStream {
+ ErrorHandler &e;
+ DiagLevel level;
+ std::string buf;
+
+public:
+ mutable llvm::raw_string_ostream os{buf};
+ SyncStream(ErrorHandler &e, DiagLevel level) : e(e), level(level) {}
+ ~SyncStream();
+};
+
[[noreturn]] void exitLld(int val);
void diagnosticHandler(const llvm::DiagnosticInfo &di);
|
|
@llvm/pr-subscribers-lld-elf Author: Fangrui Song (MaskRay) ChangesThe current diagnostic functions log/warn/error/fatal lack a context This patch introduces context-aware replacements:
Example:
(For the ELF port, the long term goal is to eliminate Full diff: https://github.com/llvm/llvm-project/pull/112319.diff 8 Files Affected:
diff --git a/lld/Common/ErrorHandler.cpp b/lld/Common/ErrorHandler.cpp
index 4e3a1bc31ade50..21755481665961 100644
--- a/lld/Common/ErrorHandler.cpp
+++ b/lld/Common/ErrorHandler.cpp
@@ -335,3 +335,21 @@ void ErrorHandler::fatal(const Twine &msg) {
error(msg);
exitLld(1);
}
+
+SyncStream::~SyncStream() {
+ os.flush();
+ switch (level) {
+ case DiagLevel::Log:
+ e.log(buf);
+ break;
+ case DiagLevel::Warn:
+ e.warn(buf);
+ break;
+ case DiagLevel::Err:
+ e.error(buf);
+ break;
+ case DiagLevel::Fatal:
+ e.fatal(buf);
+ break;
+ }
+}
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index f5d65d26ef4479..59bae75794571b 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -547,6 +547,8 @@ struct Ctx {
LinkerScript *script;
std::unique_ptr<TargetInfo> target;
+ ErrorHandler *e;
+
// These variables are initialized by Writer and should not be used before
// Writer is initialized.
uint8_t *bufferStart;
@@ -679,6 +681,25 @@ static inline void internalLinkerError(StringRef loc, const Twine &msg) {
llvm::getBugReportMsg());
}
+struct ELFSyncStream : SyncStream {
+ Ctx &ctx;
+ ELFSyncStream(Ctx &ctx, DiagLevel level)
+ : SyncStream(*ctx.e, level), ctx(ctx) {}
+};
+
+template <typename T>
+std::enable_if_t<!std::is_pointer_v<std::remove_reference_t<T>>,
+ const ELFSyncStream &>
+operator<<(const ELFSyncStream &s, T &&v) {
+ s.os << std::forward<T>(v);
+ return s;
+}
+
+ELFSyncStream Log(Ctx &ctx);
+ELFSyncStream Warn(Ctx &ctx);
+ELFSyncStream Err(Ctx &ctx);
+ELFSyncStream Fatal(Ctx &ctx);
+
} // namespace lld::elf
#endif
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index afacbbfe9099cc..4774047de8442d 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -91,6 +91,13 @@ void elf::errorOrWarn(const Twine &msg) {
error(msg);
}
+ELFSyncStream elf::Log(Ctx &ctx) { return {ctx, DiagLevel::Log}; }
+ELFSyncStream elf::Warn(Ctx &ctx) { return {ctx, DiagLevel::Warn}; }
+ELFSyncStream elf::Err(Ctx &ctx) {
+ return {ctx, ctx.arg.noinhibitExec ? DiagLevel::Warn : DiagLevel::Err};
+}
+ELFSyncStream elf::Fatal(Ctx &ctx) { return {ctx, DiagLevel::Fatal}; }
+
Ctx::Ctx() : driver(*this) {}
void Ctx::reset() {
@@ -101,6 +108,8 @@ void Ctx::reset() {
script = nullptr;
target.reset();
+ e = nullptr;
+
bufferStart = nullptr;
mainPart = nullptr;
tlsPhdr = nullptr;
@@ -169,6 +178,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
Ctx &ctx = elf::ctx;
LinkerScript script(ctx);
ctx.script = &script;
+ ctx.e = &context->e;
ctx.symAux.emplace_back();
ctx.symtab = std::make_unique<SymbolTable>(ctx);
@@ -2780,8 +2790,9 @@ static void readSecurityNotes(Ctx &ctx) {
if (ctx.arg.zForceBti && !(features & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)) {
features |= GNU_PROPERTY_AARCH64_FEATURE_1_BTI;
if (ctx.arg.zBtiReport == "none")
- warn(toString(f) + ": -z force-bti: file does not have "
- "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property");
+ Warn(ctx) << f
+ << ": -z force-bti: file does not have "
+ "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property";
} else if (ctx.arg.zForceIbt &&
!(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
if (ctx.arg.zCetReport == "none")
@@ -3048,7 +3059,7 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
oldFilenames.insert(f->getName());
for (InputFile *newFile : newInputFiles)
if (!oldFilenames.contains(newFile->getName()))
- errorOrWarn("input file '" + newFile->getName() + "' added after LTO");
+ Err(ctx) << "input file '" << newFile->getName() << "' added after LTO";
}
// Handle --exclude-libs again because lto.tmp may reference additional
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 3d02ef8b77abaa..a3e9bb9daa52ff 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -66,6 +66,12 @@ std::string lld::toString(const InputFile *f) {
return std::string(f->toStringCache);
}
+const ELFSyncStream &elf::operator<<(const ELFSyncStream &s,
+ const InputFile *f) {
+ s << toString(f);
+ return s;
+}
+
static ELFKind getELFKind(MemoryBufferRef mb, StringRef archiveName) {
unsigned char size;
unsigned char endian;
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index f80413b215047d..e00e2c83d017c2 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -386,6 +386,8 @@ ELFFileBase *createObjFile(Ctx &, MemoryBufferRef mb,
std::string replaceThinLTOSuffix(Ctx &, StringRef path);
+const ELFSyncStream &operator<<(const ELFSyncStream &, const InputFile *);
+
} // namespace elf
} // namespace lld
diff --git a/lld/ELF/Target.cpp b/lld/ELF/Target.cpp
index 96d132a2a8f369..b38446f287bf71 100644
--- a/lld/ELF/Target.cpp
+++ b/lld/ELF/Target.cpp
@@ -45,6 +45,15 @@ std::string lld::toString(RelType type) {
return std::string(s);
}
+const ELFSyncStream &elf::operator<<(const ELFSyncStream &s, RelType type) {
+ StringRef buf = getELFRelocationTypeName(s.ctx.arg.emachine, type);
+ if (buf == "Unknown")
+ s << ("Unknown (" + Twine(type) + ")").str();
+ else
+ s << buf;
+ return s;
+}
+
void elf::setTarget(Ctx &ctx) {
switch (ctx.arg.emachine) {
case EM_386:
diff --git a/lld/ELF/Target.h b/lld/ELF/Target.h
index 27986b0a127e8e..250a29d6184313 100644
--- a/lld/ELF/Target.h
+++ b/lld/ELF/Target.h
@@ -327,6 +327,8 @@ inline uint64_t overwriteULEB128(uint8_t *bufLoc, uint64_t val) {
*bufLoc = val;
return val;
}
+
+const ELFSyncStream &operator<<(const ELFSyncStream &, RelType);
} // namespace elf
} // namespace lld
diff --git a/lld/include/lld/Common/ErrorHandler.h b/lld/include/lld/Common/ErrorHandler.h
index 0b69bb6202b32c..6bc9d81c1741a8 100644
--- a/lld/include/lld/Common/ErrorHandler.h
+++ b/lld/include/lld/Common/ErrorHandler.h
@@ -73,11 +73,11 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileOutputBuffer.h"
+#include "llvm/Support/raw_ostream.h"
#include <mutex>
namespace llvm {
class DiagnosticInfo;
-class raw_ostream;
}
namespace lld {
@@ -152,6 +152,19 @@ void message(const Twine &msg, llvm::raw_ostream &s = outs());
void warn(const Twine &msg);
uint64_t errorCount();
+enum class DiagLevel { Log, Warn, Err, Fatal };
+
+class SyncStream {
+ ErrorHandler &e;
+ DiagLevel level;
+ std::string buf;
+
+public:
+ mutable llvm::raw_string_ostream os{buf};
+ SyncStream(ErrorHandler &e, DiagLevel level) : e(e), level(level) {}
+ ~SyncStream();
+};
+
[[noreturn]] void exitLld(int val);
void diagnosticHandler(const llvm::DiagnosticInfo &di);
|
|
@llvm/pr-subscribers-lld-coff Author: Fangrui Song (MaskRay) ChangesThe current diagnostic functions log/warn/error/fatal lack a context This patch introduces context-aware replacements:
Example:
(For the ELF port, the long term goal is to eliminate Full diff: https://github.com/llvm/llvm-project/pull/112319.diff 8 Files Affected:
diff --git a/lld/Common/ErrorHandler.cpp b/lld/Common/ErrorHandler.cpp
index 4e3a1bc31ade50..21755481665961 100644
--- a/lld/Common/ErrorHandler.cpp
+++ b/lld/Common/ErrorHandler.cpp
@@ -335,3 +335,21 @@ void ErrorHandler::fatal(const Twine &msg) {
error(msg);
exitLld(1);
}
+
+SyncStream::~SyncStream() {
+ os.flush();
+ switch (level) {
+ case DiagLevel::Log:
+ e.log(buf);
+ break;
+ case DiagLevel::Warn:
+ e.warn(buf);
+ break;
+ case DiagLevel::Err:
+ e.error(buf);
+ break;
+ case DiagLevel::Fatal:
+ e.fatal(buf);
+ break;
+ }
+}
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index f5d65d26ef4479..59bae75794571b 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -547,6 +547,8 @@ struct Ctx {
LinkerScript *script;
std::unique_ptr<TargetInfo> target;
+ ErrorHandler *e;
+
// These variables are initialized by Writer and should not be used before
// Writer is initialized.
uint8_t *bufferStart;
@@ -679,6 +681,25 @@ static inline void internalLinkerError(StringRef loc, const Twine &msg) {
llvm::getBugReportMsg());
}
+struct ELFSyncStream : SyncStream {
+ Ctx &ctx;
+ ELFSyncStream(Ctx &ctx, DiagLevel level)
+ : SyncStream(*ctx.e, level), ctx(ctx) {}
+};
+
+template <typename T>
+std::enable_if_t<!std::is_pointer_v<std::remove_reference_t<T>>,
+ const ELFSyncStream &>
+operator<<(const ELFSyncStream &s, T &&v) {
+ s.os << std::forward<T>(v);
+ return s;
+}
+
+ELFSyncStream Log(Ctx &ctx);
+ELFSyncStream Warn(Ctx &ctx);
+ELFSyncStream Err(Ctx &ctx);
+ELFSyncStream Fatal(Ctx &ctx);
+
} // namespace lld::elf
#endif
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index afacbbfe9099cc..4774047de8442d 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -91,6 +91,13 @@ void elf::errorOrWarn(const Twine &msg) {
error(msg);
}
+ELFSyncStream elf::Log(Ctx &ctx) { return {ctx, DiagLevel::Log}; }
+ELFSyncStream elf::Warn(Ctx &ctx) { return {ctx, DiagLevel::Warn}; }
+ELFSyncStream elf::Err(Ctx &ctx) {
+ return {ctx, ctx.arg.noinhibitExec ? DiagLevel::Warn : DiagLevel::Err};
+}
+ELFSyncStream elf::Fatal(Ctx &ctx) { return {ctx, DiagLevel::Fatal}; }
+
Ctx::Ctx() : driver(*this) {}
void Ctx::reset() {
@@ -101,6 +108,8 @@ void Ctx::reset() {
script = nullptr;
target.reset();
+ e = nullptr;
+
bufferStart = nullptr;
mainPart = nullptr;
tlsPhdr = nullptr;
@@ -169,6 +178,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
Ctx &ctx = elf::ctx;
LinkerScript script(ctx);
ctx.script = &script;
+ ctx.e = &context->e;
ctx.symAux.emplace_back();
ctx.symtab = std::make_unique<SymbolTable>(ctx);
@@ -2780,8 +2790,9 @@ static void readSecurityNotes(Ctx &ctx) {
if (ctx.arg.zForceBti && !(features & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)) {
features |= GNU_PROPERTY_AARCH64_FEATURE_1_BTI;
if (ctx.arg.zBtiReport == "none")
- warn(toString(f) + ": -z force-bti: file does not have "
- "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property");
+ Warn(ctx) << f
+ << ": -z force-bti: file does not have "
+ "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property";
} else if (ctx.arg.zForceIbt &&
!(features & GNU_PROPERTY_X86_FEATURE_1_IBT)) {
if (ctx.arg.zCetReport == "none")
@@ -3048,7 +3059,7 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
oldFilenames.insert(f->getName());
for (InputFile *newFile : newInputFiles)
if (!oldFilenames.contains(newFile->getName()))
- errorOrWarn("input file '" + newFile->getName() + "' added after LTO");
+ Err(ctx) << "input file '" << newFile->getName() << "' added after LTO";
}
// Handle --exclude-libs again because lto.tmp may reference additional
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 3d02ef8b77abaa..a3e9bb9daa52ff 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -66,6 +66,12 @@ std::string lld::toString(const InputFile *f) {
return std::string(f->toStringCache);
}
+const ELFSyncStream &elf::operator<<(const ELFSyncStream &s,
+ const InputFile *f) {
+ s << toString(f);
+ return s;
+}
+
static ELFKind getELFKind(MemoryBufferRef mb, StringRef archiveName) {
unsigned char size;
unsigned char endian;
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index f80413b215047d..e00e2c83d017c2 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -386,6 +386,8 @@ ELFFileBase *createObjFile(Ctx &, MemoryBufferRef mb,
std::string replaceThinLTOSuffix(Ctx &, StringRef path);
+const ELFSyncStream &operator<<(const ELFSyncStream &, const InputFile *);
+
} // namespace elf
} // namespace lld
diff --git a/lld/ELF/Target.cpp b/lld/ELF/Target.cpp
index 96d132a2a8f369..b38446f287bf71 100644
--- a/lld/ELF/Target.cpp
+++ b/lld/ELF/Target.cpp
@@ -45,6 +45,15 @@ std::string lld::toString(RelType type) {
return std::string(s);
}
+const ELFSyncStream &elf::operator<<(const ELFSyncStream &s, RelType type) {
+ StringRef buf = getELFRelocationTypeName(s.ctx.arg.emachine, type);
+ if (buf == "Unknown")
+ s << ("Unknown (" + Twine(type) + ")").str();
+ else
+ s << buf;
+ return s;
+}
+
void elf::setTarget(Ctx &ctx) {
switch (ctx.arg.emachine) {
case EM_386:
diff --git a/lld/ELF/Target.h b/lld/ELF/Target.h
index 27986b0a127e8e..250a29d6184313 100644
--- a/lld/ELF/Target.h
+++ b/lld/ELF/Target.h
@@ -327,6 +327,8 @@ inline uint64_t overwriteULEB128(uint8_t *bufLoc, uint64_t val) {
*bufLoc = val;
return val;
}
+
+const ELFSyncStream &operator<<(const ELFSyncStream &, RelType);
} // namespace elf
} // namespace lld
diff --git a/lld/include/lld/Common/ErrorHandler.h b/lld/include/lld/Common/ErrorHandler.h
index 0b69bb6202b32c..6bc9d81c1741a8 100644
--- a/lld/include/lld/Common/ErrorHandler.h
+++ b/lld/include/lld/Common/ErrorHandler.h
@@ -73,11 +73,11 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileOutputBuffer.h"
+#include "llvm/Support/raw_ostream.h"
#include <mutex>
namespace llvm {
class DiagnosticInfo;
-class raw_ostream;
}
namespace lld {
@@ -152,6 +152,19 @@ void message(const Twine &msg, llvm::raw_ostream &s = outs());
void warn(const Twine &msg);
uint64_t errorCount();
+enum class DiagLevel { Log, Warn, Err, Fatal };
+
+class SyncStream {
+ ErrorHandler &e;
+ DiagLevel level;
+ std::string buf;
+
+public:
+ mutable llvm::raw_string_ostream os{buf};
+ SyncStream(ErrorHandler &e, DiagLevel level) : e(e), level(level) {}
+ ~SyncStream();
+};
+
[[noreturn]] void exitLld(int val);
void diagnosticHandler(const llvm::DiagnosticInfo &di);
|
smithp35
reviewed
Oct 15, 2024
|
|
||
| ELFSyncStream Log(Ctx &ctx); | ||
| ELFSyncStream Warn(Ctx &ctx); | ||
| ELFSyncStream Err(Ctx &ctx); |
There was a problem hiding this comment.
Is the intention that all errors are now errorOrWarn, it looks we currently have error and errorOrWarn.
I can see error being useful for conditions where the output file is always going to be broken, but we can complete the link with diagnostics. The errorOrWarn can be for when the output is likely broken, or some standard has been broken. FatalError can be used if we're going crash if we keep going.
This isn't a strong distinction so I can see it being worthwhile to merge so that more diagnostics can become warnings if needed.
There was a problem hiding this comment.
Agree with the above. I think we should add a comment before these 4 functions, to explain in which situation they should be used, and what are the expectations.
There was a problem hiding this comment.
We prefer errorOrWarn to error and the former is used much more. The lengthy name problem can now be "fixed" as we are switching to Err.
I agree that we need a replacement for lld::error as well. Do you have a suggestion for the name?
There was a problem hiding this comment.
If Err is the default, then perhaps ErrAlways for the non downgradeable variety. If it isn't as common it could have a longer name.
Alternatives: ForceErr, PermErr, AlwaysErr.
There was a problem hiding this comment.
Adopted ErrAlways.
Sorry that I just realized that I did not update this yet.
lld/ELF/Config.h
Outdated
| @@ -547,6 +547,8 @@ struct Ctx { | |||
| LinkerScript *script; | |||
| std::unique_ptr<TargetInfo> target; | |||
|
|
|||
| ErrorHandler *e; | |||
There was a problem hiding this comment.
I think it would be worth using a full name as it isn't used frequently enough to benefit from a short abbreviation.
ctx.e could be almost anything. Perhaps ctx.errHandler.
oontvoo
reviewed
Oct 15, 2024
aganea
reviewed
Oct 15, 2024
There was a problem hiding this comment.
I am curious, what led you to do this?
Two extra notes:
- I have started doing https://github.com/aganea/llvm-project/tree/my for COFF. This was working for simple stuff, but as soon as we hit ManagedObjects, cl::opts or other global state, more work will be needed (which nevertheless I'd like us to do).
- As an escape hatch for err/warn/fatal, and to speed things up, I actually made the
CommonLinkerContexta thread-local, see https://github.com/aganea/llvm-project/blob/d9516dee59fab6675e5d865cc8fbe15af2d6418f/lld/Common/CommonLinkerContext.cpp#L21 and more particulary the 'my' branch above. In any case, even if we made all diagnostic functions context-aware, I think we'd still need a thread-local context when usingParallel.hfunctions; unless we pass the context too, along with the lambda.
@MaskRay If you're going to the LLVM conf. next week, we could have a chat around all this stuff!
lld/ELF/Config.h
Outdated
| @@ -547,6 +547,8 @@ struct Ctx { | |||
| LinkerScript *script; | |||
| std::unique_ptr<TargetInfo> target; | |||
|
|
|||
| ErrorHandler *e; | |||
|
|
||
| ELFSyncStream Log(Ctx &ctx); | ||
| ELFSyncStream Warn(Ctx &ctx); | ||
| ELFSyncStream Err(Ctx &ctx); |
There was a problem hiding this comment.
Agree with the above. I think we should add a comment before these 4 functions, to explain in which situation they should be used, and what are the expectations.
| @@ -2780,8 +2790,9 @@ static void readSecurityNotes(Ctx &ctx) { | |||
| if (ctx.arg.zForceBti && !(features & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)) { | |||
| features |= GNU_PROPERTY_AARCH64_FEATURE_1_BTI; | |||
| if (ctx.arg.zBtiReport == "none") | |||
| warn(toString(f) + ": -z force-bti: file does not have " | |||
| "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property"); | |||
| Warn(ctx) << f | |||
There was a problem hiding this comment.
This is great, it looks much better!
sbc100
reviewed
Oct 15, 2024
| @@ -152,6 +152,19 @@ void message(const Twine &msg, llvm::raw_ostream &s = outs()); | |||
| void warn(const Twine &msg); | |||
| uint64_t errorCount(); | |||
|
|
|||
| enum class DiagLevel { Log, Warn, Err, Fatal }; | |||
|
|
|||
| class SyncStream { | |||
There was a problem hiding this comment.
I'm curious what the "Sync" part means here? Can we come up with a more meaningful name? Something that signifies this relates to logging/error output maybe?
There was a problem hiding this comment.
The name SyncStream, also used by mold, comes from https://en.cppreference.com/w/cpp/io/basic_osyncstream
|
Thanks to everyone for the interest on this patch:) To aganea:
Many folks frown upon the global state usage in the lld codebase... For the lld/ELF code, I have eliminated all global variables except
Thanks for sharing your work. I'll check when I get some free time...
Agreed. Using thread-local variable has some overhead, especially when -fPIC is used (almost assuredly set on ELF platforms due to LLVM_ENABLE_PIC and CMAKE_POSITION_INDEPENDENT_CODE). While I think TLS for diagnostics is fine, but for the linker context I do prefer the COFF-style explicit arguments (likely negligible overhead). Context-aware diagnostic functions can simplify error reporting code, as you've noticed in This migration work has made me appreciate the benefits of languages like Jai and Odin, which have implicit context. Regarding other global states:
Absolutely. Looking forward to meeting you in person! |
kosarev
reviewed
Oct 16, 2024
MaskRay
added a commit
that referenced
this pull request
Oct 22, 2024
The current diagnostic functions log/warn/error/fatal lack a context argument and call the global `lld::errorHandler()`, which prevents multiple lld instances in one process. This patch introduces context-aware replacements: * log => Log(ctx) * warn => Warn(ctx) * errorOrWarn => Err(ctx) * error => ErrAlways(ctx) * fatal => Fatal(ctx) Example: `errorOrWarn(toString(f) + "xxx")` => `Err(ctx) << f << "xxx"`. (`toString(f)` is shortened to `f` as a bonus and may access `ctx` without accessing the global variable (see `Target.cpp`)). `ctx.e = &context->e;` can be replaced with a non-global Errorhandler when `ctx` becomes a local variable. (For the ELF port, the long term goal is to eliminate `error`. Most can be straightforwardly converted to `Err(ctx)`.) Pull Request: #112319
MaskRay
force-pushed
the
users/MaskRay/spr/elf-add-context-aware-diagnostic-functions
branch
from
b62d0cb to
dccc00a
Compare
|
Ping:) |
MaskRay
force-pushed
the
users/MaskRay/spr/elf-add-context-aware-diagnostic-functions
branch
from
dccc00a to
53611ab
Compare
MaskRay
added a commit
that referenced
this pull request
Oct 30, 2024
The current diagnostic functions log/warn/error/fatal lack a context argument and call the global `lld::errorHandler()`, which prevents multiple lld instances in one process. This patch introduces context-aware replacements: * log => Log(ctx) * warn => Warn(ctx) * errorOrWarn => Err(ctx) * error => ErrAlways(ctx) * fatal => Fatal(ctx) Example: `errorOrWarn(toString(f) + "xxx")` => `Err(ctx) << f << "xxx"`. (`toString(f)` is shortened to `f` as a bonus and may access `ctx` without accessing the global variable (see `Target.cpp`)). `ctx.e = &context->e;` can be replaced with a non-global Errorhandler when `ctx` becomes a local variable. (For the ELF port, the long term goal is to eliminate `error`. Most can be straightforwardly converted to `Err(ctx)`.) Pull Request: #112319
MaskRay
force-pushed
the
users/MaskRay/spr/elf-add-context-aware-diagnostic-functions
branch
from
53611ab to
3d1739d
Compare
ellishg
reviewed
Oct 30, 2024
| @@ -152,6 +152,21 @@ void message(const Twine &msg, llvm::raw_ostream &s = outs()); | |||
| void warn(const Twine &msg); | |||
| uint64_t errorCount(); | |||
There was a problem hiding this comment.
Is the plan to replace these error methods with elf::Warn(), elf::Err(), ..., etc? If so can we add a doc to point users to these new methods?
There was a problem hiding this comment.
For ELF, the intention is yes. Perhaps this "deprecation" notice can be added once another port decides to follow ELF?
The current diagnostic functions log/warn/error/fatal lack a context argument and call the global `lld::errorHandler()`, which prevents multiple lld instances in one process. This patch introduces context-aware replacements: * log => Log(ctx) * warn => Warn(ctx) * errorOrWarn => Err(ctx) * error => ErrAlways(ctx) * fatal => Fatal(ctx) Example: `errorOrWarn(toString(f) + "xxx")` => `Err(ctx) << f << "xxx"`. (`toString(f)` is shortened to `f` as a bonus and may access `ctx` without accessing the global variable (see `Target.cpp`)). `ctx.e = &context->e;` can be replaced with a non-global Errorhandler when `ctx` becomes a local variable. (For the ELF port, the long term goal is to eliminate `error`. Most can be straightforwardly converted to `Err(ctx)`.) Pull Request: #112319
MaskRay
force-pushed
the
users/MaskRay/spr/elf-add-context-aware-diagnostic-functions
branch
from
3d1739d to
cd2382e
Compare
|
Ping:) |
Apologies, just got back from vacation today. Will take a look tomorrow. |
smithp35
approved these changes
Nov 5, 2024
MaskRay
deleted the
users/MaskRay/spr/elf-add-context-aware-diagnostic-functions
branch
MaskRay
added a commit
to MaskRay/llvm-project
that referenced
this pull request
Dec 3, 2024
Similar to llvm#112319 for ELF. While there is some initial boilerplate, it can simplify some call sites that use Twine, especially when a printed element uses `ctx` or toString.
MaskRay
added a commit
that referenced
this pull request
Dec 4, 2024
Similar to #112319 for ELF. While there is some initial boilerplate, it can simplify some call sites that use Twine, especially when a printed element uses `ctx` or toString.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current diagnostic functions log/warn/error/fatal lack a context
argument and call the global
lld::errorHandler(), which preventsmultiple lld instances in one process.
This patch introduces context-aware replacements:
Example:
errorOrWarn(toString(f) + "xxx")=>Err(ctx) << f << "xxx".(
toString(f)is shortened tofas a bonus and may accessctxwithout accessing the global variable (see
Target.cpp)).ctx.e = &context->e;can be replaced with a non-global Errorhandlerwhen
ctxbecomes a local variable.(For the ELF port, the long term goal is to eliminate
error. Most canbe straightforwardly converted to
Err(ctx).)