[lld][COFF] Fix: Merge .drectve sections in ObjFile::readSections
#86380
Conversation
This file contains 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
…tents of multiple `.drectve` sections from OBJ files, unlike Microsoft's linker which merges them. Previously, lld-link would overwrite `.drectve` section content, only retaining the last section's directives. This behavior led to incomplete directive processing.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: Rickey Bowers Jr. (bitRAKE) ChangesThis commit addresses an issue where lld-link was not merging the contents of multiple This fix aligns lld-link's behavior with Microsoft's linker, improving compatibility and ensuring that all directives from object files are correctly processed. Full diff: https://github.com/llvm/llvm-project/pull/86380.diff 5 Files Affected:
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 22ee2f133be98a..2336acd2eb3187 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -368,111 +368,116 @@ bool LinkerDriver::isDecorated(StringRef sym) {
// Parses .drectve section contents and returns a list of files
// specified by /defaultlib.
void LinkerDriver::parseDirectives(InputFile *file) {
- StringRef s = file->getDirectives();
- if (s.empty())
+ std::vector<StringRef> directivesList = file->getDirectives();
+ if (directivesList.empty())
return;
- log("Directives: " + toString(file) + ": " + s);
-
- ArgParser parser(ctx);
- // .drectve is always tokenized using Windows shell rules.
- // /EXPORT: option can appear too many times, processing in fastpath.
- ParsedDirectives directives = parser.parseDirectives(s);
-
- for (StringRef e : directives.exports) {
- // If a common header file contains dllexported function
- // declarations, many object files may end up with having the
- // same /EXPORT options. In order to save cost of parsing them,
- // we dedup them first.
- if (!directivesExports.insert(e).second)
+ for (StringRef s : directivesList) {
+ if (s.empty())
continue;
- Export exp = parseExport(e);
- if (ctx.config.machine == I386 && ctx.config.mingw) {
- if (!isDecorated(exp.name))
- exp.name = saver().save("_" + exp.name);
- if (!exp.extName.empty() && !isDecorated(exp.extName))
- exp.extName = saver().save("_" + exp.extName);
- }
- exp.source = ExportSource::Directives;
- ctx.config.exports.push_back(exp);
- }
+ log("Directives: " + toString(file) + ": " + s);
- // Handle /include: in bulk.
- for (StringRef inc : directives.includes)
- addUndefined(inc);
+ ArgParser parser(ctx);
+ // .drectve is always tokenized using Windows shell rules.
+ // /EXPORT: option can appear too many times, processing in fastpath.
+ ParsedDirectives directives = parser.parseDirectives(s);
- // Handle /exclude-symbols: in bulk.
- for (StringRef e : directives.excludes) {
- SmallVector<StringRef, 2> vec;
- e.split(vec, ',');
- for (StringRef sym : vec)
- excludedSymbols.insert(mangle(sym));
- }
+ for (StringRef e : directives.exports) {
+ // If a common header file contains dllexported function
+ // declarations, many object files may end up with having the
+ // same /EXPORT options. In order to save cost of parsing them,
+ // we dedup them first.
+ if (!directivesExports.insert(e).second)
+ continue;
- // https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp?view=msvc-160
- for (auto *arg : directives.args) {
- switch (arg->getOption().getID()) {
- case OPT_aligncomm:
- parseAligncomm(arg->getValue());
- break;
- case OPT_alternatename:
- parseAlternateName(arg->getValue());
- break;
- case OPT_defaultlib:
- if (std::optional<StringRef> path = findLibIfNew(arg->getValue()))
- enqueuePath(*path, false, false);
- break;
- case OPT_entry:
- ctx.config.entry = addUndefined(mangle(arg->getValue()));
- break;
- case OPT_failifmismatch:
- checkFailIfMismatch(arg->getValue(), file);
- break;
- case OPT_incl:
- addUndefined(arg->getValue());
- break;
- case OPT_manifestdependency:
- ctx.config.manifestDependencies.insert(arg->getValue());
- break;
- case OPT_merge:
- parseMerge(arg->getValue());
- break;
- case OPT_nodefaultlib:
- ctx.config.noDefaultLibs.insert(findLib(arg->getValue()).lower());
- break;
- case OPT_release:
- ctx.config.writeCheckSum = true;
- break;
- case OPT_section:
- parseSection(arg->getValue());
- break;
- case OPT_stack:
- parseNumbers(arg->getValue(), &ctx.config.stackReserve,
- &ctx.config.stackCommit);
- break;
- case OPT_subsystem: {
- bool gotVersion = false;
- parseSubsystem(arg->getValue(), &ctx.config.subsystem,
- &ctx.config.majorSubsystemVersion,
- &ctx.config.minorSubsystemVersion, &gotVersion);
- if (gotVersion) {
- ctx.config.majorOSVersion = ctx.config.majorSubsystemVersion;
- ctx.config.minorOSVersion = ctx.config.minorSubsystemVersion;
+ Export exp = parseExport(e);
+ if (ctx.config.machine == I386 && ctx.config.mingw) {
+ if (!isDecorated(exp.name))
+ exp.name = saver().save("_" + exp.name);
+ if (!exp.extName.empty() && !isDecorated(exp.extName))
+ exp.extName = saver().save("_" + exp.extName);
}
- break;
+ exp.source = ExportSource::Directives;
+ ctx.config.exports.push_back(exp);
}
- // Only add flags here that link.exe accepts in
- // `#pragma comment(linker, "/flag")`-generated sections.
- case OPT_editandcontinue:
- case OPT_guardsym:
- case OPT_throwingnew:
- case OPT_inferasanlibs:
- case OPT_inferasanlibs_no:
- break;
- default:
- error(arg->getSpelling() + " is not allowed in .drectve (" +
- toString(file) + ")");
+
+ // Handle /include: in bulk.
+ for (StringRef inc : directives.includes)
+ addUndefined(inc);
+
+ // Handle /exclude-symbols: in bulk.
+ for (StringRef e : directives.excludes) {
+ SmallVector<StringRef, 2> vec;
+ e.split(vec, ',');
+ for (StringRef sym : vec)
+ excludedSymbols.insert(mangle(sym));
+ }
+
+ // https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp?view=msvc-160
+ for (auto *arg : directives.args) {
+ switch (arg->getOption().getID()) {
+ case OPT_aligncomm:
+ parseAligncomm(arg->getValue());
+ break;
+ case OPT_alternatename:
+ parseAlternateName(arg->getValue());
+ break;
+ case OPT_defaultlib:
+ if (std::optional<StringRef> path = findLibIfNew(arg->getValue()))
+ enqueuePath(*path, false, false);
+ break;
+ case OPT_entry:
+ ctx.config.entry = addUndefined(mangle(arg->getValue()));
+ break;
+ case OPT_failifmismatch:
+ checkFailIfMismatch(arg->getValue(), file);
+ break;
+ case OPT_incl:
+ addUndefined(arg->getValue());
+ break;
+ case OPT_manifestdependency:
+ ctx.config.manifestDependencies.insert(arg->getValue());
+ break;
+ case OPT_merge:
+ parseMerge(arg->getValue());
+ break;
+ case OPT_nodefaultlib:
+ ctx.config.noDefaultLibs.insert(findLib(arg->getValue()).lower());
+ break;
+ case OPT_release:
+ ctx.config.writeCheckSum = true;
+ break;
+ case OPT_section:
+ parseSection(arg->getValue());
+ break;
+ case OPT_stack:
+ parseNumbers(arg->getValue(), &ctx.config.stackReserve,
+ &ctx.config.stackCommit);
+ break;
+ case OPT_subsystem: {
+ bool gotVersion = false;
+ parseSubsystem(arg->getValue(), &ctx.config.subsystem,
+ &ctx.config.majorSubsystemVersion,
+ &ctx.config.minorSubsystemVersion, &gotVersion);
+ if (gotVersion) {
+ ctx.config.majorOSVersion = ctx.config.majorSubsystemVersion;
+ ctx.config.minorOSVersion = ctx.config.minorSubsystemVersion;
+ }
+ break;
+ }
+ // Only add flags here that link.exe accepts in
+ // `#pragma comment(linker, "/flag")`-generated sections.
+ case OPT_editandcontinue:
+ case OPT_guardsym:
+ case OPT_throwingnew:
+ case OPT_inferasanlibs:
+ case OPT_inferasanlibs_no:
+ break;
+ default:
+ error(arg->getSpelling() + " is not allowed in .drectve (" +
+ toString(file) + ")");
+ }
}
}
}
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 037fae45242c6f..ab63de2a5c76a5 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -212,7 +212,8 @@ SectionChunk *ObjFile::readSection(uint32_t sectionNumber,
if (name == ".drectve") {
ArrayRef<uint8_t> data;
cantFail(coffObj->getSectionContents(sec, data));
- directives = StringRef((const char *)data.data(), data.size());
+ // MS link accumulates the directive sections in order of appearance
+ directives.push_back(StringRef(reinterpret_cast<const char*>(data.data()), data.size()));
return nullptr;
}
@@ -1086,7 +1087,8 @@ void BitcodeFile::parse() {
if (objSym.isUsed())
ctx.config.gcroot.push_back(sym);
}
- directives = saver.save(obj->getCOFFLinkerOpts());
+// directives.push_back(saver.save(obj->getCOFFLinkerOpts()).str());
+ directives.push_back(obj->getCOFFLinkerOpts());
}
void BitcodeFile::parseLazy() {
diff --git a/lld/COFF/InputFiles.h b/lld/COFF/InputFiles.h
index 3b55cd791bfda2..22bd7734bf96c7 100644
--- a/lld/COFF/InputFiles.h
+++ b/lld/COFF/InputFiles.h
@@ -89,8 +89,8 @@ class InputFile {
// An archive file name if this file is created from an archive.
StringRef parentName;
- // Returns .drectve section contents if exist.
- StringRef getDirectives() { return directives; }
+ // Returns .drectve section(s) content if exist.
+ std::vector<StringRef> getDirectives() { return directives; }
COFFLinkerContext &ctx;
@@ -98,7 +98,7 @@ class InputFile {
InputFile(COFFLinkerContext &c, Kind k, MemoryBufferRef m, bool lazy = false)
: mb(m), ctx(c), fileKind(k), lazy(lazy) {}
- StringRef directives;
+ std::vector<StringRef> directives;
private:
const Kind fileKind;
diff --git a/lld/test/COFF/Inputs/directive-multiple.obj b/lld/test/COFF/Inputs/directive-multiple.obj
new file mode 100644
index 00000000000000..a2c0085a165662
Binary files /dev/null and b/lld/test/COFF/Inputs/directive-multiple.obj differ
diff --git a/lld/test/COFF/directives-multiple.test b/lld/test/COFF/directives-multiple.test
new file mode 100644
index 00000000000000..416b0b99882d14
--- /dev/null
+++ b/lld/test/COFF/directives-multiple.test
@@ -0,0 +1,16 @@
+# RUN: lld-link /dll %p/Inputs/directive-multiple.obj /out:%t.dll
+# RUN: llvm-readobj --coff-exports %t.dll | FileCheck -check-prefix=EXPORTS %s
+
+EXPORTS: Format: COFF-x86-64
+EXPORTS: Arch: x86_64
+EXPORTS: AddressSize: 64bit
+EXPORTS: Export {
+EXPORTS: Ordinal: 1
+EXPORTS: Name: Add
+EXPORTS: RVA: 0x1010
+EXPORTS: }
+EXPORTS: Export {
+EXPORTS: Ordinal: 2
+EXPORTS: Name: Subtract
+EXPORTS: RVA: 0x1020
+EXPORTS: }
|
You can test this locally with the following command:git-clang-format --diff 65058a8d732c3c41664a4dad1a1ae2a504d5c98e bfaa3553a649d097ac48a46284c102bdf72a2ba2 -- lld/COFF/Driver.cpp lld/COFF/InputFiles.cpp lld/COFF/InputFiles.hView the diff from clang-format here.diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 2336acd2eb..f26e355119 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -453,13 +453,13 @@ void LinkerDriver::parseDirectives(InputFile *file) {
break;
case OPT_stack:
parseNumbers(arg->getValue(), &ctx.config.stackReserve,
- &ctx.config.stackCommit);
+ &ctx.config.stackCommit);
break;
case OPT_subsystem: {
bool gotVersion = false;
parseSubsystem(arg->getValue(), &ctx.config.subsystem,
- &ctx.config.majorSubsystemVersion,
- &ctx.config.minorSubsystemVersion, &gotVersion);
+ &ctx.config.majorSubsystemVersion,
+ &ctx.config.minorSubsystemVersion, &gotVersion);
if (gotVersion) {
ctx.config.majorOSVersion = ctx.config.majorSubsystemVersion;
ctx.config.minorOSVersion = ctx.config.minorSubsystemVersion;
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index ab63de2a5c..27112eede6 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -213,7 +213,8 @@ SectionChunk *ObjFile::readSection(uint32_t sectionNumber,
ArrayRef<uint8_t> data;
cantFail(coffObj->getSectionContents(sec, data));
// MS link accumulates the directive sections in order of appearance
- directives.push_back(StringRef(reinterpret_cast<const char*>(data.data()), data.size()));
+ directives.push_back(
+ StringRef(reinterpret_cast<const char *>(data.data()), data.size()));
return nullptr;
}
@@ -1087,7 +1088,7 @@ void BitcodeFile::parse() {
if (objSym.isUsed())
ctx.config.gcroot.push_back(sym);
}
-// directives.push_back(saver.save(obj->getCOFFLinkerOpts()).str());
+ // directives.push_back(saver.save(obj->getCOFFLinkerOpts()).str());
directives.push_back(obj->getCOFFLinkerOpts());
}
|
|
✅ With the latest revision this PR passed the Python code formatter. |
foobar
This comment was marked as outdated.
This comment was marked as outdated.
aganea
reviewed
Mar 23, 2024
… functions from ArgParser::parseDirectives. File section processing more distinct from parsing of text. Include assembly source for test object file.
aganea
reviewed
Mar 25, 2024
mstorsjo
reviewed
Mar 25, 2024
mstorsjo
reviewed
Mar 25, 2024
aganea
reviewed
Apr 3, 2024
| // Returns .drectve section contents if exist. | ||
| StringRef getDirectives() { return directives; } | ||
| // Returns .drectve section(s) content if exist. | ||
| llvm::SmallVector<StringRef, 1> getDrectves() { return directives; } |
There was a problem hiding this comment.
Please return an ArrayRef here instead.
| @@ -449,13 +453,13 @@ void LinkerDriver::parseDirectives(InputFile *file) { | |||
| break; | |||
| case OPT_stack: | |||
| parseNumbers(arg->getValue(), &ctx.config.stackReserve, | |||
| &ctx.config.stackCommit); | |||
There was a problem hiding this comment.
Can you revert those? Formatting changes should be separated.
| break; | ||
| case OPT_subsystem: { | ||
| bool gotVersion = false; | ||
| parseSubsystem(arg->getValue(), &ctx.config.subsystem, | ||
| &ctx.config.majorSubsystemVersion, | ||
| &ctx.config.minorSubsystemVersion, &gotVersion); | ||
| &ctx.config.majorSubsystemVersion, |
| @@ -212,7 +212,8 @@ SectionChunk *ObjFile::readSection(uint32_t sectionNumber, | |||
| if (name == ".drectve") { | |||
| ArrayRef<uint8_t> data; | |||
| cantFail(coffObj->getSectionContents(sec, data)); | |||
| directives = StringRef((const char *)data.data(), data.size()); | |||
| // MS link accumulates the directive sections in order of appearance | |||
| directives.push_back(StringRef(reinterpret_cast<const char*>(data.data()), data.size())); | |||
There was a problem hiding this comment.
Use toStringRef() from StringExtras.h here instead.
| // Used by the resolver to parse .drectve section contents. | ||
| void parseDirectives(InputFile *file); | ||
| // Used by the resolver to iterate through .drectve section(s). | ||
| void processDirectivesSection(InputFile *file); |
There was a problem hiding this comment.
I am still a bit unhappy with the naming. I wonder if we shouldn't just leave parseDirectives for both functions.
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.
This commit addresses an issue where lld-link was not merging the contents of multiple
.drectvesections from OBJ files, unlike Microsoft's linker which merges them. Previously, lld-link would overwrite.drectvesection content, only retaining the last section's directives. This behavior led to incomplete directive processing.This fix aligns lld-link's behavior with Microsoft's linker, improving compatibility and ensuring that all directives from object files are correctly processed.