Skip to content

[ELF] Add context-aware diagnostic functions #112319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 6, 2024
Merged

[ELF] Add context-aware diagnostic functions #112319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lld/Common/ErrorHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
35 changes: 35 additions & 0 deletions lld/ELF/Config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,8 @@ struct Ctx {
LinkerScript *script;
std::unique_ptr<TargetInfo> target;

ErrorHandler *errHandler;

// These variables are initialized by Writer and should not be used before
// Writer is initialized.
uint8_t *bufferStart;
Expand Down Expand Up @@ -679,6 +681,39 @@ static inline void internalLinkerError(StringRef loc, const Twine &msg) {
llvm::getBugReportMsg());
}

struct ELFSyncStream : SyncStream {
Ctx &ctx;
ELFSyncStream(Ctx &ctx, DiagLevel level)
: SyncStream(*ctx.errHandler, 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;
}

// Report a log if --verbose is specified.
ELFSyncStream Log(Ctx &ctx);

// Report a warning. Upgraded to an error if --fatal-warnings is specified.
ELFSyncStream Warn(Ctx &ctx);

// Report an error that will suppress the output file generation. Downgraded to
// a warning if --noinhibit-exec is specified.
ELFSyncStream Err(Ctx &ctx);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted ErrAlways.

Sorry that I just realized that I did not update this yet.


// Report an error regardless of --noinhibit-exec.
ELFSyncStream ErrAlways(Ctx &ctx);

// Report a fatal error that exits immediately. This should generally be avoided
// in favor of Err.
ELFSyncStream Fatal(Ctx &ctx);

uint64_t errCount(Ctx &ctx);

} // namespace lld::elf

#endif
23 changes: 18 additions & 5 deletions lld/ELF/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,15 @@ 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::ErrAlways(Ctx &ctx) { return {ctx, DiagLevel::Err}; }
ELFSyncStream elf::Fatal(Ctx &ctx) { return {ctx, DiagLevel::Fatal}; }
uint64_t elf::errCount(Ctx &ctx) { return ctx.errHandler->errorCount; }

Ctx::Ctx() : driver(*this) {}

void Ctx::reset() {
Expand All @@ -101,6 +110,8 @@ void Ctx::reset() {
script = nullptr;
target.reset();

errHandler = nullptr;

bufferStart = nullptr;
mainPart = nullptr;
tlsPhdr = nullptr;
Expand Down Expand Up @@ -169,6 +180,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,
Ctx &ctx = elf::ctx;
LinkerScript script(ctx);
ctx.script = &script;
ctx.errHandler = &context->e;
ctx.symAux.emplace_back();
ctx.symtab = std::make_unique<SymbolTable>(ctx);

Expand All @@ -179,7 +191,7 @@ bool link(ArrayRef<const char *> args, llvm::raw_ostream &stdoutOS,

ctx.driver.linkerMain(args);

return errorCount() == 0;
return errCount(ctx) == 0;
}
} // namespace elf
} // namespace lld
Expand Down Expand Up @@ -366,7 +378,7 @@ void LinkerDriver::addFile(StringRef path, bool withLOption) {
files.push_back(createObjFile(ctx, mbref, "", inLib));
break;
default:
error(path + ": unknown file type");
ErrAlways(ctx) << path << ": unknown file type";
}
}

Expand Down Expand Up @@ -2780,8 +2792,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, it looks much better!

<< ": -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")
Expand Down Expand Up @@ -3048,7 +3061,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
Expand Down
6 changes: 6 additions & 0 deletions lld/ELF/InputFiles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions lld/ELF/InputFiles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
9 changes: 9 additions & 0 deletions lld/ELF/Target.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (" << type << ')';
else
s << buf;
return s;
}

void elf::setTarget(Ctx &ctx) {
switch (ctx.arg.emachine) {
case EM_386:
Expand Down
2 changes: 2 additions & 0 deletions lld/ELF/Target.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 16 additions & 1 deletion lld/include/lld/Common/ErrorHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -152,6 +152,21 @@ void message(const Twine &msg, llvm::raw_ostream &s = outs());
void warn(const Twine &msg);
uint64_t errorCount();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ELF, the intention is yes. Perhaps this "deprecation" notice can be added once another port decides to follow ELF?


enum class DiagLevel { Log, Warn, Err, Fatal };

// A class that synchronizes thread writing to the same stream similar
// std::osyncstream.
class SyncStream {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name SyncStream, also used by mold, comes from https://en.cppreference.com/w/cpp/io/basic_osyncstream

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);
Expand Down
Loading