This is an archive of the discontinued LLVM Phabricator instance.

Fix lib.exe detection when running within MSVC toolchain
ClosedPublic

Authored by aganea on Mar 22 2018, 2:52 PM.

Details

Summary

Fixed llvm-ar executable casing detection.

We ran into a case where llvm-lib.exe is used as lib.exe, but the MSVC v140 toolchain ends up reading the executable name as Lib.exe (first L uppercase)
This prevents llvm-lib from properly detecting the tool as 'lib.exe' and ends up printing the help page.

The culpit was the following command, generated by msbuild:

1> C:\Program Files (x86)\MSBuild\14.0\bin\Tracker.exe /d "C:\Program Files (x86)\MSBuild\14.0\bin\FileTracker32.dll" /i F:\path\mylib.tlog /r F:\path\mylib.OBJ /c "C:\Program Files (x86)\LLVM\msbuild-bin\Lib.exe" /OUT:"..\..\..\..\tmp\mylib.lib" /NOLOGO /MACHINE:X64 /ignore:4098,4099,4217,4221 ..\..\..\..\tmp\mylib.obj

Diff Detail

Repository
rL LLVM

Event Timeline

aganea created this revision.Mar 22 2018, 2:52 PM
rnk accepted this revision.Mar 23 2018, 11:10 AM

lgtm

This revision is now accepted and ready to land.Mar 23 2018, 11:10 AM
aganea updated this revision to Diff 140080.Mar 28 2018, 8:39 AM

I've added an test, please @rnk let me know if you're fine with it before I commit.

rnk added inline comments.Mar 28 2018, 11:18 AM
llvm/test/tools/llvm-ar/case-detection.test
7 ↗(On Diff #140080)

WIll this pass on Windows without a .exe suffix?

aganea added inline comments.Mar 28 2018, 11:35 AM
llvm/test/tools/llvm-ar/case-detection.test
7 ↗(On Diff #140080)

I was worried it wouldn't pass on a Unix box (which I don't have here), that's why I didn't add the .exe suffix.

I've tested on Windows 10, both in a MingW64 shell and a regular cmd.exe shell. Tested with Python 2.7 and 3.6. The test passes in all cases.

Would you like me to do other tests? Or do this step in a different manner?

aganea added inline comments.Mar 30 2018, 6:55 AM
llvm/test/tools/llvm-ar/case-detection.test
7 ↗(On Diff #140080)

@rnk - are you fine with the test? Or should I make it Windows-specific and use Lib.exe instead?

rnk accepted this revision.Mar 30 2018, 2:42 PM

Thanks, looks good.

ruiu added a comment.Mar 30 2018, 2:59 PM

Don't you need to change printHelpMessage() as well? With that, LGTM

aganea added a comment.Apr 3 2018, 6:05 AM

Don't you need to change printHelpMessage() as well? With that, LGTM

@ruiu: Would you expect anything different from what's currently there?

$ ./Ar
F:\svn\build\Debug\bin\Ar.exe: Expected options.
OVERVIEW: LLVM Archiver (llvm-ar)

  This program archives bitcode files into single libraries

USAGE: Ar.exe [options] [relpos] [count] <archive-file> [members]...
aganea updated this revision to Diff 141740.Apr 9 2018, 2:16 PM

Improved test a bit. Also, printHelpMessage() now shows the same tool name casing as on the command-line, ie:

$ ./RanLib
F:\svn\llvm\test\RanLib: error loading '': no such file or directory.

OVERVIEW: LLVM Ranlib (llvm-ranlib)

  This program generates an index to speed access to archives

USAGE: RanLib <archive-file>
$ ./Ar
F:\svn\llvm\test\Ar: An archive name must be specified.

OVERVIEW: LLVM Archiver (llvm-ar)

  This program archives bitcode files into single libraries

USAGE: Ar [options] [relpos] [count] <archive-file> [members]...
ruiu added inline comments.Apr 9 2018, 2:25 PM
tools/llvm-ar/llvm-ar.cpp
61

I'm sorry that my previous comment was vague. I don't think you need to change this and

73

this.

113–115

I meant you should replace find with find_lower (and I didn't mean that you should use formatv.)

aganea updated this revision to Diff 141771.Apr 9 2018, 5:36 PM
ruiu accepted this revision.Apr 9 2018, 5:40 PM

LGTM

Please commit.

This revision was automatically updated to reflect the committed changes.