[lld][COFF] Fix: Merge `.drectve` sections in ObjFile::readSections #86380

bitRAKE · 2024-03-23T05:39:01Z

This commit addresses an issue where lld-link was not merging the contents 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.

This fix aligns lld-link's behavior with Microsoft's linker, improving compatibility and ensuring that all directives from object files are correctly processed.

…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.

github-actions · 2024-03-23T05:39:19Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

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
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

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.

llvmbot · 2024-03-23T05:39:47Z

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-coff

Author: Rickey Bowers Jr. (bitRAKE)

Changes

This commit addresses an issue where lld-link was not merging the contents 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.

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:

(modified) lld/COFF/Driver.cpp (+101-96)
(modified) lld/COFF/InputFiles.cpp (+4-2)
(modified) lld/COFF/InputFiles.h (+3-3)
(added) lld/test/COFF/Inputs/directive-multiple.obj ()
(added) lld/test/COFF/directives-multiple.test (+16)

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: }

github-actions · 2024-03-23T05:57:31Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

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.h

View 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()); }

github-actions · 2024-03-23T05:57:31Z

✅ With the latest revision this PR passed the Python code formatter.

foobar

aganea

Thank you for your contribution!

lld/test/COFF/Inputs/directive-multiple.obj

lld/COFF/Driver.cpp

… functions from ArgParser::parseDirectives. File section processing more distinct from parsing of text. Include assembly source for test object file.

aganea

Thanks again for making the changes. Just a few more minor things:

lld/COFF/Driver.cpp

lld/COFF/InputFiles.h

lld/test/COFF/Inputs/directives-multiple.asm

lld/COFF/Driver.h

lld/COFF/InputFiles.h

lld/test/COFF/Inputs/directives-multiple.asm

mstorsjo

Looks mostly fine to me now (i.e. no further objections from me), except for opinions on the default allocation size for the SmallVector.

lld/COFF/InputFiles.h

aganea · 2024-04-03T20:29:14Z

lld/COFF/InputFiles.h

- // Returns .drectve section contents if exist.
- StringRef getDirectives() { return directives; }
+ // Returns .drectve section(s) content if exist.
+ llvm::SmallVector<StringRef, 1> getDrectves() { return directives; }


Please return an ArrayRef here instead.

aganea · 2024-04-03T20:30:42Z

lld/COFF/Driver.cpp

 break;
 case OPT_stack:
 parseNumbers(arg->getValue(), &ctx.config.stackReserve,
-  &ctx.config.stackCommit);


Can you revert those? Formatting changes should be separated.

aganea · 2024-04-03T20:30:49Z

lld/COFF/Driver.cpp

 parseSubsystem(arg->getValue(), &ctx.config.subsystem,
-  &ctx.config.majorSubsystemVersion,
-  &ctx.config.minorSubsystemVersion, &gotVersion);
+ &ctx.config.majorSubsystemVersion,


aganea · 2024-04-03T20:32:53Z

lld/COFF/InputFiles.cpp

 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()));


Use toStringRef() from StringExtras.h here instead.

aganea · 2024-04-03T20:40:22Z

lld/COFF/Driver.h

- // 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);


I am still a bit unhappy with the naming. I wonder if we shouldn't just leave parseDirectives for both functions.

llvmbot added lld lld:COFF platform:windows labels Mar 23, 2024

bogus comment

436beb0

foobar

This comment was marked as outdated.

Sign in to view

deleted the wrong line, doh!

9cbe0cc

aganea reviewed Mar 23, 2024

View reviewed changes

lld/test/COFF/Inputs/directive-multiple.obj Outdated Show resolved Hide resolved

lld/COFF/Driver.cpp Outdated Show resolved Hide resolved

Split iterator from processing. Revised naming to differentiate these…

867cb66

… 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

View reviewed changes

lld/COFF/Driver.cpp Outdated Show resolved Hide resolved

lld/COFF/InputFiles.h Outdated Show resolved Hide resolved

lld/test/COFF/Inputs/directives-multiple.asm Outdated Show resolved Hide resolved

lld/COFF/Driver.h Outdated Show resolved Hide resolved

bringing style in line with review, minor edit

6880ff8

mstorsjo reviewed Mar 25, 2024

View reviewed changes

lld/COFF/InputFiles.h Outdated Show resolved Hide resolved

lld/test/COFF/Inputs/directives-multiple.asm Outdated Show resolved Hide resolved

lld/test/COFF/Inputs/directives-multiple.asm Outdated Show resolved Hide resolved

bitRAKE added 2 commits March 25, 2024 13:41

Use yaml2obj to refine test case.

80eae2b

Use SmallVector to optimize common 0/1 case.

502b348

mstorsjo reviewed Mar 25, 2024

View reviewed changes

lld/COFF/InputFiles.h Outdated Show resolved Hide resolved

Update SmallVector to optimize common 1 case.

509bb50

bitRAKE requested a review from mstorsjo March 28, 2024 18:53

aganea reviewed Apr 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lld][COFF] Fix: Merge `.drectve` sections in ObjFile::readSections #86380

[lld][COFF] Fix: Merge `.drectve` sections in ObjFile::readSections #86380

Uh oh!

bitRAKE commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

llvmbot commented Mar 23, 2024 •

edited

Loading

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

This comment was marked as outdated.

aganea left a comment

Uh oh!

Uh oh!

aganea left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mstorsjo left a comment

Uh oh!

aganea Apr 3, 2024

aganea Apr 3, 2024

aganea Apr 3, 2024

aganea Apr 3, 2024

aganea Apr 3, 2024

Labels

4 participants

[lld][COFF] Fix: Merge .drectve sections in ObjFile::readSections #86380

Are you sure you want to change the base?

[lld][COFF] Fix: Merge .drectve sections in ObjFile::readSections #86380

Uh oh!

Conversation

bitRAKE commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

llvmbot commented Mar 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

github-actions bot commented Mar 23, 2024

github-actions bot commented Mar 23, 2024

This comment was marked as outdated.

aganea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aganea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mstorsjo left a comment

Choose a reason for hiding this comment

Uh oh!

aganea Apr 3, 2024

Choose a reason for hiding this comment

aganea Apr 3, 2024

Choose a reason for hiding this comment

aganea Apr 3, 2024

Choose a reason for hiding this comment

aganea Apr 3, 2024

Choose a reason for hiding this comment

aganea Apr 3, 2024

Choose a reason for hiding this comment

Labels

4 participants

[lld][COFF] Fix: Merge `.drectve` sections in ObjFile::readSections #86380

[lld][COFF] Fix: Merge `.drectve` sections in ObjFile::readSections #86380

llvmbot commented Mar 23, 2024 •

edited

Loading