This is an archive of the discontinued LLVM Phabricator instance.

[llvm-ranlib] Handle -D and -U command line flag
ClosedPublic

Authored by arichardson on Dec 16 2019, 8:40 AM.

Details

Summary

I have been trying to build CheriBSD (a fork for FreeBSD for the CHERI
CPU) with LLVM binutils instead of the default elftoolchain utilities.
I noticed that building static archives was failing because ranlib is
invoked with the -D flag. This failed with llvm-ranlib since it parses
the -D flag as the archive path and reports an error that more than one
archive has been passed.

This fixes https://llvm.org/PR41707

Diff Detail

Event Timeline

arichardson created this revision.Dec 16 2019, 8:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 16 2019, 8:40 AM

Drop unrelated change

Unit tests: fail. 60962 tests passed, 1 failed and 726 were skipped.

failed: LLVM.tools/llvm-ranlib/D-flag.test

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: fail. 60962 tests passed, 1 failed and 726 were skipped.

failed: LLVM.tools/llvm-ranlib/D-flag.test

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

Force UTC timezone for reproducible results

Unit tests: pass. 60963 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

emaste added inline comments.Dec 16 2019, 9:21 AM
llvm/tools/llvm-ar/llvm-ar.cpp
69

file mode made reproducible too?

arichardson marked an inline comment as done.Dec 16 2019, 9:34 AM
arichardson added inline comments.
llvm/tools/llvm-ar/llvm-ar.cpp
69

Does not seem like it, if I run chmod 400 on the .o file it still reports r-------- with -D.

rupprecht accepted this revision.Dec 17 2019, 10:44 AM

Looks good for D/U, but looks like --help and --version options are also supported as combined short args; do you mind adding those too while you're here?

Thanks for the patch!

llvm/tools/llvm-ar/llvm-ar.cpp
68

-v --version

1164–1176
} else if (arg.front() == 'h') {
  printHelpMessage();
  return 0;
} else if (arg.front() == 'v') {
  cl::PrintVersionMessage();
  return 0;
}

Actually, ranlib -vh and ranlib -hv (on my machine at least) both print the help message. I don't think we need *that* level of compatibility though -- we can go with first-one-wins.

This revision is now accepted and ready to land.Dec 17 2019, 10:44 AM

Also handle -h/-v as short options. Does the adjusted test look okay?

Unit tests: fail. 60990 tests passed, 1 failed and 728 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_class/lock.pass.cpp

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

clang-format

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jan 2 2020, 6:42 AM

The test fails on Windows: http://45.33.8.238/win/4948/step_11.txt

Please take a look, and if takes a while to fix, revert in the meantime.

The test fails on Windows: http://45.33.8.238/win/4948/step_11.txt

Please take a look, and if takes a while to fix, revert in the meantime.

I'll just relax the permissions check. We only care about the timestamp in this test so there is no need to check the rwx values.

Also handle -h/-v as short options. Does the adjusted test look okay?

Sorry, didn't have time to take a second look before the holiday break -- yep, looks good, I didn't anticipate so many buildbot failures. Would be nice if the precommit bot tested Windows too.