Page MenuHomePhabricator

[llvm-ar] Improve tool selection logic
ClosedPublic

Authored by MaskRay on Dec 10 2019, 1:52 PM.

Details

Summary

If llvm-ar is installed at arm-pokymllib32-linux-gnueabi-llvm-ar, it may
think it is llvm-lib due to the "lib" substring.

Update the heuristic to make all the following work as intended:

llvm-ar-9
llvm-ranlib.exe
arm-pokymllib32-linux-gnueabi-llvm-ar
arm-pokymllib32-linux-gnueabi-llvm-ar-9

Diff Detail

Event Timeline

MaskRay created this revision.Dec 10 2019, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 1:52 PM
rupprecht requested changes to this revision.Dec 10 2019, 2:05 PM

Update the heuristic to make all the following work as intended:

llvm-ar-9
llvm-ranlib.exe
arm-pokymllib32-linux-gnueabi-llvm-ar
arm-pokymllib32-linux-gnueabi-llvm-ar-9

This was manually verified, but can you add a test for it?
You could probably write a lit test like:

# RUN: mkdir -p bin
# RUN: cp llvm-ar bin/llvm-ar-9
# RUN: bin/llvm-ar-9 --help | FileCheck %s --check-prefix=AR
# RUN: cp llvm-ar bin/llvm-ranlib.exe
# RUN: bin/llvm-ranlib.exe --help | FileCheck %s --check-prefix=RANLIB
...
llvm/tools/llvm-ar/llvm-ar.cpp
1184

Needs to be lower-cased (+ a test case -- e.g. LLVM-AR.EXE should work)

1185–1186

This is pretty opaque, I think a more obvious algorithm should be used even if it's possibly more inefficient -- there's no need for this to be fast, it called at most 4 times.

Is the intention to match all these?
"...ar$"
"...ar-..."
"...ar.exe"

This revision now requires changes to proceed.Dec 10 2019, 2:05 PM
MaskRay marked 2 inline comments as done.Dec 10 2019, 2:34 PM

Update the heuristic to make all the following work as intended:

llvm-ar-9
llvm-ranlib.exe
arm-pokymllib32-linux-gnueabi-llvm-ar
arm-pokymllib32-linux-gnueabi-llvm-ar-9

This was manually verified, but can you add a test for it?
You could probably write a lit test like:

# RUN: mkdir -p bin
# RUN: cp llvm-ar bin/llvm-ar-9
# RUN: bin/llvm-ar-9 --help | FileCheck %s --check-prefix=AR
# RUN: cp llvm-ar bin/llvm-ranlib.exe
# RUN: bin/llvm-ranlib.exe --help | FileCheck %s --check-prefix=RANLIB
...

The path of llvm-ar is not known. Even if it does, I don't know copying an executable to a directory that may be mounted with the noexec flag will work...

llvm/tools/llvm-ar/llvm-ar.cpp
1184

I think LLVM-AR.EXE is a DOSism that we don't need to care in 2019.

1185–1186

Yes.

Update the heuristic to make all the following work as intended:

llvm-ar-9
llvm-ranlib.exe
arm-pokymllib32-linux-gnueabi-llvm-ar
arm-pokymllib32-linux-gnueabi-llvm-ar-9

This was manually verified, but can you add a test for it?
You could probably write a lit test like:

# RUN: mkdir -p bin
# RUN: cp llvm-ar bin/llvm-ar-9
# RUN: bin/llvm-ar-9 --help | FileCheck %s --check-prefix=AR
# RUN: cp llvm-ar bin/llvm-ranlib.exe
# RUN: bin/llvm-ranlib.exe --help | FileCheck %s --check-prefix=RANLIB
...

The path of llvm-ar is not known.

lit substitutes llvm-ar with the full path; see llvm/test/tools/llvm-ar/case-detection.test

Even if it does, I don't know copying an executable to a directory that may be mounted with the noexec flag will work...

Hmm, fair point. There's probably some other trick that would work... would creating a symlink work on a noexec mount?

llvm/tools/llvm-ar/llvm-ar.cpp
1184

Per D44808, the lowercasing is needed

MaskRay updated this revision to Diff 233213.Dec 10 2019, 3:50 PM

Add tests.

Detect Lib.exe

MaskRay marked 3 inline comments as done.Dec 10 2019, 3:51 PM
raj.khem accepted this revision.Dec 10 2019, 3:53 PM

LGTM now. I am testing it in yocto multilib SDK builds.

MaskRay updated this revision to Diff 233214.Dec 10 2019, 3:57 PM

Fix a copy-n-paste problem

rupprecht accepted this revision.Dec 10 2019, 4:05 PM

Looks good. Thanks for adding the tests!

This revision is now accepted and ready to land.Dec 10 2019, 4:05 PM
This revision was automatically updated to reflect the committed changes.