This is an archive of the discontinued LLVM Phabricator instance.

[ms] Add new option to llvm-lib: /llvmlibempty
ClosedPublic

Authored by epastor on Apr 26 2020, 8:48 PM.

Details

Summary

Add a new option (/llvmlibempty). If passed and llvm-lib does not give an error, it will create a valid output archive even if empty.

By default, llvm-lib mimicks lib.exe: if given no input files, it doesn't create its output file at all. This is incompatible with some build systems, so we add a command-line option to toggle this compatibility behavior.

Diff Detail

Event Timeline

epastor created this revision.Apr 26 2020, 8:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2020, 8:48 PM

When creating a dll, we currently create an empty .lib file (for similar reasons as in the patch description), while link.exe only creates a .lib file if there actually are exports. I wonder if we should change that to match link.exe by default and give lld its current behavior if this new flag here is passed? Not sure, maybe @mstorsjo has an opinion (but maybe he doesn't care about link.exe compat all that much) :)

Otherwise, this looks fine to me once it has a test.

epastor updated this revision to Diff 260487.Apr 27 2020, 3:54 PM

Add functionality test

Thoughts on making this affect the export lib creation as well?

Thoughts on making this affect the export lib creation as well?

Might be a good idea, but it's a completely separate tool (since llvm-ar/llvm-lib has no overlap with lld). I'd suggest doing it as a followup commit, rather than as part of this... especially since this one is not a change to the default behavior, whereas the export lib creation would be a change to the default behavior.

Thoughts on making this affect the export lib creation as well?

Might be a good idea, but it's a completely separate tool (since llvm-ar/llvm-lib has no overlap with lld). I'd suggest doing it as a followup commit, rather than as part of this... especially since this one is not a change to the default behavior, whereas the export lib creation would be a change to the default behavior.

I mean the .lib written by link.exe when you link a .dll (cf /IMPLIB: https://docs.microsoft.com/en-us/cpp/build/reference/implib-name-import-library?view=vs-2019)

Thoughts on making this affect the export lib creation as well?

Might be a good idea, but it's a completely separate tool (since llvm-ar/llvm-lib has no overlap with lld). I'd suggest doing it as a followup commit, rather than as part of this... especially since this one is not a change to the default behavior, whereas the export lib creation would be a change to the default behavior.

I mean the .lib written by link.exe when you link a .dll (cf /IMPLIB: https://docs.microsoft.com/en-us/cpp/build/reference/implib-name-import-library?view=vs-2019)

Right - but in these tools, that's written by lld-link, not llvm-lib... and there's no code overlap. And it would involve changing the default behavior of lld-link in a breaking fashion... so I'm suggesting that should go in a separate commit, probably with a quick RFC to the relevant lists.

thakis accepted this revision.Apr 30 2020, 11:56 AM

Sorry, I missed that _this_ change is in llvm-lib. Makes sense, thanks for talking me through it.

This revision is now accepted and ready to land.Apr 30 2020, 11:56 AM
epastor retitled this revision from [ms] Add new option to llvm-lib: /llvmemptylib to [ms] Add new option to llvm-lib: /llvmlibempty.Apr 30 2020, 12:28 PM
epastor edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.

Hi Eric,

the updated no-inputs.test test gets crashed on llvm-clang-x86_64-expensive-checks-ubuntu builder.
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/5166

  • FAIL: LLVM::no-inputs.test
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/build/bin/llvm-lib /llvmlibempty -out:/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/build/test/tools/llvm-lib/Output/no-inputs.test.tmp.a 
 #0 0x0000558112e4fd13 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/Support/Unix/Signals.inc:564:0
 #1 0x0000558112e4fda6 PrintStackTraceSignalHandler(void*) /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/Support/Unix/Signals.inc:625:0
 #2 0x0000558112e4da5d llvm::sys::RunSignalHandlers() /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/Support/Signals.cpp:68:0
 #3 0x0000558112e4f690 SignalHandler(int) /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/Support/Unix/Signals.inc:406:0
 #4 0x00007f691c9ba890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #5 0x00007f691ba99e97 raise (/lib/x86_64-linux-gnu/libc.so.6+0x3ee97)
 #6 0x00007f691ba9b801 abort (/lib/x86_64-linux-gnu/libc.so.6+0x40801)
 #7 0x000055811254936c operator new(unsigned long, void*) /usr/include/c++/7/new:169:0
 #8 0x0000558112c0ea23 std::vector<llvm::NewArchiveMember, std::allocator<llvm::NewArchiveMember> >::operator[](unsigned long) /usr/include/c++/7/bits/stl_vector.h:798:0
 #9 0x0000558112c0ccb4 llvm::libDriverMain(llvm::ArrayRef<char const*>) /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/ToolDrivers/llvm-lib/LibDriver.cpp:356:0
#10 0x0000558112547a72 main /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/tools/llvm-ar/llvm-ar.cpp:1288:0
#11 0x00007f691ba7cb97 __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b97)
#12 0x000055811254114a _start (/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/build/bin/llvm-lib+0x4ca14a)
/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/build/test/tools/llvm-lib/Output/no-inputs.test.script: line 4: 51780 Aborted                 (core dumped) /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/build/bin/llvm-lib /llvmlibempty -out:/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/build/test/tools/llvm-lib/Output/no-inputs.test.tmp.a

Hi Eric,

the updated no-inputs.test test gets crashed on llvm-clang-x86_64-expensive-checks-ubuntu builder.
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/5166

I believe this is already fixed in https://reviews.llvm.org/rG291d24838fcf

Hi Eric,

the updated no-inputs.test test gets crashed on llvm-clang-x86_64-expensive-checks-ubuntu builder.
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/5166

I believe this is already fixed in https://reviews.llvm.org/rG291d24838fcf

Hm. It doesn't seem to be fixed, but I don't see why not. I can't replicate this crash on my local machine. Can you give any more information that would help me reproduce this?

This is the expensive checks builder and I see this crash on all that kind of builders

Probably adding -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON to CMake command line will give a proper build.
Or you may try all the same CMake options as the builder uses
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/5179/steps/cmake-configure/logs/stdio

Hi Eric,

the updated no-inputs.test test gets crashed on llvm-clang-x86_64-expensive-checks-ubuntu builder.
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/5166

I believe this is already fixed in https://reviews.llvm.org/rG291d24838fcf

Hm. It doesn't seem to be fixed, but I don't see why not. I can't replicate this crash on my local machine. Can you give any more information that would help me reproduce this?

Nevermind, I see a potential problem. If neither an output nor an input is provided, we can end up indexing into an uninitialized spot in the Members vector. I'll post a patch that fixes that ASAP, and send it to you and Nico for review - I'll submit it once I get approval.

However... that's not true with these tests, and they're passing on all the other buildbots. Is there maybe a problem with this buildbot and parameters like -out:<value>?

Is there maybe a problem with this buildbot and parameters like -out:<value>?

Most likely there is no problems with this builder (in according of this situation). We got the same (similar) crash on three different systems: Ubuntu, Debian and Windows.

I'll post a patch that fixes that ASAP, and send it to you and Nico for review - I'll submit it once I get approval.

Thank you. I'll need some time to test this patch on the build system.