This is an archive of the discontinued LLVM Phabricator instance.

Differential D44808

Fix lib.exe detection when running within MSVC toolchain
ClosedPublic

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

Details

Reviewers
rnk
ruiu
Commits
rG76a0154ce5cd: [llvm-ar] Fix lib.exe detection when running within MSVC toolchain
rL329658: [llvm-ar] Fix lib.exe detection when running within MSVC toolchain
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
Herald added a subscriber: llvm-commits. · View Herald TranscriptMar 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
In D44808#1053467, @ruiu wrote:

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.

Closed by commit rL329658: [llvm-ar] Fix lib.exe detection when running within MSVC toolchain (authored by aganea). · Explain WhyApr 9 2018, 6:54 PM
This revision was automatically updated to reflect the committed changes.
rupprecht mentioned this in D71302: [llvm-ar] Improve tool selection logic.Dec 10 2019, 3:15 PM
MaskRay mentioned this in rG4d53b99c5df2: [llvm-ar] Improve tool selection heuristic.Dec 10 2019, 5:42 PM

Revision Contents

PathSize
test/
tools/
llvm-ar/
case-detection.test
case-detection.test (nonexistent)
11 lines
tools/
llvm-ar/
llvm-ar.cpp
llvm-ar.cpp (revision 329594)
21 lines

Diff 141740

test/tools/llvm-ar/case-detection.test

-- Test CamelCase tool name to ensure detection works properly
RUN: yaml2obj %S/Inputs/coff.yaml -o %t.obj
RUN: rm -rf %t1
RUN: mkdir %t1
RUN: cp llvm-ar %t1/Lib
RUN: %t1/Lib /OUT:%t.lib %t.obj | FileCheck %s -allow-empty
RUN: cp llvm-ar %t1/Ar
RUN: %t1/Ar crs %t.ar %t.obj | FileCheck %s -allow-empty
CHECK-NOT: OVERVIEW: LLVM Archiver (llvm-ar)

tools/llvm-ar/llvm-ar.cpp

Context not available.
#include "llvm/Support/Errc.h" #include "llvm/Support/Errc.h"
#include "llvm/Support/FileSystem.h" #include "llvm/Support/FileSystem.h"
#include "llvm/Support/Format.h" #include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/LineIterator.h" #include "llvm/Support/LineIterator.h"
#include "llvm/Support/ManagedStatic.h" #include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/MemoryBuffer.h"
Context not available.
This program generates an index to speed access to archives This program generates an index to speed access to archives
USAGE: llvm-ranlib <archive-file> USAGE: {0} <archive-file>
ruiuUnsubmitted
Not Done
ReplyInline Actions

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

ruiu: I'm sorry that my previous comment was vague. I don't think you need to change this and
OPTIONS: OPTIONS:
-help - Display available options -help - Display available options
Context not available.
This program archives bitcode files into single libraries This program archives bitcode files into single libraries
USAGE: llvm-ar [options] [relpos] [count] <archive-file> [members]... USAGE: {0} [options] [relpos] [count] <archive-file> [members]...
ruiuUnsubmitted
Not Done
ReplyInline Actions

this.

ruiu: this.
OPTIONS: OPTIONS:
-M - -M -
Context not available.
)"; )";
void printHelpMessage() { void printHelpMessage() {
if (Stem.find("ranlib") != StringRef::npos) if (Stem.find_lower("ranlib") != StringRef::npos)
outs() << RanlibHelp; outs() << formatv(RanlibHelp, Stem);
else if (Stem.find("ar") != StringRef::npos) else if (Stem.find_lower("ar") != StringRef::npos)
ruiuUnsubmitted
Not Done
ReplyInline Actions

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

ruiu: I meant you should replace find with find_lower (and I didn't mean that you should use formatv.)
outs() << ArHelp; outs() << formatv(ArHelp, Stem);
} }
// Show the error message and exit. // Show the error message and exit.
Context not available.
llvm::InitializeAllAsmParsers(); llvm::InitializeAllAsmParsers();
Stem = sys::path::stem(ToolName); Stem = sys::path::stem(ToolName);
if (Stem.find("dlltool") != StringRef::npos) if (Stem.find_lower("dlltool") != StringRef::npos)
return dlltoolDriverMain(makeArrayRef(argv, argc)); return dlltoolDriverMain(makeArrayRef(argv, argc));
if (Stem.find("ranlib") != StringRef::npos) if (Stem.find_lower("ranlib") != StringRef::npos)
return ranlib_main(argc, argv); return ranlib_main(argc, argv);
if (Stem.find("lib") != StringRef::npos) if (Stem.find_lower("lib") != StringRef::npos)
return libDriverMain(makeArrayRef(argv, argc)); return libDriverMain(makeArrayRef(argv, argc));
if (Stem.find("ar") != StringRef::npos) if (Stem.find_lower("ar") != StringRef::npos)
return ar_main(argc, argv); return ar_main(argc, argv);
fail("Not ranlib, ar, lib or dlltool!"); fail("Not ranlib, ar, lib or dlltool!");
} }
Context not available.