Fri, Oct 16
Hmm, that starts looking like a discussion for the mailing list, but I guess that all interested people are already here and the conversation will be better preserved for further references if being linked to the review.
Thu, Oct 15
Wed, Oct 14
rebased, added command-line option --gc-debuginfo-no-odr.
I'm fine with bd1976llvm accepting when he is happy.
I don't have any objections. My comment in the thread was more along the lines that dependent libraries could interact with whole-archive in hard to predict ways. I'm happy to have dependent libraries ignore whole-archive. I'm fine with MaskRay accepting when he is happy.
Gently ping. Is there anything I can improve in the fix so that it is accepted?
Tue, Oct 13
The code changes seem fine, but I haven't figured out why we need to sprinkle LLD_IN_TEST=1 for every LLD invocation we expect to error. What goes wrong exactly? I thought the point of your change is that, we *won't* re-run LLD when LLD_IN_TEST=2 after it reports an error.
Thu, Oct 8
Ping! Any further comments?
I've made some tests and speculatively estimate the saving up to several seconds for a relatively large library (about 100000 symbols) which is referenced several hundred times. For me, that is noticeable.
Wed, Oct 7
I wonder if it'd be possible to add some custom substitution in the lit config, to have it replace ld.lld with ld.lld -m <some-elf> in this case...
Looks sensible to me, but I'd defer to @MaskRay to approve.
Tue, Oct 6
- Simplified the comment.
I am skeptical about the change.
In the thread "[llvm-dev] RFC: ELF Autolinking", @psmith said:
- Fixed the comment.
Fri, Oct 2
Address comments. Removed stdout/stderr buffering, and replace with env LLD_IN_TEST=1 set appropriately in tests, where needed.
Tue, Sep 29
Mon, Sep 28
Good to know at least it could work on Linux. I wanted to activate coverage for this at some point in D74229 but then it wasn't working on Debug builds on Windows. I'll need to get back to it.
For the stack overflow you observed, how did that happen?
The stack overflow happens in the first place somewhere in the Windows CRT while releasing heap blocks. This is rare, as it depends on the random pattern that was in memory at the location of ObjFile::dwarf when fatal() was called by ELFFileBase::init().
Since catching stack overflows is not a reliable scenario when using CrashRecoveryContext
I think I'm OK with the functional change, but I think I would prefer not to keep the code that buffers stdout/stderr for the lit test. I think the complexity/size cost outweighs the test benefit.
Fri, Sep 25
Diff with context.
Thu, Sep 24
Wed, Sep 23
Looks good to me, I didn't review very in depth, but I see the test case that we need. :)
Tue, Sep 22
Mon, Sep 21