[LLD][COFF] Cover usage of LLD as a library

[aganea]

Summary

This patch introduces testing of LLD as library (only for the COFF driver for now). At the moment, we simply run the driver three times in a row, without shutting down LLD. This ensures the cleanup is properly done between each run.

Covering the other drivers works, but some tests need to be improved. An illustrated example is the fix done in the ELF test below. If there's interest in enabling this feature for the other drivers, perhaps respective owners could take a look, and attempt to fix the tests? @sbc100 @int3 @MaskRay The reason is that I'm printing "LLD_IN_TEST (new LLD session)" in stdout, in-between each run, to be able to differentiate the output.

[smeenai]

It's recommended to uploaded patches with full context (e.g. by using git diff -U9999 when uploading a patch using the web interface).

[ruiu]

I share your concern and I'm sorry about leaving so many global variables as uninitialized in the second run of the linker in the same process. But I'd like to find and fix the problems without abolishing global variables in lld, as I don't actually dislike them. One thing we could do is initializing global variables with invalid data -- like -1 or {nullptr} so that you'd get an error even on the first run if you do not reset them before use. I'll create a patch and send it to you.

lld/COFF/DebugTypes.cpp

93 ↗

(On Diff #229762)

Instead of defining a new function, you could add TypeServerSource::instances.clear() at the beginning of loadTypeServerSource().

lld/COFF/Writer.cpp

85

Likewise, I'd add outputSections.clear() at the beginning of Writer::run().

[blackhole12]

Moved the clear commands to where @ruiu suggested. I was unable to upload a full context diff because the web interface refuses to work properly.

[MaskRay]

@blackhole12 https://llvm.org/docs/GettingStarted.html#sending-patches

apt install arcanist (clone libphutil + clone arcanist)
cd llvm; arc install-certificate
arc diff 'HEAD^' # the comment will update the revision with the full diff

[blackhole12]

Update with full context

[blackhole12]

After further testing, this actually doesn't work, but only if you are loading an associated PDB file. I have no idea what's going on, because I don't understand the internals of TypeServerSource, but it's throwing an assertion error at DebugTypes.cpp:191

assert(it != TypeServerSource::instances.end());

My previous fix, which only cleared the caches at the very end of the link() call, does work in this scenario, which suggests to me that the TypeServerSource cache is likely being cleared too often. I highly suggest wrapping this fix into https://reviews.llvm.org/D70766, because it is clear this code is simply far too entwined with itself to properly reason about.

[aganea]

lld/COFF/DebugTypes.cpp

236 ↗

(On Diff #231826)

This can't work. loadTypeServerSource might be called several times during a single link. Please wait until D79672 lands, it will fix this particular TypeServerSource issue.

lld/COFF/Writer.cpp

602

Can you instead move outputSections in the OutputSection class, and call .clear() in lld/COFF/Driver.cpp, link(), like for the others static containers?

[blackhole12]

This update removes the debuginfo fix, which is no longer needed, and instead moves outputSections into OutputSection and calls it directly from link()

[aganea]

lld/COFF/Driver.cpp

93

Sorry for making you change another time, but I think the code would look better if following the existing nomenclature: OutputSection::instances.

lld/COFF/Writer.cpp

1079

You need to run git clang-format on your changes before uploading the patch. You simply need to add clang/tools/clang-format/git-clang-format and a copy of clang-format.exe somewhere in your %PATH% (if running on Windows).

[blackhole12]

I changed it to ::instances, but clang-format tried to reformat a bunch of code unrelated to my changes, so I only ran it against Writer.cpp to minimize spurious changes.

[aganea]

Seems good, could please remove the unrelated clang-format changes?
Do you have commit access?

lld/COFF/Writer.cpp

77

Anything unrelated like this line change should be removed from the patch. We could run clang-format separately on the files after.

107

Remove change.

310

Remove change.

851

Remove change.

1868

Please remove this change as well as the one below.

[blackhole12]

Removed unnecessary formatting changes. I do not have commit access @aganea so someone else will have to merge this change.

[aganea]

Taking over the patch, after discussing with Erik.

I went back to the original idea of having a single function for clearing outputSections. I felt that in the end, there were too many changes to support a single line of code in Driver.cpp.

[amccarth]

I read back through some of the history, but I still don't feel like I have all the context. That said, this change looks simple and straightforward to me.

Would it be feasible and useful to create a test that links two trivial programs with a single instantiation of the lld process? It seems like it would detect regressions and catch any introduction of a new global that needs to be reset.

[aganea]

In D70378#2045190, @amccarth wrote:

I read back through some of the history, but I still don't feel like I have all the context. That said, this change looks simple and straightforward to me.

Would it be feasible and useful to create a test that links two trivial programs with a single instantiation of the lld process? It seems like it would detect regressions and catch any introduction of a new global that needs to be reset.

That is great suggestion! I will see what I can do. Maybe something along the lines of LLVM_ENABLE_ASSERTIONS + LLVM_ENABLE_EXPENSIVE_CHECKS + run all LLD tests while calling link() three times in a row?

[amccarth]

In D70378#2045288, @aganea wrote:

In D70378#2045190, @amccarth wrote:

I read back through some of the history, but I still don't feel like I have all the context. That said, this change looks simple and straightforward to me.

Would it be feasible and useful to create a test that links two trivial programs with a single instantiation of the lld process? It seems like it would detect regressions and catch any introduction of a new global that needs to be reset.

That is great suggestion! I will see what I can do. Maybe something along the lines of LLVM_ENABLE_ASSERTIONS + LLVM_ENABLE_EXPENSIVE_CHECKS + run all LLD tests while calling link() three times in a row?

It might be better to run link() on different inputs. If you were to re-link the same program, a cache that wasn't cleared may return the expected values and thus elude detection. If that's difficult, I'm happy with anything that at least tries link() more than once in a single process lifetime and isn't too costly for build and test times.

[aganea]

Add support for testing LLD as a library, by running the driver several times in a row when LLD_IN_TEST is defined. The value of LLD_IN_TEST tells how many times LLD should loop (3 times right now).

@amccarth As for your suggestion, I don't see an easy way right now to pass three different sets of command-line arguments to LLD. We could maybe think of a special syntax for that purpose, and/or use response files.

In the meanwhile, all COFF tests pass with this enabled (tested on Windows & Linux). The other drivers work, but some tests would need to fixed/improved, so I intentionnally enabled the feature only for the COFF driver.

[aganea]

Fix clang-(format|tidy)

[MaskRay]

Thanks for working on this. I think making lldMain "reentrancy safe" is useful.

Currently the output is replicated atoi(getenv("LLD_IN_TEST")) times. There are also some LLD_IN_TEST (new LLD session) lines. This behavior can disturb

• tests with -NOT: CHECK lines (~20 test/ELF tests failed due to this problem).
• tests with lld .... 2>&1 | count 0 (test that there is no output)

I suggest we do it only when --verbose (port specific; inside coff::link, elf::link, ...) is specified. For non-verbose mode, is it possible to suppress output for the first N-1 invocations of lldMain?

I noticed a problem in llvm::timeTraceProfilerInitialize.

I just fixed an ELF problem related to InputFile::isInGroup (b358daddea04ea3c52e0e5bd5e851cee47f7f27f)

LLD_IN_TEST=3 is currently set for COFF. Note that this can make tests slower. I measured the performance on my machine:

• LLD_IN_TEST=1: check-lld-elf => 5.1s
• LLD_IN_TEST=2: check-lld-elf => 5.9s
• LLD_IN_TEST=3: check-lld-elf => 6.7s
• (I am glad that the ld.lld RUN lines don't take the majority of time:) )

I wonder whether 2 is sufficient.

lld/test/COFF/guardcf-lto.ll

11 ↗

(On Diff #285762)

Why can't %t.ll be used?

lld/tools/lld/lld.cpp

134 ↗

(On Diff #285762)

= 0

174 ↗

(On Diff #285762)

Probably int to avoid unsigned underflow

or for (; i > 0; --i) {

185 ↗

(On Diff #285762)

Not all ports support -mllvm or similar options. This should be moved into individual ports, right before cl::Parse* is called.

[grimar]

lld/test/ELF/cgprofile-shared-warn.s

12 ↗

(On Diff #285772)

No need to include "ld.lld" to the check line.

lld/test/ELF/zdefs.s

14 ↗

(On Diff #285772)

I think testing just "error: " is enough. You do not expect any errors here, not just "undefined symbol" error.
Also, it is safer to provide a minimal check string when -NOT is used. In case if we change "undefined symbol"
message, it might be easy to forget to update such check lines.

lld/tools/lld/lld.cpp

133 ↗

(On Diff #285772)

It needs a comment to explain what does it return.

178 ↗

(On Diff #285772)

May be I am missing something, but I read it as: you use SaveAndRestore to set
errorHandler().verbose to true and restore the value back, because want to bypass
the check in:

void ErrorHandler::log(const Twine &msg) {
  if (!verbose)
    return;
  std::lock_guard<std::mutex> lock(mu);
  lld::errs() << logName << ": " << msg << "\n";
}

My question: why instead not to use lld::errs() directly here?
E.g:

lld::errs() << ErrorHandler::logName <<  ": "LLD_IN_TEST (new LLD session)\n";

184 ↗

(On Diff #285772)

Should we return r; here when *ret != r? This should make a test fail even when asserts are disabled I think.

[aganea]

As suggested,

• Set LLD_IN_TEST=2 (still enabled for COFF only)
• Remove stdout & stderr for all iterations but the last one.

Also:

• Use CrashRecoveryContext to handle crashes or abort, and allow re-entrance.
• Ensure the cleanup is always done in case of a crash or abort.
• Disable LLD_IN_TEST for lld/test/ELF/invalid-cie-reference.s because it requires unwinding of SEH to work correctly on Windows, and that doesn't work with previous (actual?) versions of Clang (I imagine @tentzen's work will fix that)

[aganea]

Thanks for the feedback @MaskRay @grimar!

In D70378#2219292, @MaskRay wrote:

I think making lldMain "reentrancy safe" is useful.

I have another series of changes to also make it thread-safe (that is, being able to link several executables at the same time in different threads). But I'll send out an RFC first, because that requires changes in how cl::opts are stored. The goal is to ultimately build a project from inside a single process.

For non-verbose mode, is it possible to suppress output for the first N-1 invocations of lldMain?

Done.

I noticed a problem in llvm::timeTraceProfilerInitialize.

You mean the thread_locals that are not reinitialized?

LLD_IN_TEST=3 is currently set for COFF.
I wonder whether 2 is sufficient.

Done.

lld/test/COFF/guardcf-lto.ll

11 ↗

(On Diff #285762)

Because -dll outputs both %t.dll and %t.lib, which happens to also be an input, so the second iteration would fail otherwise.

lld/tools/lld/lld.cpp

178 ↗

(On Diff #285772)

Removed and replaced by what @MaskRay was proposing, ie. only print output for the last iteration.

[aganea]

lld/tools/lld/lld.cpp

167 ↗

(On Diff #286665)

@blackhole12 Since this goal of this patch is in part to fix an issue you've raised, is this API in line with what you're expecting? (when using LLD as a library)

[MaskRay]

Mostly looks good. Thanks for working on it!

lld/Common/ErrorHandler.cpp

51 ↗

(On Diff #286665)

llvm::nulls()

167 ↗

(On Diff #286665)

The two conditions can be merged

[aganea]

As suggested.

lld/tools/lld/lld.cpp

228 ↗

(On Diff #286665)

@MaskRay I've also replaced the LLVM_BUILD_TRAP with raise, just to properly throw the right signal (also take a look at CrashRecoveryContext.cpp). Since you're more knowledgable than me in this area, let me know you think of a better way.

[MaskRay]

lld/tools/lld/lld.cpp

228 ↗

(On Diff #287126)

Signals are numbered from 1 (0 is used as an existence and permission check). Use > 128

llvm/lib/Support/CrashRecoveryContext.cpp

381 ↗

(On Diff #287126)

Using 128+n looks good. We can probably replace the bash link with the more standard
https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html

Historically, shells have returned an exit status of 128+ n, where n represents the signal number....

[blackhole12]

lld/tools/lld/lld.cpp

167 ↗

(On Diff #286665)

I'm not sure why you are asking about this part in particular. When using LLD as a library, I call the appropriate ::link() function directly so I can pass in additional arguments like the error stream or standard out stream. I cannot use lldMain or safeLldMain for my use case.

[aganea]

lld/tools/lld/lld.cpp

167 ↗

(On Diff #286665)

That answers my question. How do you handle crashes or signals in that case? If you think safeLldMain can be useful to you, I could also add two extra arguments to redirect the std streams.

[blackhole12]

lld/tools/lld/lld.cpp

167 ↗

(On Diff #286665)

Currently, I don't handle anything. If LLD crashes, my compiler crashes. A safe invocation, however, would certainly be handy, as it would allow the compiler to emit an error message instead of crashing.

[mstorsjo]

Just fwiw, a smaller patch on the same topic was just committed a few days ago, from D86401. This looks like a bigger and more complete fix for the same issue - but before completing it, pay attention to pick up/remove the fix from D86401 (I see that the same vector already gets cleared by this patch as well).

[aganea]

In D70378#2234356, @mstorsjo wrote:

Just fwiw, a smaller patch on the same topic was just committed a few days ago, from D86401. This looks like a bigger and more complete fix for the same issue - but before completing it, pay attention to pick up/remove the fix from D86401 (I see that the same vector already gets cleared by this patch as well).

Thanks for the info Martin!

[MaskRay]

lld/Common/ErrorHandler.cpp

54 ↗

(On Diff #287126)

Nit: One blank line above

[aganea]

@MaskRay Thanks will do - I still have one last issue I don't understand, maybe you can help?

lld/tools/lld/lld.cpp

228 ↗

(On Diff #287126)

The test at L33 in lld/test/ELF/lto/ltopasses-custom.ll fails on Linux because below I'm calling raise(SIGABRT), which in theory should return 134. But the control flow then goes to not, in Program.inc, just after the call to sys::wait4. However at that point, the status value is 0x8600 and I don't understand why. If I run lld without LLD_IN_TEST, the abort() call for that test is called normally (not in a CrashRecoveryContext) and when reaching the same point after sys::wait4, status is 0x86 (134, that is 128+SIGABRT) in that case.
So one case, abort() exits the process, we get 0x86.
The other case, raise(SIGABRT) gets 0x8600.
Any ideas on how to fix this? That was the reason why I used LLVM_BUILTIN_TRAP before using raise, because it properly returns (137) 128+SIGKILL.
Any ideas?

[aganea]

Rebase.
Address @MaskRay's suggestions.
Made safeLldLink a public API and made it possible to provide user-defined stdout/stderr streams.

[MaskRay]

clang/tools/driver/driver.cpp

537 ↗

(On Diff #287747)

(From a non-native speaker)

"that is codes >128" Should the singular form be used?

llvm/include/llvm/Support/Signals.h

119 ↗

(On Diff #287747)

[readability-identifier-naming]

Many very old function use UpperCase, but newer functions should use lowerCase.

[aganea]

Address comments.
Added a CrashRecoveryTest.

clang/tools/driver/driver.cpp

537 ↗

(On Diff #287747)

Rephrased.

[aganea]

Ping! @MaskRay any further comments?

[MaskRay]

Looks great!

[aganea]

@MaskRay Thank you!

[MaskRay]

@rnk @amccarth Do you have more comments? ☺️

[amccarth]

LGTM.

[rnk]

Looks good to me, I didn't review very in depth, but I see the test case that we need. :)

[ayrivera]

lld/ELF/Driver.cpp

895 ↗

(On Diff #294129)

Hi,

I built locally lld and came across an issue. The ELF driver isn't recognizing multiple -mllvm options. It only process the last -mllvm option that was entered in the command line. For example, I can add -mllvm -print-after-all -debug-pass=Arguments and it only prints the information from -debug-pass=Arguments, and not the IR and the debug data. If I swap the order, then the IR is printed and not the debug information.

I noticed that this change set added a call to cl:ResetAllOptionOccurrences() (line 895) inside parseClangOption. The function parseClangOption is called inside the loop that process the -mllvm options. If I comment this line, then everything works correctly and I can pass multiple -mllvm options through the command.

Is this an intended behavior or an actual issue? Thanks for your time.

[MaskRay]

lld/ELF/Driver.cpp

895 ↗

(On Diff #294129)

Hi,

For example, I can add -mllvm -print-after-all -debug-pass=Arguments

You need two -mllvm, i.e. -mllvm -print-after-all -mllvm -debug-pass=Arguments. And with the option, I see the print-after-all dump.

[ayrivera]

lld/ELF/Driver.cpp

895 ↗

(On Diff #294129)

Hi,

You need two -mllvm, i.e. -mllvm -print-after-all -mllvm -debug-pass=Arguments. And with the option, I see the print-after-all dump.

Thanks for the quick reply. Sorry, this is a typo. I'm using both -mllvm and still got the issue.

[aganea]

lld/ELF/Driver.cpp

895 ↗

(On Diff #294129)

Is this an intended behavior or an actual issue?

At first sight, this is an actual issue. Normally, cl::ResetAllOptionOccurrences() should be called only once per compiler or linker instance, to reset the internal counters for the cl::opts. I would move it somewhere at the begining of readConfigs(). Can you try that see if that solves your issue?

[ayrivera]

lld/ELF/Driver.cpp

895 ↗

(On Diff #294129)

Hi @aganea ,

This patch https://reviews.llvm.org/D88461 , posted by @MaskRay , works. It does what you suggested. Thanks for the quick reply.

[rnk]

lld/Common/ErrorHandler.cpp

78 ↗

(On Diff #294129)

This appears to have regressed shutdown. sys::Process::Exit calls exit, not _exit, so now atexit destructors are run. That's unintended, right?

[aganea]

lld/Common/ErrorHandler.cpp

78 ↗

(On Diff #294129)

Yes, the fix is in D88348!

lld/COFF/Driver.cpp

if (canExitEarly)
     exitLld(errorCount() ? 1 : 0);
 
   freeArena();
   ObjFile::instances.clear();
   ImportFile::instances.clear();
   BitcodeFile::instances.clear();
   memset(MergeChunk::instances, 0, sizeof(MergeChunk::instances));
   TpiSource::clear();
+  OutputSection::clear();
 
   return !errorCount();
 }
 
 // Parse options of the form "old;new".
 static std::pair<StringRef, StringRef> getOldNewOptions(opt::InputArgList &args,
                                                         unsigned id) {
   auto *arg = args.getLastArg(id);

lld/COFF/Writer.h

public:
   void insertChunkAtStart(Chunk *c);
   void merge(OutputSection *other);
   void setPermissions(uint32_t c);
   uint64_t getRVA() { return header.VirtualAddress; }
   uint64_t getFileOff() { return header.PointerToRawData; }
   void writeHeaderTo(uint8_t *buf);
   void addContributingPartialSection(PartialSection *sec);
 
+  // Clear the output sections static container.
+  static void clear();
+
   // Returns the size of this section in an executable memory image.
   // This may be smaller than the raw size (the raw size is multiple
   // of disk sector size, so there may be padding at end), or may be
   // larger (if that's the case, the loader reserves spaces after end
   // of raw data).
   uint64_t getVirtualSize() { return header.VirtualSize; }
 
   // Returns the size of the section in the output file.

lld/COFF/Writer.cpp

static unsigned char dosProgram[] = {
   0xcd, 0x21, 0x54, 0x68, 0x69, 0x73, 0x20, 0x70, 0x72, 0x6f, 0x67, 0x72,
   0x61, 0x6d, 0x20, 0x63, 0x61, 0x6e, 0x6e, 0x6f, 0x74, 0x20, 0x62, 0x65,
   0x20, 0x72, 0x75, 0x6e, 0x20, 0x69, 0x6e, 0x20, 0x44, 0x4f, 0x53, 0x20,
   0x6d, 0x6f, 0x64, 0x65, 0x2e, 0x24, 0x00, 0x00
 };
 static_assert(sizeof(dosProgram) % 8 == 0,
               "DOSProgram size must be multiple of 8");
 
 static const int dosStubSize = sizeof(dos_header) + sizeof(dosProgram);
 static_assert(dosStubSize % 8 == 0, "DOSStub size must be multiple of 8");
 
 static const int numberOfDataDirectory = 16;
 
 // Global vector of all output sections. After output sections are finalized,
 // this can be indexed by Chunk::getOutputSection.
 static std::vector<OutputSection *> outputSections;
 
 OutputSection *Chunk::getOutputSection() const {
   return osidx == 0 ? nullptr : outputSections[osidx - 1];
 }
 
+void OutputSection::clear() { outputSections.clear(); }
+
 namespace {
 
 class DebugDirectoryChunk : public NonSectionChunk {
 public:
   DebugDirectoryChunk(const std::vector<std::pair<COFF::DebugType, Chunk *>> &r,
                       bool writeRepro)
       : records(r), writeRepro(writeRepro) {}
 
   size_t getSize() const override {
     return (records.size() + int(writeRepro)) * sizeof(debug_directory);
   }
 
   void writeTo(uint8_t *b) const override {
     auto *d = reinterpret_cast<debug_directory *>(b);
 
     for (const std::pair<COFF::DebugType, Chunk *>& record : records) {
       Chunk *c = record.second;
       OutputSection *os = c->getOutputSection();
       uint64_t offs = os->getFileOff() + (c->getRVA() - os->getRVA());
       fillEntry(d, record.first, c->getSize(), c->getRVA(), offs);
       ++d;
     }
 
     if (writeRepro) {

 
 static Timer codeLayoutTimer("Code Layout", Timer::root());
 static Timer diskCommitTimer("Commit Output File", Timer::root());
 
 void lld::coff::writeResult() { Writer().run(); }
 
 void OutputSection::addChunk(Chunk *c) {
   chunks.push_back(c);
 }
 
 void OutputSection::insertChunkAtStart(Chunk *c) {
   chunks.insert(chunks.begin(), c);
 }
 
 void OutputSection::setPermissions(uint32_t c) {
   header.Characteristics &= ~permMask;
   header.Characteristics |= c;
while (true) {
     assignAddresses();
 
     pass++;
   }
 }
 
 // The main function of the writer.
 void Writer::run() {
   ScopedTimer t1(codeLayoutTimer);
 
   createImportTables();
   createSections();
   createMiscChunks();
   appendImportThunks();
   createExportTable();
   mergeSections();
   removeUnusedSections();
if (sc && !sc->live) {
       if (config->verbose)
         sc->printDiscardedMessage();
       continue;
     }
     StringRef name = c->getSectionName();
     if (shouldStripSectionSuffix(sc, name))
       name = name.split('$').first;
     PartialSection *pSec = createPartialSection(name,
                                                 c->getOutputCharacteristics());
     pSec->chunks.push_back(c);
   }
 
   fixPartialSectionChars(".rsrc", data | r);
   fixPartialSectionChars(".edata", data | r);
   // Even in non MinGW cases, we might need to link against GNU import
   // libraries.
   bool hasIdata = fixGnuImportChunks();

 
 // The Windows loader doesn't seem to like empty sections,
 // so we remove them if any.
 void Writer::removeEmptySections() {
   auto isEmpty = [](OutputSection *s) { return s->getVirtualSize() == 0; };
   outputSections.erase(
       std::remove_if(outputSections.begin(), outputSections.end(), isEmpty),
       outputSections.end());
 }
 
 void Writer::assignOutputSectionIndices() {
   // Assign final output section indices, and assign each chunk to its output
   // section.
   uint32_t idx = 1;
   for (OutputSection *os : outputSections) {
     os->sectionIndex = idx;
     for (Chunk *c : os->chunks)
void Writer::sortExceptionTable() {
   auto bufAddr = [&](Chunk *c) {
     OutputSection *os = c->getOutputSection();
     return buffer->getBufferStart() + os->getFileOff() + c->getRVA() -
            os->getRVA();
   };
   uint8_t *begin = bufAddr(firstPdata);
   uint8_t *end = bufAddr(lastPdata) + lastPdata->getSize();
   if (config->machine == AMD64) {
     struct Entry { ulittle32_t begin, end, unwind; };
     parallelSort(
         MutableArrayRef<Entry>((Entry *)begin, (Entry *)end),
         [](const Entry &a, const Entry &b) { return a.begin < b.begin; });
     return;
   }
   if (config->machine == ARMNT || config->machine == ARM64) {
     struct Entry { ulittle32_t begin, unwind; };
     parallelSort(