[llvm-libtool-darwin] Print a warning if object file names are repeated

[Roger]

Summary

Print a warning if llvm-libtool-darwin if any of the object
files provided by the user have the same file name.

The tool will now print a warning if there is a name collision across:

• Two object files
• An object file and an object file from within a static library
• Two object files from different static libraries

Here is an example of the error:

$ llvm-libtool-darwin -static -o archive.a out.o out.o
error: file 'out.o' was specified multiple times.
in: out.o
in: out.o

$ llvm-libtool-darwin -static -o archive.a out.o
$ llvm-libtool-darwin -static -o combined.a archive.a out.o
error: file 'out.o' was specified multiple times.
in: archive.a
in: out.o

This change mimics apple's cctools libtool's behavior which always shows a warning in such cases.

[jhenderson]

This change does not allow static libraries to be created if any of the object files in that library would have the same file name.

This should be "This change prevents creation of static libraries if any ...". The phrase "does not allow" suggests that the patch does something, but does not make it possible for the static libraries to be created in this situation (implying that a future change would make it possible).

Note that this apple's cctools libtool allows such static libraries to be created and only throws a warning about the name collision.

I'm not an Apple/Mach-O developer, and don't really know much, if anything, about the platform or the file format, so take my thoughts with a pinch of salt. However, I wanted to question this:

1. For traditional Unix static libraries, the filenames are largely irrelevant - you can have multiple library members without any issue and it'll just work at link time. This is because such library members are picked based on their symbol names, rather than anything to do with their filenames.
2. If the existing cctools libtool reports a warning rather than an error, why not do the same here? It seems like reporting an error might cause problems where existing users are creating such a library for good reason. NB: if there is no possible good reason, i.e. the library is fundamentally broken, then an error makes sense - see my above point about not being too familiar with the format.

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

75

We often add extra indentation to make output line up nicely after FileCheck directives of different lengths.

Same in the other tests.

97

%T is generally considered deprecated and may be removed at a future point. This is because the directory is not unique to the test, so there could be mysterious runtime failures due to test ordering issues.

To workaround this, cd into a test-time created directory, with a name derived from %t, and then use paths relative to that directory.

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

62

66

I'd recommend including the "error:" prefix here too.

llvm/test/tools/llvm-libtool-darwin/create-static-lib.test

54–55

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

116

Up to you, but I think for STL-like containers, it's acceptable to follow STL-naming conventions with their methods (i.e. push_back here). Others may disagree though :)

540–542

I'd prefer not to use auto here. In fact, for Members and Files, I believe you could (should?) be using ArrayRef<>

543

Is there a reason this is a) ordered and b) using a std::map rather than LLVM's StringMap?

544

Iterators or simply Its (always use UpperCamelCase for variables).

546

LLVM comments are always full sentences ("This should imply iterators.second != Files.cend()."). I'd suggest using C-style block comments too, so that you aren't forced to put this on a new line. Finally, I'd drop the "should" (i.e. "This implies...".

You might want to add an assertion to sanity check that iterators.second != Files.cend()

547

Throughout this function: LLVM style says no braces for single line ifs and loops.

553

Probably rename this to MemberToFiles, rather than the rather meaningless "pair".

557

LLVM coding standards say that error messages shouldn't end in a .

581–583

[thevinster]

I agree with jhenderson@ here. I'm not sure why we want to error on this when it's only warning for cctools libtool. This is a backwards incompatible change for those who want to do a drop in replacement of the tool.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

109–112

These vectors feel a bit strange to me. It almost feels like NewArchiveMember should either support filename in the struct or a much less invasive change is to have this class inherit NewArchiveMember and support the filename field here. We can always downcast it if methods elsewhere require the NewArchiveMember object.

[Roger]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

109–112

The main reason why I went with this approach is because other functions writeArchive and writeArchiveToBuffers depend on getting a std::vector<NewArchiveMember> as an input. If that were not the case, I would have done something like

struct NewArchiveMemberWrapper {
  NewArchiveMember Member;
  StringRef File;
};

and used std::vector<NewArchiveMemberWrapper> instead of std::vector<NewArchiveMember>.

It almost feels like NewArchiveMember should either support filename in the struct or a much less invasive change is to have this class inherit NewArchiveMember and support the filename field here.

I intentionally avoid using inheritance here because, fundamentally, I am not trying to create another type of NewArchiveMember. I am just trying to tie an additional piece of data (a file path) to each NewArchiveMember. Also, if I created a child class (let's call it NewArchiveMemberChild) and used std::vector<NewArchiveMemberChild>, then I would have to do extra work to generate the std::vector<NewArchiveMember> that the writeArchive and writeArchiveToBuffer functions need. Using composition here is simpler and I believe more appropriate.

I also did not add a new StringRef field to NewArchiveMember because the feature I am introducing this field for is specific to llvm-libtool-darwin but NewArchiveMember is not (it is shared among multiple libraries). Adding a new field to NewArchiveMember would be leaking state/complexity relevant only to llvm-libtool-darwin out to other libraries.

These vectors feel a bit strange to me.

I will admit one thing I do not like is that the Members vector and the Files vector are programmed to always have the same size but this invariant is not implied by their types. If we had one vector of NewArchiveMemberWrapper, then this wouldn't be an issue but, again, the existing code does not make this easy. I think it might be do-able if we had the ranges-v3 library or C++20's ranges but we do not :(

[Roger]

Rebasing.

[Roger]

Rebasing

[Roger]

Rebase

[alexander-shaposhnikov]

Before reviewing the details I'd like to better understand the rationale for the proposed changes (especially given the discrepancy with cctools etc), it'd be good to include it into the summary of the diff.

[Roger]

Change this feature from throwing an error to printing a warning.

[Roger]

Fix and address comments in tests.

[Roger]

Fix comment.

llvm/test/tools/llvm-libtool-darwin/create-static-lib.test

54–55

I am just removing that line since I'm changing this behavior to print an error.

[smeenai]

This change was motivated by an issue observed internally where llvm-libtool silently accepting two input files with the same basename and putting them into the same archive led to a crash. This was ultimately down to an ODR violation on our end, but we noticed that cctools' libtool gives a warning in the same scenario, which at least gives the user some hint of what might be going on.

We originally thought of an emitting an error instead of a warning because we couldn't think of a good reason you'd want two files with the same basename in the same archive (it also results in confusing linker diagnostics, ar extractions, etc.), and because warnings are super easy to miss in build output. However, there might be cases where this situation arises naturally (especially with third-party archives out of your control), so matching cctools behavior and emitting a warning instead of an error does make more sense (and we could always add an option to upgrade the warning to an error if we desired).

[Roger]

Fix test

[jhenderson]

In D113130#3115989, @smeenai wrote:

This change was motivated by an issue observed internally where llvm-libtool silently accepting two input files with the same basename and putting them into the same archive led to a crash. This was ultimately down to an ODR violation on our end, but we noticed that cctools' libtool gives a warning in the same scenario, which at least gives the user some hint of what might be going on.

We originally thought of an emitting an error instead of a warning because we couldn't think of a good reason you'd want two files with the same basename in the same archive (it also results in confusing linker diagnostics, ar extractions, etc.), and because warnings are super easy to miss in build output. However, there might be cases where this situation arises naturally (especially with third-party archives out of your control), so matching cctools behavior and emitting a warning instead of an error does make more sense (and we could always add an option to upgrade the warning to an error if we desired).

Thanks fo the explanation, it makes sense to me.

[Roger]

Rebase to new parent

[Roger]

.

[Roger]

Rebase

[Roger]

Rebase

[Roger]

Stop manually passing FileName around.

[Roger]

Update asserts

[Roger]

Clang format

[Roger]

Small changes

[Roger]

Respond to comments

[Roger]

Addressing comments

[Roger]

Formatting

[Roger]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

540–542

Done.

543

You're right I shouldn't use an ordered map. I didn't know about StringMap (am very new to the LLVM codebase). I'll use it :)

[Roger]

I'm not an Apple/Mach-O developer, and don't really know much, if anything, about the platform or the file format, so take my thoughts with a pinch of salt. However, I wanted to question this:

1. For traditional Unix static libraries, the filenames are largely irrelevant - you can have multiple library members without any issue and it'll just work at link time. This is because such library members are picked based on their symbol names, rather than anything to do with their filenames.
2. If the existing cctools libtool reports a warning rather than an error, why not do the same here? It seems like reporting an error might cause problems where existing users are creating such a library for good reason. NB: if there is no possible good reason, i.e. the library is fundamentally broken, then an error makes sense - see my above point about not being too familiar with the format.

I changed the behavior to output a warning rather than throw an error for the reasons provided above

[jhenderson]

Some nits, but I haven't done a detailed review, and am leaving that to those actually invested in this tool.

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

69

Missing warning: prefix.

llvm/test/tools/llvm-libtool-darwin/create-static-lib.test

66

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

559–561

Nit: delete braces for single-line loop.

[Roger]

Address comments

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

69

This stanza is all part of one warning message so only the first line will have the prefix.

[smeenai]

LGTM, thanks!

[Roger]

Insert missing period from warning.

[thakis]

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

Please take a look, and revert for now if it takes a while to fix.

[smeenai]

Thanks for the heads up. We'll need to account for forward vs. back slashes in the test. I reverted for now and will recommit with a fix.

[smeenai]

The Windows test failure here is pretty annoying :(

For the test where we specify one file via -l and the other via its path, the output on Windows looks like

warning: file 'L-and-l.test.tmp-input1.o' was specified multiple times.
in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy/L-and-l.test.tmp-input1.o
in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy\L-and-l.test.tmp-input1.o

Note that the last slash is a forward slash in one path and a backslash in the other. The forward slash is coming from our path specification in the test, and the backslash is coming from sys::path::append in libtool (to concatenate -L directories and -l names).

One way to fix this would be to introduce a new %{sep} substitution in lit (similar to the existing %{pathsep}), so we could use the appropriate slashes for the platform in our test automatically. Another would be to use sys::path::Style::windows_slash in our sys::path::append call, but that could break with \\?\ paths, which can't use forward slashes.

@mstorsjo, I believe you have a bunch of experience dealing with slash differences on Windows. Any suggestions on the best way to proceed here?

[mstorsjo]

In D113130#3251852, @smeenai wrote:

For the test where we specify one file via -l and the other via its path, the output on Windows looks like

warning: file 'L-and-l.test.tmp-input1.o' was specified multiple times.
in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy/L-and-l.test.tmp-input1.o
in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy\L-and-l.test.tmp-input1.o

Note that the last slash is a forward slash in one path and a backslash in the other. The forward slash is coming from our path specification in the test, and the backslash is coming from sys::path::append in libtool (to concatenate -L directories and -l names).

One way to fix this would be to introduce a new %{sep} substitution in lit (similar to the existing %{pathsep}), so we could use the appropriate slashes for the platform in our test automatically. Another would be to use sys::path::Style::windows_slash in our sys::path::append call, but that could break with \\?\ paths, which can't use forward slashes.

@mstorsjo, I believe you have a bunch of experience dealing with slash differences on Windows. Any suggestions on the best way to proceed here?

Hmm... Overall I'm not a huge fan of tests like FileCheck -DSOMETHING=%t/suffix or similar, because as we've noticed, the FileCheck literal checks are quite brittle for such things - especially as we can configure which kind of separator that sys::path::append uses too. Although I guess we could mirror that into the lit subtsitutions too. (Also, for such cases, we could maybe make other lit substitutions, like %t, use the same path separator as is configured to be the default for sys::path - that would help fix a bunch of test cases that still are broken with forward slash on Windows.) But it's quite unintuitive to write the match patterns for such cases, to try to mimic exactly what combination of slashes you end up with in each case (when considering people writing tests on unix without access to testing thigs on Windows.)

I don't think considering using sys::path::Style::windows_slash explicitly is the right way to go either. I don't think we should complicate the code to do something non-obvious just for the sake of making the tools produce the path that the test framework expects.

For cases within code itself, I've considered adding a sys::path::equals that accepts either slash direction, and which would be case insensitive on windows (and darwin?). I wonder how hard it would be to add something similar to FileCheck. I don't offhand know exactly how it would work... But I think something like that would be good for easily writing testcases on unix, without spending lots of unnecessary fiddling on making them pass on Windows.

[smeenai]

In D113130#3252504, @mstorsjo wrote:

In D113130#3251852, @smeenai wrote:

For the test where we specify one file via -l and the other via its path, the output on Windows looks like

warning: file 'L-and-l.test.tmp-input1.o' was specified multiple times.
in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy/L-and-l.test.tmp-input1.o
in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy\L-and-l.test.tmp-input1.o

Note that the last slash is a forward slash in one path and a backslash in the other. The forward slash is coming from our path specification in the test, and the backslash is coming from sys::path::append in libtool (to concatenate -L directories and -l names).

One way to fix this would be to introduce a new %{sep} substitution in lit (similar to the existing %{pathsep}), so we could use the appropriate slashes for the platform in our test automatically. Another would be to use sys::path::Style::windows_slash in our sys::path::append call, but that could break with \\?\ paths, which can't use forward slashes.

@mstorsjo, I believe you have a bunch of experience dealing with slash differences on Windows. Any suggestions on the best way to proceed here?

Hmm... Overall I'm not a huge fan of tests like FileCheck -DSOMETHING=%t/suffix or similar, because as we've noticed, the FileCheck literal checks are quite brittle for such things - especially as we can configure which kind of separator that sys::path::append uses too. Although I guess we could mirror that into the lit subtsitutions too. (Also, for such cases, we could maybe make other lit substitutions, like %t, use the same path separator as is configured to be the default for sys::path - that would help fix a bunch of test cases that still are broken with forward slash on Windows.) But it's quite unintuitive to write the match patterns for such cases, to try to mimic exactly what combination of slashes you end up with in each case (when considering people writing tests on unix without access to testing thigs on Windows.)

I don't think considering using sys::path::Style::windows_slash explicitly is the right way to go either. I don't think we should complicate the code to do something non-obvious just for the sake of making the tools produce the path that the test framework expects.

For cases within code itself, I've considered adding a sys::path::equals that accepts either slash direction, and which would be case insensitive on windows (and darwin?). I wonder how hard it would be to add something similar to FileCheck. I don't offhand know exactly how it would work... But I think something like that would be good for easily writing testcases on unix, without spending lots of unnecessary fiddling on making them pass on Windows.

Thanks for the suggestions!

I'm pretty short on time right now, so I just loosened the test conditionals to make it pass. I agree that a slash-agnostic path comparison would be super useful for FileCheck. I imagine you could implement it via regex (which FileCheck already has support for), but I've never looked into FileCheck's code much to say for sure.

[jhenderson]

The required FileCheck regex pattern would be {{\\/}} or something to that effect, I believe. Could we add a lit substitution, similar to %\t or %/t, which is specifically for use within FileCheck patterns, so uses the regex pattern, maybe?

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

 # RUN: llvm-ar cr %t/otherDirname/libinput2.a %t-input1.o
 
 # RUN: llvm-libtool-darwin -static -o %t.lib -linput2 -L%t/dirname -L%t/otherDirname
 # RUN: llvm-ar t %t.lib | \
 # RUN:   FileCheck %s --check-prefix=SINGLE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
 # RUN: llvm-nm --print-armap %t.lib | \
 # RUN:   FileCheck %s --check-prefix=SINGLE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
 
+## Check that if two different files with the same names are explicitly
+## specified, the command gives a warning.
+# RUN: cp %t-input2.o %t/dirname
+# RUN: llvm-libtool-darwin -static -o %t.lib \
+# RUN:   %t/dirname/%basename_t.tmp-input2.o %t-input2.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT \
+# RUN:     -DFILE=%basename_t.tmp-input2.o \
+# RUN:     -DINPUTA=%t/dirname/%basename_t.tmp-input2.o \
+# RUN:     -DINPUTB=%t-input2.o
+
+# DUPLICATE-INPUT:     warning: file '[[FILE]]' was specified multiple times.
+# DUPLICATE-INPUT-DAG: [[INPUTA]]
+# DUPLICATE-INPUT-DAG: [[INPUTB]]
+
 ## -l option combined with an input file:
 # RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o -linput2 -L%t/dirname
 # RUN: llvm-ar t %t.lib | \
 # RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
 # RUN: llvm-nm --print-armap %t.lib | \
 # RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
 
-## Specify same -l option twice:
-## cctools' libtool raises a warning in this case.
-## The warning is not yet implemented for llvm-libtool-darwin.
-# RUN: llvm-libtool-darwin -static -o %t.lib -l%basename_t.tmp-input2.o -l%basename_t.tmp-input2.o -L%T
-# RUN: llvm-ar t %t.lib | \
-# RUN:   FileCheck %s --check-prefix=DOUBLE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
-# RUN: llvm-nm --print-armap %t.lib | \
-# RUN:   FileCheck %s --check-prefix=DOUBLE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
-
-# DOUBLE-NAMES:      [[PREFIX]]-input2.o
-# DOUBLE-NAMES-NEXT: [[PREFIX]]-input2.o
+## Specify the same file with a -l option and an input file:
+# RUN: rm -rf %t/copy
+# RUN: mkdir -p %t/copy
+# RUN: cp %t-input1.o %t/copy
+# RUN: llvm-libtool-darwin -static -o %t.lib \
+# RUN:   %t/copy/%basename_t.tmp-input1.o -l%basename_t.tmp-input1.o -L%t/copy 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT -DFILE=%basename_t.tmp-input1.o \
+# RUN:     -DINPUTA=%t/copy/%basename_t.tmp-input1.o \
+# RUN:     -DINPUTB=%t/copy/%basename_t.tmp-input1.o
 
-# DOUBLE-SYMBOLS:      Archive map
-# DOUBLE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
-# DOUBLE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
-# DOUBLE-SYMBOLS-EMPTY:
+## Specify same -l option twice:
+# RUN: llvm-libtool-darwin -static -o %t.lib -l%basename_t.tmp-input1.o \
+# RUN:  -l%basename_t.tmp-input1.o -L%t/copy 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT \
+# RUN:     -DFILE=%basename_t.tmp-input1.o \
+# RUN:     -DINPUTA=%t/copy/%basename_t.tmp-input1.o \
+# RUN:     -DINPUTB=%t/copy/%basename_t.tmp-input1.o
 
 ## Check that an error is thrown when the input library cannot be found:
 # RUN: not llvm-libtool-darwin -static -o %t.lib -lfile-will-not-exist.o 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=NOT-FOUND -DFILE=file-will-not-exist.o
 
 # NOT-FOUND: error: cannot locate file '[[FILE]]'
 
 ## Check that an error is thrown when the input library cannot be found

llvm/test/tools/llvm-libtool-darwin/archive-flattening.test

 # BOTH-SYMBOLS:      Archive map
 # BOTH-SYMBOLS-NEXT: _x86_64 in [[PREFIX]]-x86_64.bc
 # BOTH-SYMBOLS-NEXT: _x86_64 in [[PREFIX]]-x86_64.bc
 # BOTH-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
 # BOTH-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
 # BOTH-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
 # BOTH-SYMBOLS-EMPTY:
 
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-x86_64.bc %t.correct.ar %t-input1.o  2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT -DFILEA=%basename_t.tmp-input1.o \
+# RUN:   -DPATHA=%t-input1.o -DFILEB=%basename_t.tmp-x86_64.bc -DPATHB=%t-x86_64.bc -DPATHCORRECT=%t.correct.ar
+
+# DUPLICATE-INPUT:     warning: file '[[FILEA]]' was specified multiple times.
+# DUPLICATE-INPUT-DAG: [[PATHA]]
+# DUPLICATE-INPUT-DAG: [[PATHCORRECT]]
+# DUPLICATE-INPUT:     file '[[FILEB]]' was specified multiple times.
+# DUPLICATE-INPUT-DAG: [[PATHB]]
+# DUPLICATE-INPUT-DAG: [[PATHCORRECT]]
+
 ## Cannot read archive:
 # RUN: echo '!<arch>' >  %t-invalid-archive.lib
 # RUN: echo 'invalid' >> %t-invalid-archive.lib
 # RUN: not llvm-libtool-darwin -static -o %t.lib %t-invalid-archive.lib 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=INVALID-ARCHIVE -DARCHIVE=%t-invalid-archive.lib
 
 # INVALID-ARCHIVE: error: '[[ARCHIVE]]': truncated or malformed archive
 

llvm/test/tools/llvm-libtool-darwin/create-static-lib.test

 
 # OVERWRITE-NAMES: [[PREFIX]]-input2.o
 
 # OVERWRITE-SYMBOLS:      Archive map
 # OVERWRITE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
 # OVERWRITE-SYMBOLS-EMPTY:
 
 ## Duplicate a binary:
-## cctools' libtool raises a warning in this case.
-## The warning is not yet implemented for llvm-libtool-darwin.
 # RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o %t-input2.o %t-input1.o 2>&1 | \
-# RUN:   FileCheck %s --allow-empty --implicit-check-not={{.}}
-# RUN: llvm-ar t %t.lib | \
-# RUN:   FileCheck %s --check-prefix=DUPLICATE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
-# RUN: llvm-nm --print-armap %t.lib | \
-# RUN:   FileCheck %s --check-prefix=DUPLICATE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT -DFILE=%basename_t.tmp-input1.o \
+# RUN:     -DINPUTA=%t-input1.o -DINPUTB=%t-input1.o
 
-# DUPLICATE-NAMES:      [[PREFIX]]-input1.o
-# DUPLICATE-NAMES-NEXT: [[PREFIX]]-input2.o
-# DUPLICATE-NAMES-NEXT: [[PREFIX]]-input1.o
-
-# DUPLICATE-SYMBOLS:      Archive map
-# DUPLICATE-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
-# DUPLICATE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
-# DUPLICATE-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
-# DUPLICATE-SYMBOLS-EMPTY:
+# DUPLICATE-INPUT:     warning: file '[[FILE]]' was specified multiple times.
+# DUPLICATE-INPUT-DAG: [[INPUTA]]
+# DUPLICATE-INPUT-DAG: [[INPUTB]]
+
+## Make sure we can combine object files with the same name if
+## they are for different architectures.
+# RUN: mkdir -p %t/arm64 %t/armv7
+# RUN: llvm-as %S/Inputs/arm64-ios.ll -o %t/arm64/out.bc
+# RUN: llvm-as %S/Inputs/armv7-ios.ll -o %t/armv7/out.bc
+## Command output should be empty.
+# RUN: llvm-libtool-darwin -static %t/arm64/out.bc %t/armv7/out.bc -o %t.lib 2>&1 | \
+# RUN:   FileCheck %s --implicit-check-not=warning: --allow-empty

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/MachOUniversalWriter.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/LineIterator.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include "llvm/TextAPI/Architecture.h"
 #include <map>
 #include <type_traits>
 
 using namespace llvm;
 using namespace llvm::object;
 
 static LLVMContext LLVMCtx;
 
-typedef std::map<uint64_t, std::vector<NewArchiveMember>>
-    MembersPerArchitectureMap;
+class NewArchiveMemberList;
+typedef std::map<uint64_t, NewArchiveMemberList> MembersPerArchitectureMap;
 
 cl::OptionCategory LibtoolCategory("llvm-libtool-darwin Options");
 
 static cl::opt<std::string> OutputFile("o", cl::desc("Specify output filename"),
                                        cl::value_desc("filename"),
                                        cl::cat(LibtoolCategory));
 
 static cl::list<std::string> InputFiles(cl::Positional,
static const std::array<std::string, 3> StandardSearchDirs{
     "/lib",
     "/usr/lib",
     "/usr/local/lib",
 };
 
 struct Config {
   bool Deterministic = true; // Updated by 'D' and 'U' modifiers.
   uint32_t ArchCPUType;
   uint32_t ArchCPUSubtype;
 };
 
 static Expected<std::string> searchForFile(const Twine &FileName) {
 
   auto FindLib =
       [FileName](ArrayRef<std::string> SearchDirs) -> Optional<std::string> {
     for (StringRef Dir : SearchDirs) {
       SmallString<128> Path;
       sys::path::append(Path, Dir, FileName);
 
       if (sys::fs::exists(Path))
         return std::string(Path);
     }
     return None;
   };

 }
 
 // MembersData is an organized collection of members.
 struct MembersData {
   // MembersPerArchitectureMap is a mapping from CPU architecture to a list of
   // members.
   MembersPerArchitectureMap MembersPerArchitecture;
   std::vector<std::unique_ptr<MemoryBuffer>> FileBuffers;
+};
+
+// NewArchiveMemberList instances serve as collections of archive members and
+// information about those members.
+class NewArchiveMemberList {
+  std::vector<NewArchiveMember> Members;
+  // This vector contains the file that each NewArchiveMember from Members came
+  // from. Therefore, it has the same size as Members.
+  std::vector<StringRef> Files;
 
-  static_assert(!std::is_copy_constructible<NewArchiveMember>::value,
-                "MembersPerArchitecture has a dependency on FileBuffers so it "
-                "should not be able to be copied on its own without "
-                "FileBuffers.");
-  static_assert(!std::is_copy_assignable<NewArchiveMember>::value,
-                "MembersPerArchitecture has a dependency on FileBuffers so it "
-                "should not be able to be copied on its own without "
-                "FileBuffers.");
+public:
+  // Add a NewArchiveMember and the file it came from to the list.
+  void push_back(NewArchiveMember &&Member, StringRef File) {
+    Members.push_back(std::move(Member));
+    Files.push_back(File);
+  }
+
+  ArrayRef<NewArchiveMember> getMembers() const { return Members; }
+
+  ArrayRef<StringRef> getFiles() const { return Files; }
+
+  static_assert(
+      std::is_same<decltype(MembersData::MembersPerArchitecture)::mapped_type,
+                   NewArchiveMemberList>(),
+      "This test makes sure NewArchiveMemberList is used by MembersData since "
+      "the following asserts test invariants required for MembersData.");
+  static_assert(
+      !std::is_copy_constructible<
+          decltype(NewArchiveMemberList::Members)::value_type>::value,
+      "MembersData::MembersPerArchitecture has a dependency on "
+      "MembersData::FileBuffers so it should not be able to "
+      "be copied on its own without FileBuffers. Unfortunately, "
+      "is_copy_constructible does not detect whether the container (ie vector) "
+      "of a non-copyable type is itself non-copyable so we have to test the "
+      "actual type of the stored data (ie, value_type).");
+  static_assert(
+      !std::is_copy_assignable<
+          decltype(NewArchiveMemberList::Members)::value_type>::value,
+      "MembersData::MembersPerArchitecture has a dependency on "
+      "MembersData::FileBuffers so it should not be able to "
+      "be copied on its own without FileBuffers. Unfortunately, "
+      "is_copy_constructible does not detect whether the container (ie vector) "
+      "of a non-copyable type is itself non-copyable so we have to test the "
+      "actual type of the stored data (ie, value_type).");
 };
 
 // MembersBuilder collects and organizes all members from the files provided by
 // the user.
 class MembersBuilder {
 public:
   MembersBuilder(const Config &C) : C(C) {}
 
   Expected<MembersData> build() {
     for (StringRef FileName : InputFiles)
-      if (Error E = addMember(FileName))
+      if (Error E = AddMember(*this, FileName)())
         return std::move(E);
 
     if (!ArchType.empty()) {
       uint64_t ArchCPUID = getCPUID(C.ArchCPUType, C.ArchCPUSubtype);
       if (Data.MembersPerArchitecture.find(ArchCPUID) ==
           Data.MembersPerArchitecture.end())
         return createStringError(std::errc::invalid_argument,
                                  "no library created (no object files in input "
                                  "files matching -arch_only %s)",
                                  ArchType.c_str());
     }
     return std::move(Data);
   }
 
 private:
+  class AddMember {
+    MembersBuilder &Builder;
+    StringRef FileName;
+
+  public:
+    AddMember(MembersBuilder &Builder, StringRef FileName)
+        : Builder(Builder), FileName(FileName) {}
+
+    Error operator()() {
+      Expected<NewArchiveMember> NewMemberOrErr =
+          NewArchiveMember::getFile(FileName, Builder.C.Deterministic);
+      if (!NewMemberOrErr)
+        return createFileError(FileName, NewMemberOrErr.takeError());
+      auto &NewMember = *NewMemberOrErr;
+
+      // For regular archives, use the basename of the object path for the
+      // member name.
+      NewMember.MemberName = sys::path::filename(NewMember.MemberName);
+      file_magic Magic = identify_magic(NewMember.Buf->getBuffer());
+
+      // Flatten archives.
+      if (Magic == file_magic::archive)
+        return addArchiveMembers(std::move(NewMember));
+
+      // Flatten universal files.
+      if (Magic == file_magic::macho_universal_binary)
+        return addUniversalMembers(std::move(NewMember));
+
+      // Bitcode files.
+      if (Magic == file_magic::bitcode)
+        return verifyAndAddIRObject(std::move(NewMember));
+
+      return verifyAndAddMachOObject(std::move(NewMember));
+    }
+
+  private:
     // Check that a file's architecture [FileCPUType, FileCPUSubtype]
     // matches the architecture specified under -arch_only flag.
     bool acceptFileArch(uint32_t FileCPUType, uint32_t FileCPUSubtype) {
-    if (C.ArchCPUType != FileCPUType)
+      if (Builder.C.ArchCPUType != FileCPUType)
         return false;
 
-    switch (C.ArchCPUType) {
+      switch (Builder.C.ArchCPUType) {
       case MachO::CPU_TYPE_ARM:
       case MachO::CPU_TYPE_ARM64_32:
       case MachO::CPU_TYPE_X86_64:
-      return C.ArchCPUSubtype == FileCPUSubtype;
+        return Builder.C.ArchCPUSubtype == FileCPUSubtype;
 
       case MachO::CPU_TYPE_ARM64:
-      if (C.ArchCPUSubtype == MachO::CPU_SUBTYPE_ARM64_ALL)
+        if (Builder.C.ArchCPUSubtype == MachO::CPU_SUBTYPE_ARM64_ALL)
           return FileCPUSubtype == MachO::CPU_SUBTYPE_ARM64_ALL ||
                  FileCPUSubtype == MachO::CPU_SUBTYPE_ARM64_V8;
         else
-        return C.ArchCPUSubtype == FileCPUSubtype;
+          return Builder.C.ArchCPUSubtype == FileCPUSubtype;
 
       default:
         return true;
       }
     }
 
     Error verifyAndAddMachOObject(NewArchiveMember Member) {
       auto MBRef = Member.Buf->getMemBufferRef();
       Expected<std::unique_ptr<object::ObjectFile>> ObjOrErr =
           object::ObjectFile::createObjectFile(MBRef);
 
       // Throw error if not a valid object file.
       if (!ObjOrErr)
         return createFileError(Member.MemberName, ObjOrErr.takeError());
 
       // Throw error if not in Mach-O format.
       if (!isa<object::MachOObjectFile>(**ObjOrErr))
         return createStringError(std::errc::invalid_argument,
                                  "'%s': format not supported",
                                  Member.MemberName.data());
 
       auto *O = dyn_cast<MachOObjectFile>(ObjOrErr->get());
       uint32_t FileCPUType, FileCPUSubtype;
       std::tie(FileCPUType, FileCPUSubtype) = MachO::getCPUTypeFromArchitecture(
           MachO::getArchitectureFromName(O->getArchTriple().getArchName()));
 
       // If -arch_only is specified then skip this file if it doesn't match
       // the architecture specified.
       if (!ArchType.empty() && !acceptFileArch(FileCPUType, FileCPUSubtype)) {
         return Error::success();
       }
 
       if (!NoWarningForNoSymbols && O->symbols().empty())
         WithColor::warning() << Member.MemberName + " has no symbols\n";
 
       uint64_t FileCPUID = getCPUID(FileCPUType, FileCPUSubtype);
-    Data.MembersPerArchitecture[FileCPUID].push_back(std::move(Member));
+      Builder.Data.MembersPerArchitecture[FileCPUID].push_back(
+          std::move(Member), FileName);
       return Error::success();
     }
 
     Error verifyAndAddIRObject(NewArchiveMember Member) {
       auto MBRef = Member.Buf->getMemBufferRef();
       Expected<std::unique_ptr<object::IRObjectFile>> IROrErr =
           object::IRObjectFile::create(MBRef, LLVMCtx);
 
       // Throw error if not a valid IR object file.
       if (!IROrErr)
         return createFileError(Member.MemberName, IROrErr.takeError());
 
       Triple TT = Triple(IROrErr->get()->getTargetTriple());
 
       Expected<uint32_t> FileCPUTypeOrErr = MachO::getCPUType(TT);
       if (!FileCPUTypeOrErr)
         return FileCPUTypeOrErr.takeError();
 
       Expected<uint32_t> FileCPUSubTypeOrErr = MachO::getCPUSubType(TT);
       if (!FileCPUSubTypeOrErr)
         return FileCPUSubTypeOrErr.takeError();
 
       // If -arch_only is specified then skip this file if it doesn't match
       // the architecture specified.
       if (!ArchType.empty() &&
           !acceptFileArch(*FileCPUTypeOrErr, *FileCPUSubTypeOrErr)) {
         return Error::success();
       }
 
       uint64_t FileCPUID = getCPUID(*FileCPUTypeOrErr, *FileCPUSubTypeOrErr);
-    Data.MembersPerArchitecture[FileCPUID].push_back(std::move(Member));
+      Builder.Data.MembersPerArchitecture[FileCPUID].push_back(
+          std::move(Member), FileName);
       return Error::success();
     }
 
     Error addChildMember(const object::Archive::Child &M) {
       Expected<NewArchiveMember> NewMemberOrErr =
-        NewArchiveMember::getOldMember(M, C.Deterministic);
+          NewArchiveMember::getOldMember(M, Builder.C.Deterministic);
       if (!NewMemberOrErr)
         return NewMemberOrErr.takeError();
       auto &NewMember = *NewMemberOrErr;
 
       file_magic Magic = identify_magic(NewMember.Buf->getBuffer());
 
       if (Magic == file_magic::bitcode)
         return verifyAndAddIRObject(std::move(NewMember));
 
       return verifyAndAddMachOObject(std::move(NewMember));
     }
 
-  Error processArchive(object::Archive &Lib, StringRef FileName) {
+    Error processArchive(object::Archive &Lib) {
       Error Err = Error::success();
       for (const object::Archive::Child &Child : Lib.children(Err))
         if (Error E = addChildMember(Child))
           return createFileError(FileName, std::move(E));
       if (Err)
         return createFileError(FileName, std::move(Err));
 
       return Error::success();
     }
 
-  Error addArchiveMembers(NewArchiveMember NewMember, StringRef FileName) {
+    Error addArchiveMembers(NewArchiveMember NewMember) {
       Expected<std::unique_ptr<Archive>> LibOrErr =
           object::Archive::create(NewMember.Buf->getMemBufferRef());
       if (!LibOrErr)
         return createFileError(FileName, LibOrErr.takeError());
 
-    if (Error E = processArchive(**LibOrErr, FileName))
+      if (Error E = processArchive(**LibOrErr))
         return E;
 
       // Update vector FileBuffers with the MemoryBuffers to transfer
       // ownership.
-    Data.FileBuffers.push_back(std::move(NewMember.Buf));
+      Builder.Data.FileBuffers.push_back(std::move(NewMember.Buf));
       return Error::success();
     }
 
-  Error addUniversalMembers(NewArchiveMember NewMember, StringRef FileName) {
+    Error addUniversalMembers(NewArchiveMember NewMember) {
       Expected<std::unique_ptr<MachOUniversalBinary>> BinaryOrErr =
           MachOUniversalBinary::create(NewMember.Buf->getMemBufferRef());
       if (!BinaryOrErr)
         return createFileError(FileName, BinaryOrErr.takeError());
 
       auto *UO = BinaryOrErr->get();
       for (const MachOUniversalBinary::ObjectForArch &O : UO->objects()) {
 
         Expected<std::unique_ptr<MachOObjectFile>> MachOObjOrErr =
             O.getAsObjectFile();
         if (MachOObjOrErr) {
           NewArchiveMember NewMember =
               NewArchiveMember(MachOObjOrErr->get()->getMemoryBufferRef());
           NewMember.MemberName = sys::path::filename(NewMember.MemberName);
 
           if (Error E = verifyAndAddMachOObject(std::move(NewMember)))
             return E;
           continue;
         }
 
         Expected<std::unique_ptr<IRObjectFile>> IRObjectOrError =
             O.getAsIRObject(LLVMCtx);
         if (IRObjectOrError) {
           // A universal file member can be a MachOObjectFile, an IRObject or an
-        // Archive. In case we can successfully cast the member as an IRObject,
-        // it is safe to throw away the error generated due to casting the
-        // object as a MachOObjectFile.
+          // Archive. In case we can successfully cast the member as an
+          // IRObject, it is safe to throw away the error generated due to
+          // casting the object as a MachOObjectFile.
           consumeError(MachOObjOrErr.takeError());
 
           NewArchiveMember NewMember =
               NewArchiveMember(IRObjectOrError->get()->getMemoryBufferRef());
           NewMember.MemberName = sys::path::filename(NewMember.MemberName);
 
           if (Error E = verifyAndAddIRObject(std::move(NewMember)))
             return E;
           continue;
         }
 
         Expected<std::unique_ptr<Archive>> ArchiveOrError = O.getAsArchive();
         if (ArchiveOrError) {
           // A universal file member can be a MachOObjectFile, an IRObject or an
           // Archive. In case we can successfully cast the member as an Archive,
           // it is safe to throw away the error generated due to casting the
           // object as a MachOObjectFile.
           consumeError(MachOObjOrErr.takeError());
           consumeError(IRObjectOrError.takeError());
 
-        if (Error E = processArchive(**ArchiveOrError, FileName))
+          if (Error E = processArchive(**ArchiveOrError))
             return E;
           continue;
         }
 
         Error CombinedError = joinErrors(
             ArchiveOrError.takeError(),
             joinErrors(IRObjectOrError.takeError(), MachOObjOrErr.takeError()));
         return createFileError(FileName, std::move(CombinedError));
       }
 
       // Update vector FileBuffers with the MemoryBuffers to transfer
       // ownership.
-    Data.FileBuffers.push_back(std::move(NewMember.Buf));
+      Builder.Data.FileBuffers.push_back(std::move(NewMember.Buf));
       return Error::success();
     }
-
-  Error addMember(StringRef FileName) {
-    Expected<NewArchiveMember> NewMemberOrErr =
-        NewArchiveMember::getFile(FileName, C.Deterministic);
-    if (!NewMemberOrErr)
-      return createFileError(FileName, NewMemberOrErr.takeError());
-    auto &NewMember = *NewMemberOrErr;
-
-    // For regular archives, use the basename of the object path for the member
-    // name.
-    NewMember.MemberName = sys::path::filename(NewMember.MemberName);
-    file_magic Magic = identify_magic(NewMember.Buf->getBuffer());
-
-    // Flatten archives.
-    if (Magic == file_magic::archive)
-      return addArchiveMembers(std::move(NewMember), FileName);
-
-    // Flatten universal files.
-    if (Magic == file_magic::macho_universal_binary)
-      return addUniversalMembers(std::move(NewMember), FileName);
-
-    // Bitcode files.
-    if (Magic == file_magic::bitcode)
-      return verifyAndAddIRObject(std::move(NewMember));
-
-    return verifyAndAddMachOObject(std::move(NewMember));
-  }
+  };
 
   MembersData Data;
   const Config &C;
 };
 
 static Expected<SmallVector<Slice, 2>>
 buildSlices(ArrayRef<OwningBinary<Archive>> OutputBinaries) {
   SmallVector<Slice, 2> Slices;
 
   for (const auto &OB : OutputBinaries) {
     const Archive &A = *OB.getBinary();
     Expected<Slice> ArchiveSlice = Slice::create(A, &LLVMCtx);
     if (!ArchiveSlice)
       return ArchiveSlice.takeError();
     Slices.push_back(*ArchiveSlice);
   }
   return Slices;
 }
 
+static Error
+checkForDuplicates(const MembersPerArchitectureMap &MembersPerArch) {
+  for (const auto &M : MembersPerArch) {
+    ArrayRef<NewArchiveMember> Members = M.second.getMembers();
+    ArrayRef<StringRef> Files = M.second.getFiles();
+    StringMap<std::vector<StringRef>> MembersToFiles;
+    for (auto Iterators = std::make_pair(Members.begin(), Files.begin());
+         Iterators.first != Members.end();
+         ++Iterators.first, ++Iterators.second) {
+      assert(Iterators.second != Files.end() &&
+             "Files should be the same size as Members.");
+      MembersToFiles[Iterators.first->MemberName].push_back(*Iterators.second);
+    }
+
+    std::string ErrorData;
+    raw_string_ostream ErrorStream(ErrorData);
+    for (const auto &MemberToFile : MembersToFiles) {
+      if (MemberToFile.getValue().size() > 1) {
+        ErrorStream << "file '" << MemberToFile.getKey().str()
+                    << "' was specified multiple times.\n";
+
+        for (StringRef OriginalFile : MemberToFile.getValue())
+          ErrorStream << "in: " << OriginalFile.str() << '\n';
+
+        ErrorStream << '\n';
+      }
+    }
+
+    ErrorStream.flush();
+    if (ErrorData.size() > 0)
+      return createStringError(std::errc::invalid_argument, ErrorData.c_str());
+  }
+  return Error::success();
+}
+
 static Error createStaticLibrary(const Config &C) {
   MembersBuilder Builder(C);
   auto DataOrError = Builder.build();
   if (auto Error = DataOrError.takeError())
     return Error;
 
   const auto &NewMembers = DataOrError->MembersPerArchitecture;
 
-  if (NewMembers.size() == 1) {
-    return writeArchive(OutputFile, NewMembers.begin()->second,
+  if (Error E = checkForDuplicates(NewMembers))
+    WithColor::defaultWarningHandler(std::move(E));
+
+  if (NewMembers.size() == 1)
+    return writeArchive(OutputFile, NewMembers.begin()->second.getMembers(),
                         /*WriteSymtab=*/true,
                         /*Kind=*/object::Archive::K_DARWIN, C.Deterministic,
                         /*Thin=*/false);
-  }
 
   SmallVector<OwningBinary<Archive>, 2> OutputBinaries;
-  for (const std::pair<const uint64_t, std::vector<NewArchiveMember>> &M :
-       NewMembers) {
+  for (const std::pair<const uint64_t, NewArchiveMemberList> &M : NewMembers) {
     Expected<std::unique_ptr<MemoryBuffer>> OutputBufferOrErr =
-        writeArchiveToBuffer(M.second,
+        writeArchiveToBuffer(M.second.getMembers(),
                              /*WriteSymtab=*/true,
                              /*Kind=*/object::Archive::K_DARWIN,
                              C.Deterministic,
                              /*Thin=*/false);
     if (!OutputBufferOrErr)
       return OutputBufferOrErr.takeError();
     std::unique_ptr<MemoryBuffer> &OutputBuffer = OutputBufferOrErr.get();