Page MenuHomePhabricator

[llvm-objcopy] Improve tool selection logic to recognize llvm-strip-$major as strip
ClosedPublic

Authored by MaskRay on Mar 21 2020, 11:56 PM.

Details

Summary

Debian and some other distributions install llvm-strip as llvm-strip-$major (e.g. /usr/bin/llvm-strip-9)

D54193 made it work with llvm-strip-$major but did not add a test.
The behavior was regressed by D69146.

Fixes https://github.com/ClangBuiltLinux/linux/issues/940

Diff Detail

Event Timeline

MaskRay created this revision.Mar 21 2020, 11:56 PM
abrachet added inline comments.Mar 22 2020, 12:21 AM
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
332

StringRef is small enough that its probably faster to capture by value, no?

333

Might as well just explicitly use size_t

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
335

What would you think about smth like this:

const llvm::Regex StripRegex("^(llvm-)?strip(.*)$"); 
const llvm::Regex InstallNameToolRegex("^(llvm-)?install-name-tool(.*)$");
...
if (StripRegex.match(Tool))
  ...
else if (InstallNameToolRegex.match(Tool))
  ...

Seems sane to me if I am understanding the string arithmetic correctly. I can confirm this fixes the Debian use case that we came across.

tpimh added a subscriber: tpimh.Mar 22 2020, 3:22 AM
MaskRay marked 3 inline comments as done.Mar 22 2020, 9:10 AM
MaskRay added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
333

The return type of rfind_lower probably should be std::string::size_type. Using auto will be width agnostic. (Although it is unlikely that size_t != std:string::size_type)

335

The current code is to make it similar to D71302 llvm-ar

We need to support something like x86_64-unknown-linux-gnu-strip

MaskRay updated this revision to Diff 251897.Mar 22 2020, 9:21 AM
MaskRay marked an inline comment as done.

Add tests about x86_64-linux-gnu-strip

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
335

0/ the title of the diff "Support llvm-strip-11 as argv[0]" seems to be a little bit confusing.
1/ In any case - here and in D71302 the matching logic seems to be a tiny bit more complicated than it can be
(from the readability perspective).
Depending on what you want - if we need a strict well-controlled robust way of matching specific name patterns I'd probably use a regular expression rather than some ad-hoc code, if a simple match is good enough - i suspect StringRef::contains / StringRef::contains_lower could do their job here. Though I don't insist.

MaskRay marked an inline comment as done.Mar 22 2020, 12:05 PM
MaskRay added inline comments.
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
335

D71302 has to be complex because "lib" can be in the target triple ("arm-pokymllib32-linux-gnueabi-llvm-ar"). We need to guard against such cases. The logic is slightly more rubust. It can work with powerpc64-stripinvalid-freebsd13-objcopy (not very unrealistic).

I actually think Regex.h can increase code complexity.

MaskRay retitled this revision from [llvm-objcopy] Support llvm-strip-11 as argv[0] to [llvm-objcopy] Recognize llvm-strip-$major as a tool name.Mar 22 2020, 12:22 PM
MaskRay edited the summary of this revision. (Show Details)
jhenderson added inline comments.Mar 23 2020, 1:21 AM
llvm/test/tools/llvm-objcopy/tool-name.test
14

Maybe worth a test case for "[llvm-]strip" etc being in the middle of the name, i.e. something like "gnu-llvm-strip-11". Perhaps it could replace this last test case, as I don't think that one then provides additional coverage.

33

I would expect this to have the same set of cases as the other tools. I don't see why they are unique, just because it's emulating the install_name_tool.

MaskRay updated this revision to Diff 252115.Mar 23 2020, 12:12 PM
MaskRay marked 4 inline comments as done.

Improve tests

llvm/test/tools/llvm-objcopy/tool-name.test
33

Rearranged the tests. Hope that will provide sufficient coverage without being verbose.

MaskRay updated this revision to Diff 252116.Mar 23 2020, 12:13 PM

Delete an excess empty line

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
335

@MaskRay thanks for your patience and for the quick fix.

0/ Would be good to add a comment explaining what strings this code is intended to match
(so that people don't need to look through the tests).

1/ I suggested Regex primarily because this way it seemed to me that the code would be self-documented / easier to reason about, thus we would avoid having such bugs in the future, although, perhaps, this is subjective.

This revision is now accepted and ready to land.Mar 23 2020, 1:34 PM
MaskRay updated this revision to Diff 252133.Mar 23 2020, 1:45 PM
MaskRay marked 2 inline comments as done.

Improve comments in code.

A note on how GNU objcopy/strip makes a crunched executable:

binutils/not-strip.c (int is_strip = 0;) is linked into objcopy while binutils/is-strip.c (int is_strip = 1;) is linked into strip. Though it is strange that on my Debian, size(/usr/bin/x86_64-linux-gnu-objcopy) != size(/usr/bin/x86_64-linux-gnu-strip ...

llvm/tools/llvm-objcopy/llvm-objcopy.cpp
335

Thanks! A comment will be very useful here. I should have put up a comment as I did for D71302 ...

MaskRay added a subscriber: hans.EditedMar 23 2020, 1:53 PM

@hans If it is not too late for LLVM 10.0.0, I hope this can be picked. If it is too late, I hope it can be included in 10.0.1 ...

Debian and some other distributions will ship /usr/bin/llvm-strip-10 but not /usr/bin/llvm-strip (https://packages.debian.org/bullseye/amd64/llvm-9/filelist). Without this patch, /usr/bin/llvm-strip-10 will think it is objcopy.

llvm-{objcopy,strip,install-name-tool} is a crunched executable. llvm-strip-10 a.o (implicit --strip-all) and llvm-objcopy-10 a.o (almost a no-op) are different...
llvm-strip a.o -o b.o is good but llvm-objcopy-10 a.o -o b.o would error.

This revision was automatically updated to reflect the committed changes.
MaskRay retitled this revision from [llvm-objcopy] Recognize llvm-strip-$major as a tool name to [llvm-objcopy] Improve tool selection logic to recognize llvm-strip-$major as strip.Mar 23 2020, 3:59 PM