Skip to content

[LLD][COFF][NFC] Store live flag in ImportThunkChunk. #108459

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
Sep 13, 2024
Merged

[LLD][COFF][NFC] Store live flag in ImportThunkChunk. #108459

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
4 changes: 4 additions & 0 deletions lld/COFF/Chunks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,10 @@ void StringChunk::writeTo(uint8_t *buf) const {
buf[str.size()] = '\0';
}

ImportThunkChunk::ImportThunkChunk(COFFLinkerContext &ctx, Defined *s)
: NonSectionCodeChunk(ImportThunkKind), live(!ctx.config.doGC),
Copy link
Member

Choose a reason for hiding this comment

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

I presume the move from header to cpp file is because you want to access ctx.config here, which otherwise isn't visible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the reason.

impSymbol(s), ctx(ctx) {}

ImportThunkChunkX64::ImportThunkChunkX64(COFFLinkerContext &ctx, Defined *s)
: ImportThunkChunk(ctx, s) {
// Intel Optimization Manual says that all branch targets
Expand Down
7 changes: 5 additions & 2 deletions lld/COFF/Chunks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,10 +557,13 @@ static const uint8_t importThunkARM64EC[] = {
// contents will be a JMP instruction to some __imp_ symbol.
class ImportThunkChunk : public NonSectionCodeChunk {
public:
ImportThunkChunk(COFFLinkerContext &ctx, Defined *s)
: NonSectionCodeChunk(ImportThunkKind), impSymbol(s), ctx(ctx) {}
ImportThunkChunk(COFFLinkerContext &ctx, Defined *s);
static bool classof(const Chunk *c) { return c->kind() == ImportThunkKind; }

// We track the usage of the thunk symbol separately from the import file
// to avoid generating unnecessary thunks.
bool live;

protected:
Defined *impSymbol;
COFFLinkerContext &ctx;
Expand Down
2 changes: 1 addition & 1 deletion lld/COFF/InputFiles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,7 @@ void ObjFile::enqueuePdbFile(StringRef path, ObjFile *fromFile) {
}

ImportFile::ImportFile(COFFLinkerContext &ctx, MemoryBufferRef m)
: InputFile(ctx, ImportKind, m), live(!ctx.config.doGC), thunkLive(live) {}
: InputFile(ctx, ImportKind, m), live(!ctx.config.doGC) {}

MachineTypes ImportFile::getMachineType() const {
uint16_t machine =
Expand Down
5 changes: 1 addition & 4 deletions lld/COFF/InputFiles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,8 @@ class ImportFile : public InputFile {
// are actually in use.
//
// If the Live bit is turned off by MarkLive, Writer will ignore dllimported
// symbols provided by this import library member. We also track whether the
// imported symbol is used separately from whether the thunk is used in order
// to avoid creating unnecessary thunks.
// symbols provided by this import library member.
bool live;
bool thunkLive;
};

// Used for LTO.
Expand Down
2 changes: 1 addition & 1 deletion lld/COFF/MapFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ static void getSymbols(const COFFLinkerContext &ctx,
if (!file->thunkSym)
continue;

if (!file->thunkLive)
if (!file->thunkSym->isLive())
Copy link
Member

Choose a reason for hiding this comment

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

This bit had me a bit surprised here, when reading the context of the code below: Do I understand the existing logic here, that if we only access __imp_foo but not foo, we don't emit either of them into the map file, but if we access foo, we emit both into the map file?

(Although if GC isn't enabled, then all chunks are considered live, and they're emitted into the map file anyway.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that in most cases, these symbols will be emitted regardless of the loop. They are typically referenced by some object file and pulled in earlier during the function execution. If no object file references the symbol, it wouldn’t be pulled in the first place. However, if the code only references the thunk foo, this implicitly requires __imp_foo to be defined as well. I assume this pull is intended to handle that specific case.

In the context of ARM64EC, there are additional symbols implicitly defined, so this logic won’t fully cover those scenarios. I'll take a closer look at that.

continue;

if (auto *thunkSym = dyn_cast<Defined>(file->thunkSym))
Expand Down
2 changes: 1 addition & 1 deletion lld/COFF/MarkLive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ void markLive(COFFLinkerContext &ctx) {
addImportFile(sym->file);
} else if (auto *sym = dyn_cast<DefinedImportThunk>(b)) {
addImportFile(sym->wrappedSym->file);
sym->wrappedSym->file->thunkLive = true;
sym->getChunk()->live = true;
}
};

Expand Down
4 changes: 2 additions & 2 deletions lld/COFF/PDB.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1527,8 +1527,8 @@ void PDBLinker::addImportFilesToPDB() {
if (!file->thunkSym)
continue;

if (!file->thunkLive)
continue;
if (!file->thunkSym->isLive())
continue;

std::string dll = StringRef(file->dllName).lower();
llvm::pdb::DbiModuleDescriptorBuilder *&mod = dllToModuleDbi[dll];
Expand Down
2 changes: 1 addition & 1 deletion lld/COFF/Symbols.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ bool Symbol::isLive() const {
if (auto *imp = dyn_cast<DefinedImportData>(this))
return imp->file->live;
if (auto *imp = dyn_cast<DefinedImportThunk>(this))
return imp->wrappedSym->file->thunkLive;
return imp->getChunk()->live;
// Assume any other kind of symbol is live.
return true;
}
Expand Down
4 changes: 2 additions & 2 deletions lld/COFF/Symbols.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,12 +395,12 @@ class DefinedImportThunk : public Defined {
}

uint64_t getRVA() { return data->getRVA(); }
Chunk *getChunk() { return data; }
ImportThunkChunk *getChunk() const { return data; }

DefinedImportData *wrappedSym;

private:
Chunk *data;
ImportThunkChunk *data;
};

// If you have a symbol "foo" in your object file, a symbol name
Expand Down
2 changes: 1 addition & 1 deletion lld/COFF/Writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,7 +1258,7 @@ void Writer::appendImportThunks() {
if (!isa<DefinedImportThunk>(file->thunkSym))
fatal(toString(ctx, *file->thunkSym) + " was replaced");
DefinedImportThunk *thunk = cast<DefinedImportThunk>(file->thunkSym);
if (file->thunkLive)
if (thunk->getChunk()->live)
textSec->addChunk(thunk->getChunk());
if (file->impchkThunk)
textSec->addChunk(file->impchkThunk);
Expand Down
Loading