[Support/TarWriter] - Don't allow TarWriter to add the same file more than once.

[grimar]

Summary

This is for PR35460.

Currently when LLD adds files to TarWriter it may pass the same file multiple times.
For example it happens for clang reproduce file which specifies archive (.a) files more
than once in command line. Though that can be catched on LLD side, I think it is reasonable
to restrict TarWriter to add the same file more than once, I believe current behavior does not
make much sence.

[ruiu]

include/llvm/Support/TarWriter.h

16

Please use llvm::StringSet.

24

Just keep it return void as you are not going to use its return value.

lib/Support/TarWriter.cpp

176

Exists

177–178

Or, just if (Seen.insert(Fullpath).second) return;

[grimar]

• Addressed review comments.

[grimar]

include/llvm/Support/TarWriter.h

16

It does not work:

>D:\Work\llvm\include\llvm/ADT/StringMap.h(209): error C2039: 'Deallocate': is not a member of 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>'
1>  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xstring(2633): note: see declaration of 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>'
1>  D:\Work\llvm\include\llvm/ADT/StringMap.h(301): note: see reference to function template instantiation 'void llvm::StringMapEntry<ValueTy>::Destroy<AllocatorTy>(AllocatorTy &)' being compiled
1>          with
1>          [
1>              ValueTy=char,
1>              AllocatorTy=std::string
1>          ]

I think llvm containers does not know how to handle std::string. So I had to use std::set. It seems common in LLVM to use it for std::string.

lib/Support/TarWriter.cpp

177–178

That looks much better, thanks !

[grimar]

• Use llvm::StringSet instead of std::set<std::string>.

[ruiu]

LGTM with a test.

[grimar]

• Added testcase.

[ruiu]

unittests/Support/TarWriterTest.cpp

124–143 ↗

(On Diff #125095)

You should split this into three different test functions.

[grimar]

Addressed review comments:

• Split test into three.
• Use llvm::sys::fs::file_size API for finding output tar file size.

[ruiu]

LGTM with this change.

unittests/Support/TarWriterTest.cpp

142 ↗

(On Diff #125298)

These test names made me think for a while why you needed to check for file size (e.g. did we have a logic to pick the largest file or something?)

It turned out that you are using file size just to verify that output is correct.

Please rename these tests SingleFile, NoDuplicate and Duplicate.

include/llvm/Support/TarWriter.h

 //===-- llvm/Support/TarWriter.h - Tar archive file creator -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_TAR_WRITER_H
 #define LLVM_SUPPORT_TAR_WRITER_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
+#include <set>
 
 namespace llvm {
 class TarWriter {
 public:
   static Expected<std::unique_ptr<TarWriter>> create(StringRef OutputPath,
                                                      StringRef BaseDir);
 
-  void append(StringRef Path, StringRef Data);
+  bool append(StringRef Path, StringRef Data);
 
 private:
   TarWriter(int FD, StringRef BaseDir);
   raw_fd_ostream OS;
   std::string BaseDir;
+  std::set<std::string> Files;
 };
 }
 
 #endif

lib/Support/TarWriter.cpp

if (std::error_code EC = openFileForWrite(OutputPath, FD, sys::fs::F_None))
     return make_error<StringError>("cannot open " + OutputPath, EC);
   return std::unique_ptr<TarWriter>(new TarWriter(FD, BaseDir));
 }
 
 TarWriter::TarWriter(int FD, StringRef BaseDir)
     : OS(FD, /*shouldClose=*/true, /*unbuffered=*/false), BaseDir(BaseDir) {}
 
 // Append a given file to an archive.
-void TarWriter::append(StringRef Path, StringRef Data) {
+bool TarWriter::append(StringRef Path, StringRef Data) {
   // Write Path and Data.
   std::string Fullpath = BaseDir + "/" + sys::path::convert_to_slash(Path);
 
+  bool Exist;
+  std::tie(std::ignore, Exist) = Files.insert(Fullpath);
+  if (Exist)
+    return false;
+
   StringRef Prefix;
   StringRef Name;
   if (splitUstar(Fullpath, Prefix, Name)) {
     writeUstarHeader(OS, Prefix, Name, Data.size());
   } else {
     writePaxHeader(OS, Fullpath);
     writeUstarHeader(OS, "", "", Data.size());
   }
 
   OS << Data;
   pad(OS);
 
   // POSIX requires tar archives end with two null blocks.
   // Here, we write the terminator and then seek back, so that
   // the file being output is terminated correctly at any moment.
   uint64_t Pos = OS.tell();
   OS << std::string(BlockSize * 2, '\0');
   OS.seek(Pos);
   OS.flush();
+
+  return true;
 }